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

[Ilias Apalodimas]

Akashi-san

On Fri, 30 Sept 2022 at 09:41, AKASHI Takahiro
<takahiro.akashi at linaro.org> wrote:
>
> 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 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.
>
> 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().

You are saying efi_bootmgr_load() installs the protocol., but the
point here is it's try_load_entry() which installs the protocol,
because we need BootCurrent to have a valid value before we do that.

>
> 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?

We don't.  FWIW I am not against the cleanup.  I am just trying to
make sure you have all the details in your head before you move
forward.

>
> Regarding BootCurrent, I don't think it has nothing to do with the discussion above.

The whole initrd loading and checking code depends on BootCurrent
being set, so unless the protocol installation is called after
BootCurrent being set in try_load_entry() it will fail.  What that
code path does is check BootCurrent's LoadOptions.  The initrd DP is
encoded in the FIlePathList (iirc it's on position [1] of that array)
and that's how it tries to reason about the file being there or not.

Thanks
/Ilias

> Anyhow it *is* set before reaching "if (size) ...".
>
> -Takahiro Akashi
>
>
> > 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
> > > >