Re: [PATCH 2/2] xen/efi: Support loading initrd using GRUB2 LoadFile2 protocol

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Tue, Jun 24, 2025 at 09:31:55AM +0100, Frediano Ziglio wrote:
> Allows to load Xen using "linux" and "initrd" GRUB2 commands.
> This can be used with UKI to separate initrd in a different module
> instead of bundling all together.
> Bundling all together can be a problem with Secure Boot where
> we need to sign the bundle making harder to change it.
> As initrd content does not need to be signed for Secure Boot
> bundling it force it to be signed too.
> 
> Signed-off-by: Frediano Ziglio <frediano.ziglio@xxxxxxxxx>
> ---
>  xen/common/efi/boot.c     | 71 ++++++++++++++++++++++++++++++++++++++-
>  xen/include/efi/efidevp.h | 21 ++++++++++++
>  2 files changed, 91 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index 2a49c6d05d..87eb8bb8ae 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -849,6 +849,74 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, 
> CHAR16 *name,
>      return true;
>  }
>  
> +#pragma pack(1)
> +typedef struct {
> +    VENDOR_DEVICE_PATH              VenMediaNode;
> +    EFI_DEVICE_PATH                 EndNode;
> +} SINGLE_NODE_VENDOR_MEDIA_DEVPATH;
> +#pragma pack()
> +
> +static bool __init initrd_load_file2(const CHAR16 *name, struct file *file)

This function I haven't tested yet, but it looks okay I think.

> +{
> +    static const SINGLE_NODE_VENDOR_MEDIA_DEVPATH __initconst 
> initrd_dev_path = {
> +        {
> +            {
> +                MEDIA_DEVICE_PATH, MEDIA_VENDOR_DP, { sizeof 
> (VENDOR_DEVICE_PATH) }
> +            },
> +            LINUX_EFI_INITRD_MEDIA_GUID
> +        },
> +        {
> +            END_DEVICE_PATH_TYPE, END_ENTIRE_DEVICE_PATH_SUBTYPE,
> +            { sizeof (EFI_DEVICE_PATH) }
> +        }
> +    };
> +    static EFI_GUID __initdata lf2_proto_guid = EFI_LOAD_FILE2_PROTOCOL_GUID;
> +    EFI_DEVICE_PATH *dp;
> +    EFI_LOAD_FILE2_PROTOCOL *lf2;
> +    EFI_HANDLE handle;
> +    EFI_STATUS ret;
> +    UINTN size;
> +
> +    dp = (EFI_DEVICE_PATH *)&initrd_dev_path;
> +    ret = efi_bs->LocateDevicePath(&lf2_proto_guid, &dp, &handle);
> +    if ( EFI_ERROR(ret) )
> +    {
> +        if ( ret == EFI_NOT_FOUND)
> +            return false;
> +        PrintErrMesg(L"Error getting file with LoadFile2 interface", ret);
> +    }
> +
> +    ret = efi_bs->HandleProtocol(handle, &lf2_proto_guid, (void **)&lf2);
> +    if ( EFI_ERROR(ret) )
> +        PrintErrMesg(L"LoadFile2 file does not provide correct protocol", 
> ret);
> +
> +    size = 0;
> +    ret = lf2->LoadFile(lf2, dp, false, &size, NULL);
> +    if ( ret != EFI_BUFFER_TOO_SMALL )
> +        PrintErrMesg(L"Loading failed", ret);
> +
> +    file->addr = min(1UL << (32 + PAGE_SHIFT),
> +                     HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START);
> +    ret = efi_bs->AllocatePages(AllocateMaxAddress, EfiLoaderData,
> +                                PFN_UP(size), &file->addr);
> +    if ( EFI_ERROR(ret) )
> +        PrintErrMesg(L"Allocation failed", ret);
> +
> +    file->need_to_free = true;
> +    file->size = size;
> +
> +    ret = lf2->LoadFile(lf2, dp, false, &size, file->str);
> +    if ( EFI_ERROR(ret) )
> +    {
> +        efi_bs->FreePages(file->addr, PFN_UP(size));
> +        PrintErrMesg(L"Loading failed", ret);
> +    }
> +
> +    efi_arch_handle_module(file, name, NULL);
> +
> +    return true;
> +}
> +
>  static bool __init read_section(const EFI_LOADED_IMAGE *image,
>                                  const CHAR16 *name, struct file *file,
>                                  const char *options)
> @@ -1492,7 +1560,8 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE 
> ImageHandle,
>              kernel_verified = true;
>          }
>  
> -        if ( !read_section(loaded_image, L"ramdisk", &ramdisk, NULL) )
> +        if ( !initrd_load_file2(L"ramdisk", &ramdisk) &&
> +             !read_section(loaded_image, L"ramdisk", &ramdisk, NULL) )

Unverified initrd loaded by the bootloader should _not_ take precedence
over embedded (signed) one - if whoever decided to bundle initrd into
UKI and sign it this way, that choice should be respected. The order of
conditions should be reversed here.

>          {
>              name.s = get_value(&cfg, section.s, "ramdisk");
>              if ( name.s )
> diff --git a/xen/include/efi/efidevp.h b/xen/include/efi/efidevp.h
> index beb5785a45..b240c15d2a 100644
> --- a/xen/include/efi/efidevp.h
> +++ b/xen/include/efi/efidevp.h
> @@ -398,5 +398,26 @@ typedef union {
>  
>  } EFI_DEV_PATH_PTR;
>  
> +#define EFI_LOAD_FILE2_PROTOCOL_GUID \
> +    { 0x4006c0c1, 0xfcb3, 0x403e, {0x99, 0x6d, 0x4a, 0x6c, 0x87, 0x24, 0xe0, 
> 0x6d } }
> +
> +typedef struct EFI_LOAD_FILE2_PROTOCOL EFI_LOAD_FILE2_PROTOCOL;
> +
> +typedef
> +EFI_STATUS
> +(EFIAPI *EFI_LOAD_FILE2)(
> +    IN EFI_LOAD_FILE2_PROTOCOL      *This,
> +    IN EFI_DEVICE_PATH              *FilePath,
> +    IN BOOLEAN                      BootPolicy,
> +    IN OUT UINTN                    *BufferSize,
> +    IN VOID                         *Buffer OPTIONAL
> +    );
> +
> +struct EFI_LOAD_FILE2_PROTOCOL {
> +    EFI_LOAD_FILE2                  LoadFile;
> +};
> +
> +#define LINUX_EFI_INITRD_MEDIA_GUID \
> +    { 0x5568e427, 0x68fc, 0x4f3d, {0xac, 0x74, 0xca, 0x55, 0x52, 0x31, 0xcc, 
> 0x68} }
>  
>  #endif
> -- 
> 2.43.0
> 

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

Attachment: signature.asc
Description: PGP signature