[PATCH] efi_loader: fix efi_bootmgr_enumerate_boot_option

[Ilias Apalodimas]

Hi Mark,

On Tue, 17 Oct 2023 at 22:25, Mark Kettenis <mark.kettenis at xs4all.nl> wrote:
>
> > From: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
> > Date: Tue, 17 Oct 2023 19:51:48 +0200
> >
> > Only auto-generate boot options for removable media.
> >
> > For testing the different handling of removable and non-removable devices
> > an emulated USB device can be used:
> >
> >     make qemu-riscv64_smode_defconfig; make
> >     qemu-system-riscv64 -M virt -kernel u-boot.bin -nographic \
> >     -device qemu-xhci -drive if=none,file=disk.img,format=raw,id=USB1 \
> >     -device usb-storage,drive=USB1,removable=on
>
> Unless I misunderstand what this patch does, this may result in
> systems that don't boot after installation.
>
> On most boards U-Boot does not provide a way to create persistent EFI
> variables after ExitBootServices() is called.  This means that an OS
> installer cannot create a boot entry for the OS installation it just
> created.  Therefore the EFI boot manager will not boot the newly
> installed system.
>
> Now as long as the traditional distro boot mechanism or the EFI distro
> boot bootmeth is enabled the system will probobly still attempt to
> boot the installed system.  But do want to rely on that?

No, we don't.  We already discussed this with Heinrich (offline) and I
think we should not merge that.  The EFI spec doesn't limit the boot
manager to removable entries only. In [0] it mentions

"if the boot image is not loaded via EFI_BOOT_SERVICES.LoadImage() the
boot manager is required to check for a default application to boot.
Searching for a default application to boot happens on ** both
removable and fixed media types**. This search occurs when the device
path of the boot image listed in any boot option points directly to an
EFI_SIMPLE_FILE_SYSTEM_PROTOCOL device and does not specify the exact
file to load. The file discovery method is explained in Boot Option
Recovery. The default media boot case of a protocol other than
EFI_SIMPLE_FILE_SYSTEM_PROTOCOL is handled by the
EFI_LOAD_FILE_PROTOCOL for the target device path and does not need to
be handled by the boot manager."

So scanning both is required.

[0] https://uefi.org/specs/UEFI/2.10/03_Boot_Manager.html#load-option-processing

Regards
/Ilias
>
> > Fixes: c416f1c0bcab ("bootmenu: add removable media entries")
> > Fixes: 339b527bd450 ("Move bootorder and bootoption apis to lib")
> > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
> > ---
> >  lib/efi_loader/efi_bootmgr.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> > index a40762c74c..96d45157b3 100644
> > --- a/lib/efi_loader/efi_bootmgr.c
> > +++ b/lib/efi_loader/efi_bootmgr.c
> > @@ -346,7 +346,7 @@ error:
> >  }
> >
> >  /**
> > - * efi_bootmgr_enumerate_boot_option() - enumerate the possible bootable media
> > + * efi_bootmgr_enumerate_boot_option() - boot options for removable boot media
> >   *
> >   * @opt:             pointer to the media boot option structure
> >   * @volume_handles:  pointer to the efi handles
> > @@ -370,6 +370,9 @@ static efi_status_t efi_bootmgr_enumerate_boot_option(struct eficonfig_media_boo
> >               struct efi_device_path *device_path;
> >               struct efi_device_path *short_dp;
> >
> > +             if (!efi_disk_is_removable(volume_handles[i]))
> > +                     continue;
> > +
> >               ret = efi_search_protocol(volume_handles[i], &efi_guid_device_path, &handler);
> >               if (ret != EFI_SUCCESS)
> >                       continue;
> > @@ -674,7 +677,10 @@ efi_status_t efi_bootmgr_update_media_device_boot_option(void)
> >               goto out;
> >       }
> >
> > -     /* enumerate all devices supporting EFI_SIMPLE_FILE_SYSTEM_PROTOCOL */
> > +     /*
> > +      * Enumerate all removable devices supporting the
> > +      * EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.
> > +      */
> >       ret = efi_bootmgr_enumerate_boot_option(opt, volume_handles, count);
> >       if (ret != EFI_SUCCESS)
> >               goto out;
> > --
> > 2.40.1
> >
> >