Hi Heinrich,
On Mon, 9 May 2022 at 22:01, Heinrich Schuchardt <xypron.g...@gmx.de> wrote:
>
> On 5/9/22 10:23, Masahisa Kojima wrote:
> > On Fri, 29 Apr 2022 at 01:53, Heinrich Schuchardt <xypron.g...@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.koj...@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.
tmp is used for whole efi_load_option structure comparison,
so I need to save the original size.
Thanks,
Masahisa Kojima
>
> 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;
> >>> +}
> >>
>
