Re: [PATCH v2 2/9] efi_loader: fix efi_dp_find_obj()

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

On Sat, Mar 19, 2022 at 10:11:41AM +0100, Heinrich Schuchardt wrote:
> efi_dp_find_obj() should not return any handle with a partially matching
> device path

If so, please describe so explicitly in the function's description.
See below.

> but the handle with the maximum matching device path.
> 
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
> v2:
> 	new patch
> ---
>  include/efi_loader.h             |   4 +-
>  lib/efi_loader/efi_device_path.c | 110 +++++++++++++++++--------------
>  2 files changed, 63 insertions(+), 51 deletions(-)
> 
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 1ffcdfc485..6271d40125 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -730,8 +730,8 @@ struct efi_device_path *efi_dp_shorten(struct efi_device_path *dp);
>  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);
> -struct efi_object *efi_dp_find_obj(struct efi_device_path *dp,
> -				   struct efi_device_path **rem);
> +efi_handle_t efi_dp_find_obj(struct efi_device_path *dp,
> +			     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);
>  /* size of multi-instance device path excluding end node */
> diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
> index ddd5f132ec..aeb5264820 100644
> --- a/lib/efi_loader/efi_device_path.c
> +++ b/lib/efi_loader/efi_device_path.c
> @@ -159,69 +159,81 @@ struct efi_device_path *efi_dp_shorten(struct efi_device_path *dp)
>  	return dp;
>  }
>  
> -static struct efi_object *find_obj(struct efi_device_path *dp, bool short_path,
> -				   struct efi_device_path **rem)
> +/**
> + * find_handle() - find handle by device path
> + *
> + * If @rem is provided, the handle with the longest partial match is returned.
> + *
> + * @dp:		device path to search
> + * @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)
>  {
> -	struct efi_object *efiobj;
> -	efi_uintn_t dp_size = efi_dp_instance_size(dp);
> +	efi_handle_t handle, best_handle = NULL;
> +	efi_uintn_t len, best_len = 0;
> +
> +	len = efi_dp_instance_size(dp);
>  
> -	list_for_each_entry(efiobj, &efi_obj_list, link) {
> +	list_for_each_entry(handle, &efi_obj_list, link) {
>  		struct efi_handler *handler;
> -		struct efi_device_path *obj_dp;
> +		struct efi_device_path *dp_current;
> +		efi_uintn_t len_current;
>  		efi_status_t ret;
>  
> -		ret = efi_search_protocol(efiobj,
> -					  &efi_guid_device_path, &handler);
> +		ret = efi_search_protocol(handle, &efi_guid_device_path,
> +					  &handler);
>  		if (ret != EFI_SUCCESS)
>  			continue;
> -		obj_dp = handler->protocol_interface;
> -
> -		do {
> -			if (efi_dp_match(dp, obj_dp) == 0) {
> -				if (rem) {
> -					/*
> -					 * Allow partial matches, but inform
> -					 * the caller.
> -					 */
> -					*rem = ((void *)dp) +
> -						efi_dp_instance_size(obj_dp);
> -					return efiobj;
> -				} else {
> -					/* Only return on exact matches */
> -					if (efi_dp_instance_size(obj_dp) ==
> -					    dp_size)
> -						return efiobj;
> -				}
> -			}
> -
> -			obj_dp = efi_dp_shorten(efi_dp_next(obj_dp));
> -		} while (short_path && obj_dp);
> +		dp_current = handler->protocol_interface;
> +		if (short_path) {
> +			dp_current = efi_dp_shorten(dp_current);
> +			if (!dp_current)
> +				continue;
> +		}
> +		len_current = efi_dp_instance_size(dp_current);
> +		if (rem) {
> +			if (len_current < len)
> +				continue;
> +		} else {
> +			if (len_current != len)
> +				continue;
> +		}
> +		if (memcmp(dp_current, dp, len))
> +			continue;
> +		if (!rem)
> +			return handle;
> +		if (len_current > best_len) {
> +			best_len = len_current;
> +			best_handle = handle;
> +			*rem = (void*)((u8 *)dp + len_current);
> +		}
>  	}
> -
> -	return NULL;
> +	return best_handle;
>  }
>  
> -/*
> - * Find an efiobj from device-path, if 'rem' is not NULL, returns the
> - * remaining part of the device path after the matched object.
> +/**
> + * efi_dp_find_obj() - find handle by device path
> + *
> + * If @rem is provided, the handle with the longest partial match is returned.

What if @rem == NULL.

> + *
> + * @dp:		device path to search
> + * @rem:	pointer to receive remaining device path
> + * Return:	matching handle
>   */
> -struct efi_object *efi_dp_find_obj(struct efi_device_path *dp,
> -				   struct efi_device_path **rem)
> +efi_handle_t efi_dp_find_obj(struct efi_device_path *dp,
> +			     struct efi_device_path **rem)

The return type was also changed.
Why not change the function name to, say, efi_dp_find_handle()
"object" is an internal representation.

-Takahiro Akashi

>  {
> -	struct efi_object *efiobj;
> -
> -	/* Search for an exact match first */
> -	efiobj = find_obj(dp, false, NULL);
> -
> -	/* Then for a fuzzy match */
> -	if (!efiobj)
> -		efiobj = find_obj(dp, false, rem);
> +	efi_handle_t handle;
>  
> -	/* And now for a fuzzy short match */
> -	if (!efiobj)
> -		efiobj = find_obj(dp, true, rem);
> +	handle = find_handle(dp, false, rem);
> +	if (!handle)
> +		/* Match short form device path */
> +		handle = find_handle(dp, true, rem);
>  
> -	return efiobj;
> +	return handle;
>  }
>  
>  /*
> -- 
> 2.34.1
>