Re: [PATCH 1/2] xen/efi: Handle cases where file didn't come from ESP

["Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>]

[-- Attachment #1: Type: text/plain, Size: 4506 bytes --]

On Tue, Jun 24, 2025 at 09:31:54AM +0100, Frediano Ziglio wrote:
> A boot loader can load files from outside ESP.
> In these cases device could be not provided or path could
> be something not supported.
> In these cases allows to boot anyway, all information
> could be provided using UKI or using other boot loader
> features.
> 
> Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
> ---
>  xen/common/efi/boot.c | 27 +++++++++++++++++++++++++--
>  1 file changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index 1a9b4e7dae..2a49c6d05d 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -443,6 +443,18 @@ static EFI_FILE_HANDLE __init get_parent_handle(const EFI_LOADED_IMAGE *loaded_i
>      CHAR16 *pathend, *ptr;
>      EFI_STATUS ret;
>  
> +    /*
> +     * In some cases the image could not come from a specific device.
> +     * For instance this can happen if Xen was loaded using GRUB2 "linux"
> +     * command.
> +     */
> +    *leaf = buffer;

This feels wrong, if DeviceHandle is NULL, it will point at the
empty buffer that shouldn't really be used for anything anyway.

IMO a better option would be to add "&& dir_handle" to the condition
guarding use of file_name in efi_start() instead.

BTW, by my reading of get_parent_handle() theoretically it should be
possible to get _some_ name out of loaded_image->FilePath even without
dir_handle. But since it isn't going to be used, it's not worth it.

> +    if ( !loaded_image->DeviceHandle )
> +    {
> +        PrintStr(L"Xen image loaded without providing a device\r\n");
> +        return NULL;
> +    }
> +
>      do {
>          EFI_FILE_IO_INTERFACE *fio;
>  
> @@ -466,7 +478,15 @@ static EFI_FILE_HANDLE __init get_parent_handle(const EFI_LOADED_IMAGE *loaded_i
>  
>          if ( DevicePathType(dp) != MEDIA_DEVICE_PATH ||
>               DevicePathSubType(dp) != MEDIA_FILEPATH_DP )
> -            blexit(L"Unsupported device path component");
> +        {
> +            /*
> +             * The image could come from an unsupported device.
> +             * For instance this can happen if Xen was loaded using GRUB2
> +             * "chainloader" command and the file was not from ESP.
> +             */
> +            PrintStr(L"Unsupported device path component\r\n");
> +            return NULL;
> +        }
>  
>          if ( *buffer )
>          {
> @@ -772,6 +792,8 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
>  
>      if ( !name )
>          PrintErrMesg(L"No filename", EFI_OUT_OF_RESOURCES);
> +    if ( !dir_handle )
> +        return false;

There are a lot of places where read_file() is used without checking its
return value. Which made sense since before this change the only cases
where read_file() would return false was for the config file, in all
other cases it handled errors via blexit().
Most of those read_file() calls seems to be fine (as in, will not
explode), but may still be confusing. For example when you embed a
config with "xsm=policy" (but the actual policy is not embedded) now the
failure to load it will result just a warning ("Xen image loaded without
providing a device") not even related to the file name and it will
continue booting with unintended configuration.

For me it looks like this change is wrong: if the config specified a
file to load (and that blob was not embedded in the UKI), and yet it
couldn't be loaded, it should fail early.
Is there any (new) case where where read_file() failure (when it
actually gets to calling it) should really be non-fatal now? 

In relation to the next patch - such UKI should simply not specify
ramdisk in the embedded config, to allow loading it via "initrd"
command. This would avoid calling read_file().

>      ret = dir_handle->Open(dir_handle, &FileHandle, name,
>                             EFI_FILE_MODE_READ, 0);
>      if ( file == &cfg && ret == EFI_NOT_FOUND )
> @@ -1515,7 +1537,8 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE ImageHandle,
>          efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
>          cfg.addr = 0;
>  
> -        dir_handle->Close(dir_handle);
> +        if ( dir_handle )
> +            dir_handle->Close(dir_handle);
>  
>          if ( gop && !base_video )
>          {
> -- 
> 2.43.0
> 

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]