| Message ID | 20200914115013.814079-4-hudson@trmm.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | efi: Unified Xen hypervisor/kernel/initrd images | expand |
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.
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
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
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.
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
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
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
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
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
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; +}
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