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

[Etienne Carriere]

Hello Heinrich,

Thanks a lot for the detailed feedback on how to address root issue. Indeed
not an obvious fix.

Regards,
Etienne



On Thu, 8 Sept 2022 at 08:03, Heinrich Schuchardt <xypron.glpk at gmx.de>
wrote:

> 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);
> >   }
>
>