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.aka...@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");
> >>>
> >>
> >
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
