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

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

[Heinrich Schuchardt]

Don't try to delete a non-existent handle.

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

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

[AKASHI Takahiro]

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.

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

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

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

Re: [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@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@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
> >

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

[AKASHI Takahiro]

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

Re: [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@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@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@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
> > > >

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

[Heinrich Schuchardt]

On 9/30/22 08:54, Ilias Apalodimas wrote:
> Akashi-san
> 
> On Fri, 30 Sept 2022 at 09:41, AKASHI Takahiro
> <takahiro.akashi@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@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"

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

Re: [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@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@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@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@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
> >>>>>
>

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

[Heinrich Schuchardt]

On 9/30/22 09:29, Ilias Apalodimas wrote:
> Hi Heinrich
> 
> On Fri, 30 Sept 2022 at 10:18, Heinrich Schuchardt
> <heinrich.schuchardt@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@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@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

Thanks Takahiro for looking into this.

For 2022.01 I will put this patch into a pull request to avoid 
irritating users by the message. For further changes it is too late in 
this cycle.

Best regards

Heinrich

> 
> Cheers
> /Ilias
>>
>> whenever I return from an EFI binary.
>>
>> Best regards
>>
>> Heinrich
>>
>>

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

[Ilias Apalodimas]

On Fri, 30 Sept 2022 at 10:55, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> On 9/30/22 09:29, Ilias Apalodimas wrote:
> > Hi Heinrich
> >
> > On Fri, 30 Sept 2022 at 10:18, Heinrich Schuchardt
> > <heinrich.schuchardt@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@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@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
>
> Thanks Takahiro for looking into this.
>
> For 2022.01 I will put this patch into a pull request to avoid
> irritating users by the message. For further changes it is too late in
> this cycle.
>

I guess you mean .10

You can add my Acked-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

> Best regards
>
> Heinrich
>
> >
> > Cheers
> > /Ilias
> >>
> >> whenever I return from an EFI binary.
> >>
> >> Best regards
> >>
> >> Heinrich
> >>
> >>
>

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

[Ilias Apalodimas]

Hi Heinrich

On Fri, 30 Sept 2022 at 02:58, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> Don't try to delete a non-existent handle.
>
> 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;
> +

I am not sure what we gain with this.  efi_delete_handle() won't run
anyway since it will return EFI_INVALID_PARAMETER no?
I think the rabbit hole is a bit deeper here.  I would like to forbid
users calling efi_delete_handle().   In theory and if we want be
pedantic on the EFI spec, we should only use Install/UninstallProtocol
(or even better the multiple variant since it's the only one that
checks for duplicate DPs according to the spec) and have the
UninstallProtocol check if there are remaining protocols in the
handle.  If there aren't we should delete the handle as well.

Regards
/Ilias
>         efi_delete_handle(efi_initrd_handle);
>         efi_initrd_handle = NULL;
>  }
> --
> 2.37.2
>