[PATCH v5 0/5] efi: Unified Xen hypervisor/kernel/initrd images

[PATCH v5 0/5] efi: Unified Xen hypervisor/kernel/initrd images

[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
(`.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*

[Trammell Hudson]

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 <hudson@trmm.net>
---
 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 2/5] efi/boot.c: add file.need_to_free

[Trammell Hudson]

The config file, kernel, initrd, etc should only be freed if they
are allocated with the UEFI allocator.

Signed-off-by: Trammell Hudson <hudson@trmm.net>
---
 xen/common/efi/boot.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 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()

[Trammell Hudson]

Add a separate function to display the address ranges used by
the files and call `efi_arch_handle_module()` on the modules.

Signed-off-by: Trammell Hudson <hudson@trmm.net>
---
 xen/common/efi/boot.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 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 4/5] efi: Enable booting unified hypervisor/kernel/initrd images

[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
(`.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 <hudson@trmm.net>
---
 .gitignore                  |   1 +
 docs/misc/efi.pandoc        |  49 +++++++++++++
 xen/arch/arm/efi/efi-boot.h |  25 ++++---
 xen/arch/x86/efi/Makefile   |   2 +-
 xen/arch/x86/efi/efi-boot.h |  11 ++-
 xen/common/efi/boot.c       |  73 +++++++++++++++-----
 xen/common/efi/efi.h        |   3 +
 xen/common/efi/pe.c         | 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 0xffff82d041000000.
+All the sections are optional (`.config`, `.kernel`, `.ramdisk`, `.xsm`,
+`.ucode` (x86) and `.dtb` (ARM)) and the order does not matter.
+The virtual addresses do not need to be contiguous, although they should not
+be overlapping and should all be greater than the last virtual address of the
+hypervisor components.
+
+```
+objcopy \
+	--add-section .config=xen.cfg \
+	--change-section-vma .config=0xffff82d041000000
+	--add-section .ucode=ucode.bin \
+	--change-section-vma .ucode=0xffff82d041010000 \
+	--add-section .xsm=xsm.cfg \
+	--change-section-vma .xsm=0xffff82d041080000 \
+	--add-section .kernel=vmlinux \
+	--change-section-vma .kernel=0xffff82d041100000 \
+	--add-section .ramdisk=initrd.img \
+	--change-section-vma .ramdisk=0xffff82d042000000 \
+	xen.efi \
+	xen.unified.efi
+```
+
+The unified executable can be signed with sbsigntool to make
+it usable with UEFI secure boot:
+
+```
+sbsign \
+	--key signing.key \
+	--cert cert.pem \
+	--output xen.signed.efi \
+	xen.unified.efi
+```
diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index 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_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,
+                                           const char *section)
 {
     union string name;
 
     /*
      * The DTB must be processed before any other entries in the configuration
-     * file, as the DTB is updated as modules are loaded.
+     * file, as the DTB is updated as modules are loaded.  Prefer the one
+     * stored as a PE section in a unified image, and fall back to a file
+     * on disk if the section is not present.
      */
-    name.s = get_value(&cfg, section, "dtb");
-    if ( name.s )
+    if ( !read_section(image, ".dtb", &dtbfile, NULL) )
     {
-        split_string(name.s);
-        read_file(dir_handle, s2w(&name), &dtbfile, NULL);
-        efi_bs->FreePool(name.w);
+        name.s = get_value(&cfg, section, "dtb");
+        if ( name.s )
+        {
+            split_string(name.s);
+            read_file(dir_handle, s2w(&name), &dtbfile, NULL);
+            efi_bs->FreePool(name.w);
+        }
     }
     fdt = fdt_increase_size(&dtbfile, cfg.size + EFI_PAGE_SIZE);
     if ( !fdt )
         blexit(L"Unable to create new FDT");
 }
 
-static void __init efi_arch_cfg_file_late(EFI_FILE_HANDLE dir_handle, char *section)
+static void __init efi_arch_cfg_file_late(const EFI_LOADED_IMAGE *image,
+                                          EFI_FILE_HANDLE dir_handle,
+                                          const 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 eef3f52789..db3e29f573 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -272,14 +272,21 @@ static void __init noreturn efi_arch_post_exit_boot(void)
     unreachable();
 }
 
-static void __init efi_arch_cfg_file_early(EFI_FILE_HANDLE dir_handle, char *section)
+static void __init efi_arch_cfg_file_early(const EFI_LOADED_IMAGE *image,
+                                           EFI_FILE_HANDLE dir_handle,
+                                           const char *section)
 {
 }
 
-static void __init efi_arch_cfg_file_late(EFI_FILE_HANDLE dir_handle, char *section)
+static void __init efi_arch_cfg_file_late(const EFI_LOADED_IMAGE *image,
+                                          EFI_FILE_HANDLE dir_handle,
+                                          const 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 e0280f7a21..c14e836640 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -122,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, const char *options);
+static bool read_section(const EFI_LOADED_IMAGE *image,
+                         char *name, struct file *file, const 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);
@@ -624,6 +626,29 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
     return true;
 }
 
+static bool __init read_section(const EFI_LOADED_IMAGE *image,
+                                char *const name, struct file *file,
+                                const char *options)
+{
+    /* skip the leading "." in the section name */
+    union string name_string = { .s = name + 1 };
+
+    file->ptr = pe_find_section(image->ImageBase, image->ImageSize,
+                                name, &file->size);
+    if ( !file->ptr )
+        return false;
+
+    file->need_to_free = false;
+
+    if ( !s2w(&name_string) )
+        return false;
+
+    handle_file_info(name_string.w, file, options);
+    efi_bs->FreePool(name_string.w);
+
+    return true;
+}
+
 static void __init pre_parse(const struct file *cfg)
 {
     char *ptr = cfg->str, *end = ptr + cfg->size;
@@ -1208,9 +1233,11 @@ 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 )
@@ -1259,29 +1286,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);
+            }
         }
 
         /*
@@ -1317,7 +1354,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..c9d3752121 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 void *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..568f0823bb
--- /dev/null
+++ b/xen/common/efi/pe.c
@@ -0,0 +1,134 @@
+/*
+ * 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;
+};
+
+#if defined(__arm__) || defined (__aarch64__)
+#define PE_HEADER_MACHINE 0xaa64
+#elif defined(__x86_64__)
+#define PE_HEADER_MACHINE 0x8664
+#else
+#error "Unknown architecture"
+#endif
+
+struct PeFileHeader {
+    UINT16  Machine;
+    UINT16  NumberOfSections;
+    UINT32  TimeDateStamp;
+    UINT32  PointerToSymbolTable;
+    UINT32  NumberOfSymbols;
+    UINT16  SizeOfOptionalHeader;
+    UINT16  Characteristics;
+};
+
+struct PeHeader {
+    UINT8   Magic[4];
+    struct PeFileHeader FileHeader;
+};
+
+struct PeSectionHeader {
+    CHAR8   Name[8];
+    UINT32  VirtualSize;
+    UINT32  VirtualAddress;
+    UINT32  SizeOfRawData;
+    UINT32  PointerToRawData;
+    UINT32  PointerToRelocations;
+    UINT32  PointerToLinenumbers;
+    UINT16  NumberOfRelocations;
+    UINT16  NumberOfLinenumbers;
+    UINT32  Characteristics;
+};
+
+const void *__init pe_find_section(const void *image, const UINTN image_size,
+                                   const char *section_name, UINTN *size_out)
+{
+    const struct DosFileHeader *dos = image;
+    const struct PeHeader *pe;
+    const struct PeSectionHeader *sect;
+    const UINTN name_len = strlen(section_name);
+    UINTN offset, i;
+
+    if ( name_len > sizeof(sect->Name) ||
+         image_size < sizeof(*dos) ||
+         memcmp(dos->Magic, "MZ", 2) != 0 )
+        return NULL;
+
+    offset = dos->ExeHeader;
+    pe = image + offset;
+
+    offset += sizeof(*pe);
+    if ( image_size < offset ||
+         memcmp(pe->Magic, "PE\0\0", 4) != 0 )
+        return NULL;
+
+    /* PE32+ Subsystem type */
+    if ( pe->FileHeader.Machine != PE_HEADER_MACHINE )
+        return NULL;
+
+    offset += pe->FileHeader.SizeOfOptionalHeader;
+
+    for ( i = 0; i < pe->FileHeader.NumberOfSections; i++ )
+    {
+        sect = image + offset;
+        if ( image_size < offset + sizeof(*sect) )
+            return NULL;
+
+        if ( memcmp(sect->Name, section_name, name_len) != 0 )
+        {
+            offset += sizeof(*sect);
+            continue;
+        }
+
+        if ( image_size < sect->VirtualSize + sect->VirtualAddress )
+            blexit(L"PE invalid section size + address");
+
+        if ( size_out )
+            *size_out = sect->VirtualSize;
+
+        return image + sect->VirtualAddress;
+    }
+
+    return NULL;
+}
-- 
2.25.1

[PATCH v5 5/5] efi: Do not use command line if unified config is included

[Trammell Hudson]

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 <hudson@trmm.net>
---
 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 v5 1/5] efi/boot.c: Make file->ptr const void*

[Jan Beulich]

On 17.09.2020 17:40, Trammell Hudson wrote:
> 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 <hudson@trmm.net>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
albeit ...

> @@ -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);

... I'm not particularly happy about this part of the change.
But yes - what do you do.

Jan

Re: [PATCH v5 2/5] efi/boot.c: add file.need_to_free

[Jan Beulich]

On 17.09.2020 17:40, Trammell Hudson wrote:
> The config file, kernel, initrd, etc should only be freed if they
> are allocated with the UEFI allocator.
> 
> Signed-off-by: Trammell Hudson <hudson@trmm.net>

Hmm, this hasn't changed from v4, despite the review requests.
Any particular reason?

Jan

Re: [PATCH v5 3/5] efi/boot.c: add handle_file_info()

[Jan Beulich]

On 17.09.2020 17:40, Trammell Hudson wrote:
> Add a separate function to display the address ranges used by
> the files and call `efi_arch_handle_module()` on the modules.
> 
> Signed-off-by: Trammell Hudson <hudson@trmm.net>

You've lost the ack I gave, and ...

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

... with the const added here you ought to mention (in a post-
description remark perhaps) that this now depends on a not yet
applied patch on the list). Remember that reviewers and
committer may be different people, and even if the committer
participated in review, (s)he may have forgotten by the time
a patch is otherwise ready to go in.

And btw - briefly mentioning what changes you made compared to
the previous version (again in a post-description remark) helps
reviews quite a bit.

Jan

Re: [PATCH v5 4/5] efi: Enable booting unified hypervisor/kernel/initrd images

[Jan Beulich]

On 17.09.2020 17:40, Trammell Hudson wrote:
> @@ -624,6 +626,29 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
>      return true;
>  }
>  
> +static bool __init read_section(const EFI_LOADED_IMAGE *image,
> +                                char *const name, struct file *file,
> +                                const char *options)
> +{
> +    /* skip the leading "." in the section name */
> +    union string name_string = { .s = name + 1 };

Instead of forcing the caller to pass in a dot-prefixed name
and you assuming it's a dot here, how about ...

> +    file->ptr = pe_find_section(image->ImageBase, image->ImageSize,
> +                                name, &file->size);

... pe_find_section() looking for '.' followed by <name>?

> +    if ( !file->ptr )
> +        return false;
> +
> +    file->need_to_free = false;
> +
> +    if ( !s2w(&name_string) )
> +        return false;

You shouldn't give the false impression to the caller that the
section was not found, the more that ...

> +    handle_file_info(name_string.w, file, options);
> +    efi_bs->FreePool(name_string.w);

... you really need the transformation solely for producing
output. Just output a static surrogate message instead if this
fails? Of course, for x86'es sake you'd then need to pass the
ASCII string instead, which would save it from doing the
backwards translation, which is its only use of the parameter
(Arm doesn't use it at all).

> @@ -1208,9 +1233,11 @@ 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");

Maybe "embedded" instead of "unified"? The config file isn't unified
after all, it's the Xen binary which is.

Jan

Re: [PATCH v5 5/5] efi: Do not use command line if unified config is included

[Jan Beulich]

On 17.09.2020 17:40, Trammell Hudson wrote:
> @@ -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;

I continue to think that it's wrong to ignore the command line
altogether. And this isn't just for the Dom0 kernel part when
there's no .kernel section, but also because there may be
firmware workarounds that the user needs to be able to make use
of in order for the binary to work on certain hardware.

Jan

Re: [PATCH v5 4/5] efi: Enable booting unified hypervisor/kernel/initrd images

[Trammell Hudson]

On Friday, September 18, 2020 11:15 AM, Jan Beulich <jbeulich@suse.com> 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 <name>?

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

Re: [PATCH v5 4/5] efi: Enable booting unified hypervisor/kernel/initrd images

[Jan Beulich]

On 21.09.2020 13:47, Trammell Hudson wrote:
> On Friday, September 18, 2020 11:15 AM, Jan Beulich <jbeulich@suse.com> 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 <name>?
> 
> 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)

And I'd be happy to approve a patch to this effect, provided
it uses an inline function and not a macro to do this wrapping.

>> [...]
>>> -          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"?

That's fine with me as well.

Jan