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

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