[U-Boot] [RFC 1/1] efi_loader: refactor switch to hypervisor mode
Refactor the switch from supervisor to hypervisor to a new function called at the beginning of do_bootefi().
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) +{ + dcache_enable(); + debug("Reached HYP mode\n"); + longjmp(&hyp_jmp, 1); +} + +/** + * leave_supervisor() - switch to hypervisor mode + */ +static void leave_supervisor(void) +{ +#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(); + /* Initialize EFI drivers */ r = efi_init_obj_list(); if (r != EFI_SUCCESS) {
From: Heinrich Schuchardt xypron.glpk@gmx.de Date: Tue, 25 Dec 2018 09:26:57 +0100
Refactor the switch from supervisor to hypervisor to a new function called at the beginning of do_bootefi().
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).
Never been a huge fan of setjmp/longjmp, but I can see what you're doing here. This is tricky code though, so I think this needs to be tested on armv7 systems that support virtualisation (Cortex-A7) and systems that don't (Cortex-A9).
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?
On some boards there are commands that access secure devices. So those commands would no longer work. Obviously that is already the case when an EFI payload returns to the U-Boot command prompt.
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) +{
- dcache_enable();
- debug("Reached HYP mode\n");
- longjmp(&hyp_jmp, 1);
+}
+/**
- leave_supervisor() - switch to hypervisor mode
- */
+static void leave_supervisor(void) +{ +#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();
- /* Initialize EFI drivers */ r = efi_init_obj_list(); if (r != EFI_SUCCESS) {
-- 2.19.2
U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
On 12/25/18 1:39 PM, Mark Kettenis wrote:
From: Heinrich Schuchardt xypron.glpk@gmx.de Date: Tue, 25 Dec 2018 09:26:57 +0100
Refactor the switch from supervisor to hypervisor to a new function called at the beginning of do_bootefi().
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).
Never been a huge fan of setjmp/longjmp, but I can see what you're doing here. This is tricky code though, so I think this needs to be tested on armv7 systems that support virtualisation (Cortex-A7) and systems that don't (Cortex-A9).
For
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?
On some boards there are commands that access secure devices. So those commands would no longer work. Obviously that is already the case when an EFI payload returns to the U-Boot command prompt.
Thanks Mark for pointing this out.
We have some major differences between bootm and bootefi:
- Bootefi does not support CONFIG_ARMV8_SWITCH_TO_EL1 used by some Xilinx boards. - It ignores CONFIG_ARMV8_PSCI. - Update_os_arch_secondary_cores() is not called (needed for preparing SMP on several NXP platforms). I think these are maintained by York.
So uniting the code might be advisable.
Best regards
Heinrich
On 26.12.18 03:02, Heinrich Schuchardt wrote:
On 12/25/18 1:39 PM, Mark Kettenis wrote:
From: Heinrich Schuchardt xypron.glpk@gmx.de Date: Tue, 25 Dec 2018 09:26:57 +0100
Refactor the switch from supervisor to hypervisor to a new function called at the beginning of do_bootefi().
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).
Never been a huge fan of setjmp/longjmp, but I can see what you're doing here. This is tricky code though, so I think this needs to be tested on armv7 systems that support virtualisation (Cortex-A7) and systems that don't (Cortex-A9).
For
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?
On some boards there are commands that access secure devices. So those commands would no longer work. Obviously that is already the case when an EFI payload returns to the U-Boot command prompt.
Thanks Mark for pointing this out.
We have some major differences between bootm and bootefi:
- Bootefi does not support CONFIG_ARMV8_SWITCH_TO_EL1 used by some Xilinx boards.
Yeah, mostly because I really dislike boards that simply switch to EL1 for no good reason ;).
- It ignores CONFIG_ARMV8_PSCI.
What exactly should it honor here?
- Update_os_arch_secondary_cores() is not called (needed for preparing SMP on several NXP platforms). I think these are maintained by York.
So uniting the code might be advisable.
Well, bootm assumes that there is a single big step from U-Boot into payload (exiting U-Boot). With UEFI we have this long phase in between where we're in UEFI land, but still need full access to all U-Boot device infrastructure.
So I'm not quite sure how to unify that easily.
Alex
On 12/26/18 8:42 AM, Alexander Graf wrote:
On 26.12.18 03:02, Heinrich Schuchardt wrote:
On 12/25/18 1:39 PM, Mark Kettenis wrote:
From: Heinrich Schuchardt xypron.glpk@gmx.de Date: Tue, 25 Dec 2018 09:26:57 +0100
Refactor the switch from supervisor to hypervisor to a new function called at the beginning of do_bootefi().
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).
Never been a huge fan of setjmp/longjmp, but I can see what you're doing here. This is tricky code though, so I think this needs to be tested on armv7 systems that support virtualisation (Cortex-A7) and systems that don't (Cortex-A9).
For
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?
On some boards there are commands that access secure devices. So those commands would no longer work. Obviously that is already the case when an EFI payload returns to the U-Boot command prompt.
Thanks Mark for pointing this out.
We have some major differences between bootm and bootefi:
- Bootefi does not support CONFIG_ARMV8_SWITCH_TO_EL1 used by some Xilinx boards.
Yeah, mostly because I really dislike boards that simply switch to EL1 for no good reason ;).
- It ignores CONFIG_ARMV8_PSCI.
What exactly should it honor here?
Call armv8_setup_psci() and possibly smp_kick_all_cpus().
Regards
Heinrich
- Update_os_arch_secondary_cores() is not called (needed for preparing SMP on several NXP platforms). I think these are maintained by York.
So uniting the code might be advisable.
Well, bootm assumes that there is a single big step from U-Boot into payload (exiting U-Boot). With UEFI we have this long phase in between where we're in UEFI land, but still need full access to all U-Boot device infrastructure.
So I'm not quite sure how to unify that easily.
Alex
From: Heinrich Schuchardt xypron.glpk@gmx.de Date: Wed, 26 Dec 2018 10:57:38 +0100
On 12/26/18 8:42 AM, Alexander Graf wrote:
On 26.12.18 03:02, Heinrich Schuchardt wrote:
Thanks Mark for pointing this out.
We have some major differences between bootm and bootefi:
- Bootefi does not support CONFIG_ARMV8_SWITCH_TO_EL1 used by some Xilinx boards.
Yeah, mostly because I really dislike boards that simply switch to EL1 for no good reason ;).
The only justification I can see for this is when EL2 is sufficiently broken that it is inadvisable to have the kernel support virtualization or if EL2 is used to work around broken hardware. An example of the latter is the Socionext Synqacer where virtualization is used to fix up broken PCIe hardware. Of course that U-Boot doesn't use that SoC; the fixup happens in Tianocore.
- It ignores CONFIG_ARMV8_PSCI.
What exactly should it honor here?
Call armv8_setup_psci() and possibly smp_kick_all_cpus().
Probably because most ARMv8 SoCs actually use TF-A to implement PSCI.
On 26.12.18 13:05, Mark Kettenis wrote:
From: Heinrich Schuchardt xypron.glpk@gmx.de Date: Wed, 26 Dec 2018 10:57:38 +0100
On 12/26/18 8:42 AM, Alexander Graf wrote:
On 26.12.18 03:02, Heinrich Schuchardt wrote:
Thanks Mark for pointing this out.
We have some major differences between bootm and bootefi:
- Bootefi does not support CONFIG_ARMV8_SWITCH_TO_EL1 used by some Xilinx boards.
Yeah, mostly because I really dislike boards that simply switch to EL1 for no good reason ;).
The only justification I can see for this is when EL2 is sufficiently broken that it is inadvisable to have the kernel support virtualization or if EL2 is used to work around broken hardware. An example of the latter is the Socionext Synqacer where virtualization is used to fix up broken PCIe hardware. Of course that U-Boot doesn't use that SoC; the fixup happens in Tianocore.
In that case I would not expect to see CONFIG_ARMV8_SWITCH_TO_EL1 used though. Instead, I would expect that either ATF, something in between U-Boot and ATF or board specific code would do the fixup magic in EL2.
- It ignores CONFIG_ARMV8_PSCI.
What exactly should it honor here?
Call armv8_setup_psci() and possibly smp_kick_all_cpus().
Probably because most ARMv8 SoCs actually use TF-A to implement PSCI.
Yeah, I don't think I ran into a case where U-Boot was implementing PSCI. That's probably the reason this case never got triggered so far :/.
Alex
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?
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. 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.
Alex
- /* Initialize EFI drivers */ r = efi_init_obj_list(); if (r != EFI_SUCCESS) {
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.
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.
Regards
Heinrich
Alex
- /* Initialize EFI drivers */ r = efi_init_obj_list(); if (r != EFI_SUCCESS) {
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
participants (3)
-
Alexander Graf -
Heinrich Schuchardt -
Mark Kettenis