|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v3 for-4.22] EFI: Fix boot from a device without a file system
On Mon, May 25, 2026 at 07:37:03PM +0200, Szymon Acedański wrote:
> When netbooting a unified Xen kernel image (via GRUB chainloader),
> the resulting loaded_image->DeviceHandle does not support
> SIMPLE_FILE_SYSTEM_PROTOCOL.
>
> Instead of crashing via noreturn PrintErrMesg() in get_parent_handle(),
> we defer calling this function until filesystem access is needed.
> This way when booting UKI, get_parent_handle() is not called at all.
>
> Suggested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Suggested-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Szymon Acedański <accek@xxxxxxxxxxxxxxxxxxxxxx>
Acked-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> ---
> Changes in v3:
> - read_file(): replaced unreachable `if ( !dir_handle )` branch with
> BUG_ON(!dir_handle), per Marek's suggestion.
>
> Changes in v2:
> - Restructured along the lines Andrew and Marek both suggested on v1:
> defer get_parent_handle() until the first call site that actually
> needs a file.
>
> This mirrors the existing lazy pattern in ARM's
> allocate_module_file() in xen/arch/arm/efi/efi-boot.h, which was also
> changed to use the new ensure_dir_handle() helper.
>
> Tested (same as v2):
> - PXE-loaded GRUB chainloading UKI - failure without patch, success
> with patch
> - QEMU boot from EFI partition, with config, kernel and initrd
> on EFI partition too - success with and without patch
> - Cross-compiling ARM64 - success
>
> xen/arch/arm/efi/efi-boot.h | 12 ++++---
> xen/arch/x86/efi/efi-boot.h | 9 +++--
> xen/common/efi/boot.c | 66 +++++++++++++++++++++++--------------
> 3 files changed, 54 insertions(+), 33 deletions(-)
>
> diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
> index ea59de47e7..069cc68b0a 100644
> --- a/xen/arch/arm/efi/efi-boot.h
> +++ b/xen/arch/arm/efi/efi-boot.h
> @@ -403,7 +403,7 @@ static void __init noreturn efi_arch_post_exit_boot(void)
> }
>
> static void __init efi_arch_cfg_file_early(const EFI_LOADED_IMAGE *image,
> - EFI_FILE_HANDLE dir_handle,
> + EFI_FILE_HANDLE *dir_handle,
> const char *section)
> {
> union string name;
> @@ -419,8 +419,11 @@ static void __init efi_arch_cfg_file_early(const
> EFI_LOADED_IMAGE *image,
> name.s = get_value(&cfg, section, "dtb");
> if ( name.s )
> {
> + CHAR16 *fname;
> +
> split_string(name.s);
> - read_file(dir_handle, s2w(&name), &dtbfile, NULL);
> + ensure_dir_handle(image, dir_handle, &fname);
> + read_file(*dir_handle, s2w(&name), &dtbfile, NULL);
> efi_bs->FreePool(name.w);
> }
> }
> @@ -430,7 +433,7 @@ static void __init efi_arch_cfg_file_early(const
> EFI_LOADED_IMAGE *image,
> }
>
> static void __init efi_arch_cfg_file_late(const EFI_LOADED_IMAGE *image,
> - EFI_FILE_HANDLE dir_handle,
> + EFI_FILE_HANDLE *dir_handle,
> const char *section)
> {
> }
> @@ -665,8 +668,7 @@ static int __init allocate_module_file(const
> EFI_LOADED_IMAGE *loaded_image,
> file_info->name_len = name_len;
>
> /* Get the file system interface. */
> - if ( !*dir_handle )
> - *dir_handle = get_parent_handle(loaded_image, &fname);
> + ensure_dir_handle(loaded_image, dir_handle, &fname);
>
> /* Load the binary in memory */
> read_file(*dir_handle, s2w(&module_name), &module_binary, NULL);
> diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
> index 42a2c46b5e..d738b839ee 100644
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -284,13 +284,13 @@ static void __init noreturn
> efi_arch_post_exit_boot(void)
> }
>
> static void __init efi_arch_cfg_file_early(const EFI_LOADED_IMAGE *image,
> - EFI_FILE_HANDLE dir_handle,
> + EFI_FILE_HANDLE *dir_handle,
> const char *section)
> {
> }
>
> static void __init efi_arch_cfg_file_late(const EFI_LOADED_IMAGE *image,
> - EFI_FILE_HANDLE dir_handle,
> + EFI_FILE_HANDLE *dir_handle,
> const char *section)
> {
> union string name;
> @@ -304,9 +304,12 @@ static void __init efi_arch_cfg_file_late(const
> EFI_LOADED_IMAGE *image,
> name.s = get_value(&cfg, "global", "ucode");
> if ( name.s )
> {
> + CHAR16 *fname;
> +
> microcode_set_module(mbi.mods_count);
> split_string(name.s);
> - read_file(dir_handle, s2w(&name), &ucode, NULL);
> + ensure_dir_handle(image, dir_handle, &fname);
> + read_file(*dir_handle, s2w(&name), &ucode, NULL);
> efi_bs->FreePool(name.w);
> }
> }
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index 9ea2183c0b..2971ea8696 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -547,6 +547,17 @@ static EFI_FILE_HANDLE __init get_parent_handle(const
> EFI_LOADED_IMAGE *loaded_i
> return dir_handle;
> }
>
> +static void __init ensure_dir_handle(const EFI_LOADED_IMAGE *loaded_image,
> + EFI_FILE_HANDLE *dir_handle,
> + CHAR16 **file_name)
> +{
> + if ( *dir_handle )
> + return;
> + *dir_handle = get_parent_handle(loaded_image, file_name);
> + if ( !*dir_handle )
> + blexit(L"Cannot load files without a usable file system");
> +}
> +
> static CHAR16 *__init point_tail(CHAR16 *fn)
> {
> CHAR16 *tail = NULL;
> @@ -838,12 +849,11 @@ static bool __init read_file(EFI_FILE_HANDLE
> dir_handle, CHAR16 *name,
> if ( !name )
> PrintErrMesg(L"No filename", EFI_OUT_OF_RESOURCES);
>
> + BUG_ON(!dir_handle);
> +
> what = L"Open";
> - if ( dir_handle )
> - ret = dir_handle->Open(dir_handle, &FileHandle, name,
> - EFI_FILE_MODE_READ, 0);
> - else
> - ret = EFI_NOT_FOUND;
> + ret = dir_handle->Open(dir_handle, &FileHandle, name,
> + EFI_FILE_MODE_READ, 0);
> if ( file == &cfg && ret == EFI_NOT_FOUND )
> return false;
> if ( EFI_ERROR(ret) )
> @@ -1514,7 +1524,7 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE
> ImageHandle,
>
> if ( use_cfg_file )
> {
> - EFI_FILE_HANDLE dir_handle;
> + EFI_FILE_HANDLE dir_handle = NULL;
> EFI_HANDLE gop_handle;
> UINTN depth, cols, rows;
>
> @@ -1526,31 +1536,33 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE
> ImageHandle,
>
> gop = efi_get_gop(&gop_handle);
>
> - /* Get the file system interface. */
> - dir_handle = get_parent_handle(loaded_image, &file_name);
> -
> /* Read and parse the config file. */
> if ( read_section(loaded_image, L"config", &cfg, NULL) )
> PrintStr(L"Using builtin config file\r\n");
> - else if ( !cfg_file_name && file_name )
> + else
> {
> - CHAR16 *tail;
> + ensure_dir_handle(loaded_image, &dir_handle, &file_name);
>
> - while ( (tail = point_tail(file_name)) != NULL )
> + if ( !cfg_file_name )
> {
> - wstrcpy(tail, L".cfg");
> - if ( read_file(dir_handle, file_name, &cfg, NULL) )
> - break;
> - *tail = 0;
> + CHAR16 *tail;
> +
> + while ( (tail = point_tail(file_name)) != NULL )
> + {
> + wstrcpy(tail, L".cfg");
> + if ( read_file(dir_handle, file_name, &cfg, NULL) )
> + break;
> + *tail = 0;
> + }
> + if ( !tail )
> + blexit(L"No configuration file found.");
> + PrintStr(L"Using configuration file '");
> + PrintStr(file_name);
> + PrintStr(L"'\r\n");
> }
> - if ( !tail )
> - blexit(L"No configuration file found.");
> - PrintStr(L"Using configuration file '");
> - PrintStr(file_name);
> - PrintStr(L"'\r\n");
> + else if ( !read_file(dir_handle, cfg_file_name, &cfg, NULL) )
> + blexit(L"Configuration file not found.");
> }
> - else if ( !read_file(dir_handle, cfg_file_name, &cfg, NULL) )
> - blexit(L"Configuration file not found.");
> pre_parse(&cfg);
>
> if ( section.w )
> @@ -1567,6 +1579,7 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE
> ImageHandle,
> if ( !name.s )
> break;
> free_cfg();
> + ensure_dir_handle(loaded_image, &dir_handle, &file_name);
> if ( !read_file(dir_handle, s2w(&name), &cfg, NULL) )
> {
> PrintStr(L"Chained configuration file '");
> @@ -1578,13 +1591,14 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE
> ImageHandle,
> efi_bs->FreePool(name.w);
> }
>
> - efi_arch_cfg_file_early(loaded_image, dir_handle, section.s);
> + efi_arch_cfg_file_early(loaded_image, &dir_handle, section.s);
>
> option_str = name.s ? split_string(name.s) : NULL;
>
> if ( !read_section(loaded_image, L"kernel", &kernel, option_str) &&
> name.s )
> {
> + ensure_dir_handle(loaded_image, &dir_handle, &file_name);
> read_file(dir_handle, s2w(&name), &kernel, option_str);
> efi_bs->FreePool(name.w);
> }
> @@ -1599,6 +1613,7 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE
> ImageHandle,
> name.s = get_value(&cfg, section.s, "ramdisk");
> if ( name.s )
> {
> + ensure_dir_handle(loaded_image, &dir_handle, &file_name);
> read_file(dir_handle, s2w(&name), &ramdisk, NULL);
> efi_bs->FreePool(name.w);
> }
> @@ -1609,6 +1624,7 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE
> ImageHandle,
> name.s = get_value(&cfg, section.s, "xsm");
> if ( name.s )
> {
> + ensure_dir_handle(loaded_image, &dir_handle, &file_name);
> read_file(dir_handle, s2w(&name), &xsm, NULL);
> efi_bs->FreePool(name.w);
> }
> @@ -1634,7 +1650,7 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE
> ImageHandle,
> }
> }
>
> - efi_arch_cfg_file_late(loaded_image, dir_handle, section.s);
> + efi_arch_cfg_file_late(loaded_image, &dir_handle, section.s);
>
> free_cfg();
>
> --
> 2.53.0
>
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
Attachment:
signature.asc
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |