| Message ID | 20241206171835.2403418-3-adriano.cordova@canonical.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | efi_loader: efi_net: Fix net_dp handling | expand |
On Fri, Dec 06, 2024 at 02:18:35PM -0300, 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@canonical.com> Reported-by: Tom Rini <trini@konsulko.com> Tested-by: Tom Rini <trini@konsulko.com>
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@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(-) > > diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c > index 67593ef50c..ac8e54639b 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,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? Instead of jumping to exit, we can jump to error: with ret = EFI_DEVICE_ERROR. Best regards Heinrich > + efi_free_pool(old_net_dp); > + > + return ret; > +error: > + // Failed, restore > + efi_free_pool(new_net_dp); > + > + return ret; > } > > /**
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@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 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.
Hi Heinrich and Tom, On Sun, 26 Jan 2025 at 12:59, Tom Rini <trini@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@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
diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c index 67593ef50c..ac8e54639b 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,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; + } + + 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; + efi_free_pool(old_net_dp); + + return ret; +error: + // Failed, restore + efi_free_pool(new_net_dp); + + return ret; } /**
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@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(-)