El jue, 20 feb 2025 a las 10:12, Adriano Córdova (<[email protected]>) escribió:
> > > El jue, 20 feb 2025 a las 4:54, Heinrich Schuchardt (<[email protected]>) > escribió: > >> 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 <[email protected]> >> > --- >> > 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. >> > > Hi Heinrich, > > efi_net_set_dp is installing it (only if EFI is already up, otherwise it > saves it in net_dp). > >> >> > 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 >> > > Because it comes from the net_dp variable, which is not accessible to EFI > applications, and is always allocated by U-Boot. > > Best, > Adriano > Although it could be teoretically freed by an application. In that case we would be freeing it twice. > >> > >> > 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; >> > } >> > >> > /** >> >>
