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

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

On 3/31/22 10:48, Ilias Apalodimas 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?
>
>> +	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

Once we have initialized the EFI sub-system unaligned access is allowed.
But it may be slow. E.g. on RISC-V it may be emulated by the SBI.

Best regards

Heinrich

>
>> +				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