[PATCH v2 2/3] efi_loader: bootmgr: fix a problem in loading an image from a short-path

[Heinrich Schuchardt]

On 5/12/22 04:29, 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
>
> 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
> * not try
>    "If a device does not support the EFI_SIMPLE_FILE_SYSTEM_PROTOCOL, but
>    supports the EFI_BLOCK_IO_PROTOCOL protocol, then the EFI Boot Service
>    ConnectController must be called for this device with DriverImageHandle
>    and RemainingDevicePath set to NULL and the Recursive flag is set to TRUE."

We already have the ConnectController() logic in lib/efi_driver/. We
just have to carry it to all block devices (in a future patch set).

> (See section 3.1.2 "Load Option Processing".)
>
> 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>
> ---
>   lib/efi_loader/efi_bootmgr.c | 98 ++++++++++++++++++++++++++++++++++--
>   1 file changed, 94 insertions(+), 4 deletions(-)
>
> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> index 631a25d76e9e..3608e433503e 100644
> --- a/lib/efi_loader/efi_bootmgr.c
> +++ b/lib/efi_loader/efi_bootmgr.c
> @@ -76,6 +76,91 @@ struct efi_device_path *expand_media_path(struct efi_device_path *device_path)
>   	return full_path;
>   }
>

The logic in this patch looks good to me. Just some nits:

Please, provide a description of the function and its parameters.

> +static efi_status_t __try_load(efi_handle_t *fs_handles, efi_uintn_t num,

Please, use a name not starting with underscores conveying what the
function is good for, e.g. efi_load_from_file_path().

> +			       struct efi_device_path *fp,
> +			       efi_handle_t *handle, bool removable)
> +{
> +	struct efi_handler *handler;
> +	struct efi_device_path *dp;
> +	int i;
> +	efi_status_t ret;
> +
> +	for (i = 0; i < num; i++) {
> +		if (removable && !efi_disk_is_removable(fs_handles[i]))
> +			continue;
> +		if (!removable && efi_disk_is_removable(fs_handles[i]))
> +			continue;
> +
> +		ret = efi_search_protocol(fs_handles[i], &efi_guid_device_path,
> +					  &handler);
> +		if (ret != EFI_SUCCESS)
> +			/* unlikely */

If you want to tell the compiler that this path is unlikely:

	        if (unlikely(ret != EFI_SUCCESS))

But as this code is not called often I would simply remove the comment.

> +			continue;
> +
> +		dp = handler->protocol_interface;
> +		if (!dp)
> +			/* unlikely */

ditto

> +			continue;
> +
> +		dp = efi_dp_append(dp, fp);

You are leaking the original dp here. Please, avoid memory leaks.

> +		if (!dp)
> +			/* unlikely */

if (unlikely( ?

> +			continue;
> +
> +		ret = EFI_CALL(efi_load_image(true, efi_root, dp, NULL, 0,
> +					      handle));
> +		efi_free_pool(dp);
> +		if (ret == EFI_SUCCESS)
> +			return ret;
> +	}
> +
> +	return EFI_NOT_FOUND;
> +}
> +
> +/**
> + * try_load_from_short_path
> + * @fp:		file path
> + * @handle:	pointer to handle for newly installed image
> + *
> + * Enumerate all the devices which support file system operations,
> + * prepend its media device path to the file path, @fp, and
> + * try to load the file.
> + * This function should be called when handling a short-form path
> + * which is starting with a file device path.
> + *
> + * Return:	status code
> + */
> +static efi_status_t try_load_from_short_path(struct efi_device_path *fp,
> +					     efi_handle_t *handle)
> +{
> +	efi_handle_t *fs_handles;
> +	efi_uintn_t num;
> +	efi_status_t ret;
> +
> +	ret = EFI_CALL(efi_locate_handle_buffer(
> +					BY_PROTOCOL,
> +					&efi_simple_file_system_protocol_guid,
> +					NULL,
> +					&num, &fs_handles));
> +	if (ret != EFI_SUCCESS)
> +		return ret;
> +	if (!num)
> +		return EFI_NOT_FOUND;
> +
> +	/* removable media first */
> +	ret = __try_load(fs_handles, num, fp, handle, true);
> +	if (ret == EFI_SUCCESS)
> +		goto out;
> +
> +	/* fixed media */
> +	ret = __try_load(fs_handles, num, fp, handle, false);
> +	if (ret == EFI_SUCCESS)
> +		goto out;
> +
> +out:
> +	return ret;
> +}
> +
>   /**
>    * try_load_entry() - try to load image for boot option
>    *
> @@ -116,10 +201,15 @@ static efi_status_t try_load_entry(u16 n, efi_handle_t *handle,
>   		log_debug("trying to load \"%ls\" from %pD\n", lo.label,
>   			  lo.file_path);
>
> -		file_path = expand_media_path(lo.file_path);
> -		ret = EFI_CALL(efi_load_image(true, efi_root, file_path,
> -					      NULL, 0, handle));
> -		efi_free_pool(file_path);
> +		if (EFI_DP_TYPE(lo.file_path, MEDIA_DEVICE, FILE_PATH)) {
> +			/* file_path doesn't contain a device path */

%s/device path/device part/

In UEFI speak a file_path is a device path.

Best regards

Heinrich

> +			ret = try_load_from_short_path(lo.file_path, handle);
> +		} else {
> +			file_path = expand_media_path(lo.file_path);
> +			ret = EFI_CALL(efi_load_image(true, efi_root, file_path,
> +						      NULL, 0, handle));
> +			efi_free_pool(file_path);
> +		}
>   		if (ret != EFI_SUCCESS) {
>   			log_warning("Loading %ls '%ls' failed\n",
>   				    varname, lo.label);