[U-Boot] [RFC 1/1] efi_loader: refactor switch to hypervisor mode

Wed Dec 26 09:54:11 UTC 2018

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

Regards

Heinrich

> 
> Alex
> 
>> +
>>  	/* Initialize EFI drivers */
>>  	r = efi_init_obj_list();
>>  	if (r != EFI_SUCCESS) {
>>
> 