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

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