[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