* [PATCH v4 0/4] efi: Unified Xen hypervisor/kernel/initrd images @ 2020-09-14 11:50 Trammell Hudson 2020-09-14 11:50 ` [PATCH v4 1/4] efi/boot.c: add file.need_to_free Trammell Hudson ` (3 more replies) 0 siblings, 4 replies; 25+ messages in thread From: Trammell Hudson @ 2020-09-14 11:50 UTC (permalink / raw) To: xen-devel; +Cc: roger.pau, jbeulich, andrew.cooper3, wl This patch series adds support for bundling the xen.efi hypervisor, the xen.cfg configuration file, the Linux kernel and initrd, as well as the XSM, and architectural specific files into a single "unified" EFI executable. This allows an administrator to update the components independently without requiring rebuilding xen, as well as to replace the components in an existing image. The resulting EFI executable can be invoked directly from the UEFI Boot Manager, removing the need to use a separate loader like grub as well as removing dependencies on local filesystem access. And since it is a single file, it can be signed and validated by UEFI Secure Boot without requring the shim protocol. It is inspired by systemd-boot's unified kernel technique and borrows the function to locate PE sections from systemd's LGPL'ed code. During EFI boot, Xen looks at its own loaded image to locate the PE sections for the Xen configuration (`.config`), dom0 kernel (`.kernel`), dom0 initrd (`.ramdisk`), and XSM config (`.xsm`), which are included after building xen.efi using objcopy to add named sections for each input file. Trammell Hudson (4): efi/boot.c: add file.need_to_free efi/boot.c: add handle_file_info() efi: Enable booting unified hypervisor/kernel/initrd images efi: Do not use command line if secure boot is enabled. .gitignore | 1 + docs/misc/efi.pandoc | 49 ++++++++++++ xen/arch/arm/efi/efi-boot.h | 25 ++++-- xen/arch/x86/efi/Makefile | 2 +- xen/arch/x86/efi/efi-boot.h | 11 ++- xen/common/efi/boot.c | 154 ++++++++++++++++++++++++++++-------- xen/common/efi/efi.h | 3 + xen/common/efi/pe.c | 137 ++++++++++++++++++++++++++++++++ 8 files changed, 336 insertions(+), 46 deletions(-) create mode 100644 xen/common/efi/pe.c -- 2.25.1 ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v4 1/4] efi/boot.c: add file.need_to_free 2020-09-14 11:50 [PATCH v4 0/4] efi: Unified Xen hypervisor/kernel/initrd images Trammell Hudson @ 2020-09-14 11:50 ` Trammell Hudson 2020-09-16 6:43 ` Roger Pau Monné 2020-09-14 11:50 ` [PATCH v4 2/4] efi/boot.c: add handle_file_info() Trammell Hudson ` (2 subsequent siblings) 3 siblings, 1 reply; 25+ messages in thread From: Trammell Hudson @ 2020-09-14 11:50 UTC (permalink / raw) To: xen-devel; +Cc: roger.pau, jbeulich, andrew.cooper3, wl The config file, kernel, initrd, etc should only be freed if they are allocated with the UEFI allocator. Signed-off-by: Trammell Hudson <hudson@trmm.net> --- xen/common/efi/boot.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c index 4022a672c9..7156139174 100644 --- a/xen/common/efi/boot.c +++ b/xen/common/efi/boot.c @@ -102,6 +102,7 @@ union string { struct file { UINTN size; + bool need_to_free; union { EFI_PHYSICAL_ADDRESS addr; void *ptr; @@ -279,13 +280,13 @@ void __init noreturn blexit(const CHAR16 *str) if ( !efi_bs ) efi_arch_halt(); - if ( cfg.addr ) + if ( cfg.addr && cfg.need_to_free ) efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size)); - if ( kernel.addr ) + if ( kernel.addr && kernel.need_to_free ) efi_bs->FreePages(kernel.addr, PFN_UP(kernel.size)); - if ( ramdisk.addr ) + if ( ramdisk.addr && ramdisk.need_to_free ) efi_bs->FreePages(ramdisk.addr, PFN_UP(ramdisk.size)); - if ( xsm.addr ) + if ( xsm.addr && xsm.need_to_free ) efi_bs->FreePages(xsm.addr, PFN_UP(xsm.size)); efi_arch_blexit(); @@ -572,6 +573,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name, HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START); ret = efi_bs->AllocatePages(AllocateMaxAddress, EfiLoaderData, PFN_UP(size), &file->addr); + file->need_to_free = true; } if ( EFI_ERROR(ret) ) { -- 2.25.1 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH v4 1/4] efi/boot.c: add file.need_to_free 2020-09-14 11:50 ` [PATCH v4 1/4] efi/boot.c: add file.need_to_free Trammell Hudson @ 2020-09-16 6:43 ` Roger Pau Monné 2020-09-17 11:06 ` Jan Beulich 0 siblings, 1 reply; 25+ messages in thread From: Roger Pau Monné @ 2020-09-16 6:43 UTC (permalink / raw) To: Trammell Hudson; +Cc: xen-devel, jbeulich, andrew.cooper3, wl On Mon, Sep 14, 2020 at 07:50:10AM -0400, Trammell Hudson wrote: > The config file, kernel, initrd, etc should only be freed if they > are allocated with the UEFI allocator. > > Signed-off-by: Trammell Hudson <hudson@trmm.net> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> > --- > xen/common/efi/boot.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c > index 4022a672c9..7156139174 100644 > --- a/xen/common/efi/boot.c > +++ b/xen/common/efi/boot.c > @@ -102,6 +102,7 @@ union string { > > struct file { > UINTN size; > + bool need_to_free; > union { > EFI_PHYSICAL_ADDRESS addr; > void *ptr; > @@ -279,13 +280,13 @@ void __init noreturn blexit(const CHAR16 *str) > if ( !efi_bs ) > efi_arch_halt(); > > - if ( cfg.addr ) > + if ( cfg.addr && cfg.need_to_free ) > efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size)); > - if ( kernel.addr ) > + if ( kernel.addr && kernel.need_to_free ) > efi_bs->FreePages(kernel.addr, PFN_UP(kernel.size)); > - if ( ramdisk.addr ) > + if ( ramdisk.addr && ramdisk.need_to_free ) > efi_bs->FreePages(ramdisk.addr, PFN_UP(ramdisk.size)); > - if ( xsm.addr ) > + if ( xsm.addr && xsm.need_to_free ) > efi_bs->FreePages(xsm.addr, PFN_UP(xsm.size)); > > efi_arch_blexit(); > @@ -572,6 +573,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name, > HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START); > ret = efi_bs->AllocatePages(AllocateMaxAddress, EfiLoaderData, > PFN_UP(size), &file->addr); > + file->need_to_free = true; Strictly speaking, don't you need to set need_to_free only if AllocatePages has succeed? I guess it doesn't matter much because addr would be zapped to 0 if allocation fails. Thanks, Roger. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v4 1/4] efi/boot.c: add file.need_to_free 2020-09-16 6:43 ` Roger Pau Monné @ 2020-09-17 11:06 ` Jan Beulich 0 siblings, 0 replies; 25+ messages in thread From: Jan Beulich @ 2020-09-17 11:06 UTC (permalink / raw) To: Roger Pau Monné, Trammell Hudson; +Cc: xen-devel, andrew.cooper3, wl On 16.09.2020 08:43, Roger Pau Monné wrote: > On Mon, Sep 14, 2020 at 07:50:10AM -0400, Trammell Hudson wrote: >> @@ -279,13 +280,13 @@ void __init noreturn blexit(const CHAR16 *str) >> if ( !efi_bs ) >> efi_arch_halt(); >> >> - if ( cfg.addr ) >> + if ( cfg.addr && cfg.need_to_free ) >> efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size)); >> - if ( kernel.addr ) >> + if ( kernel.addr && kernel.need_to_free ) >> efi_bs->FreePages(kernel.addr, PFN_UP(kernel.size)); >> - if ( ramdisk.addr ) >> + if ( ramdisk.addr && ramdisk.need_to_free ) >> efi_bs->FreePages(ramdisk.addr, PFN_UP(ramdisk.size)); >> - if ( xsm.addr ) >> + if ( xsm.addr && xsm.need_to_free ) >> efi_bs->FreePages(xsm.addr, PFN_UP(xsm.size)); All these look to be able to become just "if ( xyz.need_to_free )" if ... >> @@ -572,6 +573,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name, >> HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START); >> ret = efi_bs->AllocatePages(AllocateMaxAddress, EfiLoaderData, >> PFN_UP(size), &file->addr); >> + file->need_to_free = true; > > Strictly speaking, don't you need to set need_to_free only if > AllocatePages has succeed? ... this was followed, so I think the adjustment wants making. > I guess it doesn't matter much because addr > would be zapped to 0 if allocation fails. Perhaps this zapping then also becomes unnecessary, albeit I didn't look very closely yet. Jan ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v4 2/4] efi/boot.c: add handle_file_info() 2020-09-14 11:50 [PATCH v4 0/4] efi: Unified Xen hypervisor/kernel/initrd images Trammell Hudson 2020-09-14 11:50 ` [PATCH v4 1/4] efi/boot.c: add file.need_to_free Trammell Hudson @ 2020-09-14 11:50 ` Trammell Hudson 2020-09-16 6:46 ` Roger Pau Monné 2020-09-17 11:29 ` Jan Beulich 2020-09-14 11:50 ` [PATCH v4 3/4] efi: Enable booting unified hypervisor/kernel/initrd images Trammell Hudson 2020-09-14 11:50 ` [PATCH v4 4/4] efi: Do not use command line if secure boot is enabled Trammell Hudson 3 siblings, 2 replies; 25+ messages in thread From: Trammell Hudson @ 2020-09-14 11:50 UTC (permalink / raw) To: xen-devel; +Cc: roger.pau, jbeulich, andrew.cooper3, wl Add a separate function to display the address ranges used by the files and call `efi_arch_handle_module()` on the modules. Signed-off-by: Trammell Hudson <hudson@trmm.net> --- xen/common/efi/boot.c | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c index 7156139174..57df89cacb 100644 --- a/xen/common/efi/boot.c +++ b/xen/common/efi/boot.c @@ -539,6 +539,22 @@ static char * __init split_string(char *s) return NULL; } +static void __init handle_file_info(CHAR16 *name, + struct file *file, char *options) +{ + if ( file == &cfg ) + return; + + PrintStr(name); + PrintStr(L": "); + DisplayUint(file->addr, 2 * sizeof(file->addr)); + PrintStr(L"-"); + DisplayUint(file->addr + file->size, 2 * sizeof(file->addr)); + PrintStr(newline); + + efi_arch_handle_module(file, name, options); +} + static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name, struct file *file, char *options) { @@ -583,16 +599,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name, else { file->size = size; - if ( file != &cfg ) - { - PrintStr(name); - PrintStr(L": "); - DisplayUint(file->addr, 2 * sizeof(file->addr)); - PrintStr(L"-"); - DisplayUint(file->addr + size, 2 * sizeof(file->addr)); - PrintStr(newline); - efi_arch_handle_module(file, name, options); - } + handle_file_info(name, file, options); ret = FileHandle->Read(FileHandle, &file->size, file->ptr); if ( !EFI_ERROR(ret) && file->size != size ) -- 2.25.1 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH v4 2/4] efi/boot.c: add handle_file_info() 2020-09-14 11:50 ` [PATCH v4 2/4] efi/boot.c: add handle_file_info() Trammell Hudson @ 2020-09-16 6:46 ` Roger Pau Monné 2020-09-17 11:29 ` Jan Beulich 1 sibling, 0 replies; 25+ messages in thread From: Roger Pau Monné @ 2020-09-16 6:46 UTC (permalink / raw) To: Trammell Hudson; +Cc: xen-devel, jbeulich, andrew.cooper3, wl On Mon, Sep 14, 2020 at 07:50:11AM -0400, Trammell Hudson wrote: > Add a separate function to display the address ranges used by > the files and call `efi_arch_handle_module()` on the modules. > > Signed-off-by: Trammell Hudson <hudson@trmm.net> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks! ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v4 2/4] efi/boot.c: add handle_file_info() 2020-09-14 11:50 ` [PATCH v4 2/4] efi/boot.c: add handle_file_info() Trammell Hudson 2020-09-16 6:46 ` Roger Pau Monné @ 2020-09-17 11:29 ` Jan Beulich 1 sibling, 0 replies; 25+ messages in thread From: Jan Beulich @ 2020-09-17 11:29 UTC (permalink / raw) To: Trammell Hudson; +Cc: xen-devel, roger.pau, andrew.cooper3, wl On 14.09.2020 13:50, Trammell Hudson wrote: > --- a/xen/common/efi/boot.c > +++ b/xen/common/efi/boot.c > @@ -539,6 +539,22 @@ static char * __init split_string(char *s) > return NULL; > } > > +static void __init handle_file_info(CHAR16 *name, > + struct file *file, char *options) The latter two can become const once rebased over the patch I've just sent, and then Acked-by: Jan Beulich <jbeulich@suse.com> Jan ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v4 3/4] efi: Enable booting unified hypervisor/kernel/initrd images 2020-09-14 11:50 [PATCH v4 0/4] efi: Unified Xen hypervisor/kernel/initrd images Trammell Hudson 2020-09-14 11:50 ` [PATCH v4 1/4] efi/boot.c: add file.need_to_free Trammell Hudson 2020-09-14 11:50 ` [PATCH v4 2/4] efi/boot.c: add handle_file_info() Trammell Hudson @ 2020-09-14 11:50 ` Trammell Hudson 2020-09-16 7:32 ` Roger Pau Monné 2020-09-17 12:33 ` Jan Beulich 2020-09-14 11:50 ` [PATCH v4 4/4] efi: Do not use command line if secure boot is enabled Trammell Hudson 3 siblings, 2 replies; 25+ messages in thread From: Trammell Hudson @ 2020-09-14 11:50 UTC (permalink / raw) To: xen-devel; +Cc: roger.pau, jbeulich, andrew.cooper3, wl This patch adds support for bundling the xen.efi hypervisor, the xen.cfg configuration file, the Linux kernel and initrd, as well as the XSM, and architectural specific files into a single "unified" EFI executable. This allows an administrator to update the components independently without requiring rebuilding xen, as well as to replace the components in an existing image. The resulting EFI executable can be invoked directly from the UEFI Boot Manager, removing the need to use a separate loader like grub as well as removing dependencies on local filesystem access. And since it is a single file, it can be signed and validated by UEFI Secure Boot without requring the shim protocol. It is inspired by systemd-boot's unified kernel technique and borrows the function to locate PE sections from systemd's LGPL'ed code. During EFI boot, Xen looks at its own loaded image to locate the PE sections for the Xen configuration (`.config`), dom0 kernel (`.kernel`), dom0 initrd (`.ramdisk`), and XSM config (`.xsm`), which are included after building xen.efi using objcopy to add named sections for each input file. For x86, the CPU ucode can be included in a section named `.ucode`, which is loaded in the efi_arch_cfg_file_late() stage of the boot process. On ARM systems the Device Tree can be included in a section named `.dtb`, which is loaded during the efi_arch_cfg_file_early() stage of the boot process. Signed-off-by: Trammell Hudson <hudson@trmm.net> --- .gitignore | 1 + docs/misc/efi.pandoc | 49 +++++++++++++ xen/arch/arm/efi/efi-boot.h | 25 ++++--- xen/arch/x86/efi/Makefile | 2 +- xen/arch/x86/efi/efi-boot.h | 11 ++- xen/common/efi/boot.c | 73 ++++++++++++++----- xen/common/efi/efi.h | 3 + xen/common/efi/pe.c | 137 ++++++++++++++++++++++++++++++++++++ 8 files changed, 272 insertions(+), 29 deletions(-) create mode 100644 xen/common/efi/pe.c diff --git a/.gitignore b/.gitignore index 823f4743dc..d568017804 100644 --- a/.gitignore +++ b/.gitignore @@ -299,6 +299,7 @@ xen/arch/*/efi/boot.c xen/arch/*/efi/compat.c xen/arch/*/efi/ebmalloc.c xen/arch/*/efi/efi.h +xen/arch/*/efi/pe.c xen/arch/*/efi/runtime.c xen/common/config_data.S xen/common/config.gz diff --git a/docs/misc/efi.pandoc b/docs/misc/efi.pandoc index 23c1a2732d..ac3cd58cae 100644 --- a/docs/misc/efi.pandoc +++ b/docs/misc/efi.pandoc @@ -116,3 +116,52 @@ Filenames must be specified relative to the location of the EFI binary. Extra options to be passed to Xen can also be specified on the command line, following a `--` separator option. + +## Unified Xen kernel image + +The "Unified" kernel image can be generated by adding additional +sections to the Xen EFI executable with objcopy, similar to how +[systemd-boot uses the stub to add them to the Linux kernel](https://wiki.archlinux.org/index.php/systemd-boot#Preparing_a_unified_kernel_image) + +The sections for the xen configuration file, the dom0 kernel, dom0 initrd, +XSM and CPU microcode should be added after the Xen `.pad` section, the +ending address of which can be located with: + +``` +objdump -h xen.efi \ + | perl -ane '/\.pad/ && printf "0x%016x\n", hex($F[2]) + hex($F[3])' +``` + +For all the examples the `.pad` section ended at 0xffff82d041000000. +All the sections are optional (`.config`, `.kernel`, `.ramdisk`, `.xsm`, +`.ucode` (x86) and `.dtb` (ARM)) and the order does not matter. +The virtual addresses do not need to be contiguous, although they should not +be overlapping and should all be greater than the last virtual address of the +hypervisor components. + +``` +objcopy \ + --add-section .config=xen.cfg \ + --change-section-vma .config=0xffff82d041000000 + --add-section .ucode=ucode.bin \ + --change-section-vma .ucode=0xffff82d041010000 \ + --add-section .xsm=xsm.cfg \ + --change-section-vma .xsm=0xffff82d041080000 \ + --add-section .kernel=vmlinux \ + --change-section-vma .kernel=0xffff82d041100000 \ + --add-section .ramdisk=initrd.img \ + --change-section-vma .ramdisk=0xffff82d042000000 \ + xen.efi \ + xen.unified.efi +``` + +The unified executable can be signed with sbsigntool to make +it usable with UEFI secure boot: + +``` +sbsign \ + --key signing.key \ + --cert cert.pem \ + --output xen.signed.efi \ + xen.unified.efi +``` diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h index 6527cb0bdf..08bd4d7630 100644 --- a/xen/arch/arm/efi/efi-boot.h +++ b/xen/arch/arm/efi/efi-boot.h @@ -375,27 +375,36 @@ static void __init noreturn efi_arch_post_exit_boot(void) efi_xen_start(fdt, fdt_totalsize(fdt)); } -static void __init efi_arch_cfg_file_early(EFI_FILE_HANDLE dir_handle, char *section) +static void __init efi_arch_cfg_file_early(const EFI_LOADED_IMAGE *image, + EFI_FILE_HANDLE dir_handle, + char *section) { union string name; /* * The DTB must be processed before any other entries in the configuration - * file, as the DTB is updated as modules are loaded. + * file, as the DTB is updated as modules are loaded. Prefer the one + * stored as a PE section in a unified image, and fall back to a file + * on disk if the section is not present. */ - name.s = get_value(&cfg, section, "dtb"); - if ( name.s ) + if ( !read_section(image, ".dtb", &dtbfile, NULL) ) { - split_string(name.s); - read_file(dir_handle, s2w(&name), &dtbfile, NULL); - efi_bs->FreePool(name.w); + name.s = get_value(&cfg, section, "dtb"); + if ( name.s ) + { + split_string(name.s); + read_file(dir_handle, s2w(&name), &dtbfile, NULL); + efi_bs->FreePool(name.w); + } } fdt = fdt_increase_size(&dtbfile, cfg.size + EFI_PAGE_SIZE); if ( !fdt ) blexit(L"Unable to create new FDT"); } -static void __init efi_arch_cfg_file_late(EFI_FILE_HANDLE dir_handle, char *section) +static void __init efi_arch_cfg_file_late(const EFI_LOADED_IMAGE *image, + EFI_FILE_HANDLE dir_handle, + char *section) { } diff --git a/xen/arch/x86/efi/Makefile b/xen/arch/x86/efi/Makefile index 770438a029..e857c0f2cc 100644 --- a/xen/arch/x86/efi/Makefile +++ b/xen/arch/x86/efi/Makefile @@ -8,7 +8,7 @@ cmd_objcopy_o_ihex = $(OBJCOPY) -I ihex -O binary $< $@ boot.init.o: buildid.o -EFIOBJ := boot.init.o ebmalloc.o compat.o runtime.o +EFIOBJ := boot.init.o pe.init.o ebmalloc.o compat.o runtime.o $(call cc-option-add,cflags-stack-boundary,CC,-mpreferred-stack-boundary=4) $(EFIOBJ): CFLAGS-stack-boundary := $(cflags-stack-boundary) diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h index 7188c9a551..9ab69f84d4 100644 --- a/xen/arch/x86/efi/efi-boot.h +++ b/xen/arch/x86/efi/efi-boot.h @@ -272,14 +272,21 @@ static void __init noreturn efi_arch_post_exit_boot(void) unreachable(); } -static void __init efi_arch_cfg_file_early(EFI_FILE_HANDLE dir_handle, char *section) +static void __init efi_arch_cfg_file_early(const EFI_LOADED_IMAGE *image, + EFI_FILE_HANDLE dir_handle, + char *section) { } -static void __init efi_arch_cfg_file_late(EFI_FILE_HANDLE dir_handle, char *section) +static void __init efi_arch_cfg_file_late(const EFI_LOADED_IMAGE *image, + EFI_FILE_HANDLE dir_handle, + char *section) { union string name; + if ( read_section(image, ".ucode", &ucode, NULL) ) + return; + name.s = get_value(&cfg, section, "ucode"); if ( !name.s ) name.s = get_value(&cfg, "global", "ucode"); diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c index 57df89cacb..4b1fbc9643 100644 --- a/xen/common/efi/boot.c +++ b/xen/common/efi/boot.c @@ -121,6 +121,8 @@ static CHAR16 *s2w(union string *str); static char *w2s(const union string *str); static bool read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name, struct file *file, char *options); +static bool read_section(const EFI_LOADED_IMAGE *image, + char *name, struct file *file, char *options); static size_t wstrlen(const CHAR16 * s); static int set_color(u32 mask, int bpp, u8 *pos, u8 *sz); static bool match_guid(const EFI_GUID *guid1, const EFI_GUID *guid2); @@ -623,6 +625,27 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name, return true; } +static bool __init read_section(const EFI_LOADED_IMAGE *image, + char *const name, struct file *file, + char *options) +{ + /* skip the leading "." in the section name */ + union string name_string = { .s = name + 1 }; + + file->ptr = (void *)pe_find_section(image->ImageBase, image->ImageSize, + name, &file->size); + if ( !file->ptr ) + return false; + + file->need_to_free = false; + + s2w(&name_string); + handle_file_info(name_string.w, file, options); + efi_bs->FreePool(name_string.w); + + return true; +} + static void __init pre_parse(const struct file *cfg) { char *ptr = cfg->ptr, *end = ptr + cfg->size; @@ -1207,9 +1230,13 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) /* Get the file system interface. */ dir_handle = get_parent_handle(loaded_image, &file_name); - /* Read and parse the config file. */ - if ( !cfg_file_name ) + if ( read_section(loaded_image, ".config", &cfg, NULL) ) + { + PrintStr(L"Using unified config file\r\n"); + } + else if ( !cfg_file_name ) { + /* Read and parse the config file. */ CHAR16 *tail; while ( (tail = point_tail(file_name)) != NULL ) @@ -1258,29 +1285,39 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) if ( !name.s ) blexit(L"No Dom0 kernel image specified."); - efi_arch_cfg_file_early(dir_handle, section.s); + efi_arch_cfg_file_early(loaded_image, dir_handle, section.s); option_str = split_string(name.s); - read_file(dir_handle, s2w(&name), &kernel, option_str); - efi_bs->FreePool(name.w); - - if ( !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL, - (void **)&shim_lock)) && - (status = shim_lock->Verify(kernel.ptr, kernel.size)) != EFI_SUCCESS ) - PrintErrMesg(L"Dom0 kernel image could not be verified", status); - name.s = get_value(&cfg, section.s, "ramdisk"); - if ( name.s ) + if ( !read_section(loaded_image, ".kernel", &kernel, option_str) ) { - read_file(dir_handle, s2w(&name), &ramdisk, NULL); + read_file(dir_handle, s2w(&name), &kernel, option_str); efi_bs->FreePool(name.w); + + if ( !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL, + (void **)&shim_lock)) && + (status = shim_lock->Verify(kernel.ptr, kernel.size)) != EFI_SUCCESS ) + PrintErrMesg(L"Dom0 kernel image could not be verified", status); } - name.s = get_value(&cfg, section.s, "xsm"); - if ( name.s ) + if ( !read_section(loaded_image, ".ramdisk", &ramdisk, NULL) ) { - read_file(dir_handle, s2w(&name), &xsm, NULL); - efi_bs->FreePool(name.w); + name.s = get_value(&cfg, section.s, "ramdisk"); + if ( name.s ) + { + read_file(dir_handle, s2w(&name), &ramdisk, NULL); + efi_bs->FreePool(name.w); + } + } + + if ( !read_section(loaded_image, ".xsm", &xsm, NULL) ) + { + name.s = get_value(&cfg, section.s, "xsm"); + if ( name.s ) + { + read_file(dir_handle, s2w(&name), &xsm, NULL); + efi_bs->FreePool(name.w); + } } /* @@ -1316,7 +1353,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) } } - efi_arch_cfg_file_late(dir_handle, section.s); + efi_arch_cfg_file_late(loaded_image, dir_handle, section.s); efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size)); cfg.addr = 0; diff --git a/xen/common/efi/efi.h b/xen/common/efi/efi.h index 4845d84913..c9b741bf27 100644 --- a/xen/common/efi/efi.h +++ b/xen/common/efi/efi.h @@ -47,3 +47,6 @@ const CHAR16 *wmemchr(const CHAR16 *s, CHAR16 c, UINTN n); /* EFI boot allocator. */ void *ebmalloc(size_t size); void free_ebmalloc_unused_mem(void); + +const void * pe_find_section(const UINT8 *image_base, const size_t image_size, + const char *section_name, UINTN *size_out); diff --git a/xen/common/efi/pe.c b/xen/common/efi/pe.c new file mode 100644 index 0000000000..2986545d53 --- /dev/null +++ b/xen/common/efi/pe.c @@ -0,0 +1,137 @@ +/* + * xen/common/efi/pe.c + * + * PE executable header parser. + * + * Derived from https://github.com/systemd/systemd/blob/master/src/boot/efi/pe.c + * commit 07d5ed536ec0a76b08229c7a80b910cb9acaf6b1 + * + * Copyright (C) 2015 Kay Sievers <kay@vrfy.org> + * Copyright (C) 2020 Trammell Hudson <hudson@trmm.net> + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU Lesser General Public License as published by + * the Free Software Foundation; either version 2.1 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + */ + + +#include "efi.h" + +struct DosFileHeader { + UINT8 Magic[2]; + UINT16 LastSize; + UINT16 nBlocks; + UINT16 nReloc; + UINT16 HdrSize; + UINT16 MinAlloc; + UINT16 MaxAlloc; + UINT16 ss; + UINT16 sp; + UINT16 Checksum; + UINT16 ip; + UINT16 cs; + UINT16 RelocPos; + UINT16 nOverlay; + UINT16 reserved[4]; + UINT16 OEMId; + UINT16 OEMInfo; + UINT16 reserved2[10]; + UINT32 ExeHeader; +} __attribute__((packed)); + +#define PE_HEADER_MACHINE_ARM64 0xaa64 +#define PE_HEADER_MACHINE_X64 0x8664 +#define PE_HEADER_MACHINE_I386 0x014c + +struct PeFileHeader { + UINT16 Machine; + UINT16 NumberOfSections; + UINT32 TimeDateStamp; + UINT32 PointerToSymbolTable; + UINT32 NumberOfSymbols; + UINT16 SizeOfOptionalHeader; + UINT16 Characteristics; +} __attribute__((packed)); + +struct PeHeader { + UINT8 Magic[4]; + struct PeFileHeader FileHeader; +} __attribute__((packed)); + +struct PeSectionHeader { + UINT8 Name[8]; + UINT32 VirtualSize; + UINT32 VirtualAddress; + UINT32 SizeOfRawData; + UINT32 PointerToRawData; + UINT32 PointerToRelocations; + UINT32 PointerToLinenumbers; + UINT16 NumberOfRelocations; + UINT16 NumberOfLinenumbers; + UINT32 Characteristics; +} __attribute__((packed)); + +const void *__init pe_find_section(const CHAR8 *image, const UINTN image_size, + const char *section_name, UINTN *size_out) +{ + const struct DosFileHeader *dos = (const void*)image; + const struct PeHeader *pe; + const struct PeSectionHeader *sect; + const UINTN name_len = strlen(section_name); + UINTN offset = 0; + UINTN i; + + if ( name_len > sizeof(sect->Name) || + image_size < sizeof(*dos) || + memcmp(dos->Magic, "MZ", 2) != 0 ) + return NULL; + + offset = dos->ExeHeader; + pe = (const void *) &image[offset]; + + offset += sizeof(*pe); + if ( image_size < offset || + memcmp(pe->Magic, "PE\0\0", 4) != 0 ) + return NULL; + + /* PE32+ Subsystem type */ +#if defined(__arm__) || defined (__aarch64__) + if ( pe->FileHeader.Machine != PE_HEADER_MACHINE_ARM64 ) + return NULL; +#elif defined(__x86_64__) + if ( pe->FileHeader.Machine != PE_HEADER_MACHINE_X64 ) + return NULL; +#else + /* unknown architecture */ + return NULL; +#endif + + offset += pe->FileHeader.SizeOfOptionalHeader; + + for ( i = 0; i < pe->FileHeader.NumberOfSections; i++ ) + { + sect = (const void *)&image[offset]; + if ( image_size < offset + sizeof(*sect) ) + return NULL; + + if ( memcmp(sect->Name, section_name, name_len) != 0 || + image_size < sect->VirtualSize + sect->VirtualAddress ) + { + offset += sizeof(*sect); + continue; + } + + if ( size_out ) + *size_out = sect->VirtualSize; + + return &image[sect->VirtualAddress]; + } + + return NULL; +} -- 2.25.1 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH v4 3/4] efi: Enable booting unified hypervisor/kernel/initrd images 2020-09-14 11:50 ` [PATCH v4 3/4] efi: Enable booting unified hypervisor/kernel/initrd images Trammell Hudson @ 2020-09-16 7:32 ` Roger Pau Monné 2020-09-16 8:37 ` Trammell Hudson 2020-09-17 12:33 ` Jan Beulich 1 sibling, 1 reply; 25+ messages in thread From: Roger Pau Monné @ 2020-09-16 7:32 UTC (permalink / raw) To: Trammell Hudson; +Cc: xen-devel, jbeulich, andrew.cooper3, wl On Mon, Sep 14, 2020 at 07:50:12AM -0400, Trammell Hudson wrote: > This patch adds support for bundling the xen.efi hypervisor, the xen.cfg > configuration file, the Linux kernel and initrd, as well as the XSM, > and architectural specific files into a single "unified" EFI executable. > This allows an administrator to update the components independently > without requiring rebuilding xen, as well as to replace the components > in an existing image. > > The resulting EFI executable can be invoked directly from the UEFI Boot > Manager, removing the need to use a separate loader like grub as well > as removing dependencies on local filesystem access. And since it is > a single file, it can be signed and validated by UEFI Secure Boot without > requring the shim protocol. > > It is inspired by systemd-boot's unified kernel technique and borrows the > function to locate PE sections from systemd's LGPL'ed code. During EFI > boot, Xen looks at its own loaded image to locate the PE sections for > the Xen configuration (`.config`), dom0 kernel (`.kernel`), dom0 initrd > (`.ramdisk`), and XSM config (`.xsm`), which are included after building > xen.efi using objcopy to add named sections for each input file. > > For x86, the CPU ucode can be included in a section named `.ucode`, > which is loaded in the efi_arch_cfg_file_late() stage of the boot process. > > On ARM systems the Device Tree can be included in a section named > `.dtb`, which is loaded during the efi_arch_cfg_file_early() stage of > the boot process. > > Signed-off-by: Trammell Hudson <hudson@trmm.net> Thanks, just have one comment and two style nits. > diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c > index 57df89cacb..4b1fbc9643 100644 > --- a/xen/common/efi/boot.c > +++ b/xen/common/efi/boot.c > @@ -121,6 +121,8 @@ static CHAR16 *s2w(union string *str); > static char *w2s(const union string *str); > static bool read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name, > struct file *file, char *options); > +static bool read_section(const EFI_LOADED_IMAGE *image, > + char *name, struct file *file, char *options); > static size_t wstrlen(const CHAR16 * s); > static int set_color(u32 mask, int bpp, u8 *pos, u8 *sz); > static bool match_guid(const EFI_GUID *guid1, const EFI_GUID *guid2); > @@ -623,6 +625,27 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name, > return true; > } > > +static bool __init read_section(const EFI_LOADED_IMAGE *image, > + char *const name, struct file *file, > + char *options) > +{ > + /* skip the leading "." in the section name */ > + union string name_string = { .s = name + 1 }; > + > + file->ptr = (void *)pe_find_section(image->ImageBase, image->ImageSize, > + name, &file->size); > + if ( !file->ptr ) > + return false; > + > + file->need_to_free = false; > + > + s2w(&name_string); Don't you need to check that s2w succeed, so that name_string.w is not a random pointer from stack garbage? > + handle_file_info(name_string.w, file, options); > + efi_bs->FreePool(name_string.w); > + > + return true; > +} > + > static void __init pre_parse(const struct file *cfg) > { > char *ptr = cfg->ptr, *end = ptr + cfg->size; > index 4845d84913..c9b741bf27 100644 > --- a/xen/common/efi/efi.h > +++ b/xen/common/efi/efi.h > @@ -47,3 +47,6 @@ const CHAR16 *wmemchr(const CHAR16 *s, CHAR16 c, UINTN n); > /* EFI boot allocator. */ > void *ebmalloc(size_t size); > void free_ebmalloc_unused_mem(void); > + > +const void * pe_find_section(const UINT8 *image_base, const size_t image_size, > + const char *section_name, UINTN *size_out); Nit: extra space between * and function name. > diff --git a/xen/common/efi/pe.c b/xen/common/efi/pe.c > new file mode 100644 > index 0000000000..2986545d53 > --- /dev/null > +++ b/xen/common/efi/pe.c > @@ -0,0 +1,137 @@ > +/* > + * xen/common/efi/pe.c > + * > + * PE executable header parser. > + * > + * Derived from https://github.com/systemd/systemd/blob/master/src/boot/efi/pe.c > + * commit 07d5ed536ec0a76b08229c7a80b910cb9acaf6b1 > + * > + * Copyright (C) 2015 Kay Sievers <kay@vrfy.org> > + * Copyright (C) 2020 Trammell Hudson <hudson@trmm.net> > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms of the GNU Lesser General Public License as published by > + * the Free Software Foundation; either version 2.1 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, but > + * WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * Lesser General Public License for more details. > + */ > + > + > +#include "efi.h" > + > +struct DosFileHeader { > + UINT8 Magic[2]; > + UINT16 LastSize; > + UINT16 nBlocks; > + UINT16 nReloc; > + UINT16 HdrSize; > + UINT16 MinAlloc; > + UINT16 MaxAlloc; > + UINT16 ss; > + UINT16 sp; > + UINT16 Checksum; > + UINT16 ip; > + UINT16 cs; > + UINT16 RelocPos; > + UINT16 nOverlay; > + UINT16 reserved[4]; > + UINT16 OEMId; > + UINT16 OEMInfo; > + UINT16 reserved2[10]; > + UINT32 ExeHeader; > +} __attribute__((packed)); > + > +#define PE_HEADER_MACHINE_ARM64 0xaa64 > +#define PE_HEADER_MACHINE_X64 0x8664 > +#define PE_HEADER_MACHINE_I386 0x014c > + > +struct PeFileHeader { > + UINT16 Machine; > + UINT16 NumberOfSections; > + UINT32 TimeDateStamp; > + UINT32 PointerToSymbolTable; > + UINT32 NumberOfSymbols; > + UINT16 SizeOfOptionalHeader; > + UINT16 Characteristics; > +} __attribute__((packed)); > + > +struct PeHeader { > + UINT8 Magic[4]; > + struct PeFileHeader FileHeader; > +} __attribute__((packed)); > + > +struct PeSectionHeader { > + UINT8 Name[8]; > + UINT32 VirtualSize; > + UINT32 VirtualAddress; > + UINT32 SizeOfRawData; > + UINT32 PointerToRawData; > + UINT32 PointerToRelocations; > + UINT32 PointerToLinenumbers; > + UINT16 NumberOfRelocations; > + UINT16 NumberOfLinenumbers; > + UINT32 Characteristics; > +} __attribute__((packed)); > + > +const void *__init pe_find_section(const CHAR8 *image, const UINTN image_size, > + const char *section_name, UINTN *size_out) > +{ > + const struct DosFileHeader *dos = (const void*)image; Nit: missing space between void and *. Thanks, Roger. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v4 3/4] efi: Enable booting unified hypervisor/kernel/initrd images 2020-09-16 7:32 ` Roger Pau Monné @ 2020-09-16 8:37 ` Trammell Hudson 2020-09-16 9:47 ` Jan Beulich 2020-09-16 10:15 ` Roger Pau Monné 0 siblings, 2 replies; 25+ messages in thread From: Trammell Hudson @ 2020-09-16 8:37 UTC (permalink / raw) To: Roger Pau Monné; +Cc: xen-devel, jbeulich, andrew.cooper3, wl On Wednesday, September 16, 2020 3:32 AM, Roger Pau Monné <roger.pau@citrix.com> wrote: > On Mon, Sep 14, 2020 at 07:50:12AM -0400, Trammell Hudson wrote: > > - s2w(&name_string); > > Don't you need to check that s2w succeed, so that name_string.w is not > a random pointer from stack garbage? Maybe? I don't see anywhere else in the code that s2w() is ever checked for a NULL return. Perhaps a better fix would be to modify the function to panic if it is unable to allocate. > > - const char *section_name, UINTN *size_out); > > Nit: extra space between * and function name. Ok. Both will be fixed in a v5. -- Trammell ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v4 3/4] efi: Enable booting unified hypervisor/kernel/initrd images 2020-09-16 8:37 ` Trammell Hudson @ 2020-09-16 9:47 ` Jan Beulich 2020-09-16 10:15 ` Roger Pau Monné 1 sibling, 0 replies; 25+ messages in thread From: Jan Beulich @ 2020-09-16 9:47 UTC (permalink / raw) To: Trammell Hudson; +Cc: Roger Pau Monné, xen-devel, andrew.cooper3, wl On 16.09.2020 10:37, Trammell Hudson wrote: > On Wednesday, September 16, 2020 3:32 AM, Roger Pau Monné <roger.pau@citrix.com> wrote: >> On Mon, Sep 14, 2020 at 07:50:12AM -0400, Trammell Hudson wrote: >>> - s2w(&name_string); >> >> Don't you need to check that s2w succeed, so that name_string.w is not >> a random pointer from stack garbage? > > Maybe? I don't see anywhere else in the code that s2w() is > ever checked for a NULL return. Perhaps a better fix would > be to modify the function to panic if it is unable > to allocate. In current code the result of s2w() gets exclusively passed to read_file(), where first thing we have if ( !name ) PrintErrMesg(L"No filename", EFI_OUT_OF_RESOURCES); Jan ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v4 3/4] efi: Enable booting unified hypervisor/kernel/initrd images 2020-09-16 8:37 ` Trammell Hudson 2020-09-16 9:47 ` Jan Beulich @ 2020-09-16 10:15 ` Roger Pau Monné 1 sibling, 0 replies; 25+ messages in thread From: Roger Pau Monné @ 2020-09-16 10:15 UTC (permalink / raw) To: Trammell Hudson; +Cc: xen-devel, jbeulich, andrew.cooper3, wl On Wed, Sep 16, 2020 at 08:37:44AM +0000, Trammell Hudson wrote: > On Wednesday, September 16, 2020 3:32 AM, Roger Pau Monné <roger.pau@citrix.com> wrote: > > On Mon, Sep 14, 2020 at 07:50:12AM -0400, Trammell Hudson wrote: > > > - s2w(&name_string); > > > > Don't you need to check that s2w succeed, so that name_string.w is not > > a random pointer from stack garbage? > > Maybe? I don't see anywhere else in the code that s2w() is > ever checked for a NULL return. I see some callers pass the return of s2w() straight into read_file which will check that's not NULL before proceeding, or else call PrintErrMesg which won't return. > Perhaps a better fix would > be to modify the function to panic if it is unable > to allocate. Just doing what read_file does and use PrintErrMesg seems fine to me. Roger. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v4 3/4] efi: Enable booting unified hypervisor/kernel/initrd images 2020-09-14 11:50 ` [PATCH v4 3/4] efi: Enable booting unified hypervisor/kernel/initrd images Trammell Hudson 2020-09-16 7:32 ` Roger Pau Monné @ 2020-09-17 12:33 ` Jan Beulich 2020-09-17 13:04 ` Trammell Hudson 1 sibling, 1 reply; 25+ messages in thread From: Jan Beulich @ 2020-09-17 12:33 UTC (permalink / raw) To: Trammell Hudson; +Cc: xen-devel, roger.pau, andrew.cooper3, wl On 14.09.2020 13:50, Trammell Hudson wrote: > --- a/docs/misc/efi.pandoc > +++ b/docs/misc/efi.pandoc > @@ -116,3 +116,52 @@ Filenames must be specified relative to the location of the EFI binary. > > Extra options to be passed to Xen can also be specified on the command line, > following a `--` separator option. > + > +## Unified Xen kernel image > + > +The "Unified" kernel image can be generated by adding additional > +sections to the Xen EFI executable with objcopy, similar to how > +[systemd-boot uses the stub to add them to the Linux kernel](https://wiki.archlinux.org/index.php/systemd-boot#Preparing_a_unified_kernel_image) > + > +The sections for the xen configuration file, the dom0 kernel, dom0 initrd, > +XSM and CPU microcode should be added after the Xen `.pad` section, the > +ending address of which can be located with: > + > +``` > +objdump -h xen.efi \ > + | perl -ane '/\.pad/ && printf "0x%016x\n", hex($F[2]) + hex($F[3])' > +``` > + > +For all the examples the `.pad` section ended at 0xffff82d041000000. > +All the sections are optional (`.config`, `.kernel`, `.ramdisk`, `.xsm`, > +`.ucode` (x86) and `.dtb` (ARM)) and the order does not matter. > +The virtual addresses do not need to be contiguous, although they should not > +be overlapping and should all be greater than the last virtual address of the > +hypervisor components. The .pad section is there really only for padding the image. Its space could in principle be used for placing useful stuff (and hence to limit overall in-memory image size). That said, there is a plan for a change which may involve using the initial part of .pad, but that's not certain yet. I'm pointing this out to clarify that there may be a valid reason to avoid re-using the .pad space, at least for now. > --- a/xen/arch/arm/efi/efi-boot.h > +++ b/xen/arch/arm/efi/efi-boot.h > @@ -375,27 +375,36 @@ static void __init noreturn efi_arch_post_exit_boot(void) > efi_xen_start(fdt, fdt_totalsize(fdt)); > } > > -static void __init efi_arch_cfg_file_early(EFI_FILE_HANDLE dir_handle, char *section) > +static void __init efi_arch_cfg_file_early(const EFI_LOADED_IMAGE *image, > + EFI_FILE_HANDLE dir_handle, > + char *section) Could I talk you into constifying "section" at this occasion - afaics there should be no fallout here or in the other three places where the same would apply. > --- a/xen/arch/x86/efi/efi-boot.h > +++ b/xen/arch/x86/efi/efi-boot.h > @@ -272,14 +272,21 @@ static void __init noreturn efi_arch_post_exit_boot(void) > unreachable(); > } > > -static void __init efi_arch_cfg_file_early(EFI_FILE_HANDLE dir_handle, char *section) > +static void __init efi_arch_cfg_file_early(const EFI_LOADED_IMAGE *image, > + EFI_FILE_HANDLE dir_handle, > + char *section) > { > } > > -static void __init efi_arch_cfg_file_late(EFI_FILE_HANDLE dir_handle, char *section) > +static void __init efi_arch_cfg_file_late(const EFI_LOADED_IMAGE *image, > + EFI_FILE_HANDLE dir_handle, > + char *section) > { > union string name; > > + if ( read_section(image, ".ucode", &ucode, NULL) ) > + return; > + > name.s = get_value(&cfg, section, "ucode"); With the Arm change already in mind and with further similar changes further down, may I suggest to consider passing 'section' into read_section(), thus guaranteeing consistent naming between image section and config file items, not only now but also going forward? read_section() would then check for the leading dot followed by the specified name. > --- a/xen/common/efi/boot.c > +++ b/xen/common/efi/boot.c > @@ -121,6 +121,8 @@ static CHAR16 *s2w(union string *str); > static char *w2s(const union string *str); > static bool read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name, > struct file *file, char *options); > +static bool read_section(const EFI_LOADED_IMAGE *image, > + char *name, struct file *file, char *options); > static size_t wstrlen(const CHAR16 * s); > static int set_color(u32 mask, int bpp, u8 *pos, u8 *sz); > static bool match_guid(const EFI_GUID *guid1, const EFI_GUID *guid2); > @@ -623,6 +625,27 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name, > return true; > } > > +static bool __init read_section(const EFI_LOADED_IMAGE *image, > + char *const name, struct file *file, > + char *options) > +{ > + /* skip the leading "." in the section name */ > + union string name_string = { .s = name + 1 }; > + > + file->ptr = (void *)pe_find_section(image->ImageBase, image->ImageSize, > + name, &file->size); This casting away of const-ness worries me. The sole reason why the "ptr" member is non-const looks to be the two parsing functions for the config file. How about we make "ptr" const void * and add a new "char *str" field? While it won't _guarantee_ correct code to be written, it at least allows doing so. > @@ -1207,9 +1230,13 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) > /* Get the file system interface. */ > dir_handle = get_parent_handle(loaded_image, &file_name); > > - /* Read and parse the config file. */ > - if ( !cfg_file_name ) > + if ( read_section(loaded_image, ".config", &cfg, NULL) ) > + { > + PrintStr(L"Using unified config file\r\n"); > + } Please omit the braces here. > @@ -1258,29 +1285,39 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) > if ( !name.s ) > blexit(L"No Dom0 kernel image specified."); > > - efi_arch_cfg_file_early(dir_handle, section.s); > + efi_arch_cfg_file_early(loaded_image, dir_handle, section.s); > > option_str = split_string(name.s); > - read_file(dir_handle, s2w(&name), &kernel, option_str); > - efi_bs->FreePool(name.w); > - > - if ( !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL, > - (void **)&shim_lock)) && > - (status = shim_lock->Verify(kernel.ptr, kernel.size)) != EFI_SUCCESS ) > - PrintErrMesg(L"Dom0 kernel image could not be verified", status); > > - name.s = get_value(&cfg, section.s, "ramdisk"); > - if ( name.s ) > + if ( !read_section(loaded_image, ".kernel", &kernel, option_str) ) > { > - read_file(dir_handle, s2w(&name), &ramdisk, NULL); > + read_file(dir_handle, s2w(&name), &kernel, option_str); As before, I disagree with the idea of taking pieces from disk and pieces from the unified image. If you continue to think this is a reasonable thing to do, may I ask that you add a rationale of this model to the description? And btw, my worry looks to not be without reason, since ... > efi_bs->FreePool(name.w); > + > + if ( !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL, > + (void **)&shim_lock)) && > + (status = shim_lock->Verify(kernel.ptr, kernel.size)) != EFI_SUCCESS ) > + PrintErrMesg(L"Dom0 kernel image could not be verified", status); > } > > - name.s = get_value(&cfg, section.s, "xsm"); > - if ( name.s ) > + if ( !read_section(loaded_image, ".ramdisk", &ramdisk, NULL) ) > { > - read_file(dir_handle, s2w(&name), &xsm, NULL); > - efi_bs->FreePool(name.w); > + name.s = get_value(&cfg, section.s, "ramdisk"); > + if ( name.s ) > + { > + read_file(dir_handle, s2w(&name), &ramdisk, NULL); > + efi_bs->FreePool(name.w); > + } > + } ... the RAM disk (just taken as example) is optional. How do you express the unified image's explicit intention to have no RAM disk with this fallback approach? FAOD, even if objcopy allows to add empty sections (didn't check), I'd consider an empty section different from an absent one, i.e. the former meaning "empty RAM disk" while the latter says "no RAM disk". > + if ( !read_section(loaded_image, ".xsm", &xsm, NULL) ) > + { > + name.s = get_value(&cfg, section.s, "xsm"); > + if ( name.s ) > + { > + read_file(dir_handle, s2w(&name), &xsm, NULL); > + efi_bs->FreePool(name.w); > + } > } The fallback approach may even have security implications here, as (afaik) an XSM policy can also be used to increase privileges. The builder of unified image, in omitting an XSM policy, may certainly mean "use built-in defaults" rather than intending to allow further overriding. > --- /dev/null > +++ b/xen/common/efi/pe.c > @@ -0,0 +1,137 @@ > +/* > + * xen/common/efi/pe.c > + * > + * PE executable header parser. > + * > + * Derived from https://github.com/systemd/systemd/blob/master/src/boot/efi/pe.c > + * commit 07d5ed536ec0a76b08229c7a80b910cb9acaf6b1 > + * > + * Copyright (C) 2015 Kay Sievers <kay@vrfy.org> > + * Copyright (C) 2020 Trammell Hudson <hudson@trmm.net> > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms of the GNU Lesser General Public License as published by > + * the Free Software Foundation; either version 2.1 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, but > + * WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * Lesser General Public License for more details. > + */ > + > + > +#include "efi.h" > + > +struct DosFileHeader { > + UINT8 Magic[2]; > + UINT16 LastSize; > + UINT16 nBlocks; > + UINT16 nReloc; > + UINT16 HdrSize; > + UINT16 MinAlloc; > + UINT16 MaxAlloc; > + UINT16 ss; > + UINT16 sp; > + UINT16 Checksum; > + UINT16 ip; > + UINT16 cs; > + UINT16 RelocPos; > + UINT16 nOverlay; > + UINT16 reserved[4]; > + UINT16 OEMId; > + UINT16 OEMInfo; > + UINT16 reserved2[10]; > + UINT32 ExeHeader; > +} __attribute__((packed)); > + > +#define PE_HEADER_MACHINE_ARM64 0xaa64 > +#define PE_HEADER_MACHINE_X64 0x8664 > +#define PE_HEADER_MACHINE_I386 0x014c This list isn't meant to be a complete one anyway, so please omit the I386 item as it's not needed anywhere. > +struct PeFileHeader { > + UINT16 Machine; > + UINT16 NumberOfSections; > + UINT32 TimeDateStamp; > + UINT32 PointerToSymbolTable; > + UINT32 NumberOfSymbols; > + UINT16 SizeOfOptionalHeader; > + UINT16 Characteristics; > +} __attribute__((packed)); > + > +struct PeHeader { > + UINT8 Magic[4]; > + struct PeFileHeader FileHeader; > +} __attribute__((packed)); At the example of these two (i.e. may extend to others): When the packed attribute doesn't really have any impact on structure layout (and will just adversely affect alignof() when applied to the struct or any of the fields), please omit it. We had a number of such pointless attributes in the tree, and we had to drop them for one of the more recent gcc versions to actually still compile our code without warnings (in fact errors, due to out use of -Werror). > +struct PeSectionHeader { > + UINT8 Name[8]; Better char? > +const void *__init pe_find_section(const CHAR8 *image, const UINTN image_size, > + const char *section_name, UINTN *size_out) > +{ > + const struct DosFileHeader *dos = (const void*)image; If the type of "image" was "const void *", there wouldn't be any cast needed here (and again further down). And I don't think you actually need "image" to be a pointer to a particular type? Of course ... > + const struct PeHeader *pe; > + const struct PeSectionHeader *sect; > + const UINTN name_len = strlen(section_name); > + UINTN offset = 0; Unnecessary initializer, and please fold ... > + UINTN i; ... with this line. > + if ( name_len > sizeof(sect->Name) || > + image_size < sizeof(*dos) || > + memcmp(dos->Magic, "MZ", 2) != 0 ) > + return NULL; > + > + offset = dos->ExeHeader; > + pe = (const void *) &image[offset]; ... this then needs to become "image + offset". > + > + offset += sizeof(*pe); > + if ( image_size < offset || > + memcmp(pe->Magic, "PE\0\0", 4) != 0 ) > + return NULL; > + > + /* PE32+ Subsystem type */ > +#if defined(__arm__) || defined (__aarch64__) > + if ( pe->FileHeader.Machine != PE_HEADER_MACHINE_ARM64 ) > + return NULL; > +#elif defined(__x86_64__) > + if ( pe->FileHeader.Machine != PE_HEADER_MACHINE_X64 ) > + return NULL; > +#else > + /* unknown architecture */ > + return NULL; > +#endif Instead of this, further up please #define a single constant (e.g. PE_HEADER_MACHINE) to check against without any #ifdef-ary here. This then also should lead to a build error (instead of the function returning NULL at runtime) when no enabling was done for a possible future port. > + offset += pe->FileHeader.SizeOfOptionalHeader; > + > + for ( i = 0; i < pe->FileHeader.NumberOfSections; i++ ) > + { > + sect = (const void *)&image[offset]; Please limit the scope of sect to the body of this loop, at which point this assignment can become the initializer. > + if ( image_size < offset + sizeof(*sect) ) > + return NULL; > + > + if ( memcmp(sect->Name, section_name, name_len) != 0 || > + image_size < sect->VirtualSize + sect->VirtualAddress ) Wouldn't this latter part of the condition better be treated as an error, rather than getting silently ignored? The more if the falling back to on-disk files got retained? Jan ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v4 3/4] efi: Enable booting unified hypervisor/kernel/initrd images 2020-09-17 12:33 ` Jan Beulich @ 2020-09-17 13:04 ` Trammell Hudson 2020-09-17 13:33 ` Trammell Hudson 2020-09-17 15:21 ` Jan Beulich 0 siblings, 2 replies; 25+ messages in thread From: Trammell Hudson @ 2020-09-17 13:04 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, roger.pau, andrew.cooper3, wl On Thursday, September 17, 2020 8:33 AM, Jan Beulich <jbeulich@suse.com> wrote: > On 14.09.2020 13:50, Trammell Hudson wrote: > [...] > > +For all the examples the `.pad` section ended at 0xffff82d041000000. > > +All the sections are optional (`.config`, `.kernel`, `.ramdisk`, `.xsm`, > > +`.ucode` (x86) and `.dtb` (ARM)) and the order does not matter. > > +The virtual addresses do not need to be contiguous, although they should not > > +be overlapping and should all be greater than the last virtual address of the > > +hypervisor components. > > The .pad section is there really only for padding the image. Its space > could in principle be used for placing useful stuff (and hence to limit > overall in-memory image size). That said, there is a plan for a change > which may involve using the initial part of .pad, but that's not certain > yet. I'm pointing this out to clarify that there may be a valid reason > to avoid re-using the .pad space, at least for now. The instructions show how to append after the .pad region, so there is no reuse of that space. I wish objcopy had an --append-region option so that the user didn't have to do this extra math and adjust sizes. > > --- a/xen/arch/arm/efi/efi-boot.h > > +++ b/xen/arch/arm/efi/efi-boot.h > > @@ -375,27 +375,36 @@ static void __init noreturn efi_arch_post_exit_boot(void) > > efi_xen_start(fdt, fdt_totalsize(fdt)); > > } > > -static void __init efi_arch_cfg_file_early(EFI_FILE_HANDLE dir_handle, char *section) > > +static void __init efi_arch_cfg_file_early(const EFI_LOADED_IMAGE *image, > > > > - EFI_FILE_HANDLE dir_handle, > > > > > > - char *section) > > > > > > Could I talk you into constifying "section" at this occasion - afaics > there should be no fallout here or in the other three places where > the same would apply. I'm always in favor of adding more constness. Is it ok to have that as part of this patch since we're changing the signature on the function? > [...] > > - if ( read_section(image, ".ucode", &ucode, NULL) ) > > - return; > > - name.s = get_value(&cfg, section, "ucode"); > > With the Arm change already in mind and with further similar > changes further down, may I suggest to consider passing > 'section' into read_section(), thus guaranteeing consistent > naming between image section and config file items, not only now > but also going forward? read_section() would then check for the > leading dot followed by the specified name. That could work, I think. Let me test it out for v5. > [...] > > - file->ptr = (void *)pe_find_section(image->ImageBase, image->ImageSize, > > - name, &file->size); > > This casting away of const-ness worries me. The sole reason why > the "ptr" member is non-const looks to be the two parsing functions > for the config file. How about we make "ptr" const void * and add a > new "char *str" field? While it won't guarantee correct code to > be written, it at least allows doing so. That's what I found in the const cleanup patch -- only the config file parser needed to modify the contents. It would potentially fail if someone modified the build to include the config in a read-only text segment, but they would find out fairly quickly that didn't work... > [...] > > - if ( !read_section(loaded_image, ".kernel", &kernel, option_str) ) > > - read_file(dir_handle, s2w(&name), &ramdisk, NULL); > > - read_file(dir_handle, s2w(&name), &kernel, option_str); > > As before, I disagree with the idea of taking pieces from disk and > pieces from the unified image. If you continue to think this is a > reasonable thing to do, may I ask that you add a rationale of this > model to the description? It allows distributions to continue with the status quo if they want a signed kernel + config, but allow a user provided initrd (which is what the shim protocol does today). Qubes, for instance, has discussed that as a path forward to allow a trusted kernel, while also allowing users to rebuild their own initrd since it contains machine specific data. The config is signed, so an attacker can not add any new lines to it. So if the config file has no "ramdisk" or "xsm" line then get_value() will return NULL and the read_file() will not be attempted. If, however, the config file has an "ramdisk /boot/initrd.gz", but not ".ramdisk" PE section, then that is an explicit statement from the signer that they want to load that file from disk, even though the initrd.gz is not included in the signature. > > --- /dev/null > > +++ b/xen/common/efi/pe.c > > +#define PE_HEADER_MACHINE_ARM64 0xaa64 > > +#define PE_HEADER_MACHINE_X64 0x8664 > > +#define PE_HEADER_MACHINE_I386 0x014c > > This list isn't meant to be a complete one anyway, so please omit > the I386 item as it's not needed anywhere. Sure. This file is almost verbatim from systemd-boot, so it has a few things like that which are not relevant. > > +struct PeFileHeader { > > - UINT16 Machine; > > - UINT16 NumberOfSections; > > - UINT32 TimeDateStamp; > > - UINT32 PointerToSymbolTable; > > - UINT32 NumberOfSymbols; > > - UINT16 SizeOfOptionalHeader; > > - UINT16 Characteristics; > > +} attribute((packed)); > > > > +struct PeHeader { > > - UINT8 Magic[4]; > > - struct PeFileHeader FileHeader; > > +} attribute((packed)); > > At the example of these two (i.e. may extend to others): When the > packed attribute doesn't really have any impact on structure layout > (and will just adversely affect alignof() when applied to the struct > or any of the fields), please omit it. In this case the packed does not affect the layout, but if PeFileHeader started with a UINT64, for instance, then padding would be added to PeHeader to align it without the packed. > > +struct PeSectionHeader { > > - UINT8 Name[8]; > > Better char? Maybe? I've heard that some programs use non-7bit ascii in there for things that are not normal sections (and the names are compared with memcp()). > > +const void *__init pe_find_section(const CHAR8 *image, const UINTN image_size, > > - const char *section_name, UINTN *size_out) > > - const struct DosFileHeader dos = (const void)image; > > If the type of "image" was "const void *", there wouldn't be any cast > needed here (and again further down). And I don't think you actually > need "image" to be a pointer to a particular type? I don't think it needs to be any particular type (and CHAR* is a holdover from the systemd-boot code). However, there is quite a bit of pointer math done on it that avoids casts: pe = (const void *) &image[offset]; If image were void*, then this would have to be written as something like: pe = (const void *)((uintptr_t)image + offset); (unless the gnu extension that allows void* math is enabled) > > - const struct PeSectionHeader *sect; > > - if ( name_len > sizeof(sect->Name) || > > +#elif defined(x86_64) > > > > - if ( pe->FileHeader.Machine != PE_HEADER_MACHINE_X64 ) > > - return NULL; > > > > > > > > +#else > > > > - /* unknown architecture */ > > - return NULL; > > +#endif > > > > Instead of this, further up please #define a single constant (e.g. > PE_HEADER_MACHINE) to check against without any #ifdef-ary here. > This then also should lead to a build error (instead of the > function returning NULL at runtime) when no enabling was done for > a possible future port. Ok. > > - for ( i = 0; i < pe->FileHeader.NumberOfSections; i++ ) > > - { > > - sect = (const void *)&image[offset]; > > > > > > Please limit the scope of sect to the body of this loop, at which > point this assignment can become the initializer. sect was used earlier for sizeof math to validate the name. It's also a little odd that the style guide calls for declaring variable in limited scopes, while also disallowing for loop scoped variables. > > - if ( memcmp(sect->Name, section_name, name_len) != 0 || > > - image_size < sect->VirtualSize + sect->VirtualAddress ) > > Wouldn't this latter part of the condition better be treated as an > error, rather than getting silently ignored? The more if the falling > back to on-disk files got retained? Possibly. Since the signature has been validated on the entire image, this would mean that the signer signed a bogus unified image for some reason. Probably should throw an error of some sort; wasn't sure if it made sense to include that sort of panic behaviour this deep in the code. I'll pickup the style guide issues in v5 as well. -- Trammell ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v4 3/4] efi: Enable booting unified hypervisor/kernel/initrd images 2020-09-17 13:04 ` Trammell Hudson @ 2020-09-17 13:33 ` Trammell Hudson 2020-09-17 15:10 ` Jan Beulich 2020-09-17 15:21 ` Jan Beulich 1 sibling, 1 reply; 25+ messages in thread From: Trammell Hudson @ 2020-09-17 13:33 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, roger.pau, andrew.cooper3, wl On Thursday, September 17, 2020 9:04 AM, Trammell Hudson <hudson@trmm.net> wrote: > On Thursday, September 17, 2020 8:33 AM, Jan Beulich jbeulich@suse.com wrote: > > [...] > > > - if ( read_section(image, ".ucode", &ucode, NULL) ) > > > - return; > > > > > > - name.s = get_value(&cfg, section, "ucode"); > > > > With the Arm change already in mind and with further similar > > changes further down, may I suggest to consider passing > > 'section' into read_section(), thus guaranteeing consistent > > naming between image section and config file items, not only now > > but also going forward? read_section() would then check for the > > leading dot followed by the specified name. > > That could work, I think. Let me test it out for v5. Or maybe not. section is the "section-name" of the config file that is being booted: [global] default=section-name Meanwhile, read_section() wants the PE section name, like ".ucode", which might appear as a line item in that section. -- Trammell ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v4 3/4] efi: Enable booting unified hypervisor/kernel/initrd images 2020-09-17 13:33 ` Trammell Hudson @ 2020-09-17 15:10 ` Jan Beulich 0 siblings, 0 replies; 25+ messages in thread From: Jan Beulich @ 2020-09-17 15:10 UTC (permalink / raw) To: Trammell Hudson; +Cc: xen-devel, roger.pau, andrew.cooper3, wl On 17.09.2020 15:33, Trammell Hudson wrote: > On Thursday, September 17, 2020 9:04 AM, Trammell Hudson <hudson@trmm.net> wrote: >> On Thursday, September 17, 2020 8:33 AM, Jan Beulich jbeulich@suse.com wrote: >>> [...] >>>> - if ( read_section(image, ".ucode", &ucode, NULL) ) >>>> - return; >>>> >>>> - name.s = get_value(&cfg, section, "ucode"); >>> >>> With the Arm change already in mind and with further similar >>> changes further down, may I suggest to consider passing >>> 'section' into read_section(), thus guaranteeing consistent >>> naming between image section and config file items, not only now >>> but also going forward? read_section() would then check for the >>> leading dot followed by the specified name. >> >> That could work, I think. Let me test it out for v5. > > Or maybe not. section is the "section-name" of the config file > that is being booted: > > [global] > default=section-name > > Meanwhile, read_section() wants the PE section name, like ".ucode", which might appear as a line item in that section. Oh, yes - looking at just the code fragment left in context I realize my comment was just rubbish. Sorry. Jan ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v4 3/4] efi: Enable booting unified hypervisor/kernel/initrd images 2020-09-17 13:04 ` Trammell Hudson 2020-09-17 13:33 ` Trammell Hudson @ 2020-09-17 15:21 ` Jan Beulich 1 sibling, 0 replies; 25+ messages in thread From: Jan Beulich @ 2020-09-17 15:21 UTC (permalink / raw) To: Trammell Hudson; +Cc: xen-devel, roger.pau, andrew.cooper3, wl On 17.09.2020 15:04, Trammell Hudson wrote: > On Thursday, September 17, 2020 8:33 AM, Jan Beulich <jbeulich@suse.com> wrote: >> On 14.09.2020 13:50, Trammell Hudson wrote: >> [...] >>> +For all the examples the `.pad` section ended at 0xffff82d041000000. >>> +All the sections are optional (`.config`, `.kernel`, `.ramdisk`, `.xsm`, >>> +`.ucode` (x86) and `.dtb` (ARM)) and the order does not matter. >>> +The virtual addresses do not need to be contiguous, although they should not >>> +be overlapping and should all be greater than the last virtual address of the >>> +hypervisor components. >> >> The .pad section is there really only for padding the image. Its space >> could in principle be used for placing useful stuff (and hence to limit >> overall in-memory image size). That said, there is a plan for a change >> which may involve using the initial part of .pad, but that's not certain >> yet. I'm pointing this out to clarify that there may be a valid reason >> to avoid re-using the .pad space, at least for now. > > The instructions show how to append after the .pad region, so there is > no reuse of that space. I wish objcopy had an --append-region option > so that the user didn't have to do this extra math and adjust sizes. Well, I've been trying to point out that the .pad space could be made use of, but there are constraints. >>> --- a/xen/arch/arm/efi/efi-boot.h >>> +++ b/xen/arch/arm/efi/efi-boot.h >>> @@ -375,27 +375,36 @@ static void __init noreturn efi_arch_post_exit_boot(void) >>> efi_xen_start(fdt, fdt_totalsize(fdt)); >>> } >>> -static void __init efi_arch_cfg_file_early(EFI_FILE_HANDLE dir_handle, char *section) >>> +static void __init efi_arch_cfg_file_early(const EFI_LOADED_IMAGE *image, >>> >>> - EFI_FILE_HANDLE dir_handle, >>> >>> >>> - char *section) >>> >>> >> >> Could I talk you into constifying "section" at this occasion - afaics >> there should be no fallout here or in the other three places where >> the same would apply. > > I'm always in favor of adding more constness. Is it ok to have that > as part of this patch since we're changing the signature on the function? Yes, if there's no further fallout from it. Please just mention such "on the side" changes in half a sentence in the description, so it's clear they're intentional. >>> - if ( !read_section(loaded_image, ".kernel", &kernel, option_str) ) >>> - read_file(dir_handle, s2w(&name), &ramdisk, NULL); >>> - read_file(dir_handle, s2w(&name), &kernel, option_str); >> >> As before, I disagree with the idea of taking pieces from disk and >> pieces from the unified image. If you continue to think this is a >> reasonable thing to do, may I ask that you add a rationale of this >> model to the description? > > It allows distributions to continue with the status quo if they want a > signed kernel + config, but allow a user provided initrd (which is what > the shim protocol does today). Qubes, for instance, has discussed that > as a path forward to allow a trusted kernel, while also allowing users > to rebuild their own initrd since it contains machine specific data. > > The config is signed, so an attacker can not add any new lines to it. > So if the config file has no "ramdisk" or "xsm" line then get_value() > will return NULL and the read_file() will not be attempted. > If, however, the config file has an "ramdisk /boot/initrd.gz", > but not ".ramdisk" PE section, then that is an explicit statement > from the signer that they want to load that file from disk, even > though the initrd.gz is not included in the signature. Ah yes, I can follow these arguments. Please put this or an abridged version of it in the description. It'll help me not re-asking the same question in a couple of week's time, at the very least. >>> --- /dev/null >>> +++ b/xen/common/efi/pe.c >>> +#define PE_HEADER_MACHINE_ARM64 0xaa64 >>> +#define PE_HEADER_MACHINE_X64 0x8664 >>> +#define PE_HEADER_MACHINE_I386 0x014c >> >> This list isn't meant to be a complete one anyway, so please omit >> the I386 item as it's not needed anywhere. > > Sure. This file is almost verbatim from systemd-boot, so it has > a few things like that which are not relevant. Please zap full entities we don't need (and by this I mean please don't zap e.g. structure members because we only need some of them). >>> +struct PeFileHeader { >>> - UINT16 Machine; >>> - UINT16 NumberOfSections; >>> - UINT32 TimeDateStamp; >>> - UINT32 PointerToSymbolTable; >>> - UINT32 NumberOfSymbols; >>> - UINT16 SizeOfOptionalHeader; >>> - UINT16 Characteristics; >>> +} attribute((packed)); >>> >>> +struct PeHeader { >>> - UINT8 Magic[4]; >>> - struct PeFileHeader FileHeader; >>> +} attribute((packed)); >> >> At the example of these two (i.e. may extend to others): When the >> packed attribute doesn't really have any impact on structure layout >> (and will just adversely affect alignof() when applied to the struct >> or any of the fields), please omit it. > > In this case the packed does not affect the layout, but if PeFileHeader > started with a UINT64, for instance, then padding would be added to > PeHeader to align it without the packed. Of course, in such a case the "packed" would be warranted. But quite often structures have already got laid out to optimally pack them, in which case the attribute is pointless, yet still often gets added in a purely mechanical manner by people. As said - we had to deal with fallout from the practice in the not so distant past. >>> +struct PeSectionHeader { >>> - UINT8 Name[8]; >> >> Better char? > > Maybe? I've heard that some programs use non-7bit ascii in there for things > that are not normal sections (and the names are compared with memcp()). Which memcmp() is going to happily deal with afaict. >>> +const void *__init pe_find_section(const CHAR8 *image, const UINTN image_size, >>> - const char *section_name, UINTN *size_out) >>> - const struct DosFileHeader dos = (const void)image; >> >> If the type of "image" was "const void *", there wouldn't be any cast >> needed here (and again further down). And I don't think you actually >> need "image" to be a pointer to a particular type? > > I don't think it needs to be any particular type (and CHAR* is a holdover > from the systemd-boot code). However, there is quite a bit of pointer > math done on it that avoids casts: > > pe = (const void *) &image[offset]; > > If image were void*, then this would have to be written as something like: > > pe = (const void *)((uintptr_t)image + offset); > > (unless the gnu extension that allows void* math is enabled) We use this extension all over the place. >>> - for ( i = 0; i < pe->FileHeader.NumberOfSections; i++ ) >>> - { >>> - sect = (const void *)&image[offset]; >>> >>> >> >> Please limit the scope of sect to the body of this loop, at which >> point this assignment can become the initializer. > > sect was used earlier for sizeof math to validate the name. > > It's also a little odd that the style guide calls for declaring variable > in limited scopes, while also disallowing for loop scoped variables. Because the latter are a newer language feature, originally coming from C++, which we haven't settled on yet to allow for general use. Jan ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v4 4/4] efi: Do not use command line if secure boot is enabled. 2020-09-14 11:50 [PATCH v4 0/4] efi: Unified Xen hypervisor/kernel/initrd images Trammell Hudson ` (2 preceding siblings ...) 2020-09-14 11:50 ` [PATCH v4 3/4] efi: Enable booting unified hypervisor/kernel/initrd images Trammell Hudson @ 2020-09-14 11:50 ` Trammell Hudson 2020-09-16 7:45 ` Roger Pau Monné 2020-09-17 12:51 ` Jan Beulich 3 siblings, 2 replies; 25+ messages in thread From: Trammell Hudson @ 2020-09-14 11:50 UTC (permalink / raw) To: xen-devel; +Cc: roger.pau, jbeulich, andrew.cooper3, wl If secure boot is enabled, the Xen command line arguments are ignored. If a unified Xen image is used, then the bundled configuration, dom0 kernel, and initrd are prefered over the ones listed in the config file. Unlike the shim based verification, the PE signature on a unified image covers the all of the Xen+config+kernel+initrd modules linked into the unified image. This also ensures that properly configured platforms will measure the entire runtime into the TPM for unsealing secrets or remote attestation. Signed-off-by: Trammell Hudson <hudson@trmm.net> --- xen/common/efi/boot.c | 44 ++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 41 insertions(+), 3 deletions(-) diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c index 4b1fbc9643..e65c1f1a09 100644 --- a/xen/common/efi/boot.c +++ b/xen/common/efi/boot.c @@ -949,6 +949,39 @@ static void __init setup_efi_pci(void) efi_bs->FreePool(handles); } +/* + * Logic should remain sync'ed with linux/arch/x86/xen/efi.c + * Secure Boot is enabled iff 'SecureBoot' is set and the system is + * not in Setup Mode. + */ +static bool __init efi_secure_boot(void) +{ + static const __initconst EFI_GUID global_guid = EFI_GLOBAL_VARIABLE; + uint8_t secboot, setupmode; + UINTN secboot_size = sizeof(secboot); + UINTN setupmode_size = sizeof(setupmode); + EFI_STATUS rc; + + rc = efi_rs->GetVariable(L"SecureBoot", (EFI_GUID *)&global_guid, + NULL, &secboot_size, &secboot); + if ( rc != EFI_SUCCESS ) + return false; + + rc = efi_rs->GetVariable(L"SetupMode", (EFI_GUID *)&global_guid, + NULL, &setupmode_size, &setupmode); + if ( rc != EFI_SUCCESS ) + return false; + + if ( secboot > 1) + { + PrintStr(L"Invalid SecureBoot variable=0x"); + DisplayUint(secboot, 2); + PrintStr(newline); + } + + return secboot == 1 && setupmode == 0; +} + static void __init efi_variables(void) { EFI_STATUS status; @@ -1125,8 +1158,8 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) static EFI_GUID __initdata shim_lock_guid = SHIM_LOCK_PROTOCOL_GUID; EFI_LOADED_IMAGE *loaded_image; EFI_STATUS status; - unsigned int i, argc; - CHAR16 **argv, *file_name, *cfg_file_name = NULL, *options = NULL; + unsigned int i, argc = 0; + CHAR16 **argv = NULL, *file_name, *cfg_file_name = NULL, *options = NULL; UINTN gop_mode = ~0; EFI_SHIM_LOCK_PROTOCOL *shim_lock; EFI_GRAPHICS_OUTPUT_PROTOCOL *gop = NULL; @@ -1134,6 +1167,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) bool base_video = false; char *option_str; bool use_cfg_file; + bool secure = false; __set_bit(EFI_BOOT, &efi_flags); __set_bit(EFI_LOADER, &efi_flags); @@ -1152,8 +1186,10 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) PrintErrMesg(L"No Loaded Image Protocol", status); efi_arch_load_addr_check(loaded_image); + secure = efi_secure_boot(); - if ( use_cfg_file ) + /* If UEFI Secure Boot is enabled, do not parse the command line */ + if ( use_cfg_file && !secure ) { UINTN offset = 0; @@ -1211,6 +1247,8 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) PrintStr(L"Xen " __stringify(XEN_VERSION) "." __stringify(XEN_SUBVERSION) XEN_EXTRAVERSION " (c/s " XEN_CHANGESET ") EFI loader\r\n"); + if ( secure ) + PrintStr(L"UEFI Secure Boot enabled\r\n"); efi_arch_relocate_image(0); -- 2.25.1 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH v4 4/4] efi: Do not use command line if secure boot is enabled. 2020-09-14 11:50 ` [PATCH v4 4/4] efi: Do not use command line if secure boot is enabled Trammell Hudson @ 2020-09-16 7:45 ` Roger Pau Monné 2020-09-16 8:50 ` Trammell Hudson 2020-09-17 12:51 ` Jan Beulich 1 sibling, 1 reply; 25+ messages in thread From: Roger Pau Monné @ 2020-09-16 7:45 UTC (permalink / raw) To: Trammell Hudson; +Cc: xen-devel, jbeulich, andrew.cooper3, wl On Mon, Sep 14, 2020 at 07:50:13AM -0400, Trammell Hudson wrote: > If secure boot is enabled, the Xen command line arguments are ignored. > If a unified Xen image is used, then the bundled configuration, dom0 > kernel, and initrd are prefered over the ones listed in the config file. I understand that you must ignore the cfg option when using the bundled image, but is there then an alternative way for passing the basevideo and mapbs parameters? Or there's simply no way of doing so when using secure boot with a bundled image? > Unlike the shim based verification, the PE signature on a unified image > covers the all of the Xen+config+kernel+initrd modules linked into the Extra 'the'. > unified image. This also ensures that properly configured platforms > will measure the entire runtime into the TPM for unsealing secrets or > remote attestation. > > Signed-off-by: Trammell Hudson <hudson@trmm.net> > --- > xen/common/efi/boot.c | 44 ++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 41 insertions(+), 3 deletions(-) > > diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c > index 4b1fbc9643..e65c1f1a09 100644 > --- a/xen/common/efi/boot.c > +++ b/xen/common/efi/boot.c > @@ -949,6 +949,39 @@ static void __init setup_efi_pci(void) > efi_bs->FreePool(handles); > } > > +/* > + * Logic should remain sync'ed with linux/arch/x86/xen/efi.c > + * Secure Boot is enabled iff 'SecureBoot' is set and the system is > + * not in Setup Mode. > + */ > +static bool __init efi_secure_boot(void) > +{ > + static const __initconst EFI_GUID global_guid = EFI_GLOBAL_VARIABLE; > + uint8_t secboot, setupmode; > + UINTN secboot_size = sizeof(secboot); > + UINTN setupmode_size = sizeof(setupmode); > + EFI_STATUS rc; > + > + rc = efi_rs->GetVariable(L"SecureBoot", (EFI_GUID *)&global_guid, > + NULL, &secboot_size, &secboot); > + if ( rc != EFI_SUCCESS ) > + return false; > + > + rc = efi_rs->GetVariable(L"SetupMode", (EFI_GUID *)&global_guid, > + NULL, &setupmode_size, &setupmode); > + if ( rc != EFI_SUCCESS ) > + return false; > + > + if ( secboot > 1) Nit: missing space before closing parentheses. > + { > + PrintStr(L"Invalid SecureBoot variable=0x"); > + DisplayUint(secboot, 2); Maybe better to use secboot_size * 2 here since you already have the size of the variable anyway? Thanks, Roger. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v4 4/4] efi: Do not use command line if secure boot is enabled. 2020-09-16 7:45 ` Roger Pau Monné @ 2020-09-16 8:50 ` Trammell Hudson 2020-09-16 9:57 ` Jan Beulich 0 siblings, 1 reply; 25+ messages in thread From: Trammell Hudson @ 2020-09-16 8:50 UTC (permalink / raw) To: Roger Pau Monné; +Cc: xen-devel, jbeulich, andrew.cooper3, wl On Wednesday, September 16, 2020 3:45 AM, Roger Pau Monné <roger.pau@citrix.com> wrote: > On Mon, Sep 14, 2020 at 07:50:13AM -0400, Trammell Hudson wrote: > > If secure boot is enabled, the Xen command line arguments are ignored. > > If a unified Xen image is used, then the bundled configuration, dom0 > > kernel, and initrd are prefered over the ones listed in the config file. > > I understand that you must ignore the cfg option when using the > bundled image, but is there then an alternative way for passing the > basevideo and mapbs parameters? The cfg option will be ignored regardless since a bundled config (or kernel, ramdisk, etc) takes precedence over any files, so perhaps parsing the command line is not as much of a risk as initially thought. The concern is that *any* non-signed configuration values are potentially a risk, even if we don't see exactly how the attacker can use them right now. Especially if an option is added later and we haven't thought about the security ramifications of it. > Or there's simply no way of doing so when using secure boot with a > bundled image? Should these options be available in the config file instead? That way the system owner can sign the configuration and ensure that an adversary can't change them. > > Unlike the shim based verification, the PE signature on a unified image > > covers the all of the Xen+config+kernel+initrd modules linked into the > > Extra 'the'. Fixed, along with the style issues in upcoming v5. -- Trammell ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v4 4/4] efi: Do not use command line if secure boot is enabled. 2020-09-16 8:50 ` Trammell Hudson @ 2020-09-16 9:57 ` Jan Beulich 0 siblings, 0 replies; 25+ messages in thread From: Jan Beulich @ 2020-09-16 9:57 UTC (permalink / raw) To: Trammell Hudson; +Cc: Roger Pau Monné, xen-devel, andrew.cooper3, wl On 16.09.2020 10:50, Trammell Hudson wrote: > On Wednesday, September 16, 2020 3:45 AM, Roger Pau Monné <roger.pau@citrix.com> wrote: >> On Mon, Sep 14, 2020 at 07:50:13AM -0400, Trammell Hudson wrote: >>> If secure boot is enabled, the Xen command line arguments are ignored. >>> If a unified Xen image is used, then the bundled configuration, dom0 >>> kernel, and initrd are prefered over the ones listed in the config file. >> >> I understand that you must ignore the cfg option when using the >> bundled image, but is there then an alternative way for passing the >> basevideo and mapbs parameters? > > The cfg option will be ignored regardless since a bundled config > (or kernel, ramdisk, etc) takes precedence over any files, > so perhaps parsing the command line is not as much of a risk > as initially thought. > > The concern is that *any* non-signed configuration values are > potentially a risk, even if we don't see exactly how the attacker > can use them right now. Especially if an option is added later > and we haven't thought about the security ramifications of it. > >> Or there's simply no way of doing so when using secure boot with a >> bundled image? > > Should these options be available in the config file instead? > That way the system owner can sign the configuration and ensure > that an adversary can't change them. Not really, no. /basevideo needs evaluating very early in any event, before any (regular) output gets produced. /mapbs could be parsed later, but the early boot code intentionally does not make any attempt at parsing the command line options designated for the common parts of xen.gz and xen.efi. Yet the map_bs variable has one use in early boot code (i.e. before handing over to __start_xen()). Jan ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v4 4/4] efi: Do not use command line if secure boot is enabled. 2020-09-14 11:50 ` [PATCH v4 4/4] efi: Do not use command line if secure boot is enabled Trammell Hudson 2020-09-16 7:45 ` Roger Pau Monné @ 2020-09-17 12:51 ` Jan Beulich 2020-09-17 14:05 ` Trammell Hudson 1 sibling, 1 reply; 25+ messages in thread From: Jan Beulich @ 2020-09-17 12:51 UTC (permalink / raw) To: Trammell Hudson; +Cc: xen-devel, roger.pau, andrew.cooper3, wl On 14.09.2020 13:50, Trammell Hudson wrote: > If secure boot is enabled, the Xen command line arguments are ignored. > If a unified Xen image is used, then the bundled configuration, dom0 > kernel, and initrd are prefered over the ones listed in the config file. > > Unlike the shim based verification, the PE signature on a unified image > covers the all of the Xen+config+kernel+initrd modules linked into the > unified image. This also ensures that properly configured platforms > will measure the entire runtime into the TPM for unsealing secrets or > remote attestation. The command line may also include a part handed on to the Dom0 kernel. If the Dom0 kernel image comes from disk, I don't see why that part of the command line shouldn't be honored. Similarly, if the config file doesn't come from the unified image, I think Xen's command line options should also be honored. > --- a/xen/common/efi/boot.c > +++ b/xen/common/efi/boot.c > @@ -949,6 +949,39 @@ static void __init setup_efi_pci(void) > efi_bs->FreePool(handles); > } > > +/* > + * Logic should remain sync'ed with linux/arch/x86/xen/efi.c > + * Secure Boot is enabled iff 'SecureBoot' is set and the system is > + * not in Setup Mode. > + */ > +static bool __init efi_secure_boot(void) > +{ > + static const __initconst EFI_GUID global_guid = EFI_GLOBAL_VARIABLE; > + uint8_t secboot, setupmode; > + UINTN secboot_size = sizeof(secboot); > + UINTN setupmode_size = sizeof(setupmode); > + EFI_STATUS rc; > + > + rc = efi_rs->GetVariable(L"SecureBoot", (EFI_GUID *)&global_guid, As you need the casts here just to get rid of the const again, please don't make the variable const in the first place. > + NULL, &secboot_size, &secboot); > + if ( rc != EFI_SUCCESS ) > + return false; > + > + rc = efi_rs->GetVariable(L"SetupMode", (EFI_GUID *)&global_guid, > + NULL, &setupmode_size, &setupmode); > + if ( rc != EFI_SUCCESS ) > + return false; > + > + if ( secboot > 1) > + { > + PrintStr(L"Invalid SecureBoot variable=0x"); > + DisplayUint(secboot, 2); > + PrintStr(newline); > + } > + > + return secboot == 1 && setupmode == 0; Like for SecureBoot, values other than 0 and 1 also are reserved for SetupMode and hence would better be logged in the same way. I'm still unconvinced though that logging is enough. > @@ -1134,6 +1167,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) > bool base_video = false; > char *option_str; > bool use_cfg_file; > + bool secure = false; I don't think the initializer is needed here? > @@ -1152,8 +1186,10 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable) > PrintErrMesg(L"No Loaded Image Protocol", status); > > efi_arch_load_addr_check(loaded_image); > + secure = efi_secure_boot(); > > - if ( use_cfg_file ) > + /* If UEFI Secure Boot is enabled, do not parse the command line */ > + if ( use_cfg_file && !secure ) > { If it is intentional to also change this for the shim case, then please justify the change in behavior in the description. As per the comments further up I think the ignoring of the various parts wants making depend on more than just secure boot mode anyway. Jan ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v4 4/4] efi: Do not use command line if secure boot is enabled. 2020-09-17 12:51 ` Jan Beulich @ 2020-09-17 14:05 ` Trammell Hudson 2020-09-17 15:26 ` Jan Beulich 0 siblings, 1 reply; 25+ messages in thread From: Trammell Hudson @ 2020-09-17 14:05 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, roger.pau, andrew.cooper3, wl On Thursday, September 17, 2020 8:51 AM, Jan Beulich <jbeulich@suse.com> wrote: > On 14.09.2020 13:50, Trammell Hudson wrote: > > If secure boot is enabled, the Xen command line arguments are ignored. > > If a unified Xen image is used, then the bundled configuration, dom0 > > kernel, and initrd are prefered over the ones listed in the config file. > > Unlike the shim based verification, the PE signature on a unified image > > covers the all of the Xen+config+kernel+initrd modules linked into the > > unified image. This also ensures that properly configured platforms > > will measure the entire runtime into the TPM for unsealing secrets or > > remote attestation. > > The command line may also include a part handed on to the Dom0 kernel. > If the Dom0 kernel image comes from disk, I don't see why that part of > the command line shouldn't be honored. Similarly, if the config file > doesn't come from the unified image, I think Xen's command line options > should also be honored. Ignoring the command line and breaking the shim behaviour in a unified image should be ok; that is an explicit decision by the system owner to sign and configure the new image (and the shim is not used in a unified image anyway). If we have a way to detect a unified image early enough, then we can avoid the backwards incompatibility if it is not unified. That would require moving the config parsing to above the relocation call. I'm testing that now to see if it works on x86. -- Trammell ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v4 4/4] efi: Do not use command line if secure boot is enabled. 2020-09-17 14:05 ` Trammell Hudson @ 2020-09-17 15:26 ` Jan Beulich 2020-09-17 15:45 ` Trammell Hudson 0 siblings, 1 reply; 25+ messages in thread From: Jan Beulich @ 2020-09-17 15:26 UTC (permalink / raw) To: Trammell Hudson; +Cc: xen-devel, roger.pau, andrew.cooper3, wl On 17.09.2020 16:05, Trammell Hudson wrote: > On Thursday, September 17, 2020 8:51 AM, Jan Beulich <jbeulich@suse.com> wrote: >> On 14.09.2020 13:50, Trammell Hudson wrote: >>> If secure boot is enabled, the Xen command line arguments are ignored. >>> If a unified Xen image is used, then the bundled configuration, dom0 >>> kernel, and initrd are prefered over the ones listed in the config file. >>> Unlike the shim based verification, the PE signature on a unified image >>> covers the all of the Xen+config+kernel+initrd modules linked into the >>> unified image. This also ensures that properly configured platforms >>> will measure the entire runtime into the TPM for unsealing secrets or >>> remote attestation. >> >> The command line may also include a part handed on to the Dom0 kernel. >> If the Dom0 kernel image comes from disk, I don't see why that part of >> the command line shouldn't be honored. Similarly, if the config file >> doesn't come from the unified image, I think Xen's command line options >> should also be honored. > > Ignoring the command line and breaking the shim behaviour in a > unified image should be ok; that is an explicit decision by the > system owner to sign and configure the new image (and the shim > is not used in a unified image anyway). > > If we have a way to detect a unified image early enough, then > we can avoid the backwards incompatibility if it is not unified. I was assuming this was easily possible, if necessary as about the first thing we do. If it's not as easy, perhaps something wants adding to make it so? > That would require moving the config parsing to above the relocation > call. I guess I don't understand why this would be. Jan ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v4 4/4] efi: Do not use command line if secure boot is enabled. 2020-09-17 15:26 ` Jan Beulich @ 2020-09-17 15:45 ` Trammell Hudson 0 siblings, 0 replies; 25+ messages in thread From: Trammell Hudson @ 2020-09-17 15:45 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, roger.pau, andrew.cooper3, wl On Thursday, September 17, 2020 11:26 AM, Jan Beulich <jbeulich@suse.com> wrote: > On 17.09.2020 16:05, Trammell Hudson wrote: > > If we have a way to detect a unified image early enough, then > > we can avoid the backwards incompatibility if it is not unified. > > I was assuming this was easily possible, if necessary as about the > first thing we do. If it's not as easy, perhaps something wants > adding to make it so? v5 of the patch (just sent) has changed the logic so that the config section is searched first thing, and if it is found, then and only then is the command line ignored. I believe this restores the older behaviour. > > That would require moving the config parsing to above the relocation > > call. > > I guess I don't understand why this would be. The early command line parsing happens before the call to efi_arch_relocate_image(), although testing in qemu did not seem to cause any problems with calling read_section() before the reloc. -- Trammell ^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2020-09-17 15:46 UTC | newest] Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-09-14 11:50 [PATCH v4 0/4] efi: Unified Xen hypervisor/kernel/initrd images Trammell Hudson 2020-09-14 11:50 ` [PATCH v4 1/4] efi/boot.c: add file.need_to_free Trammell Hudson 2020-09-16 6:43 ` Roger Pau Monné 2020-09-17 11:06 ` Jan Beulich 2020-09-14 11:50 ` [PATCH v4 2/4] efi/boot.c: add handle_file_info() Trammell Hudson 2020-09-16 6:46 ` Roger Pau Monné 2020-09-17 11:29 ` Jan Beulich 2020-09-14 11:50 ` [PATCH v4 3/4] efi: Enable booting unified hypervisor/kernel/initrd images Trammell Hudson 2020-09-16 7:32 ` Roger Pau Monné 2020-09-16 8:37 ` Trammell Hudson 2020-09-16 9:47 ` Jan Beulich 2020-09-16 10:15 ` Roger Pau Monné 2020-09-17 12:33 ` Jan Beulich 2020-09-17 13:04 ` Trammell Hudson 2020-09-17 13:33 ` Trammell Hudson 2020-09-17 15:10 ` Jan Beulich 2020-09-17 15:21 ` Jan Beulich 2020-09-14 11:50 ` [PATCH v4 4/4] efi: Do not use command line if secure boot is enabled Trammell Hudson 2020-09-16 7:45 ` Roger Pau Monné 2020-09-16 8:50 ` Trammell Hudson 2020-09-16 9:57 ` Jan Beulich 2020-09-17 12:51 ` Jan Beulich 2020-09-17 14:05 ` Trammell Hudson 2020-09-17 15:26 ` Jan Beulich 2020-09-17 15:45 ` Trammell Hudson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.