+ Ian Jackson for 4.16 release
> On 2 Nov 2021, at 14:45, Jan Beulich <jbeul...@suse.com> 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
>
