Re: [PATCH] xen/efi: Fix Grub2 boot on arm64

[Luca Fancellu <luca.fancellu@xxxxxxx>]

+ Ian Jackson for 4.16 release

> On 2 Nov 2021, at 14:45, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> On 02.11.2021 15:05, Luca Fancellu wrote:
>> The code introduced by commit a1743fc3a9fe9b68c265c45264dddf214fd9b882
>> ("arm/efi: Use dom0less configuration when using EFI boot") is
>> introducing a problem to boot Xen using Grub2 on ARM machine using EDK2.
>> 
>> The problem comes from the function get_parent_handle(...) that inside
>> uses the HandleProtocol on loaded_image->DeviceHandle, but the last
>> is NULL, making Xen stop the UEFI boot.
> 
> According to my reading the UEFI spec doesn't (explicitly) allow for
> this to be NULL. Could you clarify why this is the case? What other
> information may end up being invalid / absent? Is e.g. read_section()
> safe to use?

My test on an arm machine running Grub2 on top of EDK2 showed that
when Xen is started, the get_parent_handle(…) call was failing and stopping
the boot because the efi_bs->HandleProtocol(…) was called with the
loaded_image->DeviceHandle argument NULL and the call was returning
a EFI_INVALID_PARAMETER.
So the parent handle can’t be requested and the filesystem can’t be used,
but any other code that doesn’t use the handle provided by get_parent_handle(…)
can be used without problem like read_section(...).

> 
>> --- a/xen/common/efi/boot.c
>> +++ b/xen/common/efi/boot.c
>> @@ -449,6 +449,13 @@ static EFI_FILE_HANDLE __init 
>> get_parent_handle(EFI_LOADED_IMAGE *loaded_image,
>>     CHAR16 *pathend, *ptr;
>>     EFI_STATUS ret;
>> 
>> +    /*
>> +     * If DeviceHandle is NULL, we can't use the SIMPLE_FILE_SYSTEM_PROTOCOL
>> +     * to have access to the filesystem.
>> +     */
>> +    if ( !loaded_image->DeviceHandle )
>> +        return NULL;
> 
> I couldn't find anything in the spec saying that NULL (a pointer with
> the numeric value zero) could actually not be a valid handle. Could
> you point me to text saying so?

I am reading UEFI spec 2.8 A, section 7.3 Protocol Handler Services, when it 
talks about
EFI_BOOT_SERVICES.HandleProtocol() there is a table of “Status Code Returned” 
listing
the EFI_INVALID_PARAMETER when the Handle is NULL.

> 
>> @@ -581,6 +588,8 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, 
>> CHAR16 *name,
>>     EFI_STATUS ret;
>>     const CHAR16 *what = NULL;
>> 
>> +    if ( !dir_handle )
>> +        blexit(L"Error: No access to the filesystem");
> 
> dir_handle also gets passed to efi_arch_cfg_file_{early,late}() -
> those don't need any adjustment only because they merely pass the
> parameter on to read_file()?

Yes, the handling is done in read_file(...)

> 
>> @@ -1333,6 +1342,9 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
>> *SystemTable)
>>             EFI_FILE_HANDLE handle = get_parent_handle(loaded_image,
>>                                                        &file_name);
>> 
>> +            if ( !handle )
>> +                blexit(L"Error retrieving image name: no filesystem 
>> access");
> 
> I don't think this should be fatal - see the comment ahead of the
> enclosing if().

I’m not sure I get it, I put the fatal condition in part because the handle was 
dereferenced by
handle->Close(handle), but also because file_name would have not being modified 
by the call
and we have then *argv = file_name.

Cheers,
Luca

> 
> Jan
>