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

[Adriano Córdova]

El vie, 6 dic 2024 a las 11:59, Heinrich Schuchardt (<xypron.glpk at 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 at 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;
> >   }
> >
> >   /**
>
>