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