Re: [PATCH v2 2/3] efi_loader: bootmgr: fix a problem in loading an image from a short-path
Heinrich Schuchardt
xypron.glpk at gmx.de
Thu May 12 21:01:42 CEST 2022
Am 12. Mai 2022 09:25:46 MESZ schrieb AKASHI Takahiro <takahiro.akashi at linaro.org>:
>On Thu, May 12, 2022 at 08:50:34AM +0200, Heinrich Schuchardt wrote:
>> 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.
>
>I don't think we need a description here because, as you can see,
>__try_load() is an inner function and intended to be called only from
>try_load_from_short_path(). The purpose of extracting it is to avoid
>a duplicated code to go though a list of handles *twice*,
>for removable media and for fixed media.
>
>The description of try_load_from_short_path() covers both.
Please, provide a description to let me accept the patch.
Best regards
Heinrich
>
>> > +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.
>
>Actually, not.
>The original dp comes from "handler->protocol_interface" and it should
>exist while the associated handle, (fs_handles[i]), exists.
>
>-Takahiro Akashi
>
>>
>> > + 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);
More information about the U-Boot
mailing list