On 26.12.18 10:54, Heinrich Schuchardt wrote:
> On 12/26/18 8:52 AM, Alexander Graf wrote:
>>
>>
>> On 25.12.18 09:26, Heinrich Schuchardt wrote:
>>> Refactor the switch from supervisor to hypervisor to a new function called
>>> at
>>> the beginning of do_bootefi().
>>
>> Why?
>
> Currently we have duplicate code for loading and starting EFI binaries
> in cmd/bootefi.c and lib/efi_loader/efi_boottime.c. I want to clean up
> this situation.
>
>>
>>>
>>> Signed-off-by: Heinrich Schuchardt <xypron.g...@gmx.de>
>>> ---
>>> With this patch I am just moving around the switch from supervisor to
>>> hypervisor mode within the EFI subsystem. Similar switching also occurs
>>> in all other boot commands (cf. arch/arm/lib/bootm.c).
>>>
>>> Why are we running the U-Boot console in supervisor mode at all? Wouldn't
>>> it be advisable for security reasons to switch to hypervisor mode before
>>> entering the console?
>>>
>>> Best regards
>>>
>>> Heinrich
>>> ---
>>> cmd/bootefi.c | 109 ++++++++++++++++++++++----------------------------
>>> 1 file changed, 47 insertions(+), 62 deletions(-)
>>>
>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>>> index 1aaf5319e13..277d4863953 100644
>>> --- a/cmd/bootefi.c
>>> +++ b/cmd/bootefi.c
>>> @@ -31,9 +31,9 @@ DECLARE_GLOBAL_DATA_PTR;
>>> #define OBJ_LIST_NOT_INITIALIZED 1
>>>
>>> static efi_status_t efi_obj_list_initialized = OBJ_LIST_NOT_INITIALIZED;
>>> -
>>> static struct efi_device_path *bootefi_image_path;
>>> static struct efi_device_path *bootefi_device_path;
>>> +struct jmp_buf_data hyp_jmp;
>>>
>>> /* Initialize and populate EFI object list */
>>> efi_status_t efi_init_obj_list(void)
>>> @@ -122,6 +122,50 @@ void __weak allow_unaligned(void)
>>> {
>>> }
>>>
>>> +/**
>>> + * entry_hyp() - entry point when switching to hypervisor mode
>>> + */
>>> +static void entry_hyp(void)
>>
>> Potentially unused function depending on config settings?
>>
>>> +{
>>> + dcache_enable();
>>> + debug("Reached HYP mode\n");
>>
>> HYP is a 32bit ARM term. AArch64 does not know what HYP is - there it's EL2.
>>
>> Also keep in mind that this is not 100% ARM specific. We might see
>> something along the lines of this logic on RISC-V eventually too.
>>
>>> + longjmp(&hyp_jmp, 1);
>>> +}
>>> +
>>> +/**
>>> + * leave_supervisor() - switch to hypervisor mode
>>> + */
>>> +static void leave_supervisor(void)
>>
>> That's not necessarily what this function does. It may switch from
>> EL1->EL2 or from EL3->EL2.
>>
>>> +{
>>> +#ifdef CONFIG_ARMV7_NONSEC
>>> + static bool is_nonsec;
>>> +
>>> + if (armv7_boot_nonsec() && !is_nonsec) {
>>> + if (setjmp(&hyp_jmp))
>>> + return;
>>> + dcache_disable(); /* flush cache before switch to HYP */
>>> + armv7_init_nonsec();
>>> + is_nonsec = true;
>>> + secure_ram_addr(_do_nonsec_entry)(entry_hyp, 0, 0, 0);
>>> + }
>>> +#endif
>>> +
>>> +#ifdef CONFIG_ARM64
>>> + /* On AArch64 we need to make sure we call our payload in < EL3 */
>>> + if (current_el() == 3) {
>>> + if (setjmp(&hyp_jmp))
>>> + return;
>>> + smp_kick_all_cpus();
>>> + dcache_disable(); /* flush cache before switch to EL2 */
>>> +
>>> + /* Move into EL2 and keep running there */
>>> + armv8_switch_to_el2(0, 0, 0, 0, (uintptr_t)entry_hyp,
>>> + ES_TO_AARCH64);
>>> + }
>>> +#endif
>>> + return;
>>> +}
>>> +
>>> /*
>>> * Set the load options of an image from an environment variable.
>>> *
>>> @@ -233,34 +277,6 @@ static efi_status_t efi_do_enter(
>>> return ret;
>>> }
>>>
>>> -#ifdef CONFIG_ARM64
>>> -static efi_status_t efi_run_in_el2(EFIAPI efi_status_t (*entry)(
>>> - efi_handle_t image_handle, struct efi_system_table *st),
>>> - efi_handle_t image_handle, struct efi_system_table *st)
>>> -{
>>> - /* Enable caches again */
>>> - dcache_enable();
>>> -
>>> - return efi_do_enter(image_handle, st, entry);
>>> -}
>>> -#endif
>>> -
>>> -#ifdef CONFIG_ARMV7_NONSEC
>>> -static bool is_nonsec;
>>> -
>>> -static efi_status_t efi_run_in_hyp(EFIAPI efi_status_t (*entry)(
>>> - efi_handle_t image_handle, struct efi_system_table *st),
>>> - efi_handle_t image_handle, struct efi_system_table *st)
>>> -{
>>> - /* Enable caches again */
>>> - dcache_enable();
>>> -
>>> - is_nonsec = true;
>>> -
>>> - return efi_do_enter(image_handle, st, entry);
>>> -}
>>> -#endif
>>> -
>>> /*
>>> * efi_carve_out_dt_rsv() - Carve out DT reserved memory ranges
>>> *
>>> @@ -440,39 +456,6 @@ static efi_status_t do_bootefi_exec(void *efi,
>>> goto err_prepare;
>>> }
>>>
>>> -#ifdef CONFIG_ARM64
>>> - /* On AArch64 we need to make sure we call our payload in < EL3 */
>>> - if (current_el() == 3) {
>>> - smp_kick_all_cpus();
>>> - dcache_disable(); /* flush cache before switch to EL2 */
>>> -
>>> - /* Move into EL2 and keep running there */
>>> - armv8_switch_to_el2((ulong)entry,
>>> - (ulong)&image_obj->header,
>>> - (ulong)&systab, 0, (ulong)efi_run_in_el2,
>>> - ES_TO_AARCH64);
>>> -
>>> - /* Should never reach here, efi exits with longjmp */
>>> - while (1) { }
>>> - }
>>> -#endif
>>> -
>>> -#ifdef CONFIG_ARMV7_NONSEC
>>> - if (armv7_boot_nonsec() && !is_nonsec) {
>>> - dcache_disable(); /* flush cache before switch to HYP */
>>> -
>>> - armv7_init_nonsec();
>>> - secure_ram_addr(_do_nonsec_entry)(
>>> - efi_run_in_hyp,
>>> - (uintptr_t)entry,
>>> - (uintptr_t)&image_obj->header,
>>> - (uintptr_t)&systab);
>>> -
>>> - /* Should never reach here, efi exits with longjmp */
>>> - while (1) { }
>>> - }
>>> -#endif
>>> -
>>> ret = efi_do_enter(&image_obj->header, &systab, entry);
>>>
>>> err_prepare:
>>> @@ -558,6 +541,8 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int
>>> argc, char * const argv[])
>>> /* Allow unaligned memory access */
>>> allow_unaligned();
>>>
>>> + leave_supervisor();
>>
>> This means you're potentially executing object initialization code in a
>> mode it wasn't meant to be run in.
>
> Could you, please, elaborate on the potential difficulties you see.
Well, object init code may do something secure while object execution
code may not. I don't think we have such a case today, but I'm not 100%
sure.
Either way, from a refactoring patch like this, I would expect to see
separation of functional changes over code position changes. So if you
use the word "refactor", it means you do not change any behavior. That's
fine - but changing the position that switch happens is a change of
behavior and may result in more work in bisect later down the road if
something goes wrong.
>
>> Is that a smart thing to do? It's
>> definitely more than just refactoring.
>>
>> If you just find the do_bootefi_exec() function too hard to read,
>> refactoring it into calling an arch_efi_do_enter() call is probably
>> smarter. Then aarch64 and 32bit arm can just provide their own "get be
>> into target mode" functions within their own arch directories.
>>
>
> In a second version of the patch which I have not yet sent I am using a
> weak function.
>
> https://github.com/xypron/u-boot-patches/blob/efi-next/0001-efi_loader-refactor-switch-to-hypervisor-mode.patch
>
> But why call it arch_*efi*_do_enter? This is not EFI specific the same
> is done in the bootm command.
I always have a hard time to track what exactly bootm does when. If you
can combine code, I'm all for it.
Alex
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
