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

[Ilias Apalodimas]

hi Heinrich

On Thu, 8 Sept 2022 at 09:54, Heinrich Schuchardt
<heinrich.schuchardt at canonical.com> wrote:
>
>
>
> On 9/8/22 08:46, Ilias Apalodimas wrote:
> > 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?
>
>
> If you set handle=NULL in  efi_delete_handle(), this has no effect as
> the caller passes in handle and not *handle.

Yea you are right

Reviewed-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>

>
> Best regards
>
> Heinrich