Re: [PATCH v5 16/17] bootmenu: add removable media entries

[Heinrich Schuchardt <xypron.glpk@gmx.de>]

On 5/9/22 10:23, Masahisa Kojima wrote:
> On Fri, 29 Apr 2022 at 01:53, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 4/28/22 10:09, 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. This automatically generated UEFI boot option
>>> has the dedicated guid in the optional_data to distinguish it from
>>> the UEFI boot option user adds manually.
>>>
>>> This commit also provides the BOOT#### variable maintenance feature.
>>> Depending on the system hardware setup, some devices
>>> may not exist at a later system boot, so bootmenu checks the
>>> available device in each bootmenu invocation and automatically
>>> removes the BOOT#### variable corrensponding to the non-existent
>>> media device.
>>>
>>> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
>>> ---
>>> Changes in v5:
>>> - Return EFI_SUCCESS if there is no BootOrder defined
>>> - correctly handle the case if no removable device found
>>> - use guid to identify the automatically generated entry by bootmenu
>>>
>>> Newly created in v4
>>>
>>>    cmd/bootmenu.c                            |  94 +++++++++++++++
>>>    include/efi_loader.h                      |  20 ++++
>>>    lib/efi_loader/efi_bootmenu_maintenance.c | 139 ++++++++++++++++++++++
>>>    3 files changed, 253 insertions(+)
>>>
>>> diff --git a/cmd/bootmenu.c b/cmd/bootmenu.c
>>> index 860cb83182..970db3ee01 100644
>>> --- a/cmd/bootmenu.c
>>> +++ b/cmd/bootmenu.c
>>> @@ -396,6 +396,89 @@ static int is_blk_device_available(char *token)
>>>        return -ENODEV;
>>>    }
>>>
>>> +/**
>>> + * prepare_media_device_entry() - generate the media device entries
>>> + *
>>> + * This function enumerates all devices supporting EFI_SIMPLE_FILE_SYSTEM_PROTOCOL
>>> + * and generate the bootmenu entries.
>>> + * This function also provide the BOOT#### variable maintenance for
>>> + * the media device entries.
>>> + *   - Automatically create the BOOT#### variable for the newly detected device,
>>> + *     this BOOT#### variable is distinguished by the special GUID
>>> + *     stored in the EFI_LOAD_OPTION.optional_data
>>> + *   - If the device is not attached to the system, the associated BOOT#### variable
>>> + *     is automatically deleted.
>>> + *
>>> + * Return:   status code
>>> + */
>>> +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);
>>> +     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;
>>> +}
>>> +
>>>    /**
>>>     * prepare_distro_boot_entry() - generate the distro boot entries
>>>     *
>>> @@ -500,6 +583,7 @@ static int prepare_distro_boot_entry(struct bootmenu_data *menu,
>>>    static struct bootmenu_data *bootmenu_create(int delay)
>>>    {
>>>        int ret;
>>> +     efi_status_t efi_ret;
>>>        unsigned short int i = 0;
>>>        struct bootmenu_data *menu;
>>>        struct bootmenu_entry *iter = NULL;
>>> @@ -523,6 +607,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.
>>> +                      */
>>> +                     efi_ret = prepare_media_device_entry();
>>> +                     if (efi_ret != EFI_SUCCESS && efi_ret != EFI_NOT_FOUND)
>>> +                             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 533618341b..bef492ccb9 100644
>>> --- a/include/efi_loader.h
>>> +++ b/include/efi_loader.h
>>> @@ -928,6 +928,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);
>>> @@ -1076,6 +1092,10 @@ 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 8c3f94c695..33b37fd11a 100644
>>> --- a/lib/efi_loader/efi_bootmenu_maintenance.c
>>> +++ b/lib/efi_loader/efi_bootmenu_maintenance.c
>>> @@ -26,6 +26,13 @@ static struct efi_simple_text_output_protocol *cout;
>>>    #define EFI_BOOTMENU_BOOT_NAME_MAX 32
>>>    #define EFI_BOOT_ORDER_MAX_SIZE_IN_DECIMAL 6
>>>
>>> +#define EFI_BOOTMENU_AUTO_GENERATED_ENTRY_GUID \
>>> +     EFI_GUID(0x38c1acc1, 0x9fc0, 0x41f0, \
>>> +              0xb9, 0x01, 0xfa, 0x74, 0xd6, 0xd6, 0xe4, 0xde)
>>> +
>>> +static const efi_guid_t efi_guid_bootmenu_auto_generated =
>>> +             EFI_BOOTMENU_AUTO_GENERATED_ENTRY_GUID;
>>> +
>>>    typedef efi_status_t (*efi_bootmenu_entry_func)(void *data, bool *exit);
>>>
>>>    /**
>>> @@ -1104,3 +1111,135 @@ 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++) {
>>> +             char *optional_data;
>>> +             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);
>>> +             /*
>>> +              * Set the dedicated guid to optional_data, it is used to identify
>>> +              * the boot option that automatically generated by the bootmenu.
>>> +              * efi_serialize_load_option() expects optional_data is null-terminated
>>> +              * utf8 string, so set the "dummystr" string to allocate enough space
>>> +              * to store guid, instead of realloc the load_option.
>>> +              *
>>> +              * This will allocate 16 bytes for guid plus trailing 0x0000.
>>> +              * The guid does not require trailing 0x0000, but it is for safety
>>> +              * in case some program handle the optional_data as u16 string.
>>> +              */
>>> +             lo.optional_data = "dummystr";
>>
>> Why do you want to reserve 18 bytes when 16 are enough for the GUID?
>
> OK, I will only allocate 16 bytes for GUID.
>
>>
>>> +             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;
>>> +             }
>>> +             /* set the guid */
>>> +             optional_data = (char *)opt[i].lo + (opt[i].size - u16_strsize(u"dummystr"));
>>> +             memcpy(optional_data, &efi_guid_bootmenu_auto_generated, sizeof(efi_guid_t));
>>> +             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_SUCCESS; /* BootOrder is not defined, nothing to do */
>>> +
>>> +     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;
>>
>> This copying of variables is superfluous. Just keep size.
>
> I don't catch your point.
> This efi_bootmenu_delete_invalid_boot_option() function
> does not copy the variable, only keeps the variable size.

There is no need to copy the value of variable size to a new variable
tmp. You can remove the variable tmp and continue using variable size
below as both variables are of the same type.

Best regards

Heinrich

>
>>
>>> +             ret = efi_deserialize_load_option(&lo, load_option, &tmp);
>>> +             if (ret != EFI_SUCCESS)
>>> +                     goto next;
>>> +
>>> +             if (guidcmp(lo.optional_data, &efi_guid_bootmenu_auto_generated) == 0) {
>>
>> You must avoid a buffer overrun. Check size >= 16.
>
> Yes, I need to check the size of optional_data.
>
> Thanks,
> Masahisa Kojima
>
>>
>> Best regards
>>
>> Heinrich
>>
>>> +                     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;
>>> +}
>>