El jue, 20 feb 2025 a las 4:54, Heinrich Schuchardt (<xypron.g...@gmx.de>)
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 <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.
>
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
>
> >
> > 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;
> > }
> >
> > /**
>
>
