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

[Heinrich Schuchardt]

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.

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?

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