[PATCH] efi_loader: fix a problem in loading an image from a short-path

Heinrich Schuchardt xypron.glpk at gmx.de
Fri Apr 29 12:57:15 CEST 2022


On 4/28/22 06:50, AKASHI Takahiro wrote:
> Booting from a short-form device path which starts with the first element
> being a File Path Media Device Path failed because it doesn't contain
> any valid device with simple file system protocol and efi_dp_find_obj()
> in efi_load_image_from_path() will return NULL.
> For instance,
> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(0,0)/\helloworld.efi
>    -> shortened version: /\helloworld.efi

Thanks for enabling this.

Please, enhance test/py/tests/test_efi_bootmgr to test booting from a
filename only device path.

>
> With this patch applied, all the media devices with simple file system
> protocol are enumerated and the boot manager attempts to boot temporarily
> generated device paths one-by-one.
>
> This new implementation is still a bit incompatible with the UEFI
> specification in terms of:
> * not creating real boot options

The UEFI specification has:

"These new boot options must not be saved to non volatile storage,
and may not be added to BootOrder."

Is there really something missing?

> * not distinguishing removable media and fix media

struct blk_desc has field 'removable' which is mirrored in struct
efi_disk_obj.

mmc_bind() always sets removable to 1 irrespective of the device being
eMMC or SD-card. I guess this would need to be fixed.

> (See section 3.1.3 "Boot Options".)
>
> But it still gives us a closer and better solution than the current.
>
> Fixes: commit 9cdf470274ff ("efi_loader: support booting via short-form device-path")
> Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
> ---
>   include/efi_loader.h          |  3 ++
>   lib/efi_loader/efi_boottime.c | 87 +++++++++++++++++++++++++++++++----
>   lib/efi_loader/efi_file.c     | 35 ++++++++++----
>   3 files changed, 107 insertions(+), 18 deletions(-)
>
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index ba79a9afb404..9730c1375a55 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -661,6 +661,9 @@ void efi_signal_event(struct efi_event *event);
>   struct efi_simple_file_system_protocol *efi_simple_file_system(
>   		struct blk_desc *desc, int part, struct efi_device_path *dp);
>
> +/* open file from simple file system */
> +struct efi_file_handle *efi_file_from_fs(struct efi_simple_file_system_protocol *v,
> +					 struct efi_device_path *fp);
>   /* open file from device-path: */
>   struct efi_file_handle *efi_file_from_path(struct efi_device_path *fp);
>
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index 5bcb8253edba..39b0e8f7ade0 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -1868,19 +1868,21 @@ out:
>   }
>
>   /**
> - * efi_load_image_from_file() - load an image from file system
> + * __efi_load_image_from_file() - load an image from file system
>    *
>    * Read a file into a buffer allocated as EFI_BOOT_SERVICES_DATA. It is the
>    * callers obligation to update the memory type as needed.
>    *
> + * @v:			simple file system

Please, use a meaningful variable name.

>    * @file_path:		the path of the image to load
>    * @buffer:		buffer containing the loaded image
>    * @size:		size of the loaded image
>    * Return:		status code
>    */
>   static
> -efi_status_t efi_load_image_from_file(struct efi_device_path *file_path,
> -				      void **buffer, efi_uintn_t *size)
> +efi_status_t __efi_load_image_from_file(struct efi_simple_file_system_protocol *v,
> +					struct efi_device_path *file_path,
> +					void **buffer, efi_uintn_t *size)
>   {
>   	struct efi_file_handle *f;
>   	efi_status_t ret;
> @@ -1888,7 +1890,11 @@ efi_status_t efi_load_image_from_file(struct efi_device_path *file_path,
>   	efi_uintn_t bs;
>
>   	/* Open file */
> -	f = efi_file_from_path(file_path);
> +	if (v)
> +		f = efi_file_from_fs(v, file_path);
> +	else
> +		/* file_path should have a device path */
> +		f = efi_file_from_path(file_path);
>   	if (!f)
>   		return EFI_NOT_FOUND;
>
> @@ -1921,6 +1927,64 @@ error:
>   	return ret;
>   }
>
> +/**
> + * efi_load_image_from_file() - load an image from file system
> + *
> + * Read a file into a buffer allocated as EFI_BOOT_SERVICES_DATA. It is the
> + * callers obligation to update the memory type as needed.
> + *
> + * @file_path:		the path of the image to load
> + * @buffer:		buffer containing the loaded image
> + * @size:		size of the loaded image
> + * Return:		status code
> + */
> +static
> +efi_status_t efi_load_image_from_file(struct efi_device_path *file_path,
> +				      void **buffer, efi_uintn_t *size)
> +{
> +	efi_uintn_t no_handles;
> +	efi_handle_t *handles;
> +	struct efi_handler *handler;
> +	struct efi_simple_file_system_protocol *fs;
> +	int i;
> +	efi_status_t ret;
> +
> +	/* if a file_path contains a device path */
> +	if (!EFI_DP_TYPE(file_path, MEDIA_DEVICE, FILE_PATH))
> +		return __efi_load_image_from_file(NULL, file_path, buffer, size);
> +
> +	/* no explicit device specified */
> +	ret = EFI_CALL(efi_locate_handle_buffer(
> +					BY_PROTOCOL,
> +					&efi_simple_file_system_protocol_guid,
> +					NULL,
> +					&no_handles,
> +					&handles));
> +	if (ret != EFI_SUCCESS)
> +		return ret;
> +	if (!no_handles)
> +		return EFI_NOT_FOUND;
> +
> +	for (i = 0; i < no_handles; i++) {
> +		ret = efi_search_protocol(handles[i],
> +					  &efi_simple_file_system_protocol_guid,
> +					  &handler);
> +		if (ret != EFI_SUCCESS)
> +			/* unlikely */
> +			continue;
> +
> +		fs = handler->protocol_interface;
> +		if (!fs)
> +			continue;
> +
> +		ret = __efi_load_image_from_file(fs, file_path, buffer, size);
> +		if (ret == EFI_SUCCESS)
> +			return ret;
> +	}
> +
> +	return EFI_NOT_FOUND;
> +}
> +
>   /**
>    * efi_load_image_from_path() - load an image using a file path
>    *
> @@ -1940,7 +2004,7 @@ efi_status_t efi_load_image_from_path(bool boot_policy,
>   {
>   	efi_handle_t device;
>   	efi_status_t ret;
> -	struct efi_device_path *dp, *rem;
> +	struct efi_device_path *rem;
>   	struct efi_load_file_protocol *load_file_protocol = NULL;
>   	efi_uintn_t buffer_size;
>   	uint64_t addr, pages;
> @@ -1950,12 +2014,15 @@ efi_status_t efi_load_image_from_path(bool boot_policy,
>   	*buffer = NULL;
>   	*size = 0;
>
> -	dp = file_path;
> -	device = efi_dp_find_obj(dp, NULL, &rem);
> -	ret = efi_search_protocol(device, &efi_simple_file_system_protocol_guid,
> -				  NULL);
> +	/* try first for simple file system protocols */
> +	ret = efi_load_image_from_file(file_path, buffer, size);
>   	if (ret == EFI_SUCCESS)
> -		return efi_load_image_from_file(file_path, buffer, size);
> +		return ret;
> +
> +	/* TODO: does this really make sense? */
> +	device = efi_dp_find_obj(file_path, NULL, &rem);
> +	if (!device)
> +		return EFI_NOT_FOUND;
>
>   	ret = efi_search_protocol(device, &efi_guid_load_file_protocol, NULL);
>   	if (ret == EFI_SUCCESS) {
> diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c
> index 7a7077e6d032..2d6a432b168b 100644
> --- a/lib/efi_loader/efi_file.c
> +++ b/lib/efi_loader/efi_file.c
> @@ -1083,16 +1083,16 @@ static const struct efi_file_handle efi_file_handle_protocol = {
>   /**
>    * efi_file_from_path() - open file via device path
>    *
> - * @fp:		device path
> + * @v:		simple file system
> + * @fp:		file path
>    * Return:	EFI_FILE_PROTOCOL for the file or NULL
>    */
> -struct efi_file_handle *efi_file_from_path(struct efi_device_path *fp)
> +struct efi_file_handle *efi_file_from_fs(struct efi_simple_file_system_protocol *v,
> +					 struct efi_device_path *fp)
>   {
> -	struct efi_simple_file_system_protocol *v;
>   	struct efi_file_handle *f;
>   	efi_status_t ret;
>
> -	v = efi_fs_from_path(fp);
>   	if (!v)
>   		return NULL;
>
> @@ -1100,10 +1100,6 @@ struct efi_file_handle *efi_file_from_path(struct efi_device_path *fp)
>   	if (ret != EFI_SUCCESS)
>   		return NULL;
>
> -	/* Skip over device-path nodes before the file path. */
> -	while (fp && !EFI_DP_TYPE(fp, MEDIA_DEVICE, FILE_PATH))
> -		fp = efi_dp_next(fp);
> -
>   	/*
>   	 * Step through the nodes of the directory path until the actual file
>   	 * node is reached which is the final node in the device path.
> @@ -1138,6 +1134,29 @@ struct efi_file_handle *efi_file_from_path(struct efi_device_path *fp)
>   	return f;
>   }
>
> +/**
> + * efi_file_from_path() - open file via device path
> + *
> + * @fp:		device path

'fp' does not resonate 'device path'. How about using dp?

> + * Return:	EFI_FILE_PROTOCOL for the file or NULL
> + */
> +struct efi_file_handle *efi_file_from_path(struct efi_device_path *fp)
> +{
> +	struct efi_simple_file_system_protocol *v;

Please, provide a meaningful variable name.

Best regards

Heinrich

> +
> +	v = efi_fs_from_path(fp);
> +	if (!v)
> +		return NULL;
> +
> +	/* Skip over device-path nodes before the file path. */
> +	while (fp && !EFI_DP_TYPE(fp, MEDIA_DEVICE, FILE_PATH))
> +		fp = efi_dp_next(fp);
> +	if (!fp)
> +		return NULL;
> +
> +	return efi_file_from_fs(v, fp);
> +}
> +
>   static efi_status_t EFIAPI
>   efi_open_volume(struct efi_simple_file_system_protocol *this,
>   		struct efi_file_handle **root)



More information about the U-Boot mailing list