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

[Ilias Apalodimas]

Hi Heinrich

On Fri, 30 Sept 2022 at 10:18, Heinrich Schuchardt
<heinrich.schuchardt at canonical.com> wrote:
>
> On 9/30/22 08:54, Ilias Apalodimas wrote:
> > 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?
>
> I don't like to see a message
>
> "Can't remove invalid handle %p\n"

Fair enough.  Let's see if Akashi can clean up uninstalling the
protocol, otherwise I am fine with this patch

Cheers
/Ilias
>
> whenever I return from an EFI binary.
>
> Best regards
>
> Heinrich
>
>
> >>>
> >>>>
> >>>> 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
> >>>>>
>