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

[Heinrich Schuchardt]

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