[PATCH] [RFC] lib: efi_loader: don't delete invalid handles

[Heinrich Schuchardt]

On 9/7/22 10:20, Etienne Carriere wrote:
> Changes efi_delete_handle() to not free EFI handles that are not related
> to EFI objects.
>
> This change tries to resolved an issue seen since U-Boot v2022.07
> in which EFI ExitBootService  attempts to release some EFI handles twice.
>
> The issue was seen booting a EFI shell that invokes 'connect -r' and
> then boots a Linux kernel. Execution of connect command makes EFI
> subsystem to bind a block device for each root block devices EFI handles.
> However these EFI device handles are already bound to a driver and we
> can have 2 registered devices relating to the same EFI handler. On
> ExitBootService, the loop removing the devices makes these EFI handles
> to be released twice which corrupts memory.
>
> This patch prevents the memory release operation caused by the issue but
> I don't think this patch is the right way to addresse the problem. Any
> help will be much appreciated.
>
> Signed-off-by: Etienne Carriere <etienne.carriere at linaro.org>
> ---
>   lib/efi_loader/efi_boottime.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index 4da64b5d29..e38990ace2 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -619,9 +619,15 @@ efi_status_t efi_remove_all_protocols(const efi_handle_t handle)
>    */
>   void efi_delete_handle(efi_handle_t handle)
>   {
> +	efi_status_t ret;
> +
>   	if (!handle)
>   		return;

The patch does not solve the underlying problem but checking the
validity of the handle makes sense anyway as we have a lot of callers.

We can remove this check as we test the return value of
efi_remove_all_protocols().

> -	efi_remove_all_protocols(handle);
> +
> +	ret = efi_remove_all_protocols(handle);
> +	if (ret == EFI_INVALID_PARAMETER)

We should write a message here.

log_err("Can't remove invalid handle %p\n", handle);

Best regards

Heinrich

> +		return;
> +
>   	list_del(&handle->link);
>   	free(handle);
>   }