Re: [PATCH v4 10/11] bootmenu: add removable media entries

[Masahisa Kojima <masahisa.kojima@linaro.org>]

On Thu, 31 Mar 2022 at 17:48, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> On Thu, Mar 24, 2022 at 10:54:42PM +0900, Masahisa Kojima wrote:
> > UEFI specification requires booting from removal media using
> > a architecture-specific default image name such as BOOTAA64.EFI.
> > This commit adds the removable media entries into bootmenu,
> > so that user can select the removable media and boot with
> > default image.
> >
> > The bootmenu automatically enumerates the possible bootable
> > media devices supporting EFI_SIMPLE_FILE_SYSTEM_PROTOCOL,
> > add it as new UEFI boot option(BOOT####) and update BootOrder
> > variable. Depending on the system hardware setup, some devices
> > may not exist at a later system boot, so invalid BOOT#### variable
> > must be deleted.
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > ---
> > Newly created in v4
> >
> >  cmd/bootmenu.c                            |  78 +++++++++++++
> >  include/efi_loader.h                      |  22 ++++
> >  lib/efi_loader/efi_bootmenu_maintenance.c | 128 ++++++++++++++++++++++
> >  3 files changed, 228 insertions(+)
> >
> > diff --git a/cmd/bootmenu.c b/cmd/bootmenu.c
> > index 458eb90b63..7357cfeae5 100644
> > --- a/cmd/bootmenu.c
> > +++ b/cmd/bootmenu.c
> > @@ -304,6 +304,74 @@ static int prepare_uefi_bootorder_entry(struct bootmenu_data *menu,
> >       return 1;
> >  }
> >
> > +static efi_status_t prepare_media_device_entry(void)
> > +{
> > +     u32 i;
> > +     efi_status_t ret;
> > +     efi_uintn_t count;
> > +     efi_handle_t *volume_handles = NULL;
> > +     struct efi_bootmenu_media_boot_option *opt = NULL;
> > +
> > +     ret = efi_locate_handle_buffer_int(BY_PROTOCOL, &efi_simple_file_system_protocol_guid,
> > +                                        NULL, &count, (efi_handle_t **)&volume_handles);
> > +     if (ret != EFI_SUCCESS)
> > +             return ret;
> > +
> > +     opt = calloc(count, sizeof(struct efi_bootmenu_media_boot_option));
> > +     if (!opt)
> > +             goto out;
> > +
> > +     /* enumerate all devices supporting EFI_SIMPLE_FILE_SYSTEM_PROTOCOL */
> > +     ret = efi_bootmenu_enumerate_boot_option(opt, volume_handles, count);
> > +     if (ret != EFI_SUCCESS)
> > +             goto out;
> > +
> > +     /*
> > +      * System hardware configuration may vary depending on the user setup.
> > +      * The boot option is automatically added by the bootmenu.
> > +      * If the device is not attached to the system, the boot option needs
> > +      * to be deleted.
> > +      */
> > +     ret = efi_bootmenu_delete_invalid_boot_option(opt, count);
>
> Won't that fail on first boot if the BootOrder is undefined?
> efi_bootmenu_delete_invalid_boot_option() should return EFI_SUCCESS if it
> doesn't have a boot order maybe?

You are correct.
Returning EFI_SUCCESS if there is no BootOrder defined will resolve your issue.

>
> > +     if (ret != EFI_SUCCESS)
> > +             goto out;
> > +
> > +     /* add non-existent boot option */
> > +     for (i = 0; i < count; i++) {
> > +             u32 boot_index;
> > +             u16 var_name[9];
> > +
> > +             if (!opt[i].exist) {
> > +                     ret = efi_bootmenu_get_unused_bootoption(var_name, sizeof(var_name),
> > +                                                              &boot_index);
> > +                     if (ret != EFI_SUCCESS)
> > +                             goto out;
> > +
> > +                     ret = efi_set_variable_int(var_name, &efi_global_variable_guid,
> > +                                                EFI_VARIABLE_NON_VOLATILE |
> > +                                                EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > +                                                EFI_VARIABLE_RUNTIME_ACCESS,
> > +                                                opt[i].size, opt[i].lo, false);
> > +                     if (ret != EFI_SUCCESS)
> > +                             goto out;
> > +
> > +                     ret = efi_bootmenu_append_bootorder(boot_index);
> > +                     if (ret != EFI_SUCCESS)
> > +                             goto out;
> > +             }
> > +     }
> > +
> > +out:
> > +     if (opt) {
> > +             for (i = 0; i < count; i++)
> > +                     free(opt[i].lo);
> > +     }
> > +     free(opt);
> > +     efi_free_pool(volume_handles);
> > +
> > +     return ret;
> > +}
> > +
> >  static int prepare_distro_boot_entry(struct bootmenu_data *menu,
> >                                    struct bootmenu_entry **current,
> >                                    unsigned short int *index)
> > @@ -408,6 +476,16 @@ static struct bootmenu_data *bootmenu_create(int delay)
> >               goto cleanup;
> >
> >       if (IS_ENABLED(CONFIG_CMD_BOOTEFI_BOOTMGR)) {
> > +             if (i < MAX_DYNAMIC_ENTRY) {
> > +                     /*
> > +                      * UEFI specification requires booting from removal media using
> > +                      * a architecture-specific default image name such as BOOTAA64.EFI.
> > +                      */
> > +                     ret = prepare_media_device_entry();
> > +                     if (ret != EFI_SUCCESS)
> > +                             goto cleanup;
> > +             }
> > +
> >               if (i < MAX_DYNAMIC_ENTRY) {
> >                       ret = prepare_uefi_bootorder_entry(menu, &iter, &i);
> >                       if (ret < 0 && ret != -ENOENT)
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index 7b100ca030..c326dfdaf2 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -926,6 +926,22 @@ struct efi_signature_store {
> >  struct x509_certificate;
> >  struct pkcs7_message;
> >
> > +/**
> > + * struct efi_bootmenu_media_boot_option - boot option for (removable) media device
> > + *
> > + * This structure is used to enumerate possible boot option
> > + *
> > + * @lo:              Serialized load option data
> > + * @size:    Size of serialized load option data
> > + * @exist:   Flag to indicate the load option already exists
> > + *           in Non-volatile load option
> > + */
> > +struct efi_bootmenu_media_boot_option {
> > +     void *lo;
> > +     efi_uintn_t size;
> > +     bool exist;
> > +};
> > +
> >  bool efi_signature_lookup_digest(struct efi_image_regions *regs,
> >                                struct efi_signature_store *db,
> >                                bool dbx);
> > @@ -1038,6 +1054,12 @@ efi_status_t efi_console_get_u16_string
> >  efi_status_t efi_bootmenu_get_unused_bootoption(u16 *buf,
> >                                               efi_uintn_t buf_size, u32 *index);
> >  efi_status_t efi_bootmenu_append_bootorder(u16 index);
> > +efi_status_t efi_bootmenu_enumerate_boot_option(
> > +                     struct efi_bootmenu_media_boot_option *opt,
> > +                     efi_handle_t *volume_handles, efi_status_t count);
> > +efi_status_t efi_bootmenu_delete_invalid_boot_option(
> > +                     struct efi_bootmenu_media_boot_option *opt,
> > +                     efi_status_t count);
> >
> >  efi_status_t efi_disk_get_device_name(struct efi_block_io *this, char *buf, int size);
> >
> > diff --git a/lib/efi_loader/efi_bootmenu_maintenance.c b/lib/efi_loader/efi_bootmenu_maintenance.c
> > index 2ce5a47b10..3a64cf767a 100644
> > --- a/lib/efi_loader/efi_bootmenu_maintenance.c
> > +++ b/lib/efi_loader/efi_bootmenu_maintenance.c
> > @@ -24,6 +24,8 @@ static struct efi_simple_text_output_protocol *cout;
> >  #define EFI_BOOTMENU_FILE_PATH_MAX 512
> >  #define EFI_BOOTMENU_BOOT_NAME_MAX 32
> >  #define EFI_BOOT_ORDER_MAX_SIZE_IN_DECIMAL 6
> > +#define BOOTMENU_AUTO_ADD_OPTION_DATA "bootmenu"
> > +#define BOOTMENU_AUTO_ADD_OPTION_DATA_U16 u"bootmenu"
> >
> >  typedef efi_status_t (*efi_bootmenu_entry_func)(void *data, bool *exit);
> >
> > @@ -1114,3 +1116,129 @@ efi_status_t efi_bootmenu_show_maintenance_menu(void)
> >                                         ARRAY_SIZE(maintenance_menu_items),
> >                                         -1);
> >  }
> > +
> > +efi_status_t efi_bootmenu_enumerate_boot_option(
> > +                     struct efi_bootmenu_media_boot_option *opt,
> > +                     efi_handle_t *volume_handles, efi_status_t count)
> > +{
> > +     u32 i;
> > +     struct efi_handler *handler;
> > +     efi_status_t ret = EFI_SUCCESS;
> > +
> > +     for (i = 0; i < count; i++) {
> > +             u16 *dev_name, *p;
> > +             struct efi_load_option lo;
> > +             struct efi_block_io *block_io;
> > +             char buf[BOOTMENU_DEVICE_NAME_MAX];
> > +             struct efi_device_path *device_path;
> > +
> > +             ret = efi_search_protocol(volume_handles[i], &efi_guid_device_path, &handler);
> > +             if (ret != EFI_SUCCESS)
> > +                     continue;
> > +             ret = efi_protocol_open(handler, (void **)&device_path,
> > +                                     efi_root, NULL, EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> > +             if (ret != EFI_SUCCESS)
> > +                     continue;
> > +
> > +             ret = efi_search_protocol(volume_handles[i], &efi_block_io_guid, &handler);
> > +             if (ret != EFI_SUCCESS)
> > +                     continue;
> > +             ret = efi_protocol_open(handler, (void **)&block_io,
> > +                                     efi_root, NULL, EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> > +             if (ret != EFI_SUCCESS)
> > +                     continue;
> > +
> > +             efi_disk_get_device_name(block_io, buf, BOOTMENU_DEVICE_NAME_MAX);
> > +             dev_name = calloc(1, (strlen(buf) + 1) * sizeof(u16));
> > +             if (!dev_name) {
> > +                     ret = EFI_OUT_OF_RESOURCES;
> > +                     goto out;
> > +             }
> > +             p = dev_name;
> > +             utf8_utf16_strncpy(&p, buf, strlen(buf));
> > +
> > +             lo.label = dev_name;
> > +             lo.attributes = LOAD_OPTION_ACTIVE;
> > +             lo.file_path = device_path;
> > +             lo.file_path_length = efi_dp_size(device_path) + sizeof(END);
> > +             /*
> > +              * To identify the boot option that automatically added by
> > +              * the bootmenu, optional_data has special string.
> > +              * optional_data will be converted into u16 string
> > +              * in efi_serialize_load_option().
> > +              */
> > +             lo.optional_data = BOOTMENU_AUTO_ADD_OPTION_DATA;
> > +             opt[i].size = efi_serialize_load_option(&lo, (u8 **)&opt[i].lo);
> > +             if (!opt[i].size) {
> > +                     ret = EFI_OUT_OF_RESOURCES;
> > +                     free(dev_name);
> > +                     goto out;
> > +             }
> > +             free(dev_name);
> > +     }
> > +
> > +out:
> > +     return ret;
> > +}
> > +
> > +efi_status_t efi_bootmenu_delete_invalid_boot_option(
> > +                     struct efi_bootmenu_media_boot_option *opt,
> > +                     efi_status_t count)
> > +{
> > +     u16 *bootorder;
> > +     u32 i, j;
> > +     efi_status_t ret;
> > +     efi_uintn_t num, size, bootorder_size;
> > +     void *load_option;
> > +     struct efi_load_option lo;
> > +     u16 varname[] = u"Boot####";
> > +
> > +     bootorder = efi_get_var(u"BootOrder", &efi_global_variable_guid, &bootorder_size);
> > +     if (!bootorder)
> > +             return EFI_NOT_FOUND;
> > +
> > +     num = bootorder_size / sizeof(u16);
> > +     for (i = 0; i < num;) {
> > +             efi_uintn_t tmp;
> > +
> > +             efi_create_indexed_name(varname, sizeof(varname),
> > +                                     "Boot", bootorder[i]);
> > +             load_option = efi_get_var(varname, &efi_global_variable_guid, &size);
> > +             if (!load_option)
> > +                     goto next;
> > +
> > +             tmp = size;
> > +             ret = efi_deserialize_load_option(&lo, load_option, &tmp);
> > +             if (ret != EFI_SUCCESS)
> > +                     goto next;
> > +
> > +             if (u16_strncmp((u16 *)lo.optional_data, BOOTMENU_AUTO_ADD_OPTION_DATA_U16,
>
> You should convert the lo.optional_data to a u16 before doing the check
> here, other wise this might cause an unaligned access

Yes, unaligned access might occur.
I need to copy lo.optional_data to the allocated buffer first, then compare it.

Thanks,
Masahisa Kojima



>
> > +                             u16_strlen(BOOTMENU_AUTO_ADD_OPTION_DATA_U16)) == 0) {
> > +                     for (j = 0; j < count; j++) {
> > +                             if (memcmp(opt[j].lo, load_option, size) == 0) {
> > +                                     opt[j].exist = true;
> > +                                     break;
> > +                             }
> > +                     }
> > +
> > +                     if (j == count) {
> > +                             ret = delete_boot_option(bootorder, i, bootorder_size);
> > +                             if (ret != EFI_SUCCESS) {
> > +                                     free(load_option);
> > +                                     goto out;
> > +                             }
> > +
> > +                             num--;
> > +                             bootorder_size -= sizeof(u16);
> > +                             free(load_option);
> > +                             continue;
> > +                     }
> > +             }
> > +next:
> > +             free(load_option);
> > +             i++;
> > +     }
> > +
> > +out:
> > +     return ret;
> > +}
> > --
> > 2.17.1
> >
>
> Thanks
> /Ilias