[RFC PATCH] arm: EFI: Disallow EFI bootmgr when providing PSCI

[RFC PATCH] arm: EFI: Disallow EFI bootmgr when providing PSCI

[Andre Przywara]

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.

Commit f3866909e350 ("distro_bootcmd: call EFI bootmgr even without
having /EFI/boot") changed the order of EFI probing, so the EFI bootmgr
is now *always* run, resulting in the default distro boot commands now
*always* failing, even in the total absence of any UEFI directories or
boot files.

So use the newly added build option to disable the EFI bootmgr, which
makes those boards boot again using the distro boot commands.
Explicitly calling "bootefi bootmgr" still breaks the boot, though.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
Reported-by: Jernej Skrabec <jernej.skrabec@siol.net>
---
Hi,

the above is the result of my analysis, happy to stand corrected in
case I missed something. I know that this is not a proper solution,
but it's an effective stop-gap measure to fix all those boards. It looks
like a proper solution would either be:
- Let the EFI bootmgr run in the current security state.
- Install the PSCI handlers early in U-Boot.

Both solutions sound rather involved, so probably require more time.
But we need to fix this breakage now.

Cheers,
Andre

 lib/efi_loader/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index e729f727df1..a1e453fa605 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -30,6 +30,7 @@ if EFI_LOADER
 config CMD_BOOTEFI_BOOTMGR
 	bool "UEFI Boot Manager"
 	default y
+	depends on !ARMV7_PSCI
 	help
 	  Select this option if you want to select the UEFI binary to be booted
 	  via UEFI variables Boot####, BootOrder, and BootNext. This enables the
-- 
2.17.5

[RFC PATCH] arm: EFI: Disallow EFI bootmgr when providing PSCI

[Simon Glass]

On Fri, 22 Jan 2021 at 05:05, Andre Przywara <andre.przywara@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.
>
> Commit f3866909e350 ("distro_bootcmd: call EFI bootmgr even without
> having /EFI/boot") changed the order of EFI probing, so the EFI bootmgr
> is now *always* run, resulting in the default distro boot commands now
> *always* failing, even in the total absence of any UEFI directories or
> boot files.
>
> So use the newly added build option to disable the EFI bootmgr, which
> makes those boards boot again using the distro boot commands.
> Explicitly calling "bootefi bootmgr" still breaks the boot, though.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> Reported-by: Jernej Skrabec <jernej.skrabec@siol.net>
> ---
> Hi,
>
> the above is the result of my analysis, happy to stand corrected in
> case I missed something. I know that this is not a proper solution,
> but it's an effective stop-gap measure to fix all those boards. It looks
> like a proper solution would either be:
> - Let the EFI bootmgr run in the current security state.
> - Install the PSCI handlers early in U-Boot.
>
> Both solutions sound rather involved, so probably require more time.
> But we need to fix this breakage now.
>
> Cheers,
> Andre
>
>  lib/efi_loader/Kconfig | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

[RFC PATCH] arm: EFI: Disallow EFI bootmgr when providing PSCI

[Heinrich Schuchardt]

On 1/24/21 3:03 AM, Simon Glass wrote:
> On Fri, 22 Jan 2021 at 05:05, Andre Przywara <andre.przywara@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.

But I observe that it takes around 60 seconds between
SetVirtualAddressMap() and the first kernel log output.

EFI stub: Exiting boot services and installing virtual address map...

EHCI failed to shut down host controller.
<<< 60 seconds waiting without output >>>>

[    0.000000] Booting Linux on physical CPU 0x0

I have seen this regression since some time last year.

Reverting patch f3866909e350 does not solve the problem.
Reverting to U-Boot v2020.01 does not solve the problem.

Reverting the kernel from v5.9 to 5.4 solves the problem both for U-Boot
v2020.01 as well as for U-Boot v2021.01.

I have poked around with some pre-built kernels from
http://snapshot.debian.org/package/linux:

Linux 5.9.11 - 1 minute delay
Linux 5.8.14 - 1 minute delay
Linux 5.7.17 - no delay
Linux 5.6.14 - no delay
Linux 5.5.17 - no delay
Linux 5.4.19 - no delay

It seems that some change in Linux is causing the regression. Could you,
please, try to analyze it in more depth.

Best regards

Heinrich

>>
>> Commit f3866909e350 ("distro_bootcmd: call EFI bootmgr even without
>> having /EFI/boot") changed the order of EFI probing, so the EFI bootmgr
>> is now *always* run, resulting in the default distro boot commands now
>> *always* failing, even in the total absence of any UEFI directories or
>> boot files.
>>
>> So use the newly added build option to disable the EFI bootmgr, which
>> makes those boards boot again using the distro boot commands.
>> Explicitly calling "bootefi bootmgr" still breaks the boot, though.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> Reported-by: Jernej Skrabec <jernej.skrabec@siol.net>
>> ---
>> Hi,
>>
>> the above is the result of my analysis, happy to stand corrected in
>> case I missed something. I know that this is not a proper solution,
>> but it's an effective stop-gap measure to fix all those boards. It looks
>> like a proper solution would either be:
>> - Let the EFI bootmgr run in the current security state.
>> - Install the PSCI handlers early in U-Boot.
>>
>> Both solutions sound rather involved, so probably require more time.
>> But we need to fix this breakage now.
>>
>> Cheers,
>> Andre
>>
>>   lib/efi_loader/Kconfig | 1 +
>>   1 file changed, 1 insertion(+)
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
>

[RFC PATCH] arm: EFI: Disallow EFI bootmgr when providing PSCI

[Jernej Škrabec]

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@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.

I observed issue on OrangePi Plus2E which has eMMC. At that time, there was 
Android on it. If I disabled eMMC support in U-Boot, boot went just fine.

> 
> But I observe that it takes around 60 seconds between
> SetVirtualAddressMap() and the first kernel log output.

I guess this is another issue. It never booted in my case. It stopped right 
after "Starting kernel...".

Best regards,
Jernej

> 
> EFI stub: Exiting boot services and installing virtual address map...
> 
> EHCI failed to shut down host controller.
> <<< 60 seconds waiting without output >>>>
> 
> [    0.000000] Booting Linux on physical CPU 0x0
> 
> I have seen this regression since some time last year.
> 
> Reverting patch f3866909e350 does not solve the problem.
> Reverting to U-Boot v2020.01 does not solve the problem.
> 
> Reverting the kernel from v5.9 to 5.4 solves the problem both for U-Boot
> v2020.01 as well as for U-Boot v2021.01.
> 
> I have poked around with some pre-built kernels from
> http://snapshot.debian.org/package/linux:
> 
> Linux 5.9.11 - 1 minute delay
> Linux 5.8.14 - 1 minute delay
> Linux 5.7.17 - no delay
> Linux 5.6.14 - no delay
> Linux 5.5.17 - no delay
> Linux 5.4.19 - no delay
> 
> It seems that some change in Linux is causing the regression. Could you,
> please, try to analyze it in more depth.
> 
> Best regards
> 
> Heinrich
> 
> >> Commit f3866909e350 ("distro_bootcmd: call EFI bootmgr even without
> >> having /EFI/boot") changed the order of EFI probing, so the EFI bootmgr
> >> is now *always* run, resulting in the default distro boot commands now
> >> *always* failing, even in the total absence of any UEFI directories or
> >> boot files.
> >> 
> >> So use the newly added build option to disable the EFI bootmgr, which
> >> makes those boards boot again using the distro boot commands.
> >> Explicitly calling "bootefi bootmgr" still breaks the boot, though.
> >> 
> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> >> Reported-by: Jernej Skrabec <jernej.skrabec@siol.net>
> >> ---
> >> Hi,
> >> 
> >> the above is the result of my analysis, happy to stand corrected in
> >> case I missed something. I know that this is not a proper solution,
> >> but it's an effective stop-gap measure to fix all those boards. It looks
> >> like a proper solution would either be:
> >> - Let the EFI bootmgr run in the current security state.
> >> - Install the PSCI handlers early in U-Boot.
> >> 
> >> Both solutions sound rather involved, so probably require more time.
> >> But we need to fix this breakage now.
> >> 
> >> Cheers,
> >> Andre
> >> 
> >>   lib/efi_loader/Kconfig | 1 +
> >>   1 file changed, 1 insertion(+)
> > 
> > Reviewed-by: Simon Glass <sjg@chromium.org>

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

[Jernej Škrabec]

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@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.

> 
> I observed issue on OrangePi Plus2E which has eMMC. At that time, there was
> Android on it. If I disabled eMMC support in U-Boot, boot went just fine.
> 
> > But I observe that it takes around 60 seconds between
> > SetVirtualAddressMap() and the first kernel log output.
> 
> I guess this is another issue. It never booted in my case. It stopped right
> after "Starting kernel...".
> 
> Best regards,
> Jernej
> 
> > EFI stub: Exiting boot services and installing virtual address map...
> > 
> > EHCI failed to shut down host controller.
> > <<< 60 seconds waiting without output >>>>
> > 
> > [    0.000000] Booting Linux on physical CPU 0x0
> > 
> > I have seen this regression since some time last year.
> > 
> > Reverting patch f3866909e350 does not solve the problem.
> > Reverting to U-Boot v2020.01 does not solve the problem.
> > 
> > Reverting the kernel from v5.9 to 5.4 solves the problem both for U-Boot
> > v2020.01 as well as for U-Boot v2021.01.
> > 
> > I have poked around with some pre-built kernels from
> > http://snapshot.debian.org/package/linux:
> > 
> > Linux 5.9.11 - 1 minute delay
> > Linux 5.8.14 - 1 minute delay
> > Linux 5.7.17 - no delay
> > Linux 5.6.14 - no delay
> > Linux 5.5.17 - no delay
> > Linux 5.4.19 - no delay
> > 
> > It seems that some change in Linux is causing the regression. Could you,
> > please, try to analyze it in more depth.
> > 
> > Best regards
> > 
> > Heinrich
> > 
> > >> Commit f3866909e350 ("distro_bootcmd: call EFI bootmgr even without
> > >> having /EFI/boot") changed the order of EFI probing, so the EFI bootmgr
> > >> is now *always* run, resulting in the default distro boot commands now
> > >> *always* failing, even in the total absence of any UEFI directories or
> > >> boot files.
> > >> 
> > >> So use the newly added build option to disable the EFI bootmgr, which
> > >> makes those boards boot again using the distro boot commands.
> > >> Explicitly calling "bootefi bootmgr" still breaks the boot, though.
> > >> 
> > >> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > >> Reported-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > >> ---
> > >> Hi,
> > >> 
> > >> the above is the result of my analysis, happy to stand corrected in
> > >> case I missed something. I know that this is not a proper solution,
> > >> but it's an effective stop-gap measure to fix all those boards. It
> > >> looks
> > >> like a proper solution would either be:
> > >> - Let the EFI bootmgr run in the current security state.
> > >> - Install the PSCI handlers early in U-Boot.
> > >> 
> > >> Both solutions sound rather involved, so probably require more time.
> > >> But we need to fix this breakage now.
> > >> 
> > >> Cheers,
> > >> Andre
> > >> 
> > >>   lib/efi_loader/Kconfig | 1 +
> > >>   1 file changed, 1 insertion(+)
> > > 
> > > Reviewed-by: Simon Glass <sjg@chromium.org>

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

[Heinrich Schuchardt]

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@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.

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

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.

The downside of the change is that executing bootz will still fail after
a UEFI binary returns to U-Boot.

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?

Best regards

Heinrich

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)

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

[Andre Przywara]

On Sun, 24 Jan 2021 11:44:35 +0100
Heinrich Schuchardt <xypron.glpk@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@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.
 
> 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).

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).

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)

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

[Heinrich Schuchardt]

On 1/24/21 2:07 PM, Andre Przywara wrote:
> On Sun, 24 Jan 2021 11:44:35 +0100
> Heinrich Schuchardt <xypron.glpk@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@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

>
>> 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.

>
> 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?

Is this the only thing requiring running in secure mode?

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

Best regards

Heinrich

>
> 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)
>

[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@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@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@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)  
> >  
>