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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 2 Nov 2021, Luca Fancellu wrote:
> + 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(...).

It could be the case that Grub2 is doing something not entirely
compliant to the spec.


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

But it is not super obvious, that's one I suggested an additional
explicit check in my other email