[Xen-devel] kexec and xen/arch/x86/boot/head.S trampoline
This is a belated followup to my post in 2016, which was a followup to a post by Ward Vandewege in 2008 about problems introduced by Xen 3.1.3's changes in the trampoline allocation code: https://lists.xenproject.org/archives/html/xen-devel/2016-08/msg01208.html I've been maintaining an out-of-tree patch for using Xen with coreboot and the Heads runtime since my previous post and finally decided to track down what in coreboot was causing the issue. It turns out that it is not a coreboot problem, but caused by kexec. kexec allocates a 1 page segment at 0x0 and memsets most of it to zero, wiping out coreboot's EBDA structure, which xen's head.S consulted to allocate the trampoline. Xen's code looks like this: /* Set up trampoline segment 64k below EBDA */ movzwl 0x40e,%ecx /* EBDA segment */ cmp $0xa000,%ecx/* sanity check (high) */ jae 0f cmp $0x4000,%ecx/* sanity check (low) */ jae 1f 0: movzwl 0x413,%ecx /* use base memory size on failure */ shl $10-4,%ecx 1: /* * Compare the value in the BDA with the information from the * multiboot structure (if available) and use the smallest. */ testb $MBI_MEMLIMITS,(%ebx) jz 2f /* not available? BDA value will be fine */ mov MB_mem_lower(%ebx),%edx cmp $0x100,%edx /* is the multiboot value too small? */ jb 2f /* if so, do not use it */ shl $10-4,%edx cmp %ecx,%edx /* compare with BDA value */ cmovb %edx,%ecx /* and use the smaller */ 2: /* Reserve 64kb for the trampoline */ sub $0x1000,%ecx Since 0x40e is zero, it goes into the base memory size fallback, which also results in %ecx being zero. Since zero is less than whatever is in the MBI, Xen decides to use minus 0x1000 for the trampoline and faults soon afterwards as a result. Are there reasons to prefer EBDA over mbi->mem_lower? If not, it seems that easiest way to patch it would be always trust the mbi lower memory value if the memlimits bit is set (which is what my out-of-tree patch did) and only fall back to EBDA data if the mbi->flags MEMLIMITS bit is not set. And even then it would be good to to duplicate the guard for %ecx < 0x4000 || %ecx > 0xa000 when reading from 0x413 and signal an error rather than faulting. -- Trammell ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] kexec and xen/arch/x86/boot/head.S trampoline
On Wed, Feb 28, 2018 at 03:07:41PM +, Andrew Cooper wrote: > On 28/02/18 14:08, Trammell Hudson wrote: > > kexec allocates a 1 page segment at 0x0 and memsets most of it to zero, > > wiping out coreboot's EBDA structure, which xen's head.S consulted to > > allocate the trampoline. Xen's code looks like this: > > This sounds like a bug as and of itself. I presume this is to do with > IVT handling? Maybe? That's the right address and it only populates 0x78 bytes (in qemu) with contents, which would be about right for the interrupt table. I haven't tracked down where that data comes from. > [...] > > Are there reasons to prefer EBDA over mbi->mem_lower? > > I think the expectation was that the EBDA would be more reliable than > mbi->mem_lower. However, we recently hit a similar bug with PVH > handling (c/s a232346b1fe), and these days, the EBDA is quite likely not > to be present. > > I think we probably can use mbi->mem_lower (if available and sane) by > default. If the bootloader has messed that up, all bets are off > anyway. Irrespective, we should fix the EBDA lower sanity check. What do you think of something like this instead? /* * If the multiboot structure memory limits are available, * use it for the trampoline. */ testb $MBI_MEMLIMITS,(%ebx) jz 1f /* not available? BDA value will be fine */ mov MB_mem_lower(%ebx),%ecx cmp $0x100,%ecx /* is the multiboot value too small? */ jae 2f 1: /* MBI not available, set up trampoline segment 64k below EBDA */ movzwl 0x40e,%ecx /* EBDA segment */ cmp $0xa000,%ecx/* sanity check (high) */ jae 0f cmp $0x4000,%ecx/* sanity check (low) */ jae 2f 0: movzwl 0x413,%ecx /* use base memory size on failure */ cmp $0xa000,%ecx/* sanity check (high) */ jae bad_ebda cmp $0x4000,%ecx/* sanity check (low) */ jb bad_ebda 2: /* Reserve 64kb for the trampoline */ shl $10-4,%ecx /* convert to bytes */ sub $0x1000,%ecx -- Trammell ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [PATCH] EFI: Enable booting unified hypervisor/kernel/initrd images
On Friday, September 4, 2020 5:29 AM, Julien Grall wrote: > On 28/08/2020 12:51, Trammell Hudson wrote: > > > - /* PE32+ Subsystem type */ > > +#if defined(ARM) > > > > Shouldn't this be defined(aarch64) ? To be honest I'm not sure and don't have a way to check if this code works on ARM. Does an Xen EFI build on ARM even use the PE32+ format? -- Trammell
Re: Continuing the Gitlab experiment: Single-patch PRs for gitlab
On Friday, September 4, 2020 5:54 AM, George Dunlap wrote: > And I’d encourage others to try submitting simple one-or-two-patch series as > PRs to Gitlab instead, as we continue the experiment. I've reworked my unified EFI image patch to merge with the recent Makefile changes and submitted it through the website: https://gitlab.com/xen-project/xen/-/merge_requests/4 -- Trammell
Re: [PATCH] EFI: Enable booting unified hypervisor/kernel/initrd images
On Friday, September 4, 2020 9:02 AM, Roger Pau Monné wrote: > On Fri, Aug 28, 2020 at 11:51:35AM +0000, Trammell Hudson wrote: > > diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S > > index 0273f79152..ba691b1890 100644 > > --- a/xen/arch/x86/xen.lds.S > > +++ b/xen/arch/x86/xen.lds.S > > @@ -156,6 +156,7 @@ SECTIONS > > __note_gnu_build_id_end = .; > > } :note :text > > #elif defined(BUILD_ID_EFI) > > - . = ALIGN(32); /* workaround binutils section overlap bug */ > > Is this a separate bugfix? > > That's the only change to the linker script, so is this bug somehow > triggered by the new code additions this commit makes? > > It might also be nice to have some reference to the bug if possible, > so that we know when the bug has been fixed and thus we can drop the > workaround. I've split this into a separate commit with links to the mailing list discussion and bug fix applied to the binutils: https://gitlab.com/xen-project/xen/-/merge_requests/4/diffs?commit_id=acfd8f85de8954bb08b726419a680e7ba5aba499 As Jan pointed out, it doesn't directly affect the xen build process since the xen.efi is the end-product, although it does prevent users from making further changes to the executable (such as merging the pieces into the unified image) without having a bleeding edge version of binutils. > > - file->need_to_free = true; > > Don't you need to set need_to_free after AllocatePages has returned > successfully? Yes, I think so. I've fixed this in the gitlab PR. > Also the error handling in read_file is horrible to follow IMO. Yeah, I'm a fan of early-return error handling rather than the "what ?: wtf" style, although I left it as is. > [...] > For future patches it might be helpful to split non-functional changes > into a separate patch. For example the inclusion of display_file_info > could be a pre-patch, and that would help reduce the size of the diff. Things like display_file_info() were separate commits that ended squashed during one of the merges as the patch was being reworked. They could probably be out if someone wanted to rebase it again. > > - return secboot == 1 && setupmode == 0; > > Does this need to be strictly 1, or any value != 0? We discussed this briefly here on xen-devel without any real conclusion; the UEFI spec says that all other values are reserved. I'm not sure in practice if any others ever show up. > [...] > I have to admit I know very little, but don't you need to verify the > ramdisk also, like you verify the kernel? Or is the kernel the one > that's supposed to verify it's ramdisk before using it? With the unified image there is no need to do so; the xen.efi, config, kernel, initrd, flash, and ucode are all signed as one file and the shim protocol is not necessary. For the non-unified case, well, that's what started me on this patch. I was quite surprised that all of the Secure Boot support in Linux distrbutions and Xen did not sign the initrd or command lines, only the kernel image. So, yes, it seems like it should be signed, but that's not what the wider community decided to do. > [...] > > - - Derived from > > https://github.com/systemd/systemd/blob/master/src/boot/efi/pe.c > > Is it worth adding the commit that you used as the base for this file? Done. The code hasn't seen any updates in months, although it is worth noting where it came from. > > +/* > > - - 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: > [...] > > This must be in a doc somewhere, likely in misc/efi.pandoc? People > trying to use such functionality shouldn't resort to reading a comment > on a source file IMO. Thanks for the location suggestion. I wasn't sure where to store it. Moved the comment into there and markdown'ed it. I think I've addressed all of the style guide issues that you and others brought up in the latest version on the gitlab site: https://gitlab.com/xen-project/xen/-/merge_requests/4 -- Trammell
Re: [PATCH] EFI: Enable booting unified hypervisor/kernel/initrd images
On Friday, September 4, 2020 1:58 PM, Julien Grall wrote: > On 28/08/2020 12:51, 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 > > CPU microcode into a single "unified" EFI executable. > > For Arm, I would also consider to add the DTB in "unified" EFI > executable. See efi_arch_cfg_file_early() in xen/arch/arm/efi/efi-boot.h. Excellent point. The DTB is critical as well. I'll add that to the patch on Xen's gitlab. x86 has a similar call in efi_arch-cfg_file_early() to load the CPU microcode from the unified image, if it exists. > [...] > I don't seem to be able to apply the patch using git-am: You might try this one instead -- it has been based on the latest staging tree and has had a few rounds of style guide issues as well as the commit id for the systemd-boot code. https://gitlab.com/xen-project/xen/-/merge_requests/4 -- Trammell
Re: [PATCH] EFI: Enable booting unified hypervisor/kernel/initrd images
On Friday, September 4, 2020 2:05 PM, Trammell Hudson wrote: > On Friday, September 4, 2020 1:58 PM, Julien Grall jul...@xen.org wrote: > > On 28/08/2020 12:51, 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 > > > CPU microcode into a single "unified" EFI executable. > > > > For Arm, I would also consider to add the DTB in "unified" EFI > > executable. See efi_arch_cfg_file_early() in xen/arch/arm/efi/efi-boot.h. > > Excellent point. The DTB is critical as well. I'll add that to > the patch on Xen's gitlab. Here is an untested patch for loading DTB from unified image: https://gitlab.com/xen-project/xen/-/merge_requests/4/diffs?commit_id=e55336e11cc6a7ce77f0bf8ce3aa0712c7017e8b -- trammell
[PATCH v3 0/4] efi: Unified Xen hypervisor/kernel/initrd images
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 (`.initrd`), and XSM config (`.xsm`), which are included after building xen.efi using objcopy to add named sections for each input file. Trammell hudson (4): x86/xen.lds.S: Work around binutils build id alignment bug efi/boot.c: add file.need_to_free and split display_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| 47 xen/arch/arm/efi/efi-boot.h | 22 -- xen/arch/x86/efi/Makefile | 2 +- xen/arch/x86/efi/efi-boot.h | 7 +- xen/arch/x86/xen.lds.S | 1 + xen/common/efi/boot.c | 139 ++- xen/common/efi/efi.h| 3 + xen/common/efi/pe.c | 141 9 files changed, 317 insertions(+), 46 deletions(-) create mode 100644 xen/common/efi/pe.c -- 2.25.1
[PATCH v3 1/4] x86/xen.lds.S: Work around binutils build id alignment bug
binutils in most distrbutions have a bug in find_section_by_vma()
that causes objcopy round section addresses incorrectly and that
think the .buildid section overlaps with the .rodata. Aligning the
sections allows these older verisons of the tools to work on the
xen.efi executable image.
Mailing list discussion:
https://sourceware.org/pipermail/binutils/2020-August/112746.html
Fixed in:
https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=610ed3e08f13b3886fd7194fb7a248dee8724685
Signed-off-by: Trammell hudson
---
xen/arch/x86/xen.lds.S | 1 +
1 file changed, 1 insertion(+)
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 0273f79152..ba691b1890 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -156,6 +156,7 @@ SECTIONS
__note_gnu_build_id_end = .;
} :note :text
#elif defined(BUILD_ID_EFI)
+ . = ALIGN(32); /* workaround binutils section overlap bug */
DECL_SECTION(.buildid) {
__note_gnu_build_id_start = .;
*(.buildid)
--
2.25.1
[PATCH v3 2/4] efi/boot.c: add file.need_to_free and split display_file_info()
add file.need_to_free and split display_file_info()
Signed-off-by: Trammell hudson
---
xen/common/efi/boot.c | 36 ++--
1 file changed, 22 insertions(+), 14 deletions(-)
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 4022a672c9..f5bdc4b1df 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();
@@ -538,6 +539,21 @@ static char * __init split_string(char *s)
return NULL;
}
+static void __init display_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)
{
@@ -572,6 +588,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) )
{
@@ -581,16 +598,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);
-}
+display_file_info(name, file, options);
ret = FileHandle->Read(FileHandle, &file->size, file->ptr);
if ( !EFI_ERROR(ret) && file->size != size )
--
2.25.1
[PATCH v3 3/4] efi: Enable booting unified hypervisor/kernel/initrd images
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
(`.initrd`), 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
---
.gitignore | 1 +
docs/misc/efi.pandoc| 47
xen/arch/arm/efi/efi-boot.h | 22 --
xen/arch/x86/efi/Makefile | 2 +-
xen/arch/x86/efi/efi-boot.h | 7 +-
xen/common/efi/boot.c | 72 +-
xen/common/efi/efi.h| 3 +
xen/common/efi/pe.c | 141
8 files changed, 266 insertions(+), 29 deletions(-)
create mode 100644 xen/common/efi/pe.c
diff --git a/.gitignore b/.gitignore
index 823f4743dc..1c79b9f90a 100644
--- a/.gitignore
+++ b/.gitignore
@@ -296,6 +296,7 @@ xen/arch/x86/efi/mkreloc
xen/arch/*/xen.lds
xen/arch/*/asm-offsets.s
xen/arch/*/efi/boot.c
+xen/arch/*/efi/pe.c
xen/arch/*/efi/compat.c
xen/arch/*/efi/ebmalloc.c
xen/arch/*/efi/efi.h
diff --git a/docs/misc/efi.pandoc b/docs/misc/efi.pandoc
index 23c1a2732d..c882b1f8f8 100644
--- a/docs/misc/efi.pandoc
+++ b/docs/misc/efi.pandoc
@@ -116,3 +116,50 @@ 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])'
+```
+
+All the sections are optional (`.config`, `.kernel`, `.initrd`, `.xsm`,
+`.ucode` (x86) and `.dtb` (ARM)) and the order does not matter.
+The addresses do not need to be contiguous, although they should not
+be overlapping.
+
+```
+objcopy \
+ --add-section .config=xen.cfg \
+ --change-section-vma .config=0x82d04100
+ --add-section .ucode=ucode.bin \
+ --change-section-vma .ucode=0x82d04101 \
+ --add-section .xsm=xsm.cfg \
+ --change-section-vma .xsm=0x82d04108 \
+ --add-section .kernel=vmlinux \
+ --change-section-vma .kernel=0x82d04110 \
+ --add-section .ramdisk=initrd.img \
+ --change-section-vma .initrd=0x82d04200 \
+ 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..154e605fee 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -375,27 +375,33 @@ 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(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 update
[PATCH v3 4/4] efi: Do not use command line if secure boot is enabled.
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
---
xen/common/efi/boot.c | 31 ---
1 file changed, 28 insertions(+), 3 deletions(-)
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 452b5f4362..5aaebd5f20 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -947,6 +947,26 @@ 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);
+
+if ( efi_rs->GetVariable(L"SecureBoot", (EFI_GUID *)&global_guid, NULL,
&secboot_size, &secboot) != EFI_SUCCESS )
+return false;
+if ( efi_rs->GetVariable(L"SetupMode", (EFI_GUID *)&global_guid, NULL,
&setupmode_size, &setupmode) != EFI_SUCCESS )
+return false;
+
+return secboot == 1 && setupmode == 0;
+}
+
static void __init efi_variables(void)
{
EFI_STATUS status;
@@ -1123,8 +1143,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;
@@ -1132,6 +1152,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);
@@ -1150,8 +1171,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;
@@ -1209,6 +1232,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
[PATCH v3 2/4] efi/boot.c: add file.need_to_free and split display_file_info()
From: Trammell hudson
Signed-off-by: Trammell hudson
---
xen/common/efi/boot.c | 36 ++--
1 file changed, 22 insertions(+), 14 deletions(-)
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 4022a672c9..f5bdc4b1df 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();
@@ -538,6 +539,21 @@ static char * __init split_string(char *s)
return NULL;
}
+static void __init display_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)
{
@@ -572,6 +588,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) )
{
@@ -581,16 +598,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);
-}
+display_file_info(name, file, options);
ret = FileHandle->Read(FileHandle, &file->size, file->ptr);
if ( !EFI_ERROR(ret) && file->size != size )
--
2.25.1
[PATCH v3 0/4] efi: Unified Xen hypervisor/kernel/initrd images
From: Trammell hudson 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 (`.initrd`), and XSM config (`.xsm`), which are included after building xen.efi using objcopy to add named sections for each input file. Trammell hudson (4): x86/xen.lds.S: Work around binutils build id alignment bug efi/boot.c: add file.need_to_free and split display_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| 47 xen/arch/arm/efi/efi-boot.h | 22 -- xen/arch/x86/efi/Makefile | 2 +- xen/arch/x86/efi/efi-boot.h | 7 +- xen/arch/x86/xen.lds.S | 1 + xen/common/efi/boot.c | 139 ++- xen/common/efi/efi.h| 3 + xen/common/efi/pe.c | 141 9 files changed, 317 insertions(+), 46 deletions(-) create mode 100644 xen/common/efi/pe.c -- 2.25.1
[PATCH v3 1/4] x86/xen.lds.S: Work around binutils build id alignment bug
From: Trammell hudson
binutils in most distrbutions have a bug in find_section_by_vma()
that causes objcopy round section addresses incorrectly and that
think the .buildid section overlaps with the .rodata. Aligning the
sections allows these older verisons of the tools to work on the
xen.efi executable image.
Mailing list discussion:
https://sourceware.org/pipermail/binutils/2020-August/112746.html
Fixed in:
https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=610ed3e08f13b3886fd7194fb7a248dee8724685
Signed-off-by: Trammell hudson
---
xen/arch/x86/xen.lds.S | 1 +
1 file changed, 1 insertion(+)
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 0273f79152..ba691b1890 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -156,6 +156,7 @@ SECTIONS
__note_gnu_build_id_end = .;
} :note :text
#elif defined(BUILD_ID_EFI)
+ . = ALIGN(32); /* workaround binutils section overlap bug */
DECL_SECTION(.buildid) {
__note_gnu_build_id_start = .;
*(.buildid)
--
2.25.1
[PATCH v3 3/4] efi: Enable booting unified hypervisor/kernel/initrd images
From: Trammell hudson
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
(`.initrd`), 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
---
.gitignore | 1 +
docs/misc/efi.pandoc| 47
xen/arch/arm/efi/efi-boot.h | 22 --
xen/arch/x86/efi/Makefile | 2 +-
xen/arch/x86/efi/efi-boot.h | 7 +-
xen/common/efi/boot.c | 72 +-
xen/common/efi/efi.h| 3 +
xen/common/efi/pe.c | 141
8 files changed, 266 insertions(+), 29 deletions(-)
create mode 100644 xen/common/efi/pe.c
diff --git a/.gitignore b/.gitignore
index 823f4743dc..1c79b9f90a 100644
--- a/.gitignore
+++ b/.gitignore
@@ -296,6 +296,7 @@ xen/arch/x86/efi/mkreloc
xen/arch/*/xen.lds
xen/arch/*/asm-offsets.s
xen/arch/*/efi/boot.c
+xen/arch/*/efi/pe.c
xen/arch/*/efi/compat.c
xen/arch/*/efi/ebmalloc.c
xen/arch/*/efi/efi.h
diff --git a/docs/misc/efi.pandoc b/docs/misc/efi.pandoc
index 23c1a2732d..c882b1f8f8 100644
--- a/docs/misc/efi.pandoc
+++ b/docs/misc/efi.pandoc
@@ -116,3 +116,50 @@ 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])'
+```
+
+All the sections are optional (`.config`, `.kernel`, `.initrd`, `.xsm`,
+`.ucode` (x86) and `.dtb` (ARM)) and the order does not matter.
+The addresses do not need to be contiguous, although they should not
+be overlapping.
+
+```
+objcopy \
+ --add-section .config=xen.cfg \
+ --change-section-vma .config=0x82d04100
+ --add-section .ucode=ucode.bin \
+ --change-section-vma .ucode=0x82d04101 \
+ --add-section .xsm=xsm.cfg \
+ --change-section-vma .xsm=0x82d04108 \
+ --add-section .kernel=vmlinux \
+ --change-section-vma .kernel=0x82d04110 \
+ --add-section .ramdisk=initrd.img \
+ --change-section-vma .initrd=0x82d04200 \
+ 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..154e605fee 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -375,27 +375,33 @@ 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(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.
+ * fil
[PATCH v3 4/4] efi: Do not use command line if secure boot is enabled.
From: Trammell hudson
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
---
xen/common/efi/boot.c | 31 ---
1 file changed, 28 insertions(+), 3 deletions(-)
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 452b5f4362..5aaebd5f20 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -947,6 +947,26 @@ 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);
+
+if ( efi_rs->GetVariable(L"SecureBoot", (EFI_GUID *)&global_guid, NULL,
&secboot_size, &secboot) != EFI_SUCCESS )
+return false;
+if ( efi_rs->GetVariable(L"SetupMode", (EFI_GUID *)&global_guid, NULL,
&setupmode_size, &setupmode) != EFI_SUCCESS )
+return false;
+
+return secboot == 1 && setupmode == 0;
+}
+
static void __init efi_variables(void)
{
EFI_STATUS status;
@@ -1123,8 +1143,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;
@@ -1132,6 +1152,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);
@@ -1150,8 +1171,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;
@@ -1209,6 +1232,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
Re: [PATCH v3 1/4] x86/xen.lds.S: Work around binutils build id alignment bug
On Tuesday, September 8, 2020 11:04 AM, Jan Beulich wrote: > [...] > Personally I think this kind of a workaround patch is something > distros ought to be fine to carry, if they care about the > functionality and only until they get around to upgrade their > binutils. But I'll be happy to hear differing opinions. Y'all just merged something to support building with make 3.81, released in *2006*, so why require a bleeding edge binutils to work with the executable image? > I also don't see any mention anywhere of why it's 32 bytes, and not > 16 or 64 or yet something else. It is 32 because you said 32 was probably fine. -- Trammell
Re: [PATCH v3 1/4] x86/xen.lds.S: Work around binutils build id alignment bug
On Tuesday, September 8, 2020 8:29 AM, Jan Beulich wrote: > [...] As with, I think, the majority of new > features, distros would pick up your new functionality mainly for > use in new versions, and hence would likely run with new binutils > anyway by that time. It also occurs to me that the binutils used to process the xen.efi image does not need to be the same as the one used to generate it, so there are no build-time dependencies on having this fix in place. Dropping this patch from the series doesn't affect the ability of a distribution with a new binutils from being able to build unified images, so I'm fine with abandoning it. Are there any further thoughts on the other parts of the series? -- Trammell
Re: [PATCH v3 2/4] efi/boot.c: add file.need_to_free and split display_file_info()
On Monday, September 14, 2020 5:05 AM, Roger Pau Monné wrote: > Thanks! Being picky you likely wan to split this into two separate > commits: one for adding need_to_free and the other for > display_file_info. There's no relation between the two that would > require them to be on the same commit. Ok. I'll address that in the v4 of the patch. > [...] > > +static void __init display_file_info(CHAR16 *name, struct file *file, char > > *options) > > I think name at least could be constified? Oh, I wish. There should be a major pass to constify the EFI functions. So many of them are not const-correct and it worries me that literal strings are passed to those functions. > Also efi_arch_handle_module seem to do more than just printing file > info, hence I would likely rename this to handle_file_info to be more > representative of what it does. Ok. Fixed in v4. -- Trammell
Re: [PATCH v3 3/4] efi: Enable booting unified hypervisor/kernel/initrd images
On Monday, September 14, 2020 6:06 AM, Roger Pau Monné wrote: > On Mon, Sep 07, 2020 at 03:00:26PM -0400, Trammell Hudson wrote: > > [...] > > 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 > > (`.initrd`), and XSM config (`.xsm`), which are included after building > > Could we name this kernel_payload or maybe just payload instead of > initrd? > > That's a Linux specific concept. Perhaps "ramdisk" is better, since that is the name of the module in the Xen config file? That was what I used elsewhere (and messed up in the docs, as you had noticed). > [...] > > - --change-section-vma .initrd=0x82d04200 \ > > Why do you need to adjust the VMA, is this not set to a suitable > default value? > > How can users find a suitable VMA value? The default is to put the new sections at virtual address 0, which causes load errors. It seemed to be necessary to have it above the hypervisor image, although I'm also borrowing that from the systemd-boot code. I wish objcopy had an "--append-section" that would add after the last section in the file... An earlier version of the patch series had a shell script to do this process, although it was not as general as people wanted, so it was dropped in favor of documenting how to build the images with objcopy. > [...] > > - file->ptr = (void *)pe_find_section(image->ImageBase, image->ImageSize, > > This already returns a void *, so there's no need for the cast. It returns const void *, but file has a non-const pointer. Perhaps files should be immutable and that could be addressed in a const-correctness patch series. The style guide issues will also be addressed in the v4 of the patch, to be posted soon. -- Trammell
Re: [PATCH v3 4/4] efi: Do not use command line if secure boot is enabled.
On Monday, September 14, 2020 6:24 AM, Roger Pau Monné wrote: > On Mon, Sep 07, 2020 at 03:00:27PM -0400, Trammell Hudson wrote: > [...] > > - static const __initconst EFI_GUID global_guid = EFI_GLOBAL_VARIABLE; > > - uint8_t secboot, setupmode; > > - UINTN secboot_size = sizeof(secboot); > > - UINTN setupmode_size = sizeof(setupmode); > > - > > - if ( efi_rs->GetVariable(L"SecureBoot", (EFI_GUID *)&global_guid, NULL, > > &secboot_size, &secboot) != EFI_SUCCESS ) > > I'm slightly worried about the dropping of the const here, and the > fact that the variable is placed in initconst section. Isn't it > dangerous that the EFI services will try to write to it? The EFI services do not try to write to it; the API doesn't even bother with const-correctness. The prototype has IN and OUT, but they are not used for constness: typedef EFI_STATUS(EFIAPI * EFI_GET_VARIABLE) ( IN CHAR16 *VariableName, IN EFI_GUID *VendorGuid, OUT UINT32 *Attributes, OPTIONAL IN OUT UINTN *DataSize, OUT VOID *Data OPTIONAL) (So the VariableName string is also silently being turned into a non-const pointer as well, which is just ugh) > [...] > > - return secboot == 1 && setupmode == 0; > > I would print a message if secboot is > 1, since those should be > reserved. Ok. Addressed in v4, coming soon. -- Trammell
[PATCH v4 3/4] efi: Enable booting unified hypervisor/kernel/initrd images
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 --- .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 0x82d04100. +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=0x82d04100 + --add-section .ucode=ucode.bin \ + --change-section-vma .ucode=0x82d04101 \ + --add-section .xsm=xsm.cfg \ + --change-section-vma .xsm=0x82d04108 \ + --add-section .kernel=vmlinux \ + --change-section-vma .kernel=0x82d04110 \ + --add-section .ramdisk=initrd.img \ + --change-section-vma .ramdisk=0x82d04200 \ + 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_ha
[PATCH v4 2/4] efi/boot.c: add handle_file_info()
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
---
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
[PATCH v4 4/4] efi: Do not use command line if secure boot is enabled.
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
---
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
[PATCH v4 1/4] efi/boot.c: add file.need_to_free
The config file, kernel, initrd, etc should only be freed if they
are allocated with the UEFI allocator.
Signed-off-by: Trammell Hudson
---
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
[PATCH v4 0/4] efi: Unified Xen hypervisor/kernel/initrd images
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
[RFC PATCH] efi: const correct EFI functions
By defining IN as const, the EFI handler functions become almost
const-correct and allow most of the rest of the EFI boot code to
use constant strings.
There are a few places in the code that casts away the const
that should be reconsidered. The config parser code modifies the
config file in place, which would not work if it were in a read-only
segment.
Signed-off-by: Trammell Hudson
---
xen/arch/arm/efi/efi-boot.h | 8 ++--
xen/arch/x86/efi/efi-boot.h | 26 +--
xen/common/efi/boot.c | 92 +++--
xen/include/efi/efidef.h| 2 +-
4 files changed, 66 insertions(+), 62 deletions(-)
diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index 6527cb0bdf..13666bc065 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -418,9 +418,9 @@ static void __init efi_arch_memory_setup(void)
{
}
-static void __init efi_arch_handle_cmdline(CHAR16 *image_name,
- CHAR16 *cmdline_options,
- char *cfgfile_options)
+static void __init efi_arch_handle_cmdline(const CHAR16 *image_name,
+ const CHAR16 *cmdline_options,
+ const char *cfgfile_options)
{
union string name;
char *buf;
@@ -482,7 +482,7 @@ static void __init efi_arch_handle_cmdline(CHAR16
*image_name,
}
static void __init efi_arch_handle_module(struct file *file, const CHAR16
*name,
- char *options)
+ const char *options)
{
int node;
int chosen;
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 7188c9a551..ce5577012b 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -280,10 +280,10 @@ static void __init efi_arch_cfg_file_late(EFI_FILE_HANDLE
dir_handle, char *sect
{
union string name;
-name.s = get_value(&cfg, section, "ucode");
-if ( !name.s )
-name.s = get_value(&cfg, "global", "ucode");
-if ( name.s )
+name.cs = get_value(&cfg, section, "ucode");
+if ( !name.cs )
+name.cs = get_value(&cfg, "global", "ucode");
+if ( name.cs )
{
microcode_set_module(mbi.mods_count);
split_string(name.s);
@@ -292,29 +292,29 @@ static void __init efi_arch_cfg_file_late(EFI_FILE_HANDLE
dir_handle, char *sect
}
}
-static void __init efi_arch_handle_cmdline(CHAR16 *image_name,
- CHAR16 *cmdline_options,
- char *cfgfile_options)
+static void __init efi_arch_handle_cmdline(const CHAR16 *image_name,
+ const CHAR16 *cmdline_options,
+ const char *cfgfile_options)
{
union string name;
if ( cmdline_options )
{
-name.w = cmdline_options;
+name.cw = cmdline_options;
w2s(&name);
-place_string(&mbi.cmdline, name.s);
+place_string(&mbi.cmdline, name.cs);
}
if ( cfgfile_options )
place_string(&mbi.cmdline, cfgfile_options);
/* Insert image name last, as it gets prefixed to the other options. */
if ( image_name )
{
-name.w = image_name;
+name.cw = image_name;
w2s(&name);
}
else
-name.s = "xen";
-place_string(&mbi.cmdline, name.s);
+name.cs = "xen";
+place_string(&mbi.cmdline, name.cs);
if ( mbi.cmdline )
mbi.flags |= MBI_CMDLINE;
@@ -636,7 +636,7 @@ static void __init efi_arch_memory_setup(void)
}
static void __init efi_arch_handle_module(struct file *file, const CHAR16
*name,
- char *options)
+ const char *options)
{
union string local_name;
void *ptr;
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 4022a672c9..147af81286 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -98,13 +98,14 @@ union string {
CHAR16 *w;
char *s;
const char *cs;
+const CHAR16 *cw;
};
struct file {
UINTN size;
union {
EFI_PHYSICAL_ADDRESS addr;
-void *ptr;
+const void *ptr;
};
};
@@ -113,13 +114,13 @@ static CHAR16 *FormatHex(UINT64 Val, UINTN Width, CHAR16
*Buffer);
static void DisplayUint(UINT64 Val, INTN Width);
static CHAR16 *wstrcpy(CHAR16 *d, const CHAR16 *s);
static void PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode);
-static char *get_value(const struct file *cfg, const char *section,
- const char *item);
+static const char *get_value(const struct file *cfg, const char *section,
+ const char *item);
sta
Re: [RFC PATCH] efi: const correct EFI functions
On Monday, September 14, 2020 10:30 AM, Jan Beulich wrote: > On 14.09.2020 16:25, Trammell Hudson wrote: > > By defining IN as const, the EFI handler functions become almost > > const-correct and allow most of the rest of the EFI boot code to > > use constant strings. > > How does this work with combined "IN OUT"? I'm afraid there is a > reason why things aren't done the way you suggest. WTF FFS UEFI. They really do continue to find new ways to disappoint me. So I see three options: Option 1 is to retroactively modify the C spec to allow us to have a "nonconst" that will override any prior "const" modifiers (last one wins, like Duck Season/Rabbit Season). Option 2 would be to modify the imported header to change the 30 uses of "IN OUT" to "INOUT" and define that to be an empty string. Option 3 would be to write wrappers for the few functions that are used in the EFI boot path that cast-away the constness of their arguments (while also silently cursing the UEFI forum for not writing const-correct code). -- Trammell
Re: [RFC PATCH] efi: const correct EFI functions
On Monday, September 14, 2020 10:55 AM, Jan Beulich wrote: > On 14.09.2020 16:46, Trammell Hudson wrote: > > Option 3 would be to write wrappers for the few functions that are > > used in the EFI boot path that cast-away the constness of their > > arguments (while also silently cursing the UEFI forum for not > > writing const-correct code). > > This would be kind of a last resort fallback (except for the > cursing, which of course we can do at any time). Since you didn't like the time travel option, I checked to see which functions would need to be wrapped. It is a surprisingly small number: #define PrintStr(s) StdOut->OutputString(StdOut, (CHAR16 *)(s)) #define PrintErr(s) StdErr->OutputString(StdErr, (CHAR16 *)(s)) #define efi_file_open(file,handle,name,mode,attr) \ (file)->Open(file, handle, (CHAR16 *)(name), mode, attr) #define shim_verify(shim, ptr, len) \ (shim)->Verify((void *)(ptr), len) -- Trammell
Re: [RFC PATCH] efi: const correct EFI functions
On Tuesday, September 15, 2020 9:41 AM, Jan Beulich wrote: > On 14.09.2020 17:05, Trammell Hudson wrote: > > [...] I checked to see > > which functions would need to be wrapped. It is a surprisingly > > small number: > > #define PrintStr(s) StdOut->OutputString(StdOut, (CHAR16 *)(s)) > > #define PrintErr(s) StdErr->OutputString(StdErr, (CHAR16 *)(s)) > > #define efi_file_open(file,handle,name,mode,attr) \ > > (file)->Open(file, handle, (CHAR16 *)(name), mode, attr) > > #define shim_verify(shim, ptr, len) \ > > (shim)->Verify((void *)(ptr), len) > > That's surprisingly few. What about e.g. HandleProtocol() and > LocateHandle()? GUIDs shouldn't really be non-const either. Good point -- I did not track down all uses of efi_bs and every GUID. I'll send a RFC patch v2 in a minute with all of the GUIDs converted to __initconst. -- Trammell
[RFC PATCH v2] efi: const correct EFI functions
By wrapping the few EFI handler functions used by the Xen boot process
and casting away constness where safe, it is possible to allow most of
the rest of the EFI boot code to use constant strings and GUIDs.
There are a few places in the code that casts away the const that should
be reconsidered. For instance, the config parser code modifies the config
file in place, which would not work if it were in a read-only segment.
Signed-off-by: Trammell Hudson
---
xen/arch/arm/efi/efi-boot.h | 8 +-
xen/arch/x86/efi/efi-boot.h | 40 -
xen/common/efi/boot.c | 157
3 files changed, 111 insertions(+), 94 deletions(-)
diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index 6527cb0bdf..13666bc065 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -418,9 +418,9 @@ static void __init efi_arch_memory_setup(void)
{
}
-static void __init efi_arch_handle_cmdline(CHAR16 *image_name,
- CHAR16 *cmdline_options,
- char *cfgfile_options)
+static void __init efi_arch_handle_cmdline(const CHAR16 *image_name,
+ const CHAR16 *cmdline_options,
+ const char *cfgfile_options)
{
union string name;
char *buf;
@@ -482,7 +482,7 @@ static void __init efi_arch_handle_cmdline(CHAR16
*image_name,
}
static void __init efi_arch_handle_module(struct file *file, const CHAR16
*name,
- char *options)
+ const char *options)
{
int node;
int chosen;
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 7188c9a551..8e9811f3e0 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -280,10 +280,10 @@ static void __init efi_arch_cfg_file_late(EFI_FILE_HANDLE
dir_handle, char *sect
{
union string name;
-name.s = get_value(&cfg, section, "ucode");
-if ( !name.s )
-name.s = get_value(&cfg, "global", "ucode");
-if ( name.s )
+name.cs = get_value(&cfg, section, "ucode");
+if ( !name.cs )
+name.cs = get_value(&cfg, "global", "ucode");
+if ( name.cs )
{
microcode_set_module(mbi.mods_count);
split_string(name.s);
@@ -292,29 +292,29 @@ static void __init efi_arch_cfg_file_late(EFI_FILE_HANDLE
dir_handle, char *sect
}
}
-static void __init efi_arch_handle_cmdline(CHAR16 *image_name,
- CHAR16 *cmdline_options,
- char *cfgfile_options)
+static void __init efi_arch_handle_cmdline(const CHAR16 *image_name,
+ const CHAR16 *cmdline_options,
+ const char *cfgfile_options)
{
union string name;
if ( cmdline_options )
{
-name.w = cmdline_options;
+name.cw = cmdline_options;
w2s(&name);
-place_string(&mbi.cmdline, name.s);
+place_string(&mbi.cmdline, name.cs);
}
if ( cfgfile_options )
place_string(&mbi.cmdline, cfgfile_options);
/* Insert image name last, as it gets prefixed to the other options. */
if ( image_name )
{
-name.w = image_name;
+name.cw = image_name;
w2s(&name);
}
else
-name.s = "xen";
-place_string(&mbi.cmdline, name.s);
+name.cs = "xen";
+place_string(&mbi.cmdline, name.cs);
if ( mbi.cmdline )
mbi.flags |= MBI_CMDLINE;
@@ -328,8 +328,8 @@ static void __init efi_arch_handle_cmdline(CHAR16
*image_name,
static void __init efi_arch_edd(void)
{
-static EFI_GUID __initdata bio_guid = BLOCK_IO_PROTOCOL;
-static EFI_GUID __initdata devp_guid = DEVICE_PATH_PROTOCOL;
+static const EFI_GUID __initconst bio_guid = BLOCK_IO_PROTOCOL;
+static const EFI_GUID __initconst devp_guid = DEVICE_PATH_PROTOCOL;
EFI_HANDLE *handles = NULL;
unsigned int i;
UINTN size;
@@ -339,12 +339,12 @@ static void __init efi_arch_edd(void)
BUILD_BUG_ON(offsetof(struct edd_info, edd_device_params) != EDDEXTSIZE);
BUILD_BUG_ON(sizeof(struct edd_device_params) != EDDPARMSIZE);
size = 0;
-status = efi_bs->LocateHandle(ByProtocol, &bio_guid, NULL, &size, NULL);
+status = efi_locate_handle(ByProtocol, &bio_guid, NULL, &size, NULL);
if ( status == EFI_BUFFER_TOO_SMALL )
status = efi_bs->AllocatePool(EfiLoaderData, size, (void **)&handles);
if ( !EFI_ERROR(status) )
-status = efi_bs->LocateHandle(ByProtocol, &bio_guid, NULL, &size,
- handles);
+status = efi_locate_handle(ByProtocol, &bio
Re: [RFC PATCH v2] efi: const correct EFI functions
On Tuesday, September 15, 2020 12:36 PM, Jan Beulich wrote: > In order for these casts to be halfway safe, they need to happen in > inline functions, not macros. That way it'll be sufficiently clear > and certain that it's really only the const which gets changed, > but not e.g. also the pointed-to type. At some point it really looks easier to just add const to the efi headers rather than having to recreate all of their verbose APIs. The efiapi.h header was added nine years ago and has been edited once, seven years ago, so it isn't a source of significant churn. Meanwhile, if someone wants to delve into the cursed mines of sourceforge, perhaps the gnuefi devs can be convinced to update their sources. > Apart from this I think the whole change wants splitting up, to > (about) one basic change at a time. Yeah, the string and file const changes ended up mixed in the change set. They are sort of related, but it would be easiest if the EFI constness was fixed first so that the printed strings don't need additional casts. -- Trammell
Re: [PATCH v4 3/4] efi: Enable booting unified hypervisor/kernel/initrd images
On Wednesday, September 16, 2020 3:32 AM, Roger Pau Monné 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
Re: [PATCH v4 4/4] efi: Do not use command line if secure boot is enabled.
On Wednesday, September 16, 2020 3:45 AM, Roger Pau Monné 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
Re: [PATCH v4 3/4] efi: Enable booting unified hypervisor/kernel/initrd images
On Thursday, September 17, 2020 8:33 AM, Jan Beulich wrote: > On 14.09.2020 13:50, Trammell Hudson wrote: > [...] > > +For all the examples the `.pad` section ended at 0x82d04100. > > +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
Re: [PATCH v4 3/4] efi: Enable booting unified hypervisor/kernel/initrd images
On Thursday, September 17, 2020 9:04 AM, Trammell Hudson wrote: > On Thursday, September 17, 2020 8:33 AM, Jan Beulich jbeul...@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
Re: [PATCH v4 4/4] efi: Do not use command line if secure boot is enabled.
On Thursday, September 17, 2020 8:51 AM, Jan Beulich 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
[PATCH v5 0/5] efi: Unified Xen hypervisor/kernel/initrd images
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 (5): efi/boot.c: Make file->ptr const void* 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 unified config is included .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 | 162 +++- xen/common/efi/efi.h| 3 + xen/common/efi/pe.c | 134 + 8 files changed, 335 insertions(+), 52 deletions(-) create mode 100644 xen/common/efi/pe.c -- 2.25.1
[PATCH v5 1/5] efi/boot.c: Make file->ptr const void*
Other than the config file parser that edits the image inplace,
no other users of the file sections requires write access to the
data.
Signed-off-by: Trammell Hudson
---
xen/common/efi/boot.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 0ac80e47bb..157fe0e8c5 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -41,7 +41,7 @@
typedef EFI_STATUS
(/* _not_ EFIAPI */ *EFI_SHIM_LOCK_VERIFY) (
-IN VOID *Buffer,
+IN const VOID *Buffer,
IN UINT32 Size);
typedef struct {
@@ -104,7 +104,8 @@ struct file {
UINTN size;
union {
EFI_PHYSICAL_ADDRESS addr;
-void *ptr;
+char *str;
+const void *ptr;
};
};
@@ -592,7 +593,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle,
CHAR16 *name,
efi_arch_handle_module(file, name, options);
}
-ret = FileHandle->Read(FileHandle, &file->size, file->ptr);
+ret = FileHandle->Read(FileHandle, &file->size, file->str);
if ( !EFI_ERROR(ret) && file->size != size )
ret = EFI_ABORTED;
if ( EFI_ERROR(ret) )
@@ -616,7 +617,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle,
CHAR16 *name,
static void __init pre_parse(const struct file *cfg)
{
-char *ptr = cfg->ptr, *end = ptr + cfg->size;
+char *ptr = cfg->str, *end = ptr + cfg->size;
bool start = true, comment = false;
for ( ; ptr < end; ++ptr )
@@ -645,7 +646,7 @@ static void __init pre_parse(const struct file *cfg)
static char *__init get_value(const struct file *cfg, const char *section,
const char *item)
{
-char *ptr = cfg->ptr, *end = ptr + cfg->size;
+char *ptr = cfg->str, *end = ptr + cfg->size;
size_t slen = section ? strlen(section) : 0, ilen = strlen(item);
bool match = !slen;
--
2.25.1
[PATCH v5 4/5] efi: Enable booting unified hypervisor/kernel/initrd images
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. Note that the system will fall back to loading files from disk if the named sections do not exist. This allows distributions to continue with the status quo if they want a signed kernel + config, while still allowing a user provided initrd (which is how the shim protocol currently works as well). Signed-off-by: Trammell hudson --- .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 | 134 8 files changed, 269 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 0x82d04100. +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=0x82d04100 + --add-section .ucode=ucode.bin \ + --change-section-vma .ucode=0x82d04101 \ + --add-section .xsm=xsm.cfg \ + --change-section-vma .xsm=0x82d04108 \ + --add-section .kernel=vmlinux \ + --change-section-vma .kernel=0x82d04110 \ + --add-section .ramdisk=initrd.img \ + --change-section-vma .ramdisk=0x82d04200 \ + 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 27dd0b1a94..47ced1db8d 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
[PATCH v5 2/5] efi/boot.c: add file.need_to_free
The config file, kernel, initrd, etc should only be freed if they
are allocated with the UEFI allocator.
Signed-off-by: Trammell Hudson
---
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 157fe0e8c5..77530a0595 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;
char *str;
@@ -280,13 +281,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();
@@ -573,6 +574,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
[PATCH v5 3/5] efi/boot.c: add handle_file_info()
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
---
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 77530a0595..e0280f7a21 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -540,6 +540,22 @@ static char * __init split_string(char *s)
return NULL;
}
+static void __init handle_file_info(CHAR16 *name,
+const struct file *file, const 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, const char *options)
{
@@ -584,16 +600,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->str);
if ( !EFI_ERROR(ret) && file->size != size )
--
2.25.1
[PATCH v5 5/5] efi: Do not use command line if unified config is included
If a unified Xen image is used, then the bundled configuration,
Xen command line, dom0 kernel, and ramdisk are prefered over
any files listed in the config file or provided on the command line.
Unlike the shim based verification, the PE signature on a unified image
covers all of the Xen+config+kernel+initrd modules linked into the
unified image. This also ensures that, on properly configured UEFI Secure Boot
platforms, the entire runtime will be measured into the TPM for unsealing
secrets or remote attestation.
Signed-off-by: Trammell Hudson
---
xen/common/efi/boot.c | 43 ++-
1 file changed, 38 insertions(+), 5 deletions(-)
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index c14e836640..6dc3f5ac94 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -952,6 +952,35 @@ 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 __initdata 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", &global_guid,
+ NULL, &secboot_size, &secboot);
+if ( rc != EFI_SUCCESS )
+return false;
+
+rc = efi_rs->GetVariable(L"SetupMode", &global_guid,
+ NULL, &setupmode_size, &setupmode);
+if ( rc != EFI_SUCCESS )
+return false;
+
+if ( secboot > 1 || setupmode > 1 )
+blexit(L"Invalid SecureBoot/SetupMode variables");
+
+return secboot == 1 && setupmode == 0;
+}
+
static void __init efi_variables(void)
{
EFI_STATUS status;
@@ -1128,15 +1157,15 @@ 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;
union string section = { NULL }, name;
bool base_video = false;
const char *option_str;
-bool use_cfg_file;
+bool use_cfg_file, secure;
__set_bit(EFI_BOOT, &efi_flags);
__set_bit(EFI_LOADER, &efi_flags);
@@ -1155,8 +1184,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 ( use_cfg_file &&
+ !read_section(loaded_image, ".config", &cfg, NULL))
{
UINTN offset = 0;
@@ -1214,6 +1245,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);
@@ -1233,7 +1266,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE
*SystemTable)
/* Get the file system interface. */
dir_handle = get_parent_handle(loaded_image, &file_name);
-if ( read_section(loaded_image, ".config", &cfg, NULL) )
+if ( cfg.ptr )
PrintStr(L"Using unified config file\r\n");
else if ( !cfg_file_name )
{
--
2.25.1
Re: [PATCH v4 4/4] efi: Do not use command line if secure boot is enabled.
On Thursday, September 17, 2020 11:26 AM, Jan Beulich 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
Re: [PATCH v5 4/5] efi: Enable booting unified hypervisor/kernel/initrd images
On Friday, September 18, 2020 11:15 AM, Jan Beulich wrote: > On 17.09.2020 17:40, Trammell Hudson wrote: > Instead of forcing the caller to pass in a dot-prefixed name > and you assuming it's a dot here, how about ... > ... pe_find_section() looking for '.' followed by ? v6 adds a special name compare function to do this with a CHAR16 section name to avoid the extra s2w() you mentioned. (btw, even if the EFI constness patches don't go in, just making PrintStr cast away the const would allow several of these startup functions to have const CHAR16* arguments since the only reason they are non-const is to be able to print) > [...] > > - if ( read_section(loaded_image, ".config", &cfg, NULL) ) > > - PrintStr(L"Using unified config file\\r\\n"); > > Maybe "embedded" instead of "unified"? The config file isn't unified > after all, it's the Xen binary which is. How about "builtin"? I missed the reviews on the need_to_free patch; they are also incorporated into v6. -- Trammell
[PATCH v6 4/5] efi: Enable booting unified hypervisor/kernel/initrd images
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. Note that the system will fall back to loading files from disk if the named sections do not exist. This allows distributions to continue with the status quo if they want a signed kernel + config, while still allowing a user provided initrd (which is how the shim protocol currently works as well). This patch also adds constness to the section parameter of efi_arch_cfg_file_early() and efi_arch_cfg_file_late() and changes pe_find_section() to use a CHAR16 section name. Signed-off-by: Trammell hudson --- .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 | 66 +++ xen/common/efi/efi.h| 3 + xen/common/efi/pe.c | 159 8 files changed, 287 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 0x82d04100. +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=0x82d04100 + --add-section .ucode=ucode.bin \ + --change-section-vma .ucode=0x82d04101 \ + --add-section .xsm=xsm.cfg \ + --change-section-vma .xsm=0x82d04108 \ + --add-section .kernel=vmlinux \ + --change-section-vma .kernel=0x82d04110 \ + --add-section .ramdisk=initrd.img \ + --change-section-vma .ramdisk=0x82d04200 \ + 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
[PATCH v6 0/5] efi: Unified Xen hypervisor/kernel/initrd images
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 (5): efi/boot.c: Make file->ptr const void* 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 unified config is included .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 | 155 +-- xen/common/efi/efi.h| 3 + xen/common/efi/pe.c | 159 8 files changed, 353 insertions(+), 52 deletions(-) create mode 100644 xen/common/efi/pe.c -- 2.25.1
[PATCH v6 3/5] efi/boot.c: add handle_file_info()
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
Acked-by: Jan Beulich
---
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 c2ce0c7294..93cfeba7e1 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -540,6 +540,22 @@ static char * __init split_string(char *s)
return NULL;
}
+static void __init handle_file_info(CHAR16 *name,
+const struct file *file, const 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, const char *options)
{
@@ -584,16 +600,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle,
CHAR16 *name,
{
file->need_to_free = true;
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->str);
if ( !EFI_ERROR(ret) && file->size != size )
--
2.25.1
[PATCH v6 1/5] efi/boot.c: Make file->ptr const void*
Other than the config file parser that edits the image inplace,
no other users of the file sections requires write access to the
data.
Signed-off-by: Trammell Hudson
Reviewed-by: Jan Beulich
Reviewed-by: Roger Pau Monné
---
xen/common/efi/boot.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 0ac80e47bb..157fe0e8c5 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -41,7 +41,7 @@
typedef EFI_STATUS
(/* _not_ EFIAPI */ *EFI_SHIM_LOCK_VERIFY) (
-IN VOID *Buffer,
+IN const VOID *Buffer,
IN UINT32 Size);
typedef struct {
@@ -104,7 +104,8 @@ struct file {
UINTN size;
union {
EFI_PHYSICAL_ADDRESS addr;
-void *ptr;
+char *str;
+const void *ptr;
};
};
@@ -592,7 +593,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle,
CHAR16 *name,
efi_arch_handle_module(file, name, options);
}
-ret = FileHandle->Read(FileHandle, &file->size, file->ptr);
+ret = FileHandle->Read(FileHandle, &file->size, file->str);
if ( !EFI_ERROR(ret) && file->size != size )
ret = EFI_ABORTED;
if ( EFI_ERROR(ret) )
@@ -616,7 +617,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle,
CHAR16 *name,
static void __init pre_parse(const struct file *cfg)
{
-char *ptr = cfg->ptr, *end = ptr + cfg->size;
+char *ptr = cfg->str, *end = ptr + cfg->size;
bool start = true, comment = false;
for ( ; ptr < end; ++ptr )
@@ -645,7 +646,7 @@ static void __init pre_parse(const struct file *cfg)
static char *__init get_value(const struct file *cfg, const char *section,
const char *item)
{
-char *ptr = cfg->ptr, *end = ptr + cfg->size;
+char *ptr = cfg->str, *end = ptr + cfg->size;
size_t slen = section ? strlen(section) : 0, ilen = strlen(item);
bool match = !slen;
--
2.25.1
[PATCH v6 5/5] efi: Do not use command line if unified config is included
If a unified Xen image is used, then the bundled configuration,
Xen command line, dom0 kernel, and ramdisk are prefered over
any files listed in the config file or provided on the command line.
Unlike the shim based verification, the PE signature on a unified image
covers all of the Xen+config+kernel+initrd modules linked into the
unified image. This also ensures that, on properly configured UEFI Secure Boot
platforms, the entire runtime will be measured into the TPM for unsealing
secrets or remote attestation.
Signed-off-by: Trammell Hudson
---
xen/common/efi/boot.c | 43 ++-
1 file changed, 38 insertions(+), 5 deletions(-)
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index dcf939a05e..28903d6013 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -945,6 +945,35 @@ 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 __initdata 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", &global_guid,
+ NULL, &secboot_size, &secboot);
+if ( rc != EFI_SUCCESS )
+return false;
+
+rc = efi_rs->GetVariable(L"SetupMode", &global_guid,
+ NULL, &setupmode_size, &setupmode);
+if ( rc != EFI_SUCCESS )
+return false;
+
+if ( secboot > 1 || setupmode > 1 )
+blexit(L"Invalid SecureBoot/SetupMode variables");
+
+return secboot == 1 && setupmode == 0;
+}
+
static void __init efi_variables(void)
{
EFI_STATUS status;
@@ -1121,15 +1150,15 @@ 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;
union string section = { NULL }, name;
bool base_video = false;
const char *option_str;
-bool use_cfg_file;
+bool use_cfg_file, secure;
__set_bit(EFI_BOOT, &efi_flags);
__set_bit(EFI_LOADER, &efi_flags);
@@ -1148,8 +1177,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 ( use_cfg_file &&
+ !read_section(loaded_image, L"config", &cfg, NULL) )
{
UINTN offset = 0;
@@ -1207,6 +1238,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);
@@ -1226,7 +1259,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE
*SystemTable)
/* Get the file system interface. */
dir_handle = get_parent_handle(loaded_image, &file_name);
-if ( read_section(loaded_image, L"config", &cfg, NULL) )
+if ( cfg.ptr )
PrintStr(L"Using builtin config file\r\n");
else if ( !cfg_file_name )
{
--
2.25.1
[PATCH v6 2/5] efi/boot.c: add file.need_to_free
The config file, kernel, initrd, etc should only be freed if they
are allocated with the UEFI allocator.
Signed-off-by: Trammell Hudson
Reviewed-by: Roger Pau Monné
---
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 157fe0e8c5..c2ce0c7294 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;
char *str;
@@ -280,13 +281,13 @@ void __init noreturn blexit(const CHAR16 *str)
if ( !efi_bs )
efi_arch_halt();
-if ( cfg.addr )
+if ( cfg.need_to_free )
efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
-if ( kernel.addr )
+if ( kernel.need_to_free )
efi_bs->FreePages(kernel.addr, PFN_UP(kernel.size));
-if ( ramdisk.addr )
+if ( ramdisk.need_to_free )
efi_bs->FreePages(ramdisk.addr, PFN_UP(ramdisk.size));
-if ( xsm.addr )
+if ( xsm.need_to_free )
efi_bs->FreePages(xsm.addr, PFN_UP(xsm.size));
efi_arch_blexit();
@@ -581,6 +582,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle,
CHAR16 *name,
}
else
{
+file->need_to_free = true;
file->size = size;
if ( file != &cfg )
{
--
2.25.1
Re: [PATCH v6 2/5] efi/boot.c: add file.need_to_free
On Tuesday, September 29, 2020 6:17 AM, Jan Beulich wrote: > On 21.09.2020 13:51, Trammell Hudson wrote: > [...] > > Reviewed-by: Roger Pau Monné roger@citrix.com > > Strictly speaking with the changes done from v5 to v6 this tag > would have needed dropping. I guess Roger is fine with it being > kept, though. Ok. > [...] > Doesn't this need similar changes in both efi_arch_blexit()? It does -- I had missed that there was a place to free them and had assumed that they were just leaked. Fixed in v7. -- Trammell
Re: [PATCH v6 4/5] efi: Enable booting unified hypervisor/kernel/initrd images
On Tuesday, September 29, 2020 11:37 AM, Jan Beulich wrote: > On 21.09.2020 13:51, Trammell Hudson wrote: > [...] > > - file->need_to_free = false; > > In patch 2 you don't bother clearing the field, presumably because > it's static data and hence zero-filled anyway. This same assumption > would then hold here. Or else, to be consistent, the earlier patch > would want clearing added. Removed. > [...] > > - const char c = sect->Name[i]; > > - const CHAR16 cw = name[i-1]; > > - if ( cw == 0 && c == 0 ) > > Blank line between declarations and statements please. Added. > [...] > > - if ( name[i-1] < 0 ) > > - return -1; > > I'm afraid this is liable to trigger compiler warnings, for checking > an unsigned quantity to be negative. The compare function had strcmp() like semantics, but since it was not used, I've modified it to just return a 0 or -1 in all cases. So the comparison against < 0 goes away. > Also throughout here "i-1" wants spelling "i - 1". Fixed in both places. > > - /* PE32+ Subsystem type */ > > - if ( pe->FileHeader.Machine != PE_HEADER_MACHINE ) > > - return NULL; > > Comment and code don't look to be in line. Yeah, comment had drifted out of sync as some of the other tests moved around. I've removed it in the v7 version. -- Trammell
[PATCH v7 0/5] efi: Unified Xen hypervisor/kernel/initrd images
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 (5): efi/boot.c: add file.need_to_free efi/boot.c: add handle_file_info() efi/boot.c: wrap PrintStr/PrintErr to allow const CHAR16* arguments efi: Enable booting unified hypervisor/kernel/initrd images efi: Do not use command line if unified config is included .gitignore | 1 + docs/misc/efi.pandoc| 49 +++ xen/arch/arm/efi/efi-boot.h | 27 -- xen/arch/x86/efi/Makefile | 2 +- xen/arch/x86/efi/efi-boot.h | 13 ++- xen/common/efi/boot.c | 158 ++-- xen/common/efi/efi.h| 3 + xen/common/efi/pe.c | 156 +++ 8 files changed, 355 insertions(+), 54 deletions(-) create mode 100644 xen/common/efi/pe.c -- 2.25.1
[PATCH v7 2/5] efi/boot.c: add handle_file_info()
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
Acked-by: Jan Beulich
---
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 c2ce0c7294..93cfeba7e1 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -540,6 +540,22 @@ static char * __init split_string(char *s)
return NULL;
}
+static void __init handle_file_info(CHAR16 *name,
+const struct file *file, const 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, const char *options)
{
@@ -584,16 +600,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle,
CHAR16 *name,
{
file->need_to_free = true;
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->str);
if ( !EFI_ERROR(ret) && file->size != size )
--
2.25.1
[PATCH v7 3/5] efi/boot.c: wrap PrintStr/PrintErr to allow const CHAR16* arguments
This patch wraps the EFI OutputString() method so that they can be
called with const arguments. The OutputString method does not modify
its argument, although the prototype is missing const, so it is necssary
to cast away the const when calling it.
Signed-off-by: Trammell Hudson
---
xen/common/efi/boot.c | 18 --
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 93cfeba7e1..fc2ea80e43 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -151,10 +151,16 @@ static struct file __initdata cfg;
static struct file __initdata kernel;
static struct file __initdata ramdisk;
static struct file __initdata xsm;
-static CHAR16 __initdata newline[] = L"\r\n";
+static const CHAR16 __initconst newline[] = L"\r\n";
-#define PrintStr(s) StdOut->OutputString(StdOut, s)
-#define PrintErr(s) StdErr->OutputString(StdErr, s)
+static void __init PrintStr(const CHAR16 *s)
+{
+StdOut->OutputString(StdOut, (CHAR16 *)s );
+}
+static void __init PrintErr(const CHAR16 *s)
+{
+StdErr->OutputString(StdErr, (CHAR16 *)s );
+}
/*
* Include architecture specific implementation here, which references the
@@ -275,7 +281,7 @@ static bool __init match_guid(const EFI_GUID *guid1, const
EFI_GUID *guid2)
void __init noreturn blexit(const CHAR16 *str)
{
if ( str )
-PrintStr((CHAR16 *)str);
+PrintStr(str);
PrintStr(newline);
if ( !efi_bs )
@@ -540,7 +546,7 @@ static char * __init split_string(char *s)
return NULL;
}
-static void __init handle_file_info(CHAR16 *name,
+static void __init handle_file_info(const CHAR16 *name,
const struct file *file, const char
*options)
{
if ( file == &cfg )
@@ -562,7 +568,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle,
CHAR16 *name,
EFI_FILE_HANDLE FileHandle = NULL;
UINT64 size;
EFI_STATUS ret;
-CHAR16 *what = NULL;
+const CHAR16 *what = NULL;
if ( !name )
PrintErrMesg(L"No filename", EFI_OUT_OF_RESOURCES);
--
2.25.1
[PATCH v7 1/5] efi/boot.c: add file.need_to_free
The config file, kernel, initrd, etc should only be freed if they
are allocated with the UEFI allocator. On x86 the ucode, and on
ARM the dtb, are also marked as need_to_free.
Signed-off-by: Trammell Hudson
---
xen/arch/arm/efi/efi-boot.h | 2 +-
xen/arch/x86/efi/efi-boot.h | 2 +-
xen/common/efi/boot.c | 10 ++
3 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index 27dd0b1a94..82f2fc7685 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -546,7 +546,7 @@ static void __init efi_arch_cpu(void)
static void __init efi_arch_blexit(void)
{
-if ( dtbfile.addr && dtbfile.size )
+if ( dtbfile.need_to_free )
efi_bs->FreePages(dtbfile.addr, PFN_UP(dtbfile.size));
if ( memmap )
efi_bs->FreePool(memmap);
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index eef3f52789..1025000afd 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -689,7 +689,7 @@ static void __init efi_arch_cpu(void)
static void __init efi_arch_blexit(void)
{
-if ( ucode.addr )
+if ( ucode.need_to_free )
efi_bs->FreePages(ucode.addr, PFN_UP(ucode.size));
}
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 157fe0e8c5..c2ce0c7294 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;
char *str;
@@ -280,13 +281,13 @@ void __init noreturn blexit(const CHAR16 *str)
if ( !efi_bs )
efi_arch_halt();
-if ( cfg.addr )
+if ( cfg.need_to_free )
efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
-if ( kernel.addr )
+if ( kernel.need_to_free )
efi_bs->FreePages(kernel.addr, PFN_UP(kernel.size));
-if ( ramdisk.addr )
+if ( ramdisk.need_to_free )
efi_bs->FreePages(ramdisk.addr, PFN_UP(ramdisk.size));
-if ( xsm.addr )
+if ( xsm.need_to_free )
efi_bs->FreePages(xsm.addr, PFN_UP(xsm.size));
efi_arch_blexit();
@@ -581,6 +582,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle,
CHAR16 *name,
}
else
{
+file->need_to_free = true;
file->size = size;
if ( file != &cfg )
{
--
2.25.1
[PATCH v7 4/5] efi: Enable booting unified hypervisor/kernel/initrd images
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. Note that the system will fall back to loading files from disk if the named sections do not exist. This allows distributions to continue with the status quo if they want a signed kernel + config, while still allowing a user provided initrd (which is how the shim protocol currently works as well). This patch also adds constness to the section parameter of efi_arch_cfg_file_early() and efi_arch_cfg_file_late() and changes pe_find_section() to use a const CHAR16 section name. Signed-off-by: Trammell Hudson --- .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 | 64 ++- xen/common/efi/efi.h| 3 + xen/common/efi/pe.c | 156 8 files changed, 282 insertions(+), 29 deletions(-) create mode 100644 xen/common/efi/pe.c diff --git a/.gitignore b/.gitignore index 5e8c47e2db..2857fd76eb 100644 --- a/.gitignore +++ b/.gitignore @@ -317,6 +317,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 0x82d04100. +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=0x82d04100 + --add-section .ucode=ucode.bin \ + --change-section-vma .ucode=0x82d04101 \ + --add-section .xsm=xsm.cfg \ + --change-section-vma .xsm=0x82d04108 \ + --add-section .kernel=vmlinux \ + --change-section-vma .kernel=0x82d04110 \ + --add-section .ramdisk=initrd.img \ + --change-section-vma .ramdisk=0x82d04200 \ + 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
[PATCH v7 5/5] efi: Do not use command line if unified config is included
If a unified Xen image is used, then the bundled configuration,
Xen command line, dom0 kernel, and ramdisk are prefered over
any files listed in the config file or provided on the command line.
Unlike the shim based verification, the PE signature on a unified image
covers all of the Xen+config+kernel+initrd modules linked into the
unified image. This also ensures that, on properly configured UEFI Secure Boot
platforms, the entire runtime will be measured into the TPM for unsealing
secrets or remote attestation.
Signed-off-by: Trammell Hudson
---
xen/common/efi/boot.c | 43 ++-
1 file changed, 38 insertions(+), 5 deletions(-)
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index fc3c743738..644005f338 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -949,6 +949,35 @@ 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 __initdata 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", &global_guid,
+ NULL, &secboot_size, &secboot);
+if ( rc != EFI_SUCCESS )
+return false;
+
+rc = efi_rs->GetVariable(L"SetupMode", &global_guid,
+ NULL, &setupmode_size, &setupmode);
+if ( rc != EFI_SUCCESS )
+return false;
+
+if ( secboot > 1 || setupmode > 1 )
+blexit(L"Invalid SecureBoot/SetupMode variables");
+
+return secboot == 1 && setupmode == 0;
+}
+
static void __init efi_variables(void)
{
EFI_STATUS status;
@@ -1125,15 +1154,15 @@ 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;
union string section = { NULL }, name;
bool base_video = false;
const char *option_str;
-bool use_cfg_file;
+bool use_cfg_file, secure;
__set_bit(EFI_BOOT, &efi_flags);
__set_bit(EFI_LOADER, &efi_flags);
@@ -1152,8 +1181,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 ( use_cfg_file &&
+ !read_section(loaded_image, L"config", &cfg, NULL) )
{
UINTN offset = 0;
@@ -1211,6 +1242,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);
@@ -1230,7 +1263,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE
*SystemTable)
/* Get the file system interface. */
dir_handle = get_parent_handle(loaded_image, &file_name);
-if ( read_section(loaded_image, L"config", &cfg, NULL) )
+if ( cfg.ptr )
PrintStr(L"Using builtin config file\r\n");
else if ( !cfg_file_name )
{
--
2.25.1
Re: [PATCH v7 1/5] efi/boot.c: add file.need_to_free
On Wednesday, September 30, 2020 2:49 AM, Jan Beulich wrote: > On 29.09.2020 20:17, Trammell Hudson wrote: > > - if ( dtbfile.addr && dtbfile.size ) > > - if ( dtbfile.need_to_free ) > > efi_bs->FreePages(dtbfile.addr, PFN_UP(dtbfile.size)); > > if ( memmap ) > > efi_bs->FreePool(memmap); > > > > I'm afraid this isn't enough of a change for Arm, due to what > fdt_increase_size() may do. You're right. It looks like there are also potential memory leaks in the fdt_increase_size() error paths as well. I've added free calls in v8. -- Trammell
[PATCH v8 0/5] efi: Unified Xen hypervisor/kernel/initrd images
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 (5): efi/boot.c: add file.need_to_free efi/boot.c: add handle_file_info() efi/boot.c: wrap PrintStr/PrintErr to allow const CHAR16* arguments efi: Enable booting unified hypervisor/kernel/initrd images efi: Do not use command line if unified config is included .gitignore | 1 + docs/misc/efi.pandoc| 49 +++ xen/arch/arm/efi/efi-boot.h | 36 +--- xen/arch/x86/efi/Makefile | 2 +- xen/arch/x86/efi/efi-boot.h | 13 ++- xen/common/efi/boot.c | 161 ++-- xen/common/efi/efi.h| 3 + xen/common/efi/pe.c | 153 ++ 8 files changed, 362 insertions(+), 56 deletions(-) create mode 100644 xen/common/efi/pe.c -- 2.25.1
[PATCH v8 4/5] efi: Enable booting unified hypervisor/kernel/initrd images
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. Note that the system will fall back to loading files from disk if the named sections do not exist. This allows distributions to continue with the status quo if they want a signed kernel + config, while still allowing a user provided initrd (which is how the shim protocol currently works as well). This patch also adds constness to the section parameter of efi_arch_cfg_file_early() and efi_arch_cfg_file_late(), changes pe_find_section() to use a const CHAR16 section name, and adds pe_name_compare() to match section names. Signed-off-by: Trammell Hudson --- .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 | 64 ++- xen/common/efi/efi.h| 3 + xen/common/efi/pe.c | 153 8 files changed, 279 insertions(+), 29 deletions(-) create mode 100644 xen/common/efi/pe.c diff --git a/.gitignore b/.gitignore index 5e8c47e2db..2857fd76eb 100644 --- a/.gitignore +++ b/.gitignore @@ -317,6 +317,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 0x82d04100. +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=0x82d04100 + --add-section .ucode=ucode.bin \ + --change-section-vma .ucode=0x82d04101 \ + --add-section .xsm=xsm.cfg \ + --change-section-vma .xsm=0x82d04108 \ + --add-section .kernel=vmlinux \ + --change-section-vma .kernel=0x82d04110 \ + --add-section .ramdisk=initrd.img \ + --change-section-vma .ramdisk=0x82d04200 \ + 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/ar
[PATCH v8 2/5] efi/boot.c: add handle_file_info()
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
Acked-by: Jan Beulich
---
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 c2ce0c7294..93cfeba7e1 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -540,6 +540,22 @@ static char * __init split_string(char *s)
return NULL;
}
+static void __init handle_file_info(CHAR16 *name,
+const struct file *file, const 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, const char *options)
{
@@ -584,16 +600,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle,
CHAR16 *name,
{
file->need_to_free = true;
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->str);
if ( !EFI_ERROR(ret) && file->size != size )
--
2.25.1
[PATCH v8 1/5] efi/boot.c: add file.need_to_free
The config file, kernel, initrd, etc should only be freed if they
are allocated with the UEFI allocator. On x86 the ucode, and on
ARM the dtb, are also marked as need_to_free when allocated or
expanded.
This also fixes a memory leak in ARM fdt_increase_size() if there
is an error in building the new device tree.
Signed-off-by: Trammell Hudson
---
xen/arch/arm/efi/efi-boot.h | 11 +--
xen/arch/x86/efi/efi-boot.h | 2 +-
xen/common/efi/boot.c | 10 ++
3 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index 27dd0b1a94..c6200fda0e 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -314,7 +314,10 @@ static void __init *fdt_increase_size(struct file
*fdtfile, int add_size)
if ( fdt_size )
{
if ( fdt_open_into(dtbfile.ptr, new_fdt, pages * EFI_PAGE_SIZE) )
+{
+efi_bs->FreePages(fdt_addr, pages);
return NULL;
+}
}
else
{
@@ -326,7 +329,10 @@ static void __init *fdt_increase_size(struct file
*fdtfile, int add_size)
* system table that is passed in the FDT.
*/
if ( fdt_create_empty_tree(new_fdt, pages * EFI_PAGE_SIZE) )
+{
+efi_bs->FreePages(fdt_addr, pages);
return NULL;
+}
}
/*
@@ -335,12 +341,13 @@ static void __init *fdt_increase_size(struct file
*fdtfile, int add_size)
* code will free it. If the original FDT came from a configuration
* table, we don't own that memory and can't free it.
*/
-if ( dtbfile.size )
+if ( dtbfile.need_to_free )
efi_bs->FreePages(dtbfile.addr, PFN_UP(dtbfile.size));
/* Update 'file' info for new memory so we clean it up on error exits */
dtbfile.addr = fdt_addr;
dtbfile.size = pages * EFI_PAGE_SIZE;
+dtbfile.need_to_free = true;
return new_fdt;
}
@@ -546,7 +553,7 @@ static void __init efi_arch_cpu(void)
static void __init efi_arch_blexit(void)
{
-if ( dtbfile.addr && dtbfile.size )
+if ( dtbfile.need_to_free )
efi_bs->FreePages(dtbfile.addr, PFN_UP(dtbfile.size));
if ( memmap )
efi_bs->FreePool(memmap);
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index eef3f52789..1025000afd 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -689,7 +689,7 @@ static void __init efi_arch_cpu(void)
static void __init efi_arch_blexit(void)
{
-if ( ucode.addr )
+if ( ucode.need_to_free )
efi_bs->FreePages(ucode.addr, PFN_UP(ucode.size));
}
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 157fe0e8c5..c2ce0c7294 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;
char *str;
@@ -280,13 +281,13 @@ void __init noreturn blexit(const CHAR16 *str)
if ( !efi_bs )
efi_arch_halt();
-if ( cfg.addr )
+if ( cfg.need_to_free )
efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
-if ( kernel.addr )
+if ( kernel.need_to_free )
efi_bs->FreePages(kernel.addr, PFN_UP(kernel.size));
-if ( ramdisk.addr )
+if ( ramdisk.need_to_free )
efi_bs->FreePages(ramdisk.addr, PFN_UP(ramdisk.size));
-if ( xsm.addr )
+if ( xsm.need_to_free )
efi_bs->FreePages(xsm.addr, PFN_UP(xsm.size));
efi_arch_blexit();
@@ -581,6 +582,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle,
CHAR16 *name,
}
else
{
+file->need_to_free = true;
file->size = size;
if ( file != &cfg )
{
--
2.25.1
[PATCH v8 5/5] efi: Do not use command line if unified config is included
If a unified Xen image is used, then the bundled configuration,
Xen command line, dom0 kernel, and ramdisk are prefered over
any files listed in the config file or provided on the command line.
Unlike the shim based verification, the PE signature on a unified image
covers all of the Xen+config+kernel+initrd modules linked into the
unified image. This also ensures that, on properly configured UEFI Secure Boot
platforms, the entire runtime will be measured into the TPM for unsealing
secrets or remote attestation.
Signed-off-by: Trammell Hudson
---
xen/common/efi/boot.c | 43 ++-
1 file changed, 38 insertions(+), 5 deletions(-)
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 072b1cecd1..07ea5b3512 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -950,6 +950,35 @@ 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 __initdata 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", &global_guid,
+ NULL, &secboot_size, &secboot);
+if ( rc != EFI_SUCCESS )
+return false;
+
+rc = efi_rs->GetVariable(L"SetupMode", &global_guid,
+ NULL, &setupmode_size, &setupmode);
+if ( rc != EFI_SUCCESS )
+return false;
+
+if ( secboot > 1 || setupmode > 1 )
+blexit(L"Invalid SecureBoot/SetupMode variables");
+
+return secboot == 1 && setupmode == 0;
+}
+
static void __init efi_variables(void)
{
EFI_STATUS status;
@@ -1126,15 +1155,15 @@ 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;
union string section = { NULL }, name;
bool base_video = false;
const char *option_str;
-bool use_cfg_file;
+bool use_cfg_file, secure;
__set_bit(EFI_BOOT, &efi_flags);
__set_bit(EFI_LOADER, &efi_flags);
@@ -1153,8 +1182,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 ( use_cfg_file &&
+ !read_section(loaded_image, L"config", &cfg, NULL) )
{
UINTN offset = 0;
@@ -1212,6 +1243,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);
@@ -1231,7 +1264,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE
*SystemTable)
/* Get the file system interface. */
dir_handle = get_parent_handle(loaded_image, &file_name);
-if ( read_section(loaded_image, L"config", &cfg, NULL) )
+if ( cfg.ptr )
PrintStr(L"Using builtin config file\r\n");
else if ( !cfg_file_name )
{
--
2.25.1
[PATCH v8 3/5] efi/boot.c: wrap PrintStr/PrintErr to allow const CHAR16* arguments
This patch wraps the EFI OutputString() method so that they can be
called with const arguments. The OutputString method does not modify
its argument, although the prototype is missing const, so it is necssary
to cast away the const when calling it.
It also updates callers of PrintStr/PrintErr to remove unneeded un-const casts.
Signed-off-by: Trammell Hudson
Reviewed-by: Jan Beulich
---
xen/common/efi/boot.c | 21 ++---
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 93cfeba7e1..bd629eb658 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -151,10 +151,17 @@ static struct file __initdata cfg;
static struct file __initdata kernel;
static struct file __initdata ramdisk;
static struct file __initdata xsm;
-static CHAR16 __initdata newline[] = L"\r\n";
+static const CHAR16 __initconst newline[] = L"\r\n";
-#define PrintStr(s) StdOut->OutputString(StdOut, s)
-#define PrintErr(s) StdErr->OutputString(StdErr, s)
+static void __init PrintStr(const CHAR16 *s)
+{
+StdOut->OutputString(StdOut, (CHAR16 *)s );
+}
+
+static void __init PrintErr(const CHAR16 *s)
+{
+StdErr->OutputString(StdErr, (CHAR16 *)s );
+}
/*
* Include architecture specific implementation here, which references the
@@ -275,7 +282,7 @@ static bool __init match_guid(const EFI_GUID *guid1, const
EFI_GUID *guid2)
void __init noreturn blexit(const CHAR16 *str)
{
if ( str )
-PrintStr((CHAR16 *)str);
+PrintStr(str);
PrintStr(newline);
if ( !efi_bs )
@@ -316,7 +323,7 @@ static void __init PrintErrMesg(const CHAR16 *mesg,
EFI_STATUS ErrCode)
EFI_STATUS ErrIdx = ErrCode & ~EFI_ERROR_MASK;
StdOut = StdErr;
-PrintErr((CHAR16 *)mesg);
+PrintErr(mesg);
PrintErr(L": ");
if( (ErrIdx < ARRAY_SIZE(ErrCodeToStr)) && ErrCodeToStr[ErrIdx] )
@@ -540,7 +547,7 @@ static char * __init split_string(char *s)
return NULL;
}
-static void __init handle_file_info(CHAR16 *name,
+static void __init handle_file_info(const CHAR16 *name,
const struct file *file, const char
*options)
{
if ( file == &cfg )
@@ -562,7 +569,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle,
CHAR16 *name,
EFI_FILE_HANDLE FileHandle = NULL;
UINT64 size;
EFI_STATUS ret;
-CHAR16 *what = NULL;
+const CHAR16 *what = NULL;
if ( !name )
PrintErrMesg(L"No filename", EFI_OUT_OF_RESOURCES);
--
2.25.1
Re: [PATCH v8 3/5] efi/boot.c: wrap PrintStr/PrintErr to allow const CHAR16* arguments
On Wednesday, September 30, 2020 8:15 AM, Jan Beulich wrote:
> On 30.09.2020 14:00, Trammell Hudson wrote:
> > This patch wraps the EFI OutputString() method so that they can be
> > called with const arguments. The OutputString method does not modify
> > its argument, although the prototype is missing const, so it is necssary
> > to cast away the const when calling it.
> > It also updates callers of PrintStr/PrintErr to remove unneeded un-const
> > casts.
> > Signed-off-by: Trammell Hudson hud...@trmm.net
> > Reviewed-by: Jan Beulich jbeul...@suse.com
>
> This one got committed earlier today, sadly ...
Ah, I had missed it when I rebased onto upstream/master, instead
of upstream/staging.
> [...]
> > @@ -540,7 +547,7 @@ static char * __init split_string(char *s)
> > return NULL;
> > }
> > -static void __init handle_file_info(CHAR16 *name,
> > +static void __init handle_file_info(const CHAR16 *name,
> > const struct file *file, const char *options)
> > {
> > if ( file == &cfg )
>
> Obviously I had to drop this hunk, which should now be folded
> into patch 2. (If no other need for a v9 arises, I'll try to
> remember to do so while committing that one.)
I've squashed them in my tree in case there is a v9. Hopefully
it doesn't come to that...
--
Trammell
Re: [PATCH v8 4/5] efi: Enable booting unified hypervisor/kernel/initrd images
On Friday, October 2, 2020 4:27 AM, Jan Beulich wrote: > On 30.09.2020 14:00, Trammell Hudson wrote: > > - /* Read and parse the config file. */ > > I'm sorry for noticing this only now, but I don't think this comment > should be moved. If no other need for a v9 arises, this can likely > be undone while committing. I'll relocate it. > > - if ( sect->Name[0] != '.' ) > > - return -1; > > I was about to say "'true' please", but you really mean 'false" > now. (Could perhaps again be fixed while committing.) oops oops. Yes, that is a mistake. Should be false; I'll fix it. > [...] > Just as a remark (and again spotting only now) this could be had > with one less comparison: > > if ( cw != c ) > return false; > if ( c == '\0' ) > return true; > > At which the need for cw also disappears. Sure. I'll fix that, too. Since there are a few patches to the patch, I'll send out a v9 so that we don't forget any of the ones that we wanted to remember to make. -- Trammell
[PATCH v9 1/4] efi/boot.c: add file.need_to_free
The config file, kernel, initrd, etc should only be freed if they
are allocated with the UEFI allocator. On x86 the ucode, and on
ARM the dtb, are also marked as need_to_free when allocated or
expanded.
This also fixes a memory leak in ARM fdt_increase_size() if there
is an error in building the new device tree.
Signed-off-by: Trammell Hudson
Reviewed-by: Jan Beulich
---
xen/arch/arm/efi/efi-boot.h | 11 +--
xen/arch/x86/efi/efi-boot.h | 2 +-
xen/common/efi/boot.c | 10 ++
3 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index 27dd0b1a94..c6200fda0e 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -314,7 +314,10 @@ static void __init *fdt_increase_size(struct file
*fdtfile, int add_size)
if ( fdt_size )
{
if ( fdt_open_into(dtbfile.ptr, new_fdt, pages * EFI_PAGE_SIZE) )
+{
+efi_bs->FreePages(fdt_addr, pages);
return NULL;
+}
}
else
{
@@ -326,7 +329,10 @@ static void __init *fdt_increase_size(struct file
*fdtfile, int add_size)
* system table that is passed in the FDT.
*/
if ( fdt_create_empty_tree(new_fdt, pages * EFI_PAGE_SIZE) )
+{
+efi_bs->FreePages(fdt_addr, pages);
return NULL;
+}
}
/*
@@ -335,12 +341,13 @@ static void __init *fdt_increase_size(struct file
*fdtfile, int add_size)
* code will free it. If the original FDT came from a configuration
* table, we don't own that memory and can't free it.
*/
-if ( dtbfile.size )
+if ( dtbfile.need_to_free )
efi_bs->FreePages(dtbfile.addr, PFN_UP(dtbfile.size));
/* Update 'file' info for new memory so we clean it up on error exits */
dtbfile.addr = fdt_addr;
dtbfile.size = pages * EFI_PAGE_SIZE;
+dtbfile.need_to_free = true;
return new_fdt;
}
@@ -546,7 +553,7 @@ static void __init efi_arch_cpu(void)
static void __init efi_arch_blexit(void)
{
-if ( dtbfile.addr && dtbfile.size )
+if ( dtbfile.need_to_free )
efi_bs->FreePages(dtbfile.addr, PFN_UP(dtbfile.size));
if ( memmap )
efi_bs->FreePool(memmap);
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index eef3f52789..1025000afd 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -689,7 +689,7 @@ static void __init efi_arch_cpu(void)
static void __init efi_arch_blexit(void)
{
-if ( ucode.addr )
+if ( ucode.need_to_free )
efi_bs->FreePages(ucode.addr, PFN_UP(ucode.size));
}
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 8123523194..9d6dc8ff4f 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;
char *str;
@@ -287,13 +288,13 @@ void __init noreturn blexit(const CHAR16 *str)
if ( !efi_bs )
efi_arch_halt();
-if ( cfg.addr )
+if ( cfg.need_to_free )
efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
-if ( kernel.addr )
+if ( kernel.need_to_free )
efi_bs->FreePages(kernel.addr, PFN_UP(kernel.size));
-if ( ramdisk.addr )
+if ( ramdisk.need_to_free )
efi_bs->FreePages(ramdisk.addr, PFN_UP(ramdisk.size));
-if ( xsm.addr )
+if ( xsm.need_to_free )
efi_bs->FreePages(xsm.addr, PFN_UP(xsm.size));
efi_arch_blexit();
@@ -588,6 +589,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle,
CHAR16 *name,
}
else
{
+file->need_to_free = true;
file->size = size;
if ( file != &cfg )
{
--
2.25.1
[PATCH v9 0/4] efi: Unified Xen hypervisor/kernel/initrd images
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 unified config is included .gitignore | 1 + docs/misc/efi.pandoc| 49 xen/arch/arm/efi/efi-boot.h | 36 ++--- xen/arch/x86/efi/Makefile | 2 +- xen/arch/x86/efi/efi-boot.h | 13 ++- xen/common/efi/boot.c | 140 - xen/common/efi/efi.h| 3 + xen/common/efi/pe.c | 152 8 files changed, 347 insertions(+), 49 deletions(-) create mode 100644 xen/common/efi/pe.c -- 2.25.1
[PATCH v9 3/4] efi: Enable booting unified hypervisor/kernel/initrd images
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. Note that the system will fall back to loading files from disk if the named sections do not exist. This allows distributions to continue with the status quo if they want a signed kernel + config, while still allowing a user provided initrd (which is how the shim protocol currently works as well). This patch also adds constness to the section parameter of efi_arch_cfg_file_early() and efi_arch_cfg_file_late(), changes pe_find_section() to use a const CHAR16 section name, and adds pe_name_compare() to match section names. Signed-off-by: Trammell Hudson Reviewed-by: Jan Beulich --- .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 | 62 +++ xen/common/efi/efi.h| 3 + xen/common/efi/pe.c | 152 8 files changed, 277 insertions(+), 28 deletions(-) create mode 100644 xen/common/efi/pe.c diff --git a/.gitignore b/.gitignore index 188495783e..f6865c9cd8 100644 --- a/.gitignore +++ b/.gitignore @@ -327,6 +327,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 0x82d04100. +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=0x82d04100 + --add-section .ucode=ucode.bin \ + --change-section-vma .ucode=0x82d04101 \ + --add-section .xsm=xsm.cfg \ + --change-section-vma .xsm=0x82d04108 \ + --add-section .kernel=vmlinux \ + --change-section-vma .kernel=0x82d04110 \ + --add-section .ramdisk=initrd.img \ + --change-section-vma .ramdisk=0x82d04200 \ + 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 +
[PATCH v9 2/4] efi/boot.c: add handle_file_info()
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
Acked-by: Jan Beulich
---
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 9d6dc8ff4f..bd629eb658 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -547,6 +547,22 @@ static char * __init split_string(char *s)
return NULL;
}
+static void __init handle_file_info(const CHAR16 *name,
+const struct file *file, const 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, const char *options)
{
@@ -591,16 +607,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle,
CHAR16 *name,
{
file->need_to_free = true;
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->str);
if ( !EFI_ERROR(ret) && file->size != size )
--
2.25.1
[PATCH v9 4/4] efi: Do not use command line if unified config is included
If a unified Xen image is used, then the bundled configuration,
Xen command line, dom0 kernel, and ramdisk are prefered over
any files listed in the config file or provided on the command line.
Unlike the shim based verification, the PE signature on a unified image
covers all of the Xen+config+kernel+initrd modules linked into the
unified image. This also ensures that, on properly configured UEFI Secure Boot
platforms, the entire runtime will be measured into the TPM for unsealing
secrets or remote attestation.
Signed-off-by: Trammell Hudson
---
xen/common/efi/boot.c | 43 ++-
1 file changed, 38 insertions(+), 5 deletions(-)
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index bacd551bb5..8c11475214 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -950,6 +950,35 @@ 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 __initdata 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", &global_guid,
+ NULL, &secboot_size, &secboot);
+if ( rc != EFI_SUCCESS )
+return false;
+
+rc = efi_rs->GetVariable(L"SetupMode", &global_guid,
+ NULL, &setupmode_size, &setupmode);
+if ( rc != EFI_SUCCESS )
+return false;
+
+if ( secboot > 1 || setupmode > 1 )
+blexit(L"Invalid SecureBoot/SetupMode variables");
+
+return secboot == 1 && setupmode == 0;
+}
+
static void __init efi_variables(void)
{
EFI_STATUS status;
@@ -1126,15 +1155,15 @@ 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;
union string section = { NULL }, name;
bool base_video = false;
const char *option_str;
-bool use_cfg_file;
+bool use_cfg_file, secure;
__set_bit(EFI_BOOT, &efi_flags);
__set_bit(EFI_LOADER, &efi_flags);
@@ -1153,8 +1182,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 ( use_cfg_file &&
+ !read_section(loaded_image, L"config", &cfg, NULL) )
{
UINTN offset = 0;
@@ -1212,6 +1243,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);
@@ -1232,7 +1265,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE
*SystemTable)
dir_handle = get_parent_handle(loaded_image, &file_name);
/* Read and parse the config file. */
-if ( read_section(loaded_image, L"config", &cfg, NULL) )
+if ( cfg.ptr )
PrintStr(L"Using builtin config file\r\n");
else if ( !cfg_file_name )
{
--
2.25.1
Re: [PATCH v9 0/4] efi: Unified Xen hypervisor/kernel/initrd images
Any further thoughts on this patch series? Three out of four of them have been reviewed or acked by at least one reviewer, with only the last one currently unreviewed. -- Trammell On Friday, October 2, 2020 1:18 PM, Trammell Hudson wrote: > 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 unified config is included > > .gitignore | 1 + > docs/misc/efi.pandoc | 49 > xen/arch/arm/efi/efi-boot.h | 36 ++--- > xen/arch/x86/efi/Makefile | 2 +- > xen/arch/x86/efi/efi-boot.h | 13 ++- > xen/common/efi/boot.c | 140 - > xen/common/efi/efi.h | 3 + > xen/common/efi/pe.c | 152 > 8 files changed, 347 insertions(+), 49 deletions(-) > create mode 100644 xen/common/efi/pe.c > > -- > > 2.25.1
Re: [xen-unstable-smoke test] 155612: regressions - FAIL
On Friday, October 9, 2020 10:27 PM, Andrew Cooper wrote: > [...] > Looks like arm64 is crashing fairly early on boot. > > This is probably caused by "efi: Enable booting unified > hypervisor/kernel/initrd images". Darn it. I'm working out how to build and boot qemu aarch64 so that I can figure out what is going on. Also, I'm not sure that it is possible to build a unified arm image right now; objcopy (and all of the obj* tools) say "File format not recognized" on the xen.efi file. The MZ file is not what they are expecting for ARM executables. -- Trammell
Re: [xen-unstable-smoke test] 155612: regressions - FAIL
On Saturday, October 10, 2020 1:42 PM, Trammell Hudson wrote: > On Friday, October 9, 2020 10:27 PM, Andrew Cooper andrew.coop...@citrix.com > wrote: > > [...] > > Looks like arm64 is crashing fairly early on boot. > > This is probably caused by "efi: Enable booting unified > > hypervisor/kernel/initrd images". > > Darn it. I'm working out how to build and boot qemu aarch64 so > that I can figure out what is going on. Unfortunately qemu 2.11.1(Debian 1:2.11+dfsg-1ubuntu7.32) doesn't replicate this crash using the command line from https://wiki.xenproject.org/wiki/Xen_ARM_with_Virtualization_Extensions/qemu-system-aarch64 qemu-system-aarch64 \ -cpu cortex-a57 \ -smp 4 -m 4096 \ -machine virt,gic_version=3 \ -machine virtualization=true \ -machine type=virt \ -display none \ -serial mon:stdio \ -bios /usr/share/qemu-efi-aarch64/QEMU_EFI.fd \ -drive if=none,file=xenial-server-cloudimg-arm64-uefi1.img,id=hd0 \ -device virtio-blk-device,drive=hd0 \ -boot order=d [...] Xen 4.15-unstable (c/s Fri Oct 2 12:30:34 2020 +0200 git:8a62dee9ce) EFI loader Using configuration file 'BOOTAA64.cfg' virt-gicv3.dtb: 0x00013bebe000-0x00013bece000 kernel: 0x00013aecd000-0x00013bd12200 initrd: 0x000138ced000-0x00013aecc3bb __ ___ __ __ _ \ \/ /___ _ __ | || | / | ___|_ _ _ __ ___| |_ __ _| |__ | | ___ \ // _ \ '_ \ | || |_ | |___ \ __| | | | '_ \/ __| __/ _` | '_ \| |/ _ \ / \ __/ | | | |__ _|| |___) |__| |_| | | | \__ \ || (_| | |_) | | __/ /_/\_\___|_| |_||_|(_)_|/\__,_|_| |_|___/\__\__,_|_.__/|_|\___| (XEN) Xen version 4.15-unstable (hudson@) (aarch64-linux-gnu-gcc (Ubuntu/Linaro 7.5.0-3ubuntu1~18.04) 7.5.0) debug=n Sat Oct 10 08:42:30 CEST 2020 (XEN) Latest ChangeSet: Fri Oct 2 12:30:34 2020 +0200 git:8a62dee9ce (XEN) Processor: 411fd070: "ARM Limited", variant: 0x1, part 0xd07, rev 0x0 [...] It makes it all the way into the Linux kernel and user space without crashing. Hopefully someone with better access to ARM hardware can help debug! -- Trammell
Unified Xen executable for UEFI Secure Boot support
I have preliminary patches to support bundling the Xen hypervisor, xen.cfg, the Linux kernel, initrd and XSM into a single "unified" EFI executable that can be signed by sbsigntool for verification by UEFI Secure Boot. It is inspired by systemd-boot's unified kernel technique and borrows the function to locate PE sections from systemd's LGPL'ed code. The configuration, kernel, etc are added after building using objcopy to add named sections for each input file. This allows an administrator to update the components independently without requiring rebuilding xen. During EFI boot, Xen looks at its own loaded image to locate the PE sections and, if secure boot is enabled, only allows use of the unified components. The resulting EFI executable can be invoked directly from the UEFI Boot Manager, removing the need to use a separate loader like grub. Unlike the shim based verification, the signature covers the entire Xen+config+kernel+initrd unified file. This also ensures that properly configured platforms will measure the entire runtime into the TPM for unsealing secrets or remote attestation. I've tested it on qemu OVMF with Secure Boot enabled, as well as on real Thinkpad hardware. The EFI console is very slow, although it works and is able to boot into dom0. The current patch set is here, and I'd appreciate suggestions on the technique or cleanup for submission: https://github.com/osresearch/xen/tree/secureboot -- Trammell
[RFC] efi/boot: Unified Xen executable for UEFI Secure Boot support
This preliminary patch adds support for bundling the Xen hypervisor, xen.cfg,
the Linux kernel, initrd and XSM into a single "unified" EFI executable that
can be signed by sbsigntool for verification by UEFI Secure Boot. It is
inspired by systemd-boot's unified kernel technique and borrows the function to
locate PE sections from systemd's LGPL'ed code.
The configuration, kernel, etc are added after building using objcopy to add
named sections for each input file. This allows an administrator to update the
components independently without requiring rebuilding xen. During EFI boot, Xen
looks at its own loaded image to locate the PE sections and, if secure boot is
enabled, only allows use of the unified components.
The resulting EFI executable can be invoked directly from the UEFI Boot
Manager, removing the need to use a separate loader like grub. Unlike the shim
based verification, the signature covers the entire Xen+config+kernel+initrd
unified file. This also ensures that properly configured platforms will measure
the entire runtime into the TPM for unsealing secrets or remote attestation.
It has been tested on qemu OVMF with Secure Boot enabled, as well as on real
Thinkpad hardware. The EFI console is very slow, although it works and is able
to boot into dom0.
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 5a520bf..b7b08b6 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;
@@ -330,13 +331,13 @@ static 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();
@@ -619,6 +620,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle,
CHAR16 *name,
what = what ?: L"Seek";
else
{
+file->need_to_free = true;
file->addr = min(1UL << (32 + PAGE_SHIFT),
HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START);
ret = efi_bs->AllocatePages(AllocateMaxAddress, EfiLoaderData,
@@ -665,6 +667,136 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle,
CHAR16 *name,
return true;
}
+
+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_I386 0x014c
+#define PE_HEADER_MACHINE_X64 0x8664
+#define PE_HEADER_MACHINE_ARM64 0xaa64
+
+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));
+
+static void * __init pe_find_section(const void * const image_base,
+const char * section_name, UINTN * size_out)
+{
+const CHAR8 * const base = image_base;
+const struct DosFileHeader * dos = (const void*) base;
+const struct PeHeader * pe;
+const UINTN name_len = strlen(section_name);
+UINTN offset;
+
+if ( base == NULL )
+return NULL;
+
+if ( memcmp(dos->Magic, "MZ", 2) != 0 )
+return NULL;
+
+pe = (const void *) &base[dos->ExeHeader];
+if ( memcmp(pe->Magic, "PE\0\0", 4) != 0 )
+return NULL;
+
+/* PE32+ Subsystem type */
+if (pe->FileHeader.Machine != PE_HEADER_MACHINE_X64
+&& pe->FileHeader.Machine != PE_HEADER_MACHINE_ARM64
+&& pe->FileHeader.Machi
EFI executable corruption when live patching is turned off
When building xen from head with almost any combination of options, the resulting xen.efi seems properly formed. When CONFIG_LIVEPATCH is turned off, however, the resulting xen.efi is corrupted in some way and binutils no longer wants to work with it: ~/build/xen-clean/xen$ git rev-parse HEAD 81fd0d3ca4b2cd309403c6e8da662c325dd35750 ~/build/xen-clean/xen$ diff .config.orig .config 71,72c71 < CONFIG_LIVEPATCH=y < CONFIG_FAST_SYMBOL_LOOKUP=y --- > # CONFIG_LIVEPATCH is not set 105a105 > # CONFIG_COVERAGE is not set ~/build/xen-clean/xen$ objcopy xen-orig.efi test.efi ~/build/xen-clean/xen$ objcopy xen.efi test.efi objcopy: test.efi: Data Directory size (1c) exceeds space left in section (18) objcopy: test.efi: error copying private BFD data: file in wrong format ~/build/xen-clean/xen$ objcopy --version | head -1 GNU objcopy (GNU Binutils for Ubuntu) 2.34 I spent most of today unsuccessfully trying to figure out what was different between the builds (on multiple build host OS with different binutils), so I'm hoping that perhaps someone else has seen this problem. -- Trammell
Re: [RFC] efi/boot: Unified Xen executable for UEFI Secure Boot support
On Thursday, August 6, 2020 9:57 AM, Jan Beulich wrote: > On 05.08.2020 19:20, Trammell Hudson wrote: > > This preliminary patch adds support for bundling the Xen hypervisor, > > xen.cfg, > > the Linux kernel, initrd and XSM into a single "unified" EFI executable that > > can be signed by sbsigntool for verification by UEFI Secure Boot. [...] > > Looks reasonable for a PoC, thanks, but needs cleaning up I think. Thanks for the comments. It is very much a WIP and hopefully we can clean it up for merging. > A couple of specific remarks / questions: > > [...] > > - /* PE32+ Subsystem type */ > > - if (pe->FileHeader.Machine != PE_HEADER_MACHINE_X64 > > - && pe->FileHeader.Machine != PE_HEADER_MACHINE_ARM64 > > - && pe->FileHeader.Machine != PE_HEADER_MACHINE_I386) > > - return NULL; > > I don't think i386 should be recognized here, and of the two other > ones only the one matching the current target architecture should > be. That's a good point. There isn't much point in proceeding if Xen can't boot the executable anyway... > > - if ( pe->FileHeader.NumberOfSections > 96 ) > > - return NULL; > > What's this 96 about? Not sure, to be honest. The PE parsing code is directly copied from systemd-boot (including the bit about ARM and i386): https://github.com/systemd/systemd/blob/07d5ed536ec0a76b08229c7a80b910cb9acaf6b1/src/boot/efi/pe.c#L83 > Overall I think it might help if this PE parsing code (if UEFI > doesn't offer a protocol to do it for us) was put into its own > source file. I tried to putting it into a separate file and ran into link issues, seems that it needs to be mentioned in both arch/x86/Makefile and arch/x86/pe/Makefile, so this was a "just make it work" for the PoC. Now that it is working, I'll go back to see if I can figure out the makefile magic. > I also wonder if it wouldn't better be optional > (i.e. depend on a Kconfig option). My preference would be to have it always on so that any Xen executable can be unified and signed by the end user, rather than requiring the user to do a separate build from source. For instance, the Qubes install DVD has a normal xen.efi, but I can generate my own signed version for my system by unifying it with the kernel and initrd. > > - if ( efi_rs->GetVariable(L"SecureBoot", (EFI_GUID *)&global_guid, NULL, > > &size, buf) != EFI_SUCCESS ) > > - return false; > > - return buf[0] != 0; > > I.e. "SecureBoot=N" still means "enabled"? Maybe? UEFI 2.8, section 3.3 "Global Variables" says for SecureBoot: "Whether the platform firmware is operating in Secure boot mode (1) or not (0). All other values are reserved. Should be treated as read-only." > [...] > "secure" depending merely on an env var, how is this logic compatible > with the shim model? You need to keep the other approach working. Oops. Yes, I broke the shim model when booting in non-unified way with SecureBoot enabled. > Also, considering kernel and initrd are embedded, is there really a > strict need for a config file? It would seem to me that you could > boot the system fine without. The config file is still necessary for Xen options (console, etc) as well as the kernel command line. > [...] > Once you know whether you're dealing with a "unified" image, you > shouldn't have a need to make logic dependent upon read_section() > finding a particular section: Either you find all of them (and > don't even try to interpret respective config file settings), or > you read everything from disk. Another option that might be better would be to have a "special" file name -- if the config file has a leading "." then read_file() could do the PE section search instead of going to the disk. That way the config file could have some things on disk, and some things unified. For instance, microcode doesn't (always) need to be signed, since it is already signed and encrypted by Intel. Many OEM's leave the ucode regions of flash unprotected by bootguard, which allows the end user to update the microcode without requiring an OEM signature. This does potentially leave open some rollback attacks, so I'm not 100% positive it is a good idea (although the firmware volume should still be measured into the TPM and things like SGX include the microcode version in the attestation). > > --- /dev/null > > +++ b/xen/scripts/unify-xen > > @@ -0,0 +1,68 @@ > > +#!/bin/bash > > +# Merge a Linux kernel, initrd and commandline into xen.efi to produce a > > single signed > > +# EFI executable. > > +# > > +# turn off "
Re: [RFC] efi/boot: Unified Xen executable for UEFI Secure Boot support
On Thursday, August 6, 2020 2:04 PM, Jan Beulich wrote:
> On 06.08.2020 13:44, Trammell Hudson wrote:
>
> > On Thursday, August 6, 2020 9:57 AM, Jan Beulich jbeul...@suse.com wrote:
> >
> > > Overall I think it might help if this PE parsing code (if UEFI
> > > doesn't offer a protocol to do it for us) was put into its own
> > > source file.
> >
> > I tried to putting it into a separate file and ran into link issues,
> > seems that it needs to be mentioned in both arch/x86/Makefile and
> > arch/x86/pe/Makefile, so this was a "just make it work" for the PoC.
> > Now that it is working, I'll go back to see if I can figure out the
> > makefile magic.
>
> I was rather thinking of e.g. xen/common/efi/pe.c.
PE parsing code is in now in common/efi/pe.c, with a symlink in arch/x86/efi/,
and I added an extern in common/efi/efi.h. The Makefiles in both arch/x86 and
arch/x86/efi required updates to link in the extra pe.init.o file.
I think this still requires some changes for ARM.
> [...]
> > > > - if ( efi_rs->GetVariable(L"SecureBoot", (EFI_GUID *)&global_guid,
> > > > NULL, &size, buf) != EFI_SUCCESS )
> > > > -return false;
> > > >
> > > >
> > > > - return buf[0] != 0;
> > >
> > > I.e. "SecureBoot=N" still means "enabled"?
> >
> > Maybe? UEFI 2.8, section 3.3 "Global Variables" says for SecureBoot:
> > "Whether the platform firmware is operating in Secure boot mode (1) or not
> > (0). All other values
> > are reserved. Should be treated as read-only."
>
> But in your expression that's then presumably '0', not 0?
The values are the bytes '\x00' or '\x01'. UEFI uses human readable strings,
except where it doesn't.
> > > Also, considering kernel and initrd are embedded, is there really a
> > > strict need for a config file? It would seem to me that you could
> > > boot the system fine without.
> >
> > The config file is still necessary for Xen options (console, etc) as
> > well as the kernel command line.
>
> But command line options are optional. Yes, you need a config file if
> you want to pass any options. But you may be able to get away without
> command line options, and hence without config file.
My concern is that if there is no config file embedded in the unified
image, then the logic for retrieving the untrustworthy file from disk
kicks in. However, it is not a change from the status-quo, so I've
reverted the behavior (as part of also fixing the shim logic).
I also added code to load the ucode section from the unified image
if it is present, which required touching the arm tree as well to add
an additional parameter to efi_arch_cfg_file_late(). It also
appears that in the event of the error path that the ucode will
never be freed. Probably not a big deal, unless you're launching
a failing Xen from the EFI shell over and over.
> > > > +objcopy \
> > > >
> > > > - --add-section .kernel="$KERNEL" \
> > > > - --add-section .ramdisk="$RAMDISK" \
> > > > - --add-section .config="$CONFIG" \
> > > > - --change-section-vma .config=0x82d04100 \
> > > > - --change-section-vma .kernel=0x82d04101 \
> > > > - --change-section-vma .ramdisk=0x82d04200 \
> > >
> > > Of course these hard coded numbers will be eliminated in the
> > > long run?
> >
> > Ideally. We could try to parse out the address based on the objdump output,
> > although oddly systemd-boot has hardcoded ones as well.
>
> Perhaps the Linux kernel (or whatever else they work on) doesn't
> ever change addresses. The addresses shown here have changed just
> recently (they moved down by 1Gb).
Since the unify script doesn't have access to the build tree, I added
some logic that tries to deduce the correct address range and adds the
unified bits a little ways above it.
Updated patch:
diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index 6527cb0..fb763ce 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -395,7 +395,7 @@ static void __init efi_arch_cfg_file_early(EFI_FILE_HANDLE
dir_handle, char *sec
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 void * image_base,
EFI_FILE_HANDLE dir_handle, char *section)
{
}
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index b388861..ebb2616 100644
--
Re: EFI executable corruption when live patching is turned off
On Thursday, August 6, 2020 6:40 PM, Jan Beulich wrote:
> On 05.08.2020 20:19, Trammell Hudson wrote:
> [...]
> > ~/build/xen-clean/xen$ objcopy xen.efi test.efi
> > objcopy: test.efi: Data Directory size (1c) exceeds space left in section
> > (18)
> > objcopy: test.efi: error copying private BFD data: file in wrong format
> > ~/build/xen-clean/xen$ objcopy --version | head -1
> > GNU objcopy (GNU Binutils for Ubuntu) 2.34
>
> I've tried to find a sensible way to fix this in objcopy, but could
> come up with only a somewhat hackish variant:
> https://sourceware.org/pipermail/binutils/2020-August/112746.html
> Let's see what the maintainers there think, or if they have better
> suggestions (or are willing to address this themselves). The issue
> is pretty certainly not tied to LIVEPATCH, but rather to how much
> padding space there is at the end of the .rodata section.
Thanks for tracking that down! I was also almost certain it was not a
livepatch issue, although that was the easiest minimal test case that I could
produce.
As a workaround for the Xen project, what do you think of forcing alignment on
.buildid so that the tool is happy:
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 111edb5..712ffc8 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -161,6 +161,7 @@ SECTIONS
__note_gnu_build_id_end = .;
} :note :text
#elif defined(BUILD_ID_EFI)
+ . = ALIGN(4096);
DECL_SECTION(.buildid) {
__note_gnu_build_id_start = .;
*(.buildid)
--
Trammell
Re: [RFC] efi/boot: Unified Xen executable for UEFI Secure Boot support
On Thursday, August 6, 2020 8:14 PM, Andrew Cooper wrote: > For SecureBoot, it is important that nothing which is signed can be > tricked into running unsigned code. > > That includes configuration such as xen.cfg or the command line. > Consuming these from unsigned sources is ok, so long as we can guarantee > that the parsing is robust (see boothole for how this goes wrong), and > the effects are controlled. In addition to the "unsafe_fsgsbase", the Linux command line is full of potential issues, from subtle ones like "lockdown=none" to more brute force things like "init=/bin/sh". safeboot uses the signed kernel command line to pass in the root hash of the dm-verity Merkle tree, which cryptographically protects the rest of the runtime, so it definitely needs to come from a trusted source. > [...] > In the absence of a full audit of all our command line arguments, and > extra vigilance reviewing code coming in, the safer alternative is to > prohibit use of the command line, and only accept it in its Kconfig > embedded form for now. Turning off command line or config parsing might be a step too far. Since the xen.cfg in the unified image is included in the signature, any options configured in it should be trustworthy. This makes it easier for distributions to have a Xen build with boot-time work arounds for different hardware or configurations. > [...] > I think it might be worth having a CONFIG_SECURE_BOOT, selectable > initially only under CONFIG_EXPERT, and use it to force off various > other aspects of functionality, along with a list of known issues which > can be chipped away at before it can be declared supported. That makes sense to me. Either doing it at compile time (by making CONFIG_LIVEPATCH and CONFIG_KEXEC and etc depend on !CONFIG_SECURE_BOOT), or having a global variable that turns off the code (similar to the Linux lockdown patches that are triggered if UEFI secure boot is enabled). > [...] > I think it is great that work is being started in this direction, but > there is a huge quantity of work to do before a downstream could > plausibly put together a Xen system which honours the intent of SecureBoot. I'm really worried that the current shim based approach is a false sense of security -- it provides trivial ways for attackers to bypass the SecureBoot guarantees, so closing some of those easy holes with the verified unified image is definitely an incremental improvement towards a more secure system. However, I also don't want the unified image patch to get bogged down while trying to pursue every UEFI SecureBoot(tm) related issue, so perhaps the patch series should be renamed to only focus on the unified build part, not the SecureBoot part. That way downstream distributions can use it to add the security features that they need (caveat lector), without necessarily depending on the strict UEFI compliance. -- Trammell
Re: [RFC] efi/boot: Unified Xen executable for UEFI Secure Boot support
On Friday, August 7, 2020 2:23 PM, Jan Beulich wrote: > On 06.08.2020 16:15, Trammell Hudson wrote: > > --- /dev/null > > +++ b/xen/arch/x86/efi/pe.c > > @@ -0,0 +1 @@ > > +../../../common/efi/pe.c > > \ No newline at end of file > > This isn't supposed to be part of the patch; the symlinks get > created in the course of building. Fixed -- had to add the pe.c to the xen/Makefile to create the symlink. > > - s2w(&name_string); > > - PrintStr(name_string.w); > > - 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_string.w, options); > > This is a copy of existing code - please make a helper function > instead of duplicating (preferably in a separate, prereq patch, but > I guess there's anyway the open question on whether this change > can/should be split into smaller pieces). Fixed -- split into separate display file info function. > > - static const EFI_GUID global_guid = EFI_GLOBAL_VARIABLE; > > Also __initconst please. Fixed. I'm a little surprised that constant strings don't also need some sort of annotation, but perhaps there is magic that I don't see. > > - if ( efi_rs->GetVariable(L"SecureBoot", (EFI_GUID *)&global_guid, NULL, > > &size, buf) != EFI_SUCCESS ) > > - return false; > > Judging from Linux code you also need to evaluate the SetupMode var. Fixed. Added a note to maintain sync with the Linux code (which has its own note about maintaining sync with other Linux code). > > - if ( loaded_image ) > > - image_base = loaded_image->ImageBase; > > There's no point in having the if() - efi_arch_load_addr_check() > has already de-referenced loaded_image. If HandleProtocol() fails > we won't make it here. Fixed. Also removed the variable to use EFI_LOADED_IMAGE struct instead. > [...] > As said before, I think we want an all-or-nothing approach. You > want to first establish whether the image is a unified one, and > if so not fall back to reading from disk. Otherwise the claim > of secure boot above is liable to be wrong. > > I'm also unconvinced of reporting the secure boot status only in > the case you're interested in. Andrew had similar comments on this flow; I'll respond to both in a separate reply. > > - const CHAR8 * const base = image_base; > > Why the extra variable? The more that ... Fixed (by passing the EFI_LOADED_IMAGE instead so that we can check sizes, etc). > > - if ( pe->FileHeader.NumberOfSections > 96 ) > > - return NULL; > > Besides there still needing to be an explanation for this apparently > arbitrary limit, I also find the amount of consistency checking > insufficient. At the very least I'd like to see you check the COFF > magic value (0x020b). I've removed the 96 limit, which came from the original systemd-boot. And I also added more extensive bounds checking on the PE structures to ensure that they all are within the loaded image region. An attacker probably couldn't exploit it, since the signature is already checked, but better than weird memory errors. > > - if ( size_out ) > > - *size_out = sect->VirtualSize; > > Is this correct in all cases? Iirc zero tail padding can be > expressed by SizeOfRawData < VirtualSize, in which case there > won't be as many bytes to copy / use as you tell your caller. I don't know, perhaps objcopy doesn't do that? This logic is also copied directly from the systemd-boot PE parser: https://github.com/systemd/systemd/blob/master/src/boot/efi/pe.c#L101 > > - return (void*)(sect->VirtualAddress + (uintptr_t) > > image_base); > > Again no need for the cast; the function should return > const void * anyway, as the caller isn't supposed to alter > section contents (I hope). Fixed and the signature changed to return const void *. This does require a dangerous const-discarding cast since struct file has a void *. In general the edk2-derived code base is really bad about const-correctness, which might be worth a separate cleanup pass. > > --- /dev/null > > +++ b/xen/scripts/unify-xen > > @@ -0,0 +1,89 @@ > > +#!/bin/bash > > +# Build a "unified Xen" image. > > +# Usage > > +# unify xen.efi xen.cfg bzimage initrd [xsm [ucode]] > > [...] > > With all these hard coded size restrictions I take it this still is > just an example, not something that
Re: [RFC] efi/boot: Unified Xen executable for UEFI Secure Boot support
[ Responding to both Jan and Andrew's comments about config parsing and file sources when secure boot is enabled ] On Friday, August 7, 2020 2:23 PM, Jan Beulich wrote: > [...] > As said before, I think we want an all-or-nothing approach. You > want to first establish whether the image is a unified one, and > if so not fall back to reading from disk. Otherwise the claim > of secure boot above is liable to be wrong. It seems that the system owner who signs the unified Xen image can choose to use a config, kernel, initrd, microcode, or xsm from the disk if they a) reference it in the config file and b) do not embed a named section in the unified image, in which case the code will fall back to the read_file() function. This is essentially the status-quo today, including the shim verification of the kernel, in that all of the other values are essentially untrusted. However, as Andrew points out: On Monday, August 10, 2020 3:31 PM, Andrew Cooper wrote: > > On Thursday, August 6, 2020 8:14 PM, Andrew Cooper > > andrew.coop...@citrix.com wrote: > > > [...] > > > In the absence of a full audit of all our command line arguments, and > > > extra vigilance reviewing code coming in, the safer alternative is to > > > prohibit use of the command line, and only accept it in its Kconfig > > > embedded form for now. > [...] > With the proposal here, there are two signed sources; one in Kconfig, > and one in the embedded xen.cfg file. I added code that turns off argc/argv parsing if UEFI Secure Boot is enabled, although it doesn't enforce a config file. The system owner could sign a unified image without a config file embedded, in which case the x86 code path will do the read_file() approach for it and load an attacker controlled config. Much like the kexec and live patching options, it is a very caveat lector sort of thing. The owner of the machine can build and sign an insecure hypervisor, kernel, etc configuration, if they want to, although it would be nice to have some defaults to aim the footguns away from their shoes. Adding runtime checks out of the early EFI boot path for secure boot status and turning off some of the obviously risky pieces would be a good next step. > [...] > My main concern is simply to avoid giving any kind of impression that > UEFI SecureBoot is generally usable at the moment. "Generally usable with Microsoft's signing key and UEFI ecocsystem", yeah, we're not really there yet. There are still open issues in the wider Linux distributions as well about how to handle things like kernel command lines and initrd validation, so it's not just Xen. "Generally usable for users enrolling their own platform key and reviewing the system configuration details", however, I think we're fairly close to having the building blocks to put together slightly more secure systems. Thanks for all of you're detailed comments and thoughts on this patch discussion! Hopefully we can converge on something soon. -- Trammell
[PATCH] arch/x86/setup.c: Ignore early boot parameters like no-real-mode
There are parameters in xen/arch/x86/boot/cmdline.c that
are only used early in the boot process, so handlers are
necessary to avoid an "Unknown command line option" in
dmesg.
This also updates ignore_param() to generate a temporary
variable name so that the macro can be used more than once
per file.
Signed-off-by: Trammell hudson
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index c9b6af8..4b15e06 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -679,6 +679,15 @@ static void __init noreturn reinit_bsp_stack(void)
reset_stack_and_jump_nolp(init_done);
}
+/*
+ * x86 early command line parsing in xen/arch/x86/boot/cmdline.c
+ * has options that are only used during the very initial boot process,
+ * so they can be ignored now.
+ */
+ignore_param("no-real-mode");
+ignore_param("edd");
+ignore_param("edid");
+
/*
* Some scripts add "placeholder" to work around a grub error where it ate the
* first parameter.
diff --git a/xen/include/xen/param.h b/xen/include/xen/param.h
index c2fd075..b77f7f2 100644
--- a/xen/include/xen/param.h
+++ b/xen/include/xen/param.h
@@ -35,6 +35,10 @@ extern const struct kernel_param __setup_start[],
__setup_end[];
__attribute__((__aligned__(1))) char
#define __kparam __param(__initsetup)
+#define __TEMP_NAME(base,line) base##_##line
+#define _TEMP_NAME(base,line) __TEMP_NAME(base,line)
+#define TEMP_NAME(base) _TEMP_NAME(base,__LINE__)
+
#define custom_param(_name, _var) \
__setup_str __setup_str_##_var[] = _name; \
__kparam __setup_##_var = \
@@ -71,9 +75,9 @@ extern const struct kernel_param __setup_start[],
__setup_end[];
.len = sizeof(_var), \
.par.var = &_var }
#define ignore_param(_name) \
-__setup_str setup_str_ign[] = _name;\
-__kparam setup_ign =\
-{ .name = setup_str_ign,\
+__setup_str TEMP_NAME(__setup_str_ign)[] = _name;\
+__kparam TEMP_NAME(__setup_ign) =\
+{ .name = TEMP_NAME(__setup_str_ign),\
.type = OPT_IGNORE }
#ifdef CONFIG_HYPFS
Re: [PATCH] arch/x86/setup.c: Ignore early boot parameters like no-real-mode
On Wednesday, August 12, 2020 8:16 PM, Andrew Cooper wrote: > However, the use of LINE creates problems for livepatch builds, as > it causes the binary diffing tools to believe these changed, based on a > change earlier in the file. Ah, I hadn't considered that. Makes sense that the deterministic __COUNTER__ would be better for many uses. However... One concern is that the __COUNTER__ is per compilation unit, which I think would mean that every file would conflict by creating __setup_str_ign_0 for the first one, __setup_str_ign_1 for the next, etc. Unless they are static scoped or have a variable-name-friendly unique prefix, they aren't suitable for globals that share a namespace. Another is that each evaluation increments it, so the macro would need some tricks to avoid double-evaluation since it both defines __setup_str_ign_XYZ and references it in the __kparam structure. This is in contrast to __LINE__, which is constant in the macro and based on the line where it was invoked so the double evaluation is not a problem. > Instead of opencoding TEMP_NAME(), can we borrow Linux's __UNIQUE_ID() > infrastructure? COUNTER appears to have existed for ages, and > exists in all of our supported compilers. I'm definitely in favor of borrowing it from Linux, although subject to those two caveats. > If you want, I can sort that out as a prereq patch, and rebase this one > on top? That sounds fine to me. They really are two separate patches. -- Trammell
[PATCH] EFI: Enable booting unified hypervisor/kernel/initrd images
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
CPU microcode into a single "unified" EFI executable. 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.
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 configuration, kernel, etc, which are included after building xen.efi
using objcopy to add named sections for each input file. This allows an
administrator to update the components independently without requiring
rebuilding xen.
The unified image can also be signed by sbsigntool for verification
by UEFI Secure Boot. If secure boot is enabled, the Xen command line
arguments are ignored. Unlike the shim based verification, the signature
covers the entire Xen+config+kernel+initrd unified file. 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
diff --git a/xen/Makefile b/xen/Makefile
index a87bb225dc..e4e4c6d5c1 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -355,7 +355,7 @@ $(TARGET): delete-unfresh-files
$(MAKE) -C tools
$(MAKE) -f $(BASEDIR)/Rules.mk include/xen/compile.h
[ -e include/asm ] || ln -sf asm-$(TARGET_ARCH) include/asm
- [ -e arch/$(TARGET_ARCH)/efi ] && for f in boot.c runtime.c compat.c
efi.h;\
+ [ -e arch/$(TARGET_ARCH)/efi ] && for f in boot.c pe.c runtime.c
compat.c efi.h;\
do test -r arch/$(TARGET_ARCH)/efi/$$f || \
ln -nsf ../../../common/efi/$$f arch/$(TARGET_ARCH)/efi/; \
done; \
diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index 6527cb0bdf..483dec465d 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -395,7 +395,7 @@ static void __init efi_arch_cfg_file_early(EFI_FILE_HANDLE
dir_handle, char *sec
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(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 4b2b010a80..ae666aa14c 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 compat.o runtime.o
+EFIOBJ := boot.init.o pe.init.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..e2650c0440 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -276,9 +276,11 @@ static void __init efi_arch_cfg_file_early(EFI_FILE_HANDLE
dir_handle, char *sec
{
}
-static void __init efi_arch_cfg_file_late(EFI_FILE_HANDLE dir_handle, char
*section)
+static void __init efi_arch_cfg_file_late(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 )
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 0273f79152..ba691b1890 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -156,6 +156,7 @@ SECTIONS
__note_gnu_build_id_end = .;
} :note :text
#elif defined(BUILD_ID_EFI)
+ . = ALIGN(32); /* workaround binutils section overlap bug */
DECL_SECTION(.buildid) {
__note_gnu_build_id_start = .;
*(.buildid)
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 5a520bf21d..25841f 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;
@@ -121,6 +122,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(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);
@@ -330,13 +333,13 @@ static void __
Re: Working Group for Secure Boot
On Fri, Mar 12, 2021 at 04:24:53PM +0100, Marek Marczykowski-G??recki wrote: > On Thu, Mar 11, 2021 at 10:34:02AM -0800, Bob Eshleman wrote: > > We would like to start a working group for secure boot support in Xen > > to coordinate the various interested parties and set out a plan for > > the feature and its implications for the whole Xen system. > [...] > > We'd love to hear from anyone interested in such a group and how the > > community as a whole feels about such an effort. > > Count me in too. > > Also, I'm cc-ing Trammell, who might be interested too. Thanks for the invite, Marek. I'm also interested in discussing how to lockdown a running Xen system. Now that the unified EFI image patches have been merged, we can boot with a little more integrity and hopefully transfer the chain of trust to a trustworthy system. -- Trammell
