[PATCH 1/1] efi_loader: fix efi_initrd_deregister()

[Ilias Apalodimas]

Akashi-san

On Fri, 30 Sept 2022 at 04:47, AKASHI Takahiro
<takahiro.akashi at linaro.org> wrote:
>
> On Fri, Sep 30, 2022 at 01:57:48AM +0200, Heinrich Schuchardt wrote:
> > Don't try to delete a non-existent handle.
>
> It is okay as a safe guard, but it doesn't fix underlying issues.

I dont think we safeguard anything. That code path won't try to delete
anything regardless?

>
> efi_initrd_register() is called only in efi_bootmgr_load(), and so
> efi_initrd_deregister() should be called only at the paired location.

There's a reason for that.
>
> - Remove efi_initrd_deregister() from do_bootefi_exec()
> - do_efibootmgr() should look like
>       efi_bootmgr_load()
>       do_bootefi_exec()
>       efi_initrd_deregister()
> Otherwise, do_bootefi_exec() always tries to free a handle in
> the other cases (i.e. bootefi <addr>).
>
> Another issue is there in try_load_entry() called by efi_bootmgr_load().
>      (after efi_initrd_register())
>      if (size) {
>                 *load_options = malloc(size);
>                 if (!*load_options) {
>                         ret = EFI_OUT_OF_RESOURCES;
>                         goto error;
>                 }
>                 ...
>
> If malloc() fails, we should call efi_initrd_deregister() within
> try_load_entry().
>
> Should I submit a patch?

The whole implementation on the *kernel* assumes the protocol is
present if the file it's pointing is real and existing.  You also need
to have a single instance of the protocol installed.  IOW if you
install the protocol and the initrd is not there, the kernel won't
fallback on the dt /chosen/ node or the initrd= in the cmdline.  The
whole initrd loading logic depends on BootCurrnent, which iirc is not
set yet on the flow you are proposing.

Regards
/Ilias
>
> -Takahiro Akashi
>
> > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
> > ---
> >  lib/efi_loader/efi_load_initrd.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/lib/efi_loader/efi_load_initrd.c b/lib/efi_loader/efi_load_initrd.c
> > index c5e6652e66..3d6044f760 100644
> > --- a/lib/efi_loader/efi_load_initrd.c
> > +++ b/lib/efi_loader/efi_load_initrd.c
> > @@ -230,6 +230,9 @@ efi_status_t efi_initrd_register(void)
> >   */
> >  void efi_initrd_deregister(void)
> >  {
> > +     if (!efi_initrd_handle)
> > +             return;
> > +
> >       efi_delete_handle(efi_initrd_handle);
> >       efi_initrd_handle = NULL;
> >  }
> > --
> > 2.37.2
> >