[PATCH v2 0/2] efi_loader: efi_net: Fix net_dp handling
Fixes commit e55a4acb54e8 ("efi_loader: net: set EFI bootdevice device path to HTTP when loaded from wget") which generates an use after free, but first it is necessary to expose efi_reinstall_protocol_interfaces in efi_loader.h, which is done in a separated patch.
Adriano Cordova (2): efi_loader: Expose efi_reinstall_protocol_interface in efi_loader.h efi_loader: efi_net: let efi_net_set_dp properly update the device path
include/efi_loader.h | 5 +++ lib/efi_loader/efi_boottime.c | 2 +- lib/efi_loader/efi_net.c | 61 +++++++++++++++++++++++++++++------ 3 files changed, 58 insertions(+), 10 deletions(-)
This is done so that the device path protocol interface of the network device can be changed internally by u-boot when a new bootfile gets downloaded.
Signed-off-by: Adriano Cordova adriano.cordova@canonical.com Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org --- Changes in v2: added commit message
include/efi_loader.h | 5 +++++ lib/efi_loader/efi_boottime.c | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index 9afbec35eb..0d858c1e12 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -711,6 +711,11 @@ efi_status_t efi_search_protocol(const efi_handle_t handle, efi_status_t efi_add_protocol(const efi_handle_t handle, const efi_guid_t *protocol, void *protocol_interface); +/* Reinstall a protocol on a handle */ +efi_status_t EFIAPI efi_reinstall_protocol_interface( + efi_handle_t handle, + const efi_guid_t *protocol, + void *old_interface, void *new_interface); /* Open protocol */ efi_status_t efi_protocol_open(struct efi_handler *handler, void **protocol_interface, void *agent_handle, diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 080e7f78ae..58716fa795 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -3733,7 +3733,7 @@ out: * * Return: status code */ -static efi_status_t EFIAPI efi_reinstall_protocol_interface( +efi_status_t EFIAPI efi_reinstall_protocol_interface( efi_handle_t handle, const efi_guid_t *protocol, void *old_interface, void *new_interface) {
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; + } + + 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; }
/**
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);- 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();
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();
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();
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
participants (4)
-
Adriano Cordova -
Adriano Cordova Fedeli -
Heinrich Schuchardt -
Tom Rini