[PATCH v2 1/1] lib: efi_loader: don't delete invalid handles

[Ilias Apalodimas]

Hi all,

On Thu, 8 Sept 2022 at 09:18, Heinrich Schuchardt
<heinrich.schuchardt at canonical.com> wrote:
>
> From: Etienne Carriere <etienne.carriere at linaro.org>
>
> From: Etienne Carriere <etienne.carriere at linaro.org>
>
> Change efi_delete_handle() to not free EFI handles twice.
>
> This change tries to resolved an issue seen since U-Boot v2022.07
> in which 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
> but does not resolve the underlying problem.
>
> Signed-off-by: Etienne Carriere <etienne.carriere at linaro.org>
>
> Add log message.
> Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
> ---
> v2:
>         add log message
>         remove superfluous NULL test
>         tweak commit message
> ---
>  lib/efi_loader/efi_boottime.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index 4da64b5d29..6f7333638a 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -619,9 +619,14 @@ efi_status_t efi_remove_all_protocols(const efi_handle_t handle)
>   */
>  void efi_delete_handle(efi_handle_t handle)
>  {
> -       if (!handle)
> +       efi_status_t ret;
> +
> +       ret = efi_remove_all_protocols(handle);
> +       if (ret == EFI_INVALID_PARAMETER) {
> +               log_err("Can't remove invalid handle %p\n", handle);
>                 return;
> -       efi_remove_all_protocols(handle);
> +       }
> +
>         list_del(&handle->link);
>         free(handle);

Can't we just set the handle NULL here?

Thanks
/Ilias
>  }
> --
> 2.37.2
>