[linux-sunxi] Re: [RFC PATCH] arm: EFI: Disallow EFI bootmgr when providing PSCI

[Andre Przywara]

On Sun, 24 Jan 2021 15:45:23 +0100
Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:

> On 1/24/21 2:07 PM, Andre Przywara wrote:
> > On Sun, 24 Jan 2021 11:44:35 +0100
> > Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> >
> > Hi Heinrich,
> >
> > many thanks for digging into this!
> >  
> >> On 1/24/21 9:47 AM, Jernej Škrabec wrote:  
> >>> Dne nedelja, 24. januar 2021 ob 09:33:15 CET je Jernej Škrabec napisal(a):  
> >>>> Hi!
> >>>>
> >>>> Dne nedelja, 24. januar 2021 ob 09:27:02 CET je Heinrich Schuchardt
> >>>>
> >>>> napisal(a):  
> >>>>> On 1/24/21 3:03 AM, Simon Glass wrote:  
> >>>>>> On Fri, 22 Jan 2021 at 05:05, Andre Przywara <andre.przywara at arm.com>  
> >>>>
> >>>> wrote:  
> >>>>>>> When "bootefi bootmgr" is run, it switches the CPU into non-secure
> >>>>>>> state. This breaks platforms like 32-bit Allwinner boards that rely on
> >>>>>>> running in secure state until late in the process, when they install
> >>>>>>> the PSCI handler in secure memory and drop into non-secure state.
> >>>>>>> They hang just before entering the kernel, after the "Starting the
> >>>>>>> kernel" message.  
> >>>>>
> >>>>> Dear Andre,
> >>>>>
> >>>>> thank you for reporting the issue.
> >>>>>
> >>>>> I have an Orange Pi PC with a 32 bit Allwinner CPU.
> >>>>> orangepi_pc_defconfig has CONFIG_ARMV7_PSCI=y.
> >>>>>
> >>>>> I use origin/master (e716c9022970dac9b) and the Orange PI boots
> >>>>> successfully using GRUB EFI into Linux 5.9.  
> >>>
> >>> Just one clarification - issue here is that "bootefi bootmgr" command
> >>> when unsuccessful breaks booting with bootm command.  
> >>
> >> If I press the enter key to get into the console circumventing
> >> distro-boot, booting via bootz works. If I wait until distro_boot is
> >> finished, booting via bootz fails. Both with Linux 5.7.17.  
> >
> > Yes, that was exactly Jernej's and my observation.
> >  
> >> This confirms your finding that there is a problem with the
> >> initialization of the UEFI sub-system.
> >>
> >> lib/efi_loader/efi_setup.c:192 is the only place where we call
> >> switch_to_non_secure_mode().
> >>
> >> With the line removed:
> >>
> >> * Booting via bootz is successful.
> >> * The EFI stub shows: Entering in SVC mode with MMU enabled
> >> * Booting via bootefi fails  
> >
> > Ah, thanks for giving this a try. I know next to nothing about U-Boot's
> > UEFI internals, so didn't dare to touch this code.
> >  
> >> switch_to_non_secure_mode() is safe to be called repeatedly. So we could
> >> move the switch_to_non_secure_mode() call to do_bootefi_exec(). This is
> >> after the boot manager has searched for a bootable image.
> >>
> >> With the change (see diff below):
> >>
> >> * Booting via bootz is successful.
> >> * EFI stub shows: Entering in HYP mode with MMU enabled
> >> * Booting via bootefi is successful.  
> >
> > Oh nice! I wasn't sure how much the UEFI initialisation actually relies
> > on non-secure mode, and if switching to non-secure *after* the
> > initialisation would change things.
> >  
> >> The downside of the change is that executing bootz will still fail after
> >> a UEFI binary returns to U-Boot.  
> >
> > OK, that is not nice, but not really a big issue. At least it's a lot
> > better than disabling bootmgr altogether or no bootz after a bootmgr
> > call.
> > If you deem this change being not very intrusive, I would very much
> > prefer this over my patch here.  
> 
> Here is the patch:
> 
> [PATCH 1/1] efi_loader: switch to non-secure mode later
> https://lists.denx.de/pipermail/u-boot/2021-January/438533.html

Thanks, I will give this a spin ASAP.

> >> Running a shell in secure mode seems unwise. So we should dig a bit deeper:
> >>
> >> Where in the code is the PSCI handler installed and where occurs the
> >> switch to non-secure state when booting via bootz/bootm? Can we move
> >> this to before distro-boot?  
> >
> > Well, yes, I think installing the PSCI handlers (and doing
> > everything that requires secure state) early is the best way forward,
> > but this is quite some change, and I would rather plug this problem now.
> >
> > At the moment all of the v7 PSCI code is run as late as possible, so
> > U-Boot can run in secure state. I agree this *sounds* scary, but
> > running in secure is actually quite common for many ARM32 machines (even
> > Linux sometimes runs with the NS bit cleared).  
> 
> If we wanted security, I guess, we would have to move this stuff to TF-A.

So yeah, Samuel is investigating TF-A for 32-bit Allwinner chip, AFAIK.
But this will take some time, I guess, and won't probably never replace
the existing PSCI implementation in U-Boot completely.

And actually the term "security" is somewhat misleading here, not sure
who we protect against. On 32-bit Allwinner U-Boot is the first and
only "firmware" code that is running, so it has all the rights to do
that in secure state.

> > For v7 Allwinner SoCs specifically we need access to the secure-only
> > SID registers for the MAC address generation, also secure SRAM becomes
> > inaccessible in non-secure world (as expected, but in contrast to the
> > ARMv8 chips).  
> 
> Where would I find that code where we generate the MAC address?

The unique SoC serial number is read from the SID "device", the
interesting part for us are some r/o ID registers. The code is in
arch/arm/mach-sunxi/cpu_info.c:sunxi_get_sid(), the MAC address is
generated from that in board/sunxi/board.c:setup_environment(). (Use
latest master, as this code recently changed).

This method already bites us when 64-bit SoCs run with the secure boot
fuse burnt. I have some code to read the MAC address from the DT
instead (or rather to make that actually work), and TF-A can use the
same algorithm to generate the MAC address (just didn't manage to use
the ARMv8 CRC instructions for that yet).
When starting in secure and dropping early, we could just read the SID
register very early and store it somewhere, so not a real problem.

> Is this the only thing requiring running in secure mode?

There might be other places where secure state is required, CNTFRQ
writes require the highest EL, maybe some core clock setup as well?

So it's surely possible, I am just a bit wary of touching 32-bit code
too much, as I have only an A20 and H3 to test on (plus an A10 I need to
get running first).

> Or do you need secure-mode when setting the MAC address in
> sun8i_eth_write_hwaddr(), _sunxi_write_hwaddr()?

No, this is a pure EMAC operation, totally ignorant of the NS bit.

Thanks!
Andre

> 
> >
> > So I think eventually we will need to bite the bullet and teach
> > U-Boot to cope with non-secure in sunxi-v7, but this needs some
> > time and requires possibly intrusive changes.
> >
> > Cheers,
> > Andre
> >  
> >>
> >> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> >> index c8eb5c32b0..81dd8e0284 100644
> >> --- a/cmd/bootefi.c
> >> +++ b/cmd/bootefi.c
> >> @@ -8,6 +8,7 @@
> >>    #define LOG_CATEGORY LOGC_EFI
> >>
> >>    #include <common.h>
> >> +#include <bootm.h>
> >>    #include <charset.h>
> >>    #include <command.h>
> >>    #include <dm.h>
> >> @@ -338,6 +339,9 @@ static efi_status_t
> >> do_bootefi_exec(efi_handle_t handle, void *load_options)
> >>           efi_uintn_t exit_data_size = 0;
> >>           u16 *exit_data = NULL;
> >>
> >> +       /* On ARM switch from EL3 or secure mode to EL2 or
> >> non-secure mode */
> >> +       switch_to_non_secure_mode();
> >> +
> >>           /* Call our payload! */
> >>           ret = EFI_CALL(efi_start_image(handle, &exit_data_size,
> >> &exit_data));
> >>           if (ret != EFI_SUCCESS) {
> >> diff --git a/lib/efi_loader/efi_setup.c
> >> b/lib/efi_loader/efi_setup.c index 5800cbf6d4..588fbda736 100644
> >> --- a/lib/efi_loader/efi_setup.c
> >> +++ b/lib/efi_loader/efi_setup.c
> >> @@ -188,9 +188,6 @@ efi_status_t efi_init_obj_list(void)
> >>           /* Allow unaligned memory access */
> >>           allow_unaligned();
> >>
> >> -       /* On ARM switch from EL3 or secure mode to EL2 or
> >> non-secure mode */
> >> -       switch_to_non_secure_mode();
> >> -
> >>           /* Initialize root node */
> >>           ret = efi_root_node_register();
> >>           if (ret != EFI_SUCCESS)  
> >  
>