Re: [PATCH v2 3/9] efi_loader: efi_dp_find_obj() add protocol check

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

On Sat, Mar 19, 2022 at 10:11:42AM +0100, Heinrich Schuchardt wrote:
> Let function efi_dp_find_obj() additionally check if a given protocol is
> installed on the handle relating to the device-path.

Please describe why you want to add another parameter to the function.
Otherwise, we don't understand why the change is necessary.
The caller can simply call efi_search_protocol() to determine whether the given
protocol is installed or not on the handle.

> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
> v2:
> 	new patch
> ---
>  include/efi_loader.h             |  1 +
>  lib/efi_loader/efi_boottime.c    |  2 +-
>  lib/efi_loader/efi_capsule.c     |  2 +-
>  lib/efi_loader/efi_device_path.c | 23 ++++++++++++++++-------
>  lib/efi_loader/efi_disk.c        |  2 +-
>  5 files changed, 20 insertions(+), 10 deletions(-)
> 
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 6271d40125..1ae47a8713 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -731,6 +731,7 @@ struct efi_device_path *efi_dp_next(const struct efi_device_path *dp);
>  int efi_dp_match(const struct efi_device_path *a,
>  		 const struct efi_device_path *b);
>  efi_handle_t efi_dp_find_obj(struct efi_device_path *dp,
> +			     const efi_guid_t *guid,
>  			     struct efi_device_path **rem);
>  /* get size of the first device path instance excluding end node */
>  efi_uintn_t efi_dp_instance_size(const struct efi_device_path *dp);
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index d0f3e05e70..a7bc371f54 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -1750,7 +1750,7 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path,
>  	info->system_table = &systab;
>  
>  	if (device_path) {
> -		info->device_handle = efi_dp_find_obj(device_path, NULL);
> +		info->device_handle = efi_dp_find_obj(device_path, NULL, NULL);
>  
>  		dp = efi_dp_append(device_path, file_path);
>  		if (!dp) {
> diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> index c8e2d510a5..3d80d98c1f 100644
> --- a/lib/efi_loader/efi_capsule.c
> +++ b/lib/efi_loader/efi_capsule.c
> @@ -681,7 +681,7 @@ static bool device_is_present_and_system_part(struct efi_device_path *dp)
>  {
>  	efi_handle_t handle;
>  
> -	handle = efi_dp_find_obj(dp, NULL);
> +	handle = efi_dp_find_obj(dp, NULL, NULL);
>  	if (!handle)
>  		return false;
>  
> diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
> index aeb5264820..0a8802903d 100644
> --- a/lib/efi_loader/efi_device_path.c
> +++ b/lib/efi_loader/efi_device_path.c
> @@ -160,17 +160,19 @@ struct efi_device_path *efi_dp_shorten(struct efi_device_path *dp)
>  }
>  
>  /**
> - * find_handle() - find handle by device path
> + * find_handle() - find handle by device path and installed protocol
>   *
>   * If @rem is provided, the handle with the longest partial match is returned.
>   *
>   * @dp:		device path to search
> + * @guid:	GUID of protocol that must be installed on path or NULL
>   * @short_path:	use short form device path for matching
>   * @rem:	pointer to receive remaining device path
>   * Return:	matching handle
>   */
> -static efi_handle_t find_handle(struct efi_device_path *dp, bool short_path,
> -			        struct efi_device_path **rem)
> +static efi_handle_t find_handle(struct efi_device_path *dp,
> +				const efi_guid_t *guid, bool short_path,
> +				struct efi_device_path **rem)
>  {
>  	efi_handle_t handle, best_handle = NULL;
>  	efi_uintn_t len, best_len = 0;
> @@ -183,6 +185,11 @@ static efi_handle_t find_handle(struct efi_device_path *dp, bool short_path,
>  		efi_uintn_t len_current;
>  		efi_status_t ret;
>  
> +		if (guid) {
> +			ret = efi_search_protocol(handle, guid, &handler);
> +			if (ret != EFI_SUCCESS)
> +				continue;
> +		}
>  		ret = efi_search_protocol(handle, &efi_guid_device_path,
>  					  &handler);
>  		if (ret != EFI_SUCCESS)
> @@ -195,13 +202,13 @@ static efi_handle_t find_handle(struct efi_device_path *dp, bool short_path,
>  		}
>  		len_current = efi_dp_instance_size(dp_current);
>  		if (rem) {
> -			if (len_current < len)
> +			if (len_current > len)

It looks a bug fix, but it's not related to this patch?

-Takahiro Akashi

>  				continue;
>  		} else {
>  			if (len_current != len)
>  				continue;
>  		}
> -		if (memcmp(dp_current, dp, len))
> +		if (memcmp(dp_current, dp, len_current))
>  			continue;
>  		if (!rem)
>  			return handle;
> @@ -220,18 +227,20 @@ static efi_handle_t find_handle(struct efi_device_path *dp, bool short_path,
>   * If @rem is provided, the handle with the longest partial match is returned.
>   *
>   * @dp:		device path to search
> + * @guid:	GUID of protocol that must be installed on path or NULL
>   * @rem:	pointer to receive remaining device path
>   * Return:	matching handle
>   */
>  efi_handle_t efi_dp_find_obj(struct efi_device_path *dp,
> +			     const efi_guid_t *guid,
>  			     struct efi_device_path **rem)
>  {
>  	efi_handle_t handle;
>  
> -	handle = find_handle(dp, false, rem);
> +	handle = find_handle(dp, guid, false, rem);
>  	if (!handle)
>  		/* Match short form device path */
> -		handle = find_handle(dp, true, rem);
> +		handle = find_handle(dp, guid, true, rem);
>  
>  	return handle;
>  }
> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> index 45127d1768..d36a35d94f 100644
> --- a/lib/efi_loader/efi_disk.c
> +++ b/lib/efi_loader/efi_disk.c
> @@ -302,7 +302,7 @@ efi_fs_from_path(struct efi_device_path *full_path)
>  	efi_free_pool(file_path);
>  
>  	/* Get the EFI object for the partition */
> -	efiobj = efi_dp_find_obj(device_path, NULL);
> +	efiobj = efi_dp_find_obj(device_path, NULL, NULL);
>  	efi_free_pool(device_path);
>  	if (!efiobj)
>  		return NULL;
> -- 
> 2.34.1
>