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

El vie, 6 dic 2024 a las 11:59, Heinrich Schuchardt (xypron.glpk@gmx.de) escribió:

On 12/6/24 13:27, 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

lib/efi_loader/efi_net.c | 53 +++++++++++++++++++++++++++++++++++----- 1 file changed, 47 insertions(+), 6 deletions(-)

diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c index 67593ef50c..1e87cd7be6 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,56 @@ 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;
  
  

• old_net_dp = net_dp;
  net_dp = NULL;
  if (!strcmp(dev, "Net"))
          net_dp = efi_dp_from_eth();
  else if (!strcmp(dev, "Http"))
          net_dp = efi_dp_from_http(server);
  
  

• if (!net_dp)
  

• if (!net_dp) {
  

•         net_dp = old_net_dp;
          return EFI_OUT_OF_RESOURCES;
  

• }
  

• if (!netobj)
  

•         return EFI_SUCCESS;
  

• phandler = NULL;
  

• efi_search_protocol(&netobj->header, &efi_guid_device_path,
  

&phandler);

...

• if (!phandler)
  

•         goto add;
  

• if (phandler->protocol_interface != old_net_dp) {
  

•         ret = EFI_DEVICE_ERROR;
  

•         goto error;
  

• }
  

This is not an error condition. Some other code may have changed the device-path. We must not free what we haven't allocated. But we should still try to replace it.

Will remove the condition.

So can we have a logic like:

old_dp = net_dp;

new_dp = ...

OpenProtocol(handle, GUID, &old_dp, NULL, NULL, EFI_OPEN_PROTOCOL_GET_PROTOCOL);

if (!efi_dp_match(old_dp, new_dp) { FreePool(new_dp); return EFI_SUCCESS; }

ret = ReinstallProtocolInterface(handle, GUID, old_dp, new_dp);

if (ret == EFI_SUCCESS) { FreePool(net_dp) } else { FreePool(new_dp); }

return ret;

Best regards

Heinrich

...

• ret = efi_reinstall_protocol_interface(&netobj->header,
  

&efi_guid_device_path,

...

•                                        old_net_dp, net_dp);
  

• if (ret != EFI_SUCCESS)
  

Can net_dp be freed if an error occurred?

No, because the error could have been on uninstalling what net_dp points to, but new_net_dp could.

...

•         return ret;
  

• efi_free_pool(old_net_dp);
  
  return EFI_SUCCESS;
  

+add:

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

•                        net_dp);
  

• if (ret == EFI_SUCCESS) {
  

•         efi_free_pool(old_net_dp);
  

•         return ret;
  

• }
  

+error:

• // Failed, restore
  

• efi_free_pool(net_dp);
  

• net_dp = old_net_dp;
  

• return ret;
  

  }

  /**