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

[Jan Beulich <jbeulich@suse.com>]

On 04.11.2021 15:12, Luca Fancellu wrote:
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -449,6 +449,15 @@ static EFI_FILE_HANDLE __init get_parent_handle(EFI_LOADED_IMAGE *loaded_image,
>      CHAR16 *pathend, *ptr;
>      EFI_STATUS ret;
>  
> +    /*
> +     * Grub2 running on top of EDK2 has been observed to supply a NULL
> +     * DeviceHandle. We can't use that to gain access to the filesystem.
> +     * However the system can still boot if it doesn’t require access to the
> +     * filesystem.
> +     */
> +    if ( !loaded_image->DeviceHandle )
> +        return NULL;
> +
>      do {
>          EFI_FILE_IO_INTERFACE *fio;
>  
> @@ -581,6 +590,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");
>      if ( !name )
>          PrintErrMesg(L"No filename", EFI_OUT_OF_RESOURCES);
>      ret = dir_handle->Open(dir_handle, &FileHandle, name,
> @@ -1333,8 +1344,18 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>              EFI_FILE_HANDLE handle = get_parent_handle(loaded_image,
>                                                         &file_name);
>  
> -            handle->Close(handle);
> -            *argv = file_name;
> +            if ( !handle )
> +            {
> +                PrintErr(L"Error retrieving image name: no filesystem access."
> +                         L" Setting default to xen.efi");
> +                PrintErr(newline);
> +                *argv = L"xen.efi";
> +            }
> +            else
> +            {
> +                handle->Close(handle);
> +                *argv = file_name;
> +            }
>          }
>  
>          name.s = get_value(&cfg, section.s, "options");
> @@ -1369,7 +1390,8 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>      /* Get the number of boot modules specified on the DT or an error (<0) */
>      dt_modules_found = efi_check_dt_boot(dir_handle);
>  
> -    dir_handle->Close(dir_handle);
> +    if ( dir_handle )
> +        dir_handle->Close(dir_handle);
>  
>      if ( dt_modules_found < 0 )
>          /* efi_check_dt_boot throws some error */
> 

I'm sorry, but I think we need to take a step back here and revisit
the earlier change. If that hadn't moved obtaining dir_handle out by
one level of scope, nothing bad would have happened to the case that
you're now trying to fix, I understand? So perhaps that part wants
undoing, with efi_check_dt_boot() instead getting passed loaded_image.
That way, down the call tree the needed handle can be obtained via
another call to get_parent_handle(), and quite likely in the scenario
you're trying to fix here execution wouldn't even make it there. This
then wouldn't be much different to the image name retrieval calling
get_parent_handle() a 2nd time, rather than trying to re-use
dir_handle.

Net effect being that I think get_parent_handle() would then again
only be called when the returned handle is actually needed, and hence
when failure of HandleProtocol() (for DeviceHandle being NULL just
like for any other reason) is indeed an error that needs reporting.

Jan