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

[Adriano Córdova]

El jue, 20 feb 2025 a las 10:12, Adriano Córdova (<adrianox at gmail.com>)
escribió:

>
>
> El jue, 20 feb 2025 a las 4:54, Heinrich Schuchardt (<xypron.glpk at 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.cordova at 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
>

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;
>> >   }
>> >
>> >   /**
>>
>>