Hi Heinrich and Tom,
On Sun, 26 Jan 2025 at 12:59, Tom Rini trini@konsulko.com wrote:
On Sat, Jan 04, 2025 at 12:05:24AM +0100, Heinrich Schuchardt wrote:
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(-)
[snip]
@@ -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();
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?
Because the net_dp can be set before EFI starts.
Because prior to this patch we set net_dp = NULL at the start of the function and with this patch nothing is updating net_dp until much later in the function.
Instead of jumping to exit, we can jump to error: with
ret = EFI_DEVICE_ERROR.
I'm not entirely sure what you mean by that. But can you perhaps v3 this? I still can't re-enable CONFIG_CMD_BOOTEFI_SELFTEST on any of my platforms due to this problem.
-- Tom
I sent a v3. This piece of code will be replaced in the series implementing support for multiple EFI network devices, sent some days ago.
Thanks, Adriano