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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

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

> @@ -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()?

> @@ -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().

Jan