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

[Adriano Cordova Fedeli]

Hi Heinrich and Tom,

On Sun, 26 Jan 2025 at 12:59, Tom Rini <trini at 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 at 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();
> > >     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;
> >
> > 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