[U-Boot] [PATCH v2 1/1] efi_loader: bootmgr: support BootNext and BootCurrent variable behavior

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

On Wed, Mar 06, 2019 at 05:21:54AM +0100, Heinrich Schuchardt wrote:
> On 3/6/19 1:17 AM, AKASHI Takahiro wrote:
> > On Tue, Mar 05, 2019 at 08:38:40PM +0100, Heinrich Schuchardt wrote:
> >> On 3/5/19 2:58 AM, AKASHI Takahiro wrote:
> >>> See UEFI v2.7, section 3.1.2 for details of the specification.
> >>>
> >>> With efidebug command, you can run any EFI boot option as follows:
> >>>   => efi boot add 1 SHELL ...
> >>>   => efi boot add 2 HELLO ...
> >>>   => efi boot order 1 2
> >>>   => efi bootmgr
> >>>      (starting SHELL ...)
> >>>
> >>>   => efi boot next 2
> >>>   => efi bootmgr
> >>>      (starting HELLO ...)
> >>>   => env print -e
> >>>   <snip ...>
> >>>   BootCurrent: {boot,run}(blob)
> >>>   00000000:  02 00                    ..
> >>>   BootOrder: {boot,run}(blob)
> >>>   00000000:  01 00 02 00              ....
> >>>
> >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >>
> >> Please, use scripts/get_maintainer.pl to determine the correct
> >> recipients. You missed Alex's new email address.
> > 
> > Okay.
> > 
> >>> ---
> >>>  lib/efi_loader/efi_bootmgr.c | 40 ++++++++++++++++++++++++++++++++----
> >>>  1 file changed, 36 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> >>> index 417016102b48..1575c5c09e46 100644
> >>> --- a/lib/efi_loader/efi_bootmgr.c
> >>> +++ b/lib/efi_loader/efi_bootmgr.c
> >>> @@ -141,6 +141,7 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,
> >>>  	efi_deserialize_load_option(&lo, load_option);
> >>>  
> >>>  	if (lo.attributes & LOAD_OPTION_ACTIVE) {
> >>> +		u32 attributes;
> >>>  		efi_status_t ret;
> >>>  
> >>>  		debug("%s: trying to load \"%ls\" from %pD\n",
> >>> @@ -151,6 +152,16 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,
> >>>  		if (ret != EFI_SUCCESS)
> >>>  			goto error;
> >>>  
> >>> +		attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS |
> >>> +			     EFI_VARIABLE_RUNTIME_ACCESS;
> >>> +		size = sizeof(n);
> >>> +		ret = EFI_CALL(efi_set_variable(
> >>> +				L"BootCurrent",
> >>> +				(efi_guid_t *)&efi_global_variable_guid,
> >>> +				attributes, size, &n));
> >>> +		if (ret != EFI_SUCCESS)
> >>> +			goto error;
> >>> +
> >>>  		printf("Booting: %ls\n", lo.label);
> >>>  		efi_dp_split_file_path(lo.file_path, device_path, file_path);
> >>>  	}
> >>> @@ -162,21 +173,42 @@ error:
> >>>  }
> >>>  
> >>>  /*
> >>> - * Attempt to load, in the order specified by BootOrder EFI variable, the
> >>> - * available load-options, finding and returning the first one that can
> >>> - * be loaded successfully.
> >>> + * Attempt to load from BootNext or in the order specified by BootOrder
> >>> + * EFI variable, the available load-options, finding and returning
> >>> + * the first one that can be loaded successfully.
> >>>   */
> >>>  void *efi_bootmgr_load(struct efi_device_path **device_path,
> >>>  		       struct efi_device_path **file_path)
> >>>  {
> >>> -	uint16_t *bootorder;
> >>> +	u16 bootnext, *bootorder;
> >>
> >> bootnext has enough space for the terminating \n. That is way too small.
> >>
> >> You want to call efi_get_variable() twice. Get the buffer size needed in
> >> the first round. malloc() a buffer. Then actually read the variable.
> >> Finally free() the buffer.
> > 
> > No.
> > "BootNext" is always a 16-bit integer, and "bootnext" is a u16 variable,
> > not a pointer.
> > So we don't need to call get_variable() twice. That is why I didn't use
> > get_var() here. I believe that you seem to like "code efficiency."
> 
> I see.
> 
> > 
> >>>  	efi_uintn_t size;
> >>>  	void *image = NULL;
> >>>  	int i, num;
> >>> +	efi_status_t ret;
> >>>  
> >>>  	bs = systab.boottime;
> >>>  	rs = systab.runtime;
> >>>  
> >>> +	/* BootNext */
> >>> +	size = sizeof(bootnext);
> >>> +	ret = EFI_CALL(efi_get_variable(L"BootNext",
> >>> +					(efi_guid_t *)&efi_global_variable_guid,
> >>> +					NULL, &size, &bootnext));
> >>> +	if (ret == EFI_SUCCESS) {
> 
> As we expect the variable to have size 2, we should check the size field
> too.

Here we see EFI_SUCCESS only if the size of variable is 1 or 2.
In case of size of 1, it's not in correct format, but I think
that it's safe and acceptable.
So basically I don't think that we need check the size.
(except for a message below)

> >>
> >> The expected value of ret for an existing variable of size > 0 is
> >> EFI_BUFFER_TOO_SMALL.
> 
> Now let's assume that the variable has been created with the wrong size
> (e.g. using `env set -e`). In that case we should either try to delete
> it or write an error message or both.

I simply want to write a message.

> >>
> >>> +		/* delete BootNext */
> >>> +		ret = EFI_CALL(efi_set_variable(
> >>> +					L"BootNext",
> >>> +					(efi_guid_t *)&efi_global_variable_guid,
> >>> +					0, 0, &bootnext));
> >>> +		if (ret == EFI_SUCCESS) {
> >>
> >> Why would loading the boot entry depend on the return value here?
> > 
> > Deleting "BootNex" is required by UEFI spec.
> > In addition, if we fail to delete it, we will see the same application
> > start again when executing boot manager next time.
> > 
> 
> BootNext is a non-volatile variable. Once we have a non-volatile backend
> for variables the call may fail because the the NV storage is not
> writable. Another case leading to an error would be the variable having
> been created with EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS
> (which we do not yet support).

Since we have no idea how we will manage "non-volatile" variables
in UEFI U-Boot, it is still early to discuss any behavior based on
not-agreed assumptions.

For example, if NV storage is not writable, we will probably not be
able to define BootNext variable.

> Should this stop the boot process? If yes, we need at least an error
> message. But I would propose that if the NV storage is not writable we
> continue booting after writing a debug message.

I'm afraid that this can be used as a DoS attack.
Anyhow, we should be "conservative" :)

-Takahiro Akashi


> Best regards
> 
> Heinrich
> 
> > Thanks,
> > -Takahiro Akashi
> > 
> >>> +			image = try_load_entry(bootnext,
> >>
> >> Best regards
> >>
> >> Heinrich
> >>
> >>> +					       device_path, file_path);
> >>> +			if (image)
> >>> +				goto error;
> >>> +		}
> >>> +	}
> >>> +
> >>> +	/* BootOrder */
> >>>  	bootorder = get_var(L"BootOrder", &efi_global_variable_guid, &size);
> >>>  	if (!bootorder) {
> >>>  		printf("BootOrder not defined\n");
> >>>
> >>
> > 
>