Re: [PATCH v2 2/2] efi_loader: efi_net: let efi_net_set_dp properly update the device path

On 06.12.24 18:18, Adriano Cordova wrote:

This commit fixes an use after free introduced in Commit e55a4acb54 (" efi_loader: net: set EFI bootdevice device path to HTTP when loaded from wget"). The logic in efi_net_set_dp is reworked so that when the function is invoked it not only changes the value of the static variable net_dp (this is how the function was implemented in e55a4acb54) but also updates the protocol interface of the device path protocol in case efi has started.

Fixes: e55a4acb54e8 ("efi_loader: net: set EFI bootdevice device path to HTTP when loaded from wget") Signed-off-by: Adriano Cordova adriano.cordova@canonical.com

---

Changes in v2: use reinstall_protocol_interface() and new_net_dp in the body of efi_net_set_dp()

lib/efi_loader/efi_net.c | 61 ++++++++++++++++++++++++++++++++++------ 1 file changed, 52 insertions(+), 9 deletions(-)

diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c index 67593ef50c..ac8e54639b 100644 --- a/lib/efi_loader/efi_net.c +++ b/lib/efi_loader/efi_net.c @@ -925,12 +925,15 @@ efi_status_t efi_net_register(void) &netobj->net); if (r != EFI_SUCCESS) goto failure_to_add_protocol;

• if (!net_dp)

• efi_net_set_dp("Net", NULL);
  

• r = efi_add_protocol(&netobj->header, &efi_guid_device_path,

• 	     net_dp);
  

• if (net_dp)

• r = efi_add_protocol(&netobj->header, &efi_guid_device_path,
  

• 	       net_dp);
  

• else

• r = efi_net_set_dp("Net", NULL);
  

  if (r != EFI_SUCCESS) goto failure_to_add_protocol;
• r = efi_add_protocol(&netobj->header, &efi_pxe_base_code_protocol_guid, &netobj->pxe); if (r != EFI_SUCCESS)

@@ -1055,18 +1058,58 @@ out_of_resources: */ efi_status_t efi_net_set_dp(const char *dev, const char *server) {

• efi_free_pool(net_dp);

• efi_status_t ret;
• struct efi_handler *phandler;
• struct efi_device_path *old_net_dp, *new_net_dp;

• net_dp = NULL;

• old_net_dp = net_dp;
• new_net_dp = NULL; if (!strcmp(dev, "Net"))

• net_dp = efi_dp_from_eth();
  

• new_net_dp = efi_dp_from_eth();
  

  else if (!strcmp(dev, "Http"))

• net_dp = efi_dp_from_http(server);
  

• new_net_dp = efi_dp_from_http(server);
  
  

• if (!net_dp)

• if (!new_net_dp) { return EFI_OUT_OF_RESOURCES;
• }
• // If netobj is not started yet, end here.
• if (!netobj) {

• goto exit;
  

This leads to a build failure with clang as you have not initialized variable ret.

https://source.denx.de/u-boot/custodians/u-boot-efi/-/jobs/987832

• }

• phandler = NULL;

• efi_search_protocol(&netobj->header, &efi_guid_device_path, &phandler);

• // If the device path protocol is not yet installed, install it

• if (!phandler)

• goto add;
  

• // If it is already installed, try to update it

• ret = efi_reinstall_protocol_interface(&netobj->header, &efi_guid_device_path,

• 			       old_net_dp, new_net_dp);
  

• if (ret != EFI_SUCCESS)

• goto error;
  

• net_dp = new_net_dp;

• efi_free_pool(old_net_dp);

  return EFI_SUCCESS;

+add:

• ret = efi_add_protocol(&netobj->header, &efi_guid_device_path,

• 	       new_net_dp);
  

• if (ret != EFI_SUCCESS)

• goto error;
  

+exit:

• net_dp = new_net_dp;

Why should we set net_dp if there is no net_obj?

Instead of jumping to exit, we can jump to error: with

ret = EFI_DEVICE_ERROR.

Best regards

Heinrich

• efi_free_pool(old_net_dp);
• return ret;

+error:

• // Failed, restore

• efi_free_pool(new_net_dp);

• return ret; }

  /**