On 1/27/25 13:34, 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.cord...@canonical.com>
---
Changes in v3: initialize ret variable in efi_net_set_dp
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 67593ef50c0..f696a47060c 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);
Hello Adriano,
thanks for spotting and fixing the issue.
Patch 1 of the series is already merged.
Why would we create a device-path here and not install it?
If this is really intended, please, add a comment.
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 = EFI_SUCCESS;
+ 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;
+ }
+
+ 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);
How can we be sure that old_net_dp is a device-tree that we installed
and was not installed by an EFI application?
Best regards
Heinrich
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;
+ efi_free_pool(old_net_dp);
+
+ return ret;
+error:
+ // Failed, restore
+ efi_free_pool(new_net_dp);
+
+ return ret;
}
/**
