[PATCH] efi_loader: capsule: detect possible ESP device path
Dan Carpenter
dan.carpenter at linaro.org
Wed May 8 14:04:32 CEST 2024
On Wed, May 08, 2024 at 07:24:01PM +0800, Weizhao Ouyang wrote:
> diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> index de0d49ebebda..919e3cba071b 100644
> --- a/lib/efi_loader/efi_capsule.c
> +++ b/lib/efi_loader/efi_capsule.c
> @@ -922,6 +922,105 @@ static bool device_is_present_and_system_part(struct efi_device_path *dp)
> return true;
> }
>
> +/**
> + * get_esp_from_boot_option_file_path - get the expand device path
> + *
> + * Get a possible efi system partition by expanding a boot option
> + * file path.
> + *
> + * @boot_dev The device path pointing to a boot option
> + * Return: The full ESP device path or NULL if fail
> + */
> +static struct efi_device_path *get_esp_from_boot_option_file_path(struct efi_device_path *boot_dev)
> +{
> + efi_status_t ret = EFI_SUCCESS;
> + efi_handle_t handle;
> + struct efi_device_path *dp = boot_dev;
> + struct efi_device_path *full_path = NULL;
> +
> + ret = EFI_CALL(efi_locate_device_path(&efi_simple_file_system_protocol_guid,
> + &dp,
> + &handle));
> + if (ret != EFI_SUCCESS)
> + ret = EFI_CALL(efi_locate_device_path(&efi_block_io_guid, &dp, &handle));
> +
> + /* Expand Media Device Path */
> + if (ret == EFI_SUCCESS && EFI_DP_TYPE(dp, END, END)) {
Flip this around. Always do failure handling. Never do success
handling. full_path is always NULL. Just return that. Delete
the variable.
if (ret != EFI_SUCCESS ||
!EFI_DP_TYPE(dp, END, END))
return NULL;
> + struct efi_device_path *temp_dp;
> + struct efi_block_io *block_io;
> + void *buffer;
> + efi_handle_t *simple_file_system_handle;
> + efi_uintn_t number_handles, index;
> + u32 size;
> + u32 temp_size;
> +
> + temp_dp = boot_dev;
> + ret = EFI_CALL(efi_locate_device_path(&efi_simple_file_system_protocol_guid,
> + &temp_dp,
> + &handle));
> + /*
> + * For device path pointing to simple file system, it only expands to one
> + * full path
> + */
> + if (ret == EFI_SUCCESS && EFI_DP_TYPE(temp_dp, END, END)) {
"Always" was hyperbole. This success handling is fine.
> + if (device_is_present_and_system_part(temp_dp))
> + return temp_dp;
> + }
> +
> + /*
> + * For device path only pointing to the removable device handle, try to
> + * search all its children handles
> + */
> + ret = EFI_CALL(efi_locate_device_path(&efi_block_io_guid, &temp_dp, &handle));
ret is not used. Delete.
> + EFI_CALL(efi_connect_controller(handle, NULL, NULL, true));
> +
> + /* Align with edk2, issue a dummy read to the device to check the device change */
> + ret = EFI_CALL(efi_handle_protocol(handle, &efi_block_io_guid, (void **)&block_io));
> + if (ret == EFI_SUCCESS) {
if (ret != EFI_SUCCESS)
return NULL;
> + buffer = memalign(block_io->media->io_align, block_io->media->block_size);
> + if (buffer) {
if (!buffer)
return NULL;
> + ret = EFI_CALL(block_io->read_blocks(block_io,
> + block_io->media->media_id,
> + 0,
> + block_io->media->block_size,
> + buffer));
Delete the unused "ret = " assignment.
> + free(buffer);
> + } else {
> + return full_path;
> + }
> + } else {
> + return full_path;
> + }
> +
> + /* detect the default boot file from removable media */
> + size = efi_dp_size(boot_dev) - sizeof(struct efi_device_path);
> + EFI_CALL(efi_locate_handle_buffer(BY_PROTOCOL,
> + &efi_simple_file_system_protocol_guid,
> + NULL,
> + &number_handles,
> + &simple_file_system_handle));
> + for (index = 0; index < number_handles; index++) {
> + EFI_CALL(efi_handle_protocol(simple_file_system_handle[index],
> + &efi_guid_device_path,
> + (void **)&temp_dp));
> +
> + log_debug("Search ESP %pD\n", temp_dp);
> + temp_size = efi_dp_size(temp_dp) - sizeof(struct efi_device_path);
> + if (size <= temp_size && !memcmp(temp_dp, boot_dev, size)) {
> + if (device_is_present_and_system_part(temp_dp)) {
You could combine these conditions:
if (size <= temp_size &&
memcmp(temp_dp, boot_dev, size) == 0 &&
device_is_present_and_system_part(temp_dp)) {
efi_free_pool(simple_file_system_handle);
return temp_dp;
}
> + efi_free_pool(simple_file_system_handle);
> + return temp_dp;
> + }
> + }
> + }
> +
> + if (simple_file_system_handle)
> + efi_free_pool(simple_file_system_handle);
> + }
> +
> + return full_path;
> +}
> +
> /**
> * find_boot_device - identify the boot device
> *
> @@ -938,6 +1037,7 @@ static efi_status_t find_boot_device(void)
> int i, num;
> struct efi_simple_file_system_protocol *volume;
> struct efi_device_path *boot_dev = NULL;
> + struct efi_device_path *full_path = NULL;
No need to initialize this to NULL, it disables static checker warnings
for uninitialized variables so it can lead to bugs.
> efi_status_t ret;
>
> /* find active boot device in BootNext */
> @@ -961,8 +1061,14 @@ static efi_status_t find_boot_device(void)
> if (device_is_present_and_system_part(boot_dev)) {
> goto found;
> } else {
> - efi_free_pool(boot_dev);
> - boot_dev = NULL;
> + full_path = get_esp_from_boot_option_file_path(boot_dev);
> + if (full_path) {
> + boot_dev = full_path;
Later, we free full_path but do we ever free the original boot_dev?
> + goto found;
> + } else {
> + efi_free_pool(boot_dev);
> + boot_dev = NULL;
> + }
Better to delete indenting where you can:
full_path = get_esp_from_boot_option_file_path(boot_dev);
if (full_path) {
boot_dev = full_path;
goto found;
}
efi_free_pool(boot_dev);
boot_dev = NULL;
regards,
dan carpenter
More information about the U-Boot
mailing list