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

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

Ilias,

On Fri, Sep 30, 2022 at 09:18:35AM +0300, Ilias Apalodimas wrote:
> Akashi-san
> 
> On Fri, 30 Sept 2022 at 04:47, AKASHI Takahiro
> <takahiro.akashi@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.

Yes, I confirmed that before I made my comment.

> The
> whole initrd loading logic depends on BootCurrnent, which iirc is not
> set yet on the flow you are proposing.

I don't get your point.

In do_efibootmgr(), what I suggested above is:
- efi_bootmgr_load() installs LOAD_FILE2_PROTOCOL if initrd file exists,
  and if this function fails, LOAD_FILE2_PROTOCOL must be uninstalled any way.
- after returning from UEFI app invoked by do_bootefi_exec(),
- we should simply uninstall LOAD_FILE2_RPTOCOL by calling efi_initrd_deregister().

In "bootefi <addr>" case, efi_bootmgr_load() is not called, so
LOAD_FILE2_PROTOCOL won't be installed for loading initrd file.
Why do we have to call efi_initrd_deregister() in that case?

Regarding BootCurrent, I don't think it has nothing to do with the discussion above.
Anyhow it *is* set before reaching "if (size) ...".

-Takahiro Akashi


> Regards
> /Ilias
> >
> > -Takahiro Akashi
> >
> > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@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
> > >