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

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