Re: [PATCH] efi_loader: capsule: detect possible ESP device path

On 5/8/24 14:04, Dan Carpenter wrote:

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.

Errors should be handled and not ignored.

...

• 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.

ditto

Best regards

Heinrich

...

• 		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