[PATCH v3 0/4] efi: Unified Xen hypervisor/kernel/initrd images

[PATCH v3 0/4] efi: Unified Xen hypervisor/kernel/initrd images

[Trammell Hudson]

From: Trammell hudson <hudson@trmm.net>

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

[Trammell Hudson]

From: Trammell hudson <hudson@trmm.net>

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 <hudson@trmm.net>
---
 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()

[Trammell Hudson]

From: Trammell hudson <hudson@trmm.net>

Signed-off-by: Trammell hudson <hudson@trmm.net>
---
 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

[Trammell Hudson]

From: Trammell hudson <hudson@trmm.net>

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 <hudson@trmm.net>
---
 .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=0xffff82d041000000
+	--add-section .ucode=ucode.bin \
+	--change-section-vma .ucode=0xffff82d041010000 \
+	--add-section .xsm=xsm.cfg \
+	--change-section-vma .xsm=0xffff82d041080000 \
+	--add-section .kernel=vmlinux \
+	--change-section-vma .kernel=0xffff82d041100000 \
+	--add-section .ramdisk=initrd.img \
+	--change-section-vma .initrd=0xffff82d042000000 \
+	xen.efi \
+	xen.unified.efi
+```
+
+The unified executable can be signed with sbsigntool to make
+it usable with UEFI secure boot:
+
+```
+sbsign \
+	--key signing.key \
+	--cert cert.pem \
+	--output xen.signed.efi \
+	xen.unified.efi
+```
diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index 6527cb0bdf..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 updated as modules are loaded.  Prefer the one
+     * stored as a PE section in a unified image, and fall back to a file
+     * on disk if the section is not present.
      */
-    name.s = get_value(&cfg, section, "dtb");
-    if ( name.s )
+    if ( !read_section(image, ".dtb", &dtbfile, NULL) )
     {
-        split_string(name.s);
-        read_file(dir_handle, s2w(&name), &dtbfile, NULL);
-        efi_bs->FreePool(name.w);
+	    name.s = get_value(&cfg, section, "dtb");
+	    if ( name.s )
+	    {
+		split_string(name.s);
+		read_file(dir_handle, s2w(&name), &dtbfile, NULL);
+		efi_bs->FreePool(name.w);
+	    }
     }
+
     fdt = fdt_increase_size(&dtbfile, cfg.size + EFI_PAGE_SIZE);
     if ( !fdt )
         blexit(L"Unable to create new FDT");
 }
 
-static void __init efi_arch_cfg_file_late(EFI_FILE_HANDLE dir_handle, char *section)
+static void __init efi_arch_cfg_file_late(EFI_LOADED_IMAGE *image, EFI_FILE_HANDLE dir_handle, char *section)
 {
 }
 
diff --git a/xen/arch/x86/efi/Makefile b/xen/arch/x86/efi/Makefile
index 770438a029..e857c0f2cc 100644
--- a/xen/arch/x86/efi/Makefile
+++ b/xen/arch/x86/efi/Makefile
@@ -8,7 +8,7 @@ cmd_objcopy_o_ihex = $(OBJCOPY) -I ihex -O binary $< $@
 
 boot.init.o: buildid.o
 
-EFIOBJ := boot.init.o ebmalloc.o compat.o runtime.o
+EFIOBJ := boot.init.o pe.init.o ebmalloc.o compat.o runtime.o
 
 $(call cc-option-add,cflags-stack-boundary,CC,-mpreferred-stack-boundary=4)
 $(EFIOBJ): CFLAGS-stack-boundary := $(cflags-stack-boundary)
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 7188c9a551..ecc1bbd17d 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -272,14 +272,17 @@ static void __init noreturn efi_arch_post_exit_boot(void)
     unreachable();
 }
 
-static void __init efi_arch_cfg_file_early(EFI_FILE_HANDLE dir_handle, char *section)
+static void __init efi_arch_cfg_file_early(EFI_LOADED_IMAGE *image, EFI_FILE_HANDLE dir_handle, char *section)
 {
 }
 
-static void __init efi_arch_cfg_file_late(EFI_FILE_HANDLE dir_handle, char *section)
+static void __init efi_arch_cfg_file_late(EFI_LOADED_IMAGE *image, EFI_FILE_HANDLE dir_handle, char *section)
 {
     union string name;
 
+    if ( read_section(image, ".ucode", &ucode, NULL) )
+        return;
+
     name.s = get_value(&cfg, section, "ucode");
     if ( !name.s )
         name.s = get_value(&cfg, "global", "ucode");
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index f5bdc4b1df..452b5f4362 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -121,6 +121,8 @@ static CHAR16 *s2w(union string *str);
 static char *w2s(const union string *str);
 static bool read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
                       struct file *file, char *options);
+static bool read_section(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);
@@ -622,6 +624,26 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
     return true;
 }
 
+static bool __init read_section(EFI_LOADED_IMAGE *image,
+                                char *const name, struct file *file, char *options)
+{
+    /* skip the leading "." in the section name */
+    union string name_string = { .s = name + 1 };
+
+    file->ptr = (void *)pe_find_section(image->ImageBase, image->ImageSize,
+        name, &file->size);
+    if ( !file->ptr )
+        return false;
+
+    file->need_to_free = false;
+
+    s2w(&name_string);
+    display_file_info(name_string.w, file, options);
+    efi_bs->FreePool(name_string.w);
+
+    return true;
+}
+
 static void __init pre_parse(const struct file *cfg)
 {
     char *ptr = cfg->ptr, *end = ptr + cfg->size;
@@ -1206,9 +1228,13 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
         /* Get the file system interface. */
         dir_handle = get_parent_handle(loaded_image, &file_name);
 
-        /* Read and parse the config file. */
-        if ( !cfg_file_name )
+        if ( read_section(loaded_image, ".config", &cfg, NULL) )
+        {
+            PrintStr(L"Using unified config file\r\n");
+        }
+        else if ( !cfg_file_name )
         {
+            /* Read and parse the config file. */
             CHAR16 *tail;
 
             while ( (tail = point_tail(file_name)) != NULL )
@@ -1257,29 +1283,39 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
         if ( !name.s )
             blexit(L"No Dom0 kernel image specified.");
 
-        efi_arch_cfg_file_early(dir_handle, section.s);
+        efi_arch_cfg_file_early(loaded_image, dir_handle, section.s);
 
         option_str = split_string(name.s);
-        read_file(dir_handle, s2w(&name), &kernel, option_str);
-        efi_bs->FreePool(name.w);
-
-        if ( !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL,
-                        (void **)&shim_lock)) &&
-             (status = shim_lock->Verify(kernel.ptr, kernel.size)) != EFI_SUCCESS )
-            PrintErrMesg(L"Dom0 kernel image could not be verified", status);
 
-        name.s = get_value(&cfg, section.s, "ramdisk");
-        if ( name.s )
+        if ( !read_section(loaded_image, ".kernel", &kernel, option_str) )
         {
-            read_file(dir_handle, s2w(&name), &ramdisk, NULL);
+            read_file(dir_handle, s2w(&name), &kernel, option_str);
             efi_bs->FreePool(name.w);
+
+            if ( !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL,
+                            (void **)&shim_lock)) &&
+                 (status = shim_lock->Verify(kernel.ptr, kernel.size)) != EFI_SUCCESS )
+                PrintErrMesg(L"Dom0 kernel image could not be verified", status);
         }
 
-        name.s = get_value(&cfg, section.s, "xsm");
-        if ( name.s )
+        if ( !read_section(loaded_image, ".ramdisk", &ramdisk, NULL) )
         {
-            read_file(dir_handle, s2w(&name), &xsm, NULL);
-            efi_bs->FreePool(name.w);
+            name.s = get_value(&cfg, section.s, "ramdisk");
+            if ( name.s )
+            {
+                read_file(dir_handle, s2w(&name), &ramdisk, NULL);
+                efi_bs->FreePool(name.w);
+            }
+        }
+
+        if ( !read_section(loaded_image, ".xsm", &xsm, NULL) )
+        {
+            name.s = get_value(&cfg, section.s, "xsm");
+            if ( name.s )
+            {
+                read_file(dir_handle, s2w(&name), &xsm, NULL);
+                efi_bs->FreePool(name.w);
+            }
         }
 
         /*
@@ -1315,7 +1351,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
             }
         }
 
-        efi_arch_cfg_file_late(dir_handle, section.s);
+        efi_arch_cfg_file_late(loaded_image, dir_handle, section.s);
 
         efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
         cfg.addr = 0;
diff --git a/xen/common/efi/efi.h b/xen/common/efi/efi.h
index 4845d84913..c9b741bf27 100644
--- a/xen/common/efi/efi.h
+++ b/xen/common/efi/efi.h
@@ -47,3 +47,6 @@ const CHAR16 *wmemchr(const CHAR16 *s, CHAR16 c, UINTN n);
 /* EFI boot allocator. */
 void *ebmalloc(size_t size);
 void free_ebmalloc_unused_mem(void);
+
+const void * pe_find_section(const UINT8 *image_base, const size_t image_size,
+        const char *section_name, UINTN *size_out);
diff --git a/xen/common/efi/pe.c b/xen/common/efi/pe.c
new file mode 100644
index 0000000000..097c6848a8
--- /dev/null
+++ b/xen/common/efi/pe.c
@@ -0,0 +1,141 @@
+/*
+ * xen/common/efi/pe.c
+ *
+ * PE executable header parser.
+ *
+ * Derived from https://github.com/systemd/systemd/blob/master/src/boot/efi/pe.c
+ * commit 07d5ed536ec0a76b08229c7a80b910cb9acaf6b1
+ *
+ * Copyright (C) 2015 Kay Sievers <kay@vrfy.org>
+ * Copyright (C) 2020 Trammell Hudson <hudson@trmm.net>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU Lesser General Public License as published by
+ * the Free Software Foundation; either version 2.1 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ */
+
+
+#include "efi.h"
+
+struct DosFileHeader {
+    UINT8   Magic[2];
+    UINT16  LastSize;
+    UINT16  nBlocks;
+    UINT16  nReloc;
+    UINT16  HdrSize;
+    UINT16  MinAlloc;
+    UINT16  MaxAlloc;
+    UINT16  ss;
+    UINT16  sp;
+    UINT16  Checksum;
+    UINT16  ip;
+    UINT16  cs;
+    UINT16  RelocPos;
+    UINT16  nOverlay;
+    UINT16  reserved[4];
+    UINT16  OEMId;
+    UINT16  OEMInfo;
+    UINT16  reserved2[10];
+    UINT32  ExeHeader;
+} __attribute__((packed));
+
+#define PE_HEADER_MACHINE_ARM64         0xaa64
+#define PE_HEADER_MACHINE_X64           0x8664
+#define PE_HEADER_MACHINE_I386          0x014c
+
+struct PeFileHeader {
+    UINT16  Machine;
+    UINT16  NumberOfSections;
+    UINT32  TimeDateStamp;
+    UINT32  PointerToSymbolTable;
+    UINT32  NumberOfSymbols;
+    UINT16  SizeOfOptionalHeader;
+    UINT16  Characteristics;
+} __attribute__((packed));
+
+struct PeHeader {
+    UINT8   Magic[4];
+    struct PeFileHeader FileHeader;
+} __attribute__((packed));
+
+struct PeSectionHeader {
+    UINT8   Name[8];
+    UINT32  VirtualSize;
+    UINT32  VirtualAddress;
+    UINT32  SizeOfRawData;
+    UINT32  PointerToRawData;
+    UINT32  PointerToRelocations;
+    UINT32  PointerToLinenumbers;
+    UINT16  NumberOfRelocations;
+    UINT16  NumberOfLinenumbers;
+    UINT32  Characteristics;
+} __attribute__((packed));
+
+const void * __init pe_find_section(const CHAR8 * image, const UINTN image_size,
+                              const char * section_name, UINTN * size_out)
+{
+    const struct DosFileHeader * dos = (const void*) image;
+    const struct PeHeader * pe;
+    const struct PeSectionHeader * sect;
+    const UINTN name_len = strlen(section_name);
+    UINTN offset = 0;
+
+    if ( name_len > sizeof(sect->Name) )
+        return NULL;
+
+    if ( image_size < sizeof(*dos) )
+        return NULL;
+    if ( memcmp(dos->Magic, "MZ", 2) != 0 )
+        return NULL;
+
+    offset = dos->ExeHeader;
+    pe = (const void *) &image[offset];
+
+    offset += sizeof(*pe);
+    if ( image_size < offset )
+        return NULL;
+
+    if ( memcmp(pe->Magic, "PE\0\0", 4) != 0 )
+        return NULL;
+
+    /* PE32+ Subsystem type */
+#if defined(__arm__) || defined (__aarch64__)
+    if ( pe->FileHeader.Machine != PE_HEADER_MACHINE_ARM64 )
+        return NULL;
+#elif defined(__x86_64__)
+    if ( pe->FileHeader.Machine != PE_HEADER_MACHINE_X64 )
+        return NULL;
+#else
+    /* unknown architecture */
+    return NULL;
+#endif
+
+    offset += pe->FileHeader.SizeOfOptionalHeader;
+
+    for ( UINTN i = 0 ; i < pe->FileHeader.NumberOfSections ; i++ )
+    {
+        sect = (const void *) &image[offset];
+        if ( image_size < offset + sizeof(*sect) )
+            return NULL;
+
+        if ( memcmp(sect->Name, section_name, name_len) != 0 ||
+             image_size < sect->VirtualSize + sect->VirtualAddress )
+        {
+            offset += sizeof(*sect);
+            continue;
+        }
+
+        if ( size_out )
+            *size_out = sect->VirtualSize;
+
+        return &image[sect->VirtualAddress];
+    }
+
+    return NULL;
+}
-- 
2.25.1

[PATCH v3 4/4] efi: Do not use command line if secure boot is enabled.

[Trammell Hudson]

From: Trammell hudson <hudson@trmm.net>

If secure boot is enabled, the Xen command line arguments are ignored.
If a unified Xen image is used, then the bundled configuration, dom0
kernel, and initrd are prefered over the ones listed in the config file.

Unlike the shim based verification, the PE signature on a unified image
covers the all of the Xen+config+kernel+initrd modules linked into the
unified image. This also ensures that properly configured platforms
will measure the entire runtime into the TPM for unsealing secrets or
remote attestation.

Signed-off-by: Trammell Hudson <hudson@trmm.net>
---
 xen/common/efi/boot.c | 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

[Jan Beulich]

On 07.09.2020 21:00, Trammell Hudson wrote:
> --- 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)

It being "just" 32 bytes may make it look as if we could take this
without much thinking, but I'm then struggling where we would draw
the boundary. The binutils bug having got fixed (or at least worked
around), I don't really like this getting applied uniformly, the
more that nothing would normally have the requirement you have (to
be able to objcopy the whole thing).

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.

I also don't see any mention anywhere of why it's 32 bytes, and not
16 or 64 or yet something else.

Finally, please Cc maintainers on patch submissions.

Jan

Re: [PATCH v3 1/4] x86/xen.lds.S: Work around binutils build id alignment bug

[Trammell Hudson]

On Tuesday, September 8, 2020 11:04 AM, Jan Beulich <jbeulich@suse.com> 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

[Jan Beulich]

On 08.09.2020 11:30, Trammell Hudson wrote:
> On Tuesday, September 8, 2020 11:04 AM, Jan Beulich <jbeulich@suse.com> 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?

Building Xen has to work on the tool chain versions we document it
works on (and we're in the process of discussing to raise the
base line). Playing with the output of our build system is an
entirely different thing. 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.

>> 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.

Well, that's then setting us up for running into the same issue
again in case this "probably" turns out wrong. Referring to the
size of the structure created by binutils to insert the build ID,
otoh, could maybe give a proper reason. However, there's also the
question whether this (not) functioning also depends on the
particular size and/or alignment of the preceding section. Iirc
this was the reason why you had thought there was a connection to
live patching enabled / disabled in the build.

Jan

Re: [PATCH v3 2/4] efi/boot.c: add file.need_to_free and split display_file_info()

[Roger Pau Monné]

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.

On Mon, Sep 07, 2020 at 03:00:25PM -0400, Trammell Hudson wrote:
> From: Trammell hudson <hudson@trmm.net>
> 
> Signed-off-by: Trammell hudson <hudson@trmm.net>
> ---
>  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)

I think name at least could be constified?

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.

Roger.

Re: [PATCH v3 1/4] x86/xen.lds.S: Work around binutils build id alignment bug

[Trammell Hudson]

On Tuesday, September 8, 2020 8:29 AM, Jan Beulich <jbeulich@suse.com> 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 1/4] x86/xen.lds.S: Work around binutils build id alignment bug

[Jan Beulich]

On 14.09.2020 11:14, Trammell Hudson wrote:
> On Tuesday, September 8, 2020 8:29 AM, Jan Beulich <jbeulich@suse.com> 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?

It's on my list of things to look at, yes. I'm sorry it's taking
some time to get there.

Jan

Re: [PATCH v3 3/4] efi: Enable booting unified hypervisor/kernel/initrd images

[Roger Pau Monné]

On Mon, Sep 07, 2020 at 03:00:26PM -0400, Trammell Hudson wrote:
> From: Trammell hudson <hudson@trmm.net>
> 
> 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

Could we name this kernel_payload or maybe just payload instead of
initrd?

That's a Linux specific concept.

> xen.efi using objcopy to add named sections for each input file.
> 
> For x86, the CPU ucode can be included in a section named `.ucode`,
> which is loaded in the efi_arch_cfg_file_late() stage of the boot process.
> 
> On ARM systems the Device Tree can be included in a section named
> `.dtb`, which is loaded during the efi_arch_cfg_file_early() stage of
> the boot process.
> 
> Signed-off-by: Trammell hudson <hudson@trmm.net>
> ---
>  .gitignore                  |   1 +
>  docs/misc/efi.pandoc        |  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

Please sort alphabetically like to rest of the entries in efi/.

>  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=0xffff82d041000000
> +	--add-section .ucode=ucode.bin \
> +	--change-section-vma .ucode=0xffff82d041010000 \
> +	--add-section .xsm=xsm.cfg \
> +	--change-section-vma .xsm=0xffff82d041080000 \
> +	--add-section .kernel=vmlinux \
> +	--change-section-vma .kernel=0xffff82d041100000 \
> +	--add-section .ramdisk=initrd.img \

This seems like a typo, you should use .initrd instead?

> +	--change-section-vma .initrd=0xffff82d042000000 \

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?

> +	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)

image should be const.

>  {
>      union string name;
>  
>      /*
>       * The DTB must be processed before any other entries in the configuration
> -     * file, as the DTB is updated as modules are loaded.
> +     * file, as the DTB is updated as modules are loaded.  Prefer the one
> +     * stored as a PE section in a unified image, and fall back to a file
> +     * on disk if the section is not present.
>       */
> -    name.s = get_value(&cfg, section, "dtb");
> -    if ( name.s )
> +    if ( !read_section(image, ".dtb", &dtbfile, NULL) )
>      {
> -        split_string(name.s);
> -        read_file(dir_handle, s2w(&name), &dtbfile, NULL);
> -        efi_bs->FreePool(name.w);
> +	    name.s = get_value(&cfg, section, "dtb");
> +	    if ( name.s )
> +	    {
> +		split_string(name.s);
> +		read_file(dir_handle, s2w(&name), &dtbfile, NULL);
> +		efi_bs->FreePool(name.w);
> +	    }

You seem to have used hard tabs instead of spaces.

>      }
> +
>      fdt = fdt_increase_size(&dtbfile, cfg.size + EFI_PAGE_SIZE);
>      if ( !fdt )
>          blexit(L"Unable to create new FDT");
>  }
>  
> -static void __init efi_arch_cfg_file_late(EFI_FILE_HANDLE dir_handle, char *section)
> +static void __init efi_arch_cfg_file_late(EFI_LOADED_IMAGE *image, EFI_FILE_HANDLE dir_handle, char *section)
>  {
>  }
>  
> diff --git a/xen/arch/x86/efi/Makefile b/xen/arch/x86/efi/Makefile
> index 770438a029..e857c0f2cc 100644
> --- a/xen/arch/x86/efi/Makefile
> +++ b/xen/arch/x86/efi/Makefile
> @@ -8,7 +8,7 @@ cmd_objcopy_o_ihex = $(OBJCOPY) -I ihex -O binary $< $@
>  
>  boot.init.o: buildid.o
>  
> -EFIOBJ := boot.init.o ebmalloc.o compat.o runtime.o
> +EFIOBJ := boot.init.o pe.init.o ebmalloc.o compat.o runtime.o
>  
>  $(call cc-option-add,cflags-stack-boundary,CC,-mpreferred-stack-boundary=4)
>  $(EFIOBJ): CFLAGS-stack-boundary := $(cflags-stack-boundary)
> diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
> index 7188c9a551..ecc1bbd17d 100644
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -272,14 +272,17 @@ static void __init noreturn efi_arch_post_exit_boot(void)
>      unreachable();
>  }
>  
> -static void __init efi_arch_cfg_file_early(EFI_FILE_HANDLE dir_handle, char *section)
> +static void __init efi_arch_cfg_file_early(EFI_LOADED_IMAGE *image, EFI_FILE_HANDLE dir_handle, char *section)
>  {
>  }
>  
> -static void __init efi_arch_cfg_file_late(EFI_FILE_HANDLE dir_handle, char *section)
> +static void __init efi_arch_cfg_file_late(EFI_LOADED_IMAGE *image, EFI_FILE_HANDLE dir_handle, char *section)

Same here, image could be const.

>  {
>      union string name;
>  
> +    if ( read_section(image, ".ucode", &ucode, NULL) )
> +        return;
> +
>      name.s = get_value(&cfg, section, "ucode");
>      if ( !name.s )
>          name.s = get_value(&cfg, "global", "ucode");
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index f5bdc4b1df..452b5f4362 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -121,6 +121,8 @@ static CHAR16 *s2w(union string *str);
>  static char *w2s(const union string *str);
>  static bool read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
>                        struct file *file, char *options);
> +static bool read_section(EFI_LOADED_IMAGE *image,
> +        char *name, struct file *file, char *options);

Aling overflow parameters to the start parentheses please.

>  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);
> @@ -622,6 +624,26 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
>      return true;
>  }
>  
> +static bool __init read_section(EFI_LOADED_IMAGE *image,

image can be const I think.

> +                                char *const name, struct file *file, char *optio/pe_find_sectionns)

options should also be const IMO, but that would require a change to
efi_arch_handle_module AFAICT.

> +{
> +    /* skip the leading "." in the section name */
> +    union string name_string = { .s = name + 1 };
> +
> +    file->ptr = (void *)pe_find_section(image->ImageBase, image->ImageSize,

This already returns a void *, so there's no need for the cast.

> +        name, &file->size);

Same comment regarding alignment of overflow parameters.

> +    if ( !file->ptr )
> +        return false;
> +
> +    file->need_to_free = false;
> +
> +    s2w(&name_string);
> +    display_file_info(name_string.w, file, options);
> +    efi_bs->FreePool(name_string.w);
> +
> +    return true;
> +}
> +
>  static void __init pre_parse(const struct file *cfg)
>  {
>      char *ptr = cfg->ptr, *end = ptr + cfg->size;
> @@ -1206,9 +1228,13 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>          /* Get the file system interface. */
>          dir_handle = get_parent_handle(loaded_image, &file_name);
>  
> -        /* Read and parse the config file. */
> -        if ( !cfg_file_name )
> +        if ( read_section(loaded_image, ".config", &cfg, NULL) )
> +        {
> +            PrintStr(L"Using unified config file\r\n");
> +        }
> +        else if ( !cfg_file_name )
>          {
> +            /* Read and parse the config file. */
>              CHAR16 *tail;
>  
>              while ( (tail = point_tail(file_name)) != NULL )
> @@ -1257,29 +1283,39 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>          if ( !name.s )
>              blexit(L"No Dom0 kernel image specified.");
>  
> -        efi_arch_cfg_file_early(dir_handle, section.s);
> +        efi_arch_cfg_file_early(loaded_image, dir_handle, section.s);
>  
>          option_str = split_string(name.s);
> -        read_file(dir_handle, s2w(&name), &kernel, option_str);
> -        efi_bs->FreePool(name.w);
> -
> -        if ( !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL,
> -                        (void **)&shim_lock)) &&
> -             (status = shim_lock->Verify(kernel.ptr, kernel.size)) != EFI_SUCCESS )
> -            PrintErrMesg(L"Dom0 kernel image could not be verified", status);
>  
> -        name.s = get_value(&cfg, section.s, "ramdisk");
> -        if ( name.s )
> +        if ( !read_section(loaded_image, ".kernel", &kernel, option_str) )
>          {
> -            read_file(dir_handle, s2w(&name), &ramdisk, NULL);
> +            read_file(dir_handle, s2w(&name), &kernel, option_str);
>              efi_bs->FreePool(name.w);
> +
> +            if ( !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL,
> +                            (void **)&shim_lock)) &&
> +                 (status = shim_lock->Verify(kernel.ptr, kernel.size)) != EFI_SUCCESS )
> +                PrintErrMesg(L"Dom0 kernel image could not be verified", status);
>          }
>  
> -        name.s = get_value(&cfg, section.s, "xsm");
> -        if ( name.s )
> +        if ( !read_section(loaded_image, ".ramdisk", &ramdisk, NULL) )

In the commit message you mention .initrd, but here you seem to use
.ramdisk instead as the section name for the kernel payload?

>          {
> -            read_file(dir_handle, s2w(&name), &xsm, NULL);
> -            efi_bs->FreePool(name.w);
> +            name.s = get_value(&cfg, section.s, "ramdisk");
> +            if ( name.s )
> +            {
> +                read_file(dir_handle, s2w(&name), &ramdisk, NULL);
> +                efi_bs->FreePool(name.w);
> +            }
> +        }
> +
> +        if ( !read_section(loaded_image, ".xsm", &xsm, NULL) )
> +        {
> +            name.s = get_value(&cfg, section.s, "xsm");
> +            if ( name.s )
> +            {
> +                read_file(dir_handle, s2w(&name), &xsm, NULL);
> +                efi_bs->FreePool(name.w);
> +            }
>          }
>  
>          /*
> @@ -1315,7 +1351,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>              }
>          }
>  
> -        efi_arch_cfg_file_late(dir_handle, section.s);
> +        efi_arch_cfg_file_late(loaded_image, dir_handle, section.s);
>  
>          efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
>          cfg.addr = 0;
> diff --git a/xen/common/efi/efi.h b/xen/common/efi/efi.h
> index 4845d84913..c9b741bf27 100644
> --- a/xen/common/efi/efi.h
> +++ b/xen/common/efi/efi.h
> @@ -47,3 +47,6 @@ const CHAR16 *wmemchr(const CHAR16 *s, CHAR16 c, UINTN n);
>  /* EFI boot allocator. */
>  void *ebmalloc(size_t size);
>  void free_ebmalloc_unused_mem(void);
> +
> +const void * pe_find_section(const UINT8 *image_base, const size_t image_size,
> +        const char *section_name, UINTN *size_out);
> diff --git a/xen/common/efi/pe.c b/xen/common/efi/pe.c
> new file mode 100644
> index 0000000000..097c6848a8
> --- /dev/null
> +++ b/xen/common/efi/pe.c
> @@ -0,0 +1,141 @@
> +/*
> + * xen/common/efi/pe.c
> + *
> + * PE executable header parser.
> + *
> + * Derived from https://github.com/systemd/systemd/blob/master/src/boot/efi/pe.c
> + * commit 07d5ed536ec0a76b08229c7a80b910cb9acaf6b1
> + *
> + * Copyright (C) 2015 Kay Sievers <kay@vrfy.org>
> + * Copyright (C) 2020 Trammell Hudson <hudson@trmm.net>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU Lesser General Public License as published by
> + * the Free Software Foundation; either version 2.1 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + */
> +
> +
> +#include "efi.h"
> +
> +struct DosFileHeader {
> +    UINT8   Magic[2];
> +    UINT16  LastSize;
> +    UINT16  nBlocks;
> +    UINT16  nReloc;
> +    UINT16  HdrSize;
> +    UINT16  MinAlloc;
> +    UINT16  MaxAlloc;
> +    UINT16  ss;
> +    UINT16  sp;
> +    UINT16  Checksum;
> +    UINT16  ip;
> +    UINT16  cs;
> +    UINT16  RelocPos;
> +    UINT16  nOverlay;
> +    UINT16  reserved[4];
> +    UINT16  OEMId;
> +    UINT16  OEMInfo;
> +    UINT16  reserved2[10];
> +    UINT32  ExeHeader;
> +} __attribute__((packed));
> +
> +#define PE_HEADER_MACHINE_ARM64         0xaa64
> +#define PE_HEADER_MACHINE_X64           0x8664
> +#define PE_HEADER_MACHINE_I386          0x014c
> +
> +struct PeFileHeader {
> +    UINT16  Machine;
> +    UINT16  NumberOfSections;
> +    UINT32  TimeDateStamp;
> +    UINT32  PointerToSymbolTable;
> +    UINT32  NumberOfSymbols;
> +    UINT16  SizeOfOptionalHeader;
> +    UINT16  Characteristics;
> +} __attribute__((packed));
> +
> +struct PeHeader {
> +    UINT8   Magic[4];
> +    struct PeFileHeader FileHeader;
> +} __attribute__((packed));
> +
> +struct PeSectionHeader {
> +    UINT8   Name[8];
> +    UINT32  VirtualSize;
> +    UINT32  VirtualAddress;
> +    UINT32  SizeOfRawData;
> +    UINT32  PointerToRawData;
> +    UINT32  PointerToRelocations;
> +    UINT32  PointerToLinenumbers;
> +    UINT16  NumberOfRelocations;
> +    UINT16  NumberOfLinenumbers;
> +    UINT32  Characteristics;
> +} __attribute__((packed));
> +
> +const void * __init pe_find_section(const CHAR8 * image, const UINTN image_size,
> +                              const char * section_name, UINTN * size_out)

You seem to have changed this quite extensively, so the spaces between
the * and the parameter name (or __init) should be removed.

> +{
> +    const struct DosFileHeader * dos = (const void*) image;
> +    const struct PeHeader * pe;
> +    const struct PeSectionHeader * sect;
> +    const UINTN name_len = strlen(section_name);
> +    UINTN offset = 0;
> +
> +    if ( name_len > sizeof(sect->Name) )
> +        return NULL;
> +
> +    if ( image_size < sizeof(*dos) )
> +        return NULL;
> +    if ( memcmp(dos->Magic, "MZ", 2) != 0 )
> +        return NULL;

Please join all the ifs into a single one.

> +
> +    offset = dos->ExeHeader;
> +    pe = (const void *) &image[offset];

Extra space after the closing parentheses.

> +
> +    offset += sizeof(*pe);
> +    if ( image_size < offset )
> +        return NULL;
> +
> +    if ( memcmp(pe->Magic, "PE\0\0", 4) != 0 )
> +        return NULL;

Same here, you can join the two ifs into a single one.

> +
> +    /* PE32+ Subsystem type */
> +#if defined(__arm__) || defined (__aarch64__)
> +    if ( pe->FileHeader.Machine != PE_HEADER_MACHINE_ARM64 )
> +        return NULL;
> +#elif defined(__x86_64__)
> +    if ( pe->FileHeader.Machine != PE_HEADER_MACHINE_X64 )
> +        return NULL;
> +#else
> +    /* unknown architecture */
> +    return NULL;
> +#endif
> +
> +    offset += pe->FileHeader.SizeOfOptionalHeader;
> +
> +    for ( UINTN i = 0 ; i < pe->FileHeader.NumberOfSections ; i++ )

Please define i at the top of the function, and there's an extra space
before the semicolon:

for ( i = 0; i < ...

> +    {
> +        sect = (const void *) &image[offset];
> +        if ( image_size < offset + sizeof(*sect) )
> +            return NULL;
> +
> +        if ( memcmp(sect->Name, section_name, name_len) != 0 ||
> +             image_size < sect->VirtualSize + sect->VirtualAddress )
> +        {
> +            offset += sizeof(*sect);
> +            continue;
> +        }
> +
> +        if ( size_out )
> +            *size_out = sect->VirtualSize;
> +
> +        return &image[sect->VirtualAddress];

Oh, so the virtual address does indeed matter, and hence will need a
bit more explanation in the document I think.

Thanks, Roger.

Re: [PATCH v3 4/4] efi: Do not use command line if secure boot is enabled.

[Roger Pau Monné]

On Mon, Sep 07, 2020 at 03:00:27PM -0400, Trammell Hudson wrote:
> From: Trammell hudson <hudson@trmm.net>
> 
> If secure boot is enabled, the Xen command line arguments are ignored.
> If a unified Xen image is used, then the bundled configuration, dom0
> kernel, and initrd are prefered over the ones listed in the config file.
> 
> Unlike the shim based verification, the PE signature on a unified image
> covers the all of the Xen+config+kernel+initrd modules linked into the
> unified image. This also ensures that properly configured platforms
> will measure the entire runtime into the TPM for unsealing secrets or
> remote attestation.
> 
> Signed-off-by: Trammell Hudson <hudson@trmm.net>
> ---
>  xen/common/efi/boot.c | 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 )

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?

Line length also.

> +        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;

I would print a message if secboot is > 1, since those should be
reserved.

Roger.

Re: [PATCH v3 2/4] efi/boot.c: add file.need_to_free and split display_file_info()

[Trammell Hudson]

On Monday, September 14, 2020 5:05 AM, Roger Pau Monné <roger.pau@citrix.com> 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

[Trammell Hudson]

On Monday, September 14, 2020 6:06 AM, Roger Pau Monné <roger.pau@citrix.com> 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=0xffff82d042000000 \
>
> 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.

[Trammell Hudson]

On Monday, September 14, 2020 6:24 AM, Roger Pau Monné <roger.pau@citrix.com> 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

Re: [PATCH v3 3/4] efi: Enable booting unified hypervisor/kernel/initrd images

[Jan Beulich]

On 14.09.2020 13:19, Trammell Hudson wrote:
> On Monday, September 14, 2020 6:06 AM, Roger Pau Monné <roger.pau@citrix.com> wrote:
>> On Mon, Sep 07, 2020 at 03:00:26PM -0400, Trammell Hudson wrote:
>>> -   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.

But casting away const-ness is really bad. If file->ptr can't
be adjusted accordingly, I'm afraid the function will need to
return a pointer to non-const for the time being.

Jan

Re: [PATCH v3 4/4] efi: Do not use command line if secure boot is enabled.

[Jan Beulich]

On 14.09.2020 13:36, Trammell Hudson wrote:
> On Monday, September 14, 2020 6:24 AM, Roger Pau Monné <roger.pau@citrix.com> 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)

And I think this underlying aspect if the reason for a lot of apparently
missing const in our EFI interfacing code.

Jan

[PATCH v3 2/4] efi/boot.c: add file.need_to_free and split display_file_info()

[Trammell Hudson]

add file.need_to_free and split display_file_info()

Signed-off-by: Trammell hudson <hudson@trmm.net>
---
 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