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

[PATCH v4 0/4] 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 (4):
  efi/boot.c: add file.need_to_free
  efi/boot.c: add handle_file_info()
  efi: Enable booting unified hypervisor/kernel/initrd images
  efi: Do not use command line if secure boot is enabled.

 .gitignore                  |   1 +
 docs/misc/efi.pandoc        |  49 ++++++++++++
 xen/arch/arm/efi/efi-boot.h |  25 ++++--
 xen/arch/x86/efi/Makefile   |   2 +-
 xen/arch/x86/efi/efi-boot.h |  11 ++-
 xen/common/efi/boot.c       | 154 ++++++++++++++++++++++++++++--------
 xen/common/efi/efi.h        |   3 +
 xen/common/efi/pe.c         | 137 ++++++++++++++++++++++++++++++++
 8 files changed, 336 insertions(+), 46 deletions(-)
 create mode 100644 xen/common/efi/pe.c

-- 
2.25.1

[PATCH v4 1/4] 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 4022a672c9..7156139174 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -102,6 +102,7 @@ union string {
 
 struct file {
     UINTN size;
+    bool need_to_free;
     union {
         EFI_PHYSICAL_ADDRESS addr;
         void *ptr;
@@ -279,13 +280,13 @@ void __init noreturn blexit(const CHAR16 *str)
     if ( !efi_bs )
         efi_arch_halt();
 
-    if ( cfg.addr )
+    if ( cfg.addr && cfg.need_to_free )
         efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
-    if ( kernel.addr )
+    if ( kernel.addr && kernel.need_to_free )
         efi_bs->FreePages(kernel.addr, PFN_UP(kernel.size));
-    if ( ramdisk.addr )
+    if ( ramdisk.addr && ramdisk.need_to_free )
         efi_bs->FreePages(ramdisk.addr, PFN_UP(ramdisk.size));
-    if ( xsm.addr )
+    if ( xsm.addr && xsm.need_to_free )
         efi_bs->FreePages(xsm.addr, PFN_UP(xsm.size));
 
     efi_arch_blexit();
@@ -572,6 +573,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
                          HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START);
         ret = efi_bs->AllocatePages(AllocateMaxAddress, EfiLoaderData,
                                     PFN_UP(size), &file->addr);
+        file->need_to_free = true;
     }
     if ( EFI_ERROR(ret) )
     {
-- 
2.25.1

[PATCH v4 2/4] 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 7156139174..57df89cacb 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -539,6 +539,22 @@ static char * __init split_string(char *s)
     return NULL;
 }
 
+static void __init handle_file_info(CHAR16 *name,
+                                    struct file *file, char *options)
+{
+    if ( file == &cfg )
+        return;
+
+    PrintStr(name);
+    PrintStr(L": ");
+    DisplayUint(file->addr, 2 * sizeof(file->addr));
+    PrintStr(L"-");
+    DisplayUint(file->addr + file->size, 2 * sizeof(file->addr));
+    PrintStr(newline);
+
+    efi_arch_handle_module(file, name, options);
+}
+
 static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
                              struct file *file, char *options)
 {
@@ -583,16 +599,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
     else
     {
         file->size = size;
-        if ( file != &cfg )
-        {
-            PrintStr(name);
-            PrintStr(L": ");
-            DisplayUint(file->addr, 2 * sizeof(file->addr));
-            PrintStr(L"-");
-            DisplayUint(file->addr + size, 2 * sizeof(file->addr));
-            PrintStr(newline);
-            efi_arch_handle_module(file, name, options);
-        }
+        handle_file_info(name, file, options);
 
         ret = FileHandle->Read(FileHandle, &file->size, file->ptr);
         if ( !EFI_ERROR(ret) && file->size != size )
-- 
2.25.1

[PATCH v4 3/4] 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.

Signed-off-by: Trammell Hudson <hudson@trmm.net>
---
 .gitignore                  |   1 +
 docs/misc/efi.pandoc        |  49 +++++++++++++
 xen/arch/arm/efi/efi-boot.h |  25 ++++---
 xen/arch/x86/efi/Makefile   |   2 +-
 xen/arch/x86/efi/efi-boot.h |  11 ++-
 xen/common/efi/boot.c       |  73 ++++++++++++++-----
 xen/common/efi/efi.h        |   3 +
 xen/common/efi/pe.c         | 137 ++++++++++++++++++++++++++++++++++++
 8 files changed, 272 insertions(+), 29 deletions(-)
 create mode 100644 xen/common/efi/pe.c

diff --git a/.gitignore b/.gitignore
index 823f4743dc..d568017804 100644
--- a/.gitignore
+++ b/.gitignore
@@ -299,6 +299,7 @@ xen/arch/*/efi/boot.c
 xen/arch/*/efi/compat.c
 xen/arch/*/efi/ebmalloc.c
 xen/arch/*/efi/efi.h
+xen/arch/*/efi/pe.c
 xen/arch/*/efi/runtime.c
 xen/common/config_data.S
 xen/common/config.gz
diff --git a/docs/misc/efi.pandoc b/docs/misc/efi.pandoc
index 23c1a2732d..ac3cd58cae 100644
--- a/docs/misc/efi.pandoc
+++ b/docs/misc/efi.pandoc
@@ -116,3 +116,52 @@ Filenames must be specified relative to the location of the EFI binary.
 
 Extra options to be passed to Xen can also be specified on the command line,
 following a `--` separator option.
+
+## Unified Xen kernel image
+
+The "Unified" kernel image can be generated by adding additional
+sections to the Xen EFI executable with objcopy, similar to how
+[systemd-boot uses the stub to add them to the Linux kernel](https://wiki.archlinux.org/index.php/systemd-boot#Preparing_a_unified_kernel_image)
+
+The sections for the xen configuration file, the dom0 kernel, dom0 initrd,
+XSM and CPU microcode should be added after the Xen `.pad` section, the
+ending address of which can be located with:
+
+```
+objdump -h xen.efi \
+	| perl -ane '/\.pad/ && printf "0x%016x\n", hex($F[2]) + hex($F[3])'
+```
+
+For all the examples the `.pad` section ended at 0xffff82d041000000.
+All the sections are optional (`.config`, `.kernel`, `.ramdisk`, `.xsm`,
+`.ucode` (x86) and `.dtb` (ARM)) and the order does not matter.
+The virtual addresses do not need to be contiguous, although they should not
+be overlapping and should all be greater than the last virtual address of the
+hypervisor components.
+
+```
+objcopy \
+	--add-section .config=xen.cfg \
+	--change-section-vma .config=0xffff82d041000000
+	--add-section .ucode=ucode.bin \
+	--change-section-vma .ucode=0xffff82d041010000 \
+	--add-section .xsm=xsm.cfg \
+	--change-section-vma .xsm=0xffff82d041080000 \
+	--add-section .kernel=vmlinux \
+	--change-section-vma .kernel=0xffff82d041100000 \
+	--add-section .ramdisk=initrd.img \
+	--change-section-vma .ramdisk=0xffff82d042000000 \
+	xen.efi \
+	xen.unified.efi
+```
+
+The unified executable can be signed with sbsigntool to make
+it usable with UEFI secure boot:
+
+```
+sbsign \
+	--key signing.key \
+	--cert cert.pem \
+	--output xen.signed.efi \
+	xen.unified.efi
+```
diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index 6527cb0bdf..08bd4d7630 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -375,27 +375,36 @@ static void __init noreturn efi_arch_post_exit_boot(void)
     efi_xen_start(fdt, fdt_totalsize(fdt));
 }
 
-static void __init efi_arch_cfg_file_early(EFI_FILE_HANDLE dir_handle, char *section)
+static void __init efi_arch_cfg_file_early(const EFI_LOADED_IMAGE *image,
+                                           EFI_FILE_HANDLE dir_handle,
+                                           char *section)
 {
     union string name;
 
     /*
      * The DTB must be processed before any other entries in the configuration
-     * file, as the DTB is updated as modules are loaded.
+     * file, as the DTB is updated as modules are loaded.  Prefer the one
+     * stored as a PE section in a unified image, and fall back to a file
+     * on disk if the section is not present.
      */
-    name.s = get_value(&cfg, section, "dtb");
-    if ( name.s )
+    if ( !read_section(image, ".dtb", &dtbfile, NULL) )
     {
-        split_string(name.s);
-        read_file(dir_handle, s2w(&name), &dtbfile, NULL);
-        efi_bs->FreePool(name.w);
+        name.s = get_value(&cfg, section, "dtb");
+        if ( name.s )
+        {
+            split_string(name.s);
+            read_file(dir_handle, s2w(&name), &dtbfile, NULL);
+            efi_bs->FreePool(name.w);
+        }
     }
     fdt = fdt_increase_size(&dtbfile, cfg.size + EFI_PAGE_SIZE);
     if ( !fdt )
         blexit(L"Unable to create new FDT");
 }
 
-static void __init efi_arch_cfg_file_late(EFI_FILE_HANDLE dir_handle, char *section)
+static void __init efi_arch_cfg_file_late(const EFI_LOADED_IMAGE *image,
+                                          EFI_FILE_HANDLE dir_handle,
+                                          char *section)
 {
 }
 
diff --git a/xen/arch/x86/efi/Makefile b/xen/arch/x86/efi/Makefile
index 770438a029..e857c0f2cc 100644
--- a/xen/arch/x86/efi/Makefile
+++ b/xen/arch/x86/efi/Makefile
@@ -8,7 +8,7 @@ cmd_objcopy_o_ihex = $(OBJCOPY) -I ihex -O binary $< $@
 
 boot.init.o: buildid.o
 
-EFIOBJ := boot.init.o ebmalloc.o compat.o runtime.o
+EFIOBJ := boot.init.o pe.init.o ebmalloc.o compat.o runtime.o
 
 $(call cc-option-add,cflags-stack-boundary,CC,-mpreferred-stack-boundary=4)
 $(EFIOBJ): CFLAGS-stack-boundary := $(cflags-stack-boundary)
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 7188c9a551..9ab69f84d4 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -272,14 +272,21 @@ static void __init noreturn efi_arch_post_exit_boot(void)
     unreachable();
 }
 
-static void __init efi_arch_cfg_file_early(EFI_FILE_HANDLE dir_handle, char *section)
+static void __init efi_arch_cfg_file_early(const EFI_LOADED_IMAGE *image,
+                                           EFI_FILE_HANDLE dir_handle,
+                                           char *section)
 {
 }
 
-static void __init efi_arch_cfg_file_late(EFI_FILE_HANDLE dir_handle, char *section)
+static void __init efi_arch_cfg_file_late(const EFI_LOADED_IMAGE *image,
+                                          EFI_FILE_HANDLE dir_handle,
+                                          char *section)
 {
     union string name;
 
+    if ( read_section(image, ".ucode", &ucode, NULL) )
+        return;
+
     name.s = get_value(&cfg, section, "ucode");
     if ( !name.s )
         name.s = get_value(&cfg, "global", "ucode");
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 57df89cacb..4b1fbc9643 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -121,6 +121,8 @@ static CHAR16 *s2w(union string *str);
 static char *w2s(const union string *str);
 static bool read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
                       struct file *file, char *options);
+static bool read_section(const EFI_LOADED_IMAGE *image,
+                         char *name, struct file *file, char *options);
 static size_t wstrlen(const CHAR16 * s);
 static int set_color(u32 mask, int bpp, u8 *pos, u8 *sz);
 static bool match_guid(const EFI_GUID *guid1, const EFI_GUID *guid2);
@@ -623,6 +625,27 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
     return true;
 }
 
+static bool __init read_section(const EFI_LOADED_IMAGE *image,
+                                char *const name, struct file *file,
+                                char *options)
+{
+    /* skip the leading "." in the section name */
+    union string name_string = { .s = name + 1 };
+
+    file->ptr = (void *)pe_find_section(image->ImageBase, image->ImageSize,
+                                        name, &file->size);
+    if ( !file->ptr )
+        return false;
+
+    file->need_to_free = false;
+
+    s2w(&name_string);
+    handle_file_info(name_string.w, file, options);
+    efi_bs->FreePool(name_string.w);
+
+    return true;
+}
+
 static void __init pre_parse(const struct file *cfg)
 {
     char *ptr = cfg->ptr, *end = ptr + cfg->size;
@@ -1207,9 +1230,13 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
         /* Get the file system interface. */
         dir_handle = get_parent_handle(loaded_image, &file_name);
 
-        /* Read and parse the config file. */
-        if ( !cfg_file_name )
+        if ( read_section(loaded_image, ".config", &cfg, NULL) )
+        {
+            PrintStr(L"Using unified config file\r\n");
+        }
+        else if ( !cfg_file_name )
         {
+            /* Read and parse the config file. */
             CHAR16 *tail;
 
             while ( (tail = point_tail(file_name)) != NULL )
@@ -1258,29 +1285,39 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
         if ( !name.s )
             blexit(L"No Dom0 kernel image specified.");
 
-        efi_arch_cfg_file_early(dir_handle, section.s);
+        efi_arch_cfg_file_early(loaded_image, dir_handle, section.s);
 
         option_str = split_string(name.s);
-        read_file(dir_handle, s2w(&name), &kernel, option_str);
-        efi_bs->FreePool(name.w);
-
-        if ( !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL,
-                        (void **)&shim_lock)) &&
-             (status = shim_lock->Verify(kernel.ptr, kernel.size)) != EFI_SUCCESS )
-            PrintErrMesg(L"Dom0 kernel image could not be verified", status);
 
-        name.s = get_value(&cfg, section.s, "ramdisk");
-        if ( name.s )
+        if ( !read_section(loaded_image, ".kernel", &kernel, option_str) )
         {
-            read_file(dir_handle, s2w(&name), &ramdisk, NULL);
+            read_file(dir_handle, s2w(&name), &kernel, option_str);
             efi_bs->FreePool(name.w);
+
+            if ( !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL,
+                            (void **)&shim_lock)) &&
+                 (status = shim_lock->Verify(kernel.ptr, kernel.size)) != EFI_SUCCESS )
+                PrintErrMesg(L"Dom0 kernel image could not be verified", status);
         }
 
-        name.s = get_value(&cfg, section.s, "xsm");
-        if ( name.s )
+        if ( !read_section(loaded_image, ".ramdisk", &ramdisk, NULL) )
         {
-            read_file(dir_handle, s2w(&name), &xsm, NULL);
-            efi_bs->FreePool(name.w);
+            name.s = get_value(&cfg, section.s, "ramdisk");
+            if ( name.s )
+            {
+                read_file(dir_handle, s2w(&name), &ramdisk, NULL);
+                efi_bs->FreePool(name.w);
+            }
+        }
+
+        if ( !read_section(loaded_image, ".xsm", &xsm, NULL) )
+        {
+            name.s = get_value(&cfg, section.s, "xsm");
+            if ( name.s )
+            {
+                read_file(dir_handle, s2w(&name), &xsm, NULL);
+                efi_bs->FreePool(name.w);
+            }
         }
 
         /*
@@ -1316,7 +1353,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
             }
         }
 
-        efi_arch_cfg_file_late(dir_handle, section.s);
+        efi_arch_cfg_file_late(loaded_image, dir_handle, section.s);
 
         efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
         cfg.addr = 0;
diff --git a/xen/common/efi/efi.h b/xen/common/efi/efi.h
index 4845d84913..c9b741bf27 100644
--- a/xen/common/efi/efi.h
+++ b/xen/common/efi/efi.h
@@ -47,3 +47,6 @@ const CHAR16 *wmemchr(const CHAR16 *s, CHAR16 c, UINTN n);
 /* EFI boot allocator. */
 void *ebmalloc(size_t size);
 void free_ebmalloc_unused_mem(void);
+
+const void * pe_find_section(const UINT8 *image_base, const size_t image_size,
+        const char *section_name, UINTN *size_out);
diff --git a/xen/common/efi/pe.c b/xen/common/efi/pe.c
new file mode 100644
index 0000000000..2986545d53
--- /dev/null
+++ b/xen/common/efi/pe.c
@@ -0,0 +1,137 @@
+/*
+ * xen/common/efi/pe.c
+ *
+ * PE executable header parser.
+ *
+ * Derived from https://github.com/systemd/systemd/blob/master/src/boot/efi/pe.c
+ * commit 07d5ed536ec0a76b08229c7a80b910cb9acaf6b1
+ *
+ * Copyright (C) 2015 Kay Sievers <kay@vrfy.org>
+ * Copyright (C) 2020 Trammell Hudson <hudson@trmm.net>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU Lesser General Public License as published by
+ * the Free Software Foundation; either version 2.1 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ */
+
+
+#include "efi.h"
+
+struct DosFileHeader {
+    UINT8   Magic[2];
+    UINT16  LastSize;
+    UINT16  nBlocks;
+    UINT16  nReloc;
+    UINT16  HdrSize;
+    UINT16  MinAlloc;
+    UINT16  MaxAlloc;
+    UINT16  ss;
+    UINT16  sp;
+    UINT16  Checksum;
+    UINT16  ip;
+    UINT16  cs;
+    UINT16  RelocPos;
+    UINT16  nOverlay;
+    UINT16  reserved[4];
+    UINT16  OEMId;
+    UINT16  OEMInfo;
+    UINT16  reserved2[10];
+    UINT32  ExeHeader;
+} __attribute__((packed));
+
+#define PE_HEADER_MACHINE_ARM64         0xaa64
+#define PE_HEADER_MACHINE_X64           0x8664
+#define PE_HEADER_MACHINE_I386          0x014c
+
+struct PeFileHeader {
+    UINT16  Machine;
+    UINT16  NumberOfSections;
+    UINT32  TimeDateStamp;
+    UINT32  PointerToSymbolTable;
+    UINT32  NumberOfSymbols;
+    UINT16  SizeOfOptionalHeader;
+    UINT16  Characteristics;
+} __attribute__((packed));
+
+struct PeHeader {
+    UINT8   Magic[4];
+    struct PeFileHeader FileHeader;
+} __attribute__((packed));
+
+struct PeSectionHeader {
+    UINT8   Name[8];
+    UINT32  VirtualSize;
+    UINT32  VirtualAddress;
+    UINT32  SizeOfRawData;
+    UINT32  PointerToRawData;
+    UINT32  PointerToRelocations;
+    UINT32  PointerToLinenumbers;
+    UINT16  NumberOfRelocations;
+    UINT16  NumberOfLinenumbers;
+    UINT32  Characteristics;
+} __attribute__((packed));
+
+const void *__init pe_find_section(const CHAR8 *image, const UINTN image_size,
+                                   const char *section_name, UINTN *size_out)
+{
+    const struct DosFileHeader *dos = (const void*)image;
+    const struct PeHeader *pe;
+    const struct PeSectionHeader *sect;
+    const UINTN name_len = strlen(section_name);
+    UINTN offset = 0;
+    UINTN i;
+
+    if ( name_len > sizeof(sect->Name) ||
+         image_size < sizeof(*dos) ||
+         memcmp(dos->Magic, "MZ", 2) != 0 )
+        return NULL;
+
+    offset = dos->ExeHeader;
+    pe = (const void *) &image[offset];
+
+    offset += sizeof(*pe);
+    if ( image_size < offset ||
+         memcmp(pe->Magic, "PE\0\0", 4) != 0 )
+        return NULL;
+
+    /* PE32+ Subsystem type */
+#if defined(__arm__) || defined (__aarch64__)
+    if ( pe->FileHeader.Machine != PE_HEADER_MACHINE_ARM64 )
+        return NULL;
+#elif defined(__x86_64__)
+    if ( pe->FileHeader.Machine != PE_HEADER_MACHINE_X64 )
+        return NULL;
+#else
+    /* unknown architecture */
+    return NULL;
+#endif
+
+    offset += pe->FileHeader.SizeOfOptionalHeader;
+
+    for ( i = 0; i < pe->FileHeader.NumberOfSections; i++ )
+    {
+        sect = (const void *)&image[offset];
+        if ( image_size < offset + sizeof(*sect) )
+            return NULL;
+
+        if ( memcmp(sect->Name, section_name, name_len) != 0 ||
+             image_size < sect->VirtualSize + sect->VirtualAddress )
+        {
+            offset += sizeof(*sect);
+            continue;
+        }
+
+        if ( size_out )
+            *size_out = sect->VirtualSize;
+
+        return &image[sect->VirtualAddress];
+    }
+
+    return NULL;
+}
-- 
2.25.1

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

[Trammell Hudson]

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

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

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

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 4b1fbc9643..e65c1f1a09 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -949,6 +949,39 @@ static void __init setup_efi_pci(void)
     efi_bs->FreePool(handles);
 }
 
+/*
+ * Logic should remain sync'ed with linux/arch/x86/xen/efi.c
+ * Secure Boot is enabled iff 'SecureBoot' is set and the system is
+ * not in Setup Mode.
+ */
+static bool __init efi_secure_boot(void)
+{
+    static const __initconst EFI_GUID global_guid = EFI_GLOBAL_VARIABLE;
+    uint8_t secboot, setupmode;
+    UINTN secboot_size = sizeof(secboot);
+    UINTN setupmode_size = sizeof(setupmode);
+    EFI_STATUS rc;
+
+    rc = efi_rs->GetVariable(L"SecureBoot", (EFI_GUID *)&global_guid,
+                             NULL, &secboot_size, &secboot);
+    if ( rc != EFI_SUCCESS )
+        return false;
+
+    rc = efi_rs->GetVariable(L"SetupMode", (EFI_GUID *)&global_guid,
+                             NULL, &setupmode_size, &setupmode);
+    if ( rc != EFI_SUCCESS )
+        return false;
+
+    if ( secboot > 1)
+    {
+        PrintStr(L"Invalid SecureBoot variable=0x");
+        DisplayUint(secboot, 2);
+        PrintStr(newline);
+    }
+
+    return secboot == 1 && setupmode == 0;
+}
+
 static void __init efi_variables(void)
 {
     EFI_STATUS status;
@@ -1125,8 +1158,8 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
     static EFI_GUID __initdata shim_lock_guid = SHIM_LOCK_PROTOCOL_GUID;
     EFI_LOADED_IMAGE *loaded_image;
     EFI_STATUS status;
-    unsigned int i, argc;
-    CHAR16 **argv, *file_name, *cfg_file_name = NULL, *options = NULL;
+    unsigned int i, argc = 0;
+    CHAR16 **argv = NULL, *file_name, *cfg_file_name = NULL, *options = NULL;
     UINTN gop_mode = ~0;
     EFI_SHIM_LOCK_PROTOCOL *shim_lock;
     EFI_GRAPHICS_OUTPUT_PROTOCOL *gop = NULL;
@@ -1134,6 +1167,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
     bool base_video = false;
     char *option_str;
     bool use_cfg_file;
+    bool secure = false;
 
     __set_bit(EFI_BOOT, &efi_flags);
     __set_bit(EFI_LOADER, &efi_flags);
@@ -1152,8 +1186,10 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
         PrintErrMesg(L"No Loaded Image Protocol", status);
 
     efi_arch_load_addr_check(loaded_image);
+    secure = efi_secure_boot();
 
-    if ( use_cfg_file )
+    /* If UEFI Secure Boot is enabled, do not parse the command line */
+    if ( use_cfg_file && !secure )
     {
         UINTN offset = 0;
 
@@ -1211,6 +1247,8 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
 
     PrintStr(L"Xen " __stringify(XEN_VERSION) "." __stringify(XEN_SUBVERSION)
              XEN_EXTRAVERSION " (c/s " XEN_CHANGESET ") EFI loader\r\n");
+    if ( secure )
+        PrintStr(L"UEFI Secure Boot enabled\r\n");
 
     efi_arch_relocate_image(0);
 
-- 
2.25.1

Re: [PATCH v4 1/4] efi/boot.c: add file.need_to_free

[Roger Pau Monné]

On Mon, Sep 14, 2020 at 07:50:10AM -0400, Trammell Hudson wrote:
> The config file, kernel, initrd, etc should only be freed if they
> are allocated with the UEFI allocator.
> 
> Signed-off-by: Trammell Hudson <hudson@trmm.net>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

> ---
>  xen/common/efi/boot.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index 4022a672c9..7156139174 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -102,6 +102,7 @@ union string {
>  
>  struct file {
>      UINTN size;
> +    bool need_to_free;
>      union {
>          EFI_PHYSICAL_ADDRESS addr;
>          void *ptr;
> @@ -279,13 +280,13 @@ void __init noreturn blexit(const CHAR16 *str)
>      if ( !efi_bs )
>          efi_arch_halt();
>  
> -    if ( cfg.addr )
> +    if ( cfg.addr && cfg.need_to_free )
>          efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
> -    if ( kernel.addr )
> +    if ( kernel.addr && kernel.need_to_free )
>          efi_bs->FreePages(kernel.addr, PFN_UP(kernel.size));
> -    if ( ramdisk.addr )
> +    if ( ramdisk.addr && ramdisk.need_to_free )
>          efi_bs->FreePages(ramdisk.addr, PFN_UP(ramdisk.size));
> -    if ( xsm.addr )
> +    if ( xsm.addr && xsm.need_to_free )
>          efi_bs->FreePages(xsm.addr, PFN_UP(xsm.size));
>  
>      efi_arch_blexit();
> @@ -572,6 +573,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
>                           HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START);
>          ret = efi_bs->AllocatePages(AllocateMaxAddress, EfiLoaderData,
>                                      PFN_UP(size), &file->addr);
> +        file->need_to_free = true;

Strictly speaking, don't you need to set need_to_free only if
AllocatePages has succeed? I guess it doesn't matter much because addr
would be zapped to 0 if allocation fails.

Thanks, Roger.

Re: [PATCH v4 2/4] efi/boot.c: add handle_file_info()

[Roger Pau Monné]

On Mon, Sep 14, 2020 at 07:50:11AM -0400, Trammell Hudson wrote:
> Add a separate function to display the address ranges used by
> the files and call `efi_arch_handle_module()` on the modules.
> 
> Signed-off-by: Trammell Hudson <hudson@trmm.net>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks!

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

[Roger Pau Monné]

On Mon, Sep 14, 2020 at 07:50:12AM -0400, Trammell Hudson wrote:
> This patch adds support for bundling the xen.efi hypervisor, the xen.cfg
> configuration file, the Linux kernel and initrd, as well as the XSM,
> and architectural specific files into a single "unified" EFI executable.
> This allows an administrator to update the components independently
> without requiring rebuilding xen, as well as to replace the components
> in an existing image.
> 
> The resulting EFI executable can be invoked directly from the UEFI Boot
> Manager, removing the need to use a separate loader like grub as well
> as removing dependencies on local filesystem access.  And since it is
> a single file, it can be signed and validated by UEFI Secure Boot without
> requring the shim protocol.
> 
> It is inspired by systemd-boot's unified kernel technique and borrows the
> function to locate PE sections from systemd's LGPL'ed code.  During EFI
> boot, Xen looks at its own loaded image to locate the PE sections for
> the Xen configuration (`.config`), dom0 kernel (`.kernel`), dom0 initrd
> (`.ramdisk`), and XSM config (`.xsm`), which are included after building
> xen.efi using objcopy to add named sections for each input file.
> 
> For x86, the CPU ucode can be included in a section named `.ucode`,
> which is loaded in the efi_arch_cfg_file_late() stage of the boot process.
> 
> On ARM systems the Device Tree can be included in a section named
> `.dtb`, which is loaded during the efi_arch_cfg_file_early() stage of
> the boot process.
> 
> Signed-off-by: Trammell Hudson <hudson@trmm.net>

Thanks, just have one comment and two style nits.

> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index 57df89cacb..4b1fbc9643 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -121,6 +121,8 @@ static CHAR16 *s2w(union string *str);
>  static char *w2s(const union string *str);
>  static bool read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
>                        struct file *file, char *options);
> +static bool read_section(const EFI_LOADED_IMAGE *image,
> +                         char *name, struct file *file, char *options);
>  static size_t wstrlen(const CHAR16 * s);
>  static int set_color(u32 mask, int bpp, u8 *pos, u8 *sz);
>  static bool match_guid(const EFI_GUID *guid1, const EFI_GUID *guid2);
> @@ -623,6 +625,27 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
>      return true;
>  }
>  
> +static bool __init read_section(const EFI_LOADED_IMAGE *image,
> +                                char *const name, struct file *file,
> +                                char *options)
> +{
> +    /* skip the leading "." in the section name */
> +    union string name_string = { .s = name + 1 };
> +
> +    file->ptr = (void *)pe_find_section(image->ImageBase, image->ImageSize,
> +                                        name, &file->size);
> +    if ( !file->ptr )
> +        return false;
> +
> +    file->need_to_free = false;
> +
> +    s2w(&name_string);

Don't you need to check that s2w succeed, so that name_string.w is not
a random pointer from stack garbage?

> +    handle_file_info(name_string.w, file, options);
> +    efi_bs->FreePool(name_string.w);
> +
> +    return true;
> +}
> +
>  static void __init pre_parse(const struct file *cfg)
>  {
>      char *ptr = cfg->ptr, *end = ptr + cfg->size;
> index 4845d84913..c9b741bf27 100644
> --- a/xen/common/efi/efi.h
> +++ b/xen/common/efi/efi.h
> @@ -47,3 +47,6 @@ const CHAR16 *wmemchr(const CHAR16 *s, CHAR16 c, UINTN n);
>  /* EFI boot allocator. */
>  void *ebmalloc(size_t size);
>  void free_ebmalloc_unused_mem(void);
> +
> +const void * pe_find_section(const UINT8 *image_base, const size_t image_size,
> +        const char *section_name, UINTN *size_out);

Nit: extra space between * and function name.

> diff --git a/xen/common/efi/pe.c b/xen/common/efi/pe.c
> new file mode 100644
> index 0000000000..2986545d53
> --- /dev/null
> +++ b/xen/common/efi/pe.c
> @@ -0,0 +1,137 @@
> +/*
> + * xen/common/efi/pe.c
> + *
> + * PE executable header parser.
> + *
> + * Derived from https://github.com/systemd/systemd/blob/master/src/boot/efi/pe.c
> + * commit 07d5ed536ec0a76b08229c7a80b910cb9acaf6b1
> + *
> + * Copyright (C) 2015 Kay Sievers <kay@vrfy.org>
> + * Copyright (C) 2020 Trammell Hudson <hudson@trmm.net>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU Lesser General Public License as published by
> + * the Free Software Foundation; either version 2.1 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + */
> +
> +
> +#include "efi.h"
> +
> +struct DosFileHeader {
> +    UINT8   Magic[2];
> +    UINT16  LastSize;
> +    UINT16  nBlocks;
> +    UINT16  nReloc;
> +    UINT16  HdrSize;
> +    UINT16  MinAlloc;
> +    UINT16  MaxAlloc;
> +    UINT16  ss;
> +    UINT16  sp;
> +    UINT16  Checksum;
> +    UINT16  ip;
> +    UINT16  cs;
> +    UINT16  RelocPos;
> +    UINT16  nOverlay;
> +    UINT16  reserved[4];
> +    UINT16  OEMId;
> +    UINT16  OEMInfo;
> +    UINT16  reserved2[10];
> +    UINT32  ExeHeader;
> +} __attribute__((packed));
> +
> +#define PE_HEADER_MACHINE_ARM64         0xaa64
> +#define PE_HEADER_MACHINE_X64           0x8664
> +#define PE_HEADER_MACHINE_I386          0x014c
> +
> +struct PeFileHeader {
> +    UINT16  Machine;
> +    UINT16  NumberOfSections;
> +    UINT32  TimeDateStamp;
> +    UINT32  PointerToSymbolTable;
> +    UINT32  NumberOfSymbols;
> +    UINT16  SizeOfOptionalHeader;
> +    UINT16  Characteristics;
> +} __attribute__((packed));
> +
> +struct PeHeader {
> +    UINT8   Magic[4];
> +    struct PeFileHeader FileHeader;
> +} __attribute__((packed));
> +
> +struct PeSectionHeader {
> +    UINT8   Name[8];
> +    UINT32  VirtualSize;
> +    UINT32  VirtualAddress;
> +    UINT32  SizeOfRawData;
> +    UINT32  PointerToRawData;
> +    UINT32  PointerToRelocations;
> +    UINT32  PointerToLinenumbers;
> +    UINT16  NumberOfRelocations;
> +    UINT16  NumberOfLinenumbers;
> +    UINT32  Characteristics;
> +} __attribute__((packed));
> +
> +const void *__init pe_find_section(const CHAR8 *image, const UINTN image_size,
> +                                   const char *section_name, UINTN *size_out)
> +{
> +    const struct DosFileHeader *dos = (const void*)image;

Nit: missing space between void and *.

Thanks, Roger.

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

[Roger Pau Monné]

On Mon, Sep 14, 2020 at 07:50:13AM -0400, Trammell Hudson wrote:
> If secure boot is enabled, the Xen command line arguments are ignored.
> If a unified Xen image is used, then the bundled configuration, dom0
> kernel, and initrd are prefered over the ones listed in the config file.

I understand that you must ignore the cfg option when using the
bundled image, but is there then an alternative way for passing the
basevideo and mapbs parameters?

Or there's simply no way of doing so when using secure boot with a
bundled image?

> Unlike the shim based verification, the PE signature on a unified image
> covers the all of the Xen+config+kernel+initrd modules linked into the

Extra 'the'.

> unified image. This also ensures that properly configured platforms
> will measure the entire runtime into the TPM for unsealing secrets or
> remote attestation.
> 
> Signed-off-by: Trammell Hudson <hudson@trmm.net>
> ---
>  xen/common/efi/boot.c | 44 ++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 41 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index 4b1fbc9643..e65c1f1a09 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -949,6 +949,39 @@ static void __init setup_efi_pci(void)
>      efi_bs->FreePool(handles);
>  }
>  
> +/*
> + * Logic should remain sync'ed with linux/arch/x86/xen/efi.c
> + * Secure Boot is enabled iff 'SecureBoot' is set and the system is
> + * not in Setup Mode.
> + */
> +static bool __init efi_secure_boot(void)
> +{
> +    static const __initconst EFI_GUID global_guid = EFI_GLOBAL_VARIABLE;
> +    uint8_t secboot, setupmode;
> +    UINTN secboot_size = sizeof(secboot);
> +    UINTN setupmode_size = sizeof(setupmode);
> +    EFI_STATUS rc;
> +
> +    rc = efi_rs->GetVariable(L"SecureBoot", (EFI_GUID *)&global_guid,
> +                             NULL, &secboot_size, &secboot);
> +    if ( rc != EFI_SUCCESS )
> +        return false;
> +
> +    rc = efi_rs->GetVariable(L"SetupMode", (EFI_GUID *)&global_guid,
> +                             NULL, &setupmode_size, &setupmode);
> +    if ( rc != EFI_SUCCESS )
> +        return false;
> +
> +    if ( secboot > 1)

Nit: missing space before closing parentheses.

> +    {
> +        PrintStr(L"Invalid SecureBoot variable=0x");
> +        DisplayUint(secboot, 2);

Maybe better to use secboot_size * 2 here since you already have the
size of the variable anyway?

Thanks, Roger.

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

[Trammell Hudson]

On Wednesday, September 16, 2020 3:32 AM, Roger Pau Monné <roger.pau@citrix.com> wrote:
> On Mon, Sep 14, 2020 at 07:50:12AM -0400, Trammell Hudson wrote:
> > -   s2w(&name_string);
>
> Don't you need to check that s2w succeed, so that name_string.w is not
> a random pointer from stack garbage?

Maybe? I don't see anywhere else in the code that s2w() is
ever checked for a NULL return. Perhaps a better fix would
be to modify the function to panic if it is unable
to allocate.


> > -          const char *section_name, UINTN *size_out);
>
> Nit: extra space between * and function name.

Ok.  Both will be fixed in a v5.

--
Trammell

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

[Trammell Hudson]

On Wednesday, September 16, 2020 3:45 AM, Roger Pau Monné <roger.pau@citrix.com> wrote:
> On Mon, Sep 14, 2020 at 07:50:13AM -0400, Trammell Hudson wrote:
> > If secure boot is enabled, the Xen command line arguments are ignored.
> > If a unified Xen image is used, then the bundled configuration, dom0
> > kernel, and initrd are prefered over the ones listed in the config file.
>
> I understand that you must ignore the cfg option when using the
> bundled image, but is there then an alternative way for passing the
> basevideo and mapbs parameters?

The cfg option will be ignored regardless since a bundled config
(or kernel, ramdisk, etc) takes precedence over any files,
so perhaps parsing the command line is not as much of a risk
as initially thought.

The concern is that *any* non-signed configuration values are
potentially a risk, even if we don't see exactly how the attacker
can use them right now. Especially if an option is added later
and we haven't thought about the security ramifications of it.

> Or there's simply no way of doing so when using secure boot with a
> bundled image?

Should these options be available in the config file instead?
That way the system owner can sign the configuration and ensure
that an adversary can't change them.

> > Unlike the shim based verification, the PE signature on a unified image
> > covers the all of the Xen+config+kernel+initrd modules linked into the
>
> Extra 'the'.

Fixed, along with the style issues in upcoming v5.

--
Trammell

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

[Jan Beulich]

On 16.09.2020 10:37, Trammell Hudson wrote:
> On Wednesday, September 16, 2020 3:32 AM, Roger Pau Monné <roger.pau@citrix.com> wrote:
>> On Mon, Sep 14, 2020 at 07:50:12AM -0400, Trammell Hudson wrote:
>>> -   s2w(&name_string);
>>
>> Don't you need to check that s2w succeed, so that name_string.w is not
>> a random pointer from stack garbage?
> 
> Maybe? I don't see anywhere else in the code that s2w() is
> ever checked for a NULL return. Perhaps a better fix would
> be to modify the function to panic if it is unable
> to allocate.

In current code the result of s2w() gets exclusively passed to
read_file(), where first thing we have

    if ( !name )
        PrintErrMesg(L"No filename", EFI_OUT_OF_RESOURCES);

Jan

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

[Jan Beulich]

On 16.09.2020 10:50, Trammell Hudson wrote:
> On Wednesday, September 16, 2020 3:45 AM, Roger Pau Monné <roger.pau@citrix.com> wrote:
>> On Mon, Sep 14, 2020 at 07:50:13AM -0400, Trammell Hudson wrote:
>>> If secure boot is enabled, the Xen command line arguments are ignored.
>>> If a unified Xen image is used, then the bundled configuration, dom0
>>> kernel, and initrd are prefered over the ones listed in the config file.
>>
>> I understand that you must ignore the cfg option when using the
>> bundled image, but is there then an alternative way for passing the
>> basevideo and mapbs parameters?
> 
> The cfg option will be ignored regardless since a bundled config
> (or kernel, ramdisk, etc) takes precedence over any files,
> so perhaps parsing the command line is not as much of a risk
> as initially thought.
> 
> The concern is that *any* non-signed configuration values are
> potentially a risk, even if we don't see exactly how the attacker
> can use them right now. Especially if an option is added later
> and we haven't thought about the security ramifications of it.
> 
>> Or there's simply no way of doing so when using secure boot with a
>> bundled image?
> 
> Should these options be available in the config file instead?
> That way the system owner can sign the configuration and ensure
> that an adversary can't change them.

Not really, no. /basevideo needs evaluating very early in any event,
before any (regular) output gets produced. /mapbs could be parsed
later, but the early boot code intentionally does not make any
attempt at parsing the command line options designated for the
common parts of xen.gz and xen.efi. Yet the map_bs variable has one
use in early boot code (i.e. before handing over to __start_xen()).

Jan

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

[Roger Pau Monné]

On Wed, Sep 16, 2020 at 08:37:44AM +0000, Trammell Hudson wrote:
> On Wednesday, September 16, 2020 3:32 AM, Roger Pau Monné <roger.pau@citrix.com> wrote:
> > On Mon, Sep 14, 2020 at 07:50:12AM -0400, Trammell Hudson wrote:
> > > -   s2w(&name_string);
> >
> > Don't you need to check that s2w succeed, so that name_string.w is not
> > a random pointer from stack garbage?
> 
> Maybe? I don't see anywhere else in the code that s2w() is
> ever checked for a NULL return.

I see some callers pass the return of s2w() straight into read_file
which will check that's not NULL before proceeding, or else call
PrintErrMesg which won't return.

> Perhaps a better fix would
> be to modify the function to panic if it is unable
> to allocate.

Just doing what read_file does and use PrintErrMesg seems fine to me.

Roger.

Re: [PATCH v4 1/4] efi/boot.c: add file.need_to_free

[Jan Beulich]

On 16.09.2020 08:43, Roger Pau Monné wrote:
> On Mon, Sep 14, 2020 at 07:50:10AM -0400, Trammell Hudson wrote:
>> @@ -279,13 +280,13 @@ void __init noreturn blexit(const CHAR16 *str)
>>      if ( !efi_bs )
>>          efi_arch_halt();
>>  
>> -    if ( cfg.addr )
>> +    if ( cfg.addr && cfg.need_to_free )
>>          efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
>> -    if ( kernel.addr )
>> +    if ( kernel.addr && kernel.need_to_free )
>>          efi_bs->FreePages(kernel.addr, PFN_UP(kernel.size));
>> -    if ( ramdisk.addr )
>> +    if ( ramdisk.addr && ramdisk.need_to_free )
>>          efi_bs->FreePages(ramdisk.addr, PFN_UP(ramdisk.size));
>> -    if ( xsm.addr )
>> +    if ( xsm.addr && xsm.need_to_free )
>>          efi_bs->FreePages(xsm.addr, PFN_UP(xsm.size));

All these look to be able to become just "if ( xyz.need_to_free )"
if ...

>> @@ -572,6 +573,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
>>                           HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START);
>>          ret = efi_bs->AllocatePages(AllocateMaxAddress, EfiLoaderData,
>>                                      PFN_UP(size), &file->addr);
>> +        file->need_to_free = true;
> 
> Strictly speaking, don't you need to set need_to_free only if
> AllocatePages has succeed?

... this was followed, so I think the adjustment wants making.

> I guess it doesn't matter much because addr
> would be zapped to 0 if allocation fails.

Perhaps this zapping then also becomes unnecessary, albeit I
didn't look very closely yet.

Jan

Re: [PATCH v4 2/4] efi/boot.c: add handle_file_info()

[Jan Beulich]

On 14.09.2020 13:50, Trammell Hudson wrote:
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -539,6 +539,22 @@ static char * __init split_string(char *s)
>      return NULL;
>  }
>  
> +static void __init handle_file_info(CHAR16 *name,
> +                                    struct file *file, char *options)

The latter two can become const once rebased over the patch I've
just sent, and then
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

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

[Jan Beulich]

On 14.09.2020 13:50, Trammell Hudson wrote:
> --- a/docs/misc/efi.pandoc
> +++ b/docs/misc/efi.pandoc
> @@ -116,3 +116,52 @@ Filenames must be specified relative to the location of the EFI binary.
>  
>  Extra options to be passed to Xen can also be specified on the command line,
>  following a `--` separator option.
> +
> +## Unified Xen kernel image
> +
> +The "Unified" kernel image can be generated by adding additional
> +sections to the Xen EFI executable with objcopy, similar to how
> +[systemd-boot uses the stub to add them to the Linux kernel](https://wiki.archlinux.org/index.php/systemd-boot#Preparing_a_unified_kernel_image)
> +
> +The sections for the xen configuration file, the dom0 kernel, dom0 initrd,
> +XSM and CPU microcode should be added after the Xen `.pad` section, the
> +ending address of which can be located with:
> +
> +```
> +objdump -h xen.efi \
> +	| perl -ane '/\.pad/ && printf "0x%016x\n", hex($F[2]) + hex($F[3])'
> +```
> +
> +For all the examples the `.pad` section ended at 0xffff82d041000000.
> +All the sections are optional (`.config`, `.kernel`, `.ramdisk`, `.xsm`,
> +`.ucode` (x86) and `.dtb` (ARM)) and the order does not matter.
> +The virtual addresses do not need to be contiguous, although they should not
> +be overlapping and should all be greater than the last virtual address of the
> +hypervisor components.

The .pad section is there really only for padding the image. Its space
could in principle be used for placing useful stuff (and hence to limit
overall in-memory image size). That said, there is a plan for a change
which may involve using the initial part of .pad, but that's not certain
yet. I'm pointing this out to clarify that there may be a valid reason
to avoid re-using the .pad space, at least for now.

> --- a/xen/arch/arm/efi/efi-boot.h
> +++ b/xen/arch/arm/efi/efi-boot.h
> @@ -375,27 +375,36 @@ static void __init noreturn efi_arch_post_exit_boot(void)
>      efi_xen_start(fdt, fdt_totalsize(fdt));
>  }
>  
> -static void __init efi_arch_cfg_file_early(EFI_FILE_HANDLE dir_handle, char *section)
> +static void __init efi_arch_cfg_file_early(const EFI_LOADED_IMAGE *image,
> +                                           EFI_FILE_HANDLE dir_handle,
> +                                           char *section)

Could I talk you into constifying "section" at this occasion - afaics
there should be no fallout here or in the other three places where
the same would apply.

> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -272,14 +272,21 @@ static void __init noreturn efi_arch_post_exit_boot(void)
>      unreachable();
>  }
>  
> -static void __init efi_arch_cfg_file_early(EFI_FILE_HANDLE dir_handle, char *section)
> +static void __init efi_arch_cfg_file_early(const EFI_LOADED_IMAGE *image,
> +                                           EFI_FILE_HANDLE dir_handle,
> +                                           char *section)
>  {
>  }
>  
> -static void __init efi_arch_cfg_file_late(EFI_FILE_HANDLE dir_handle, char *section)
> +static void __init efi_arch_cfg_file_late(const EFI_LOADED_IMAGE *image,
> +                                          EFI_FILE_HANDLE dir_handle,
> +                                          char *section)
>  {
>      union string name;
>  
> +    if ( read_section(image, ".ucode", &ucode, NULL) )
> +        return;
> +
>      name.s = get_value(&cfg, section, "ucode");

With the Arm change already in mind and with further similar
changes further down, may I suggest to consider passing
'section' into read_section(), thus guaranteeing consistent
naming between image section and config file items, not only now
but also going forward? read_section() would then check for the
leading dot followed by the specified name.

> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -121,6 +121,8 @@ static CHAR16 *s2w(union string *str);
>  static char *w2s(const union string *str);
>  static bool read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
>                        struct file *file, char *options);
> +static bool read_section(const EFI_LOADED_IMAGE *image,
> +                         char *name, struct file *file, char *options);
>  static size_t wstrlen(const CHAR16 * s);
>  static int set_color(u32 mask, int bpp, u8 *pos, u8 *sz);
>  static bool match_guid(const EFI_GUID *guid1, const EFI_GUID *guid2);
> @@ -623,6 +625,27 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
>      return true;
>  }
>  
> +static bool __init read_section(const EFI_LOADED_IMAGE *image,
> +                                char *const name, struct file *file,
> +                                char *options)
> +{
> +    /* skip the leading "." in the section name */
> +    union string name_string = { .s = name + 1 };
> +
> +    file->ptr = (void *)pe_find_section(image->ImageBase, image->ImageSize,
> +                                        name, &file->size);

This casting away of const-ness worries me. The sole reason why
the "ptr" member is non-const looks to be the two parsing functions
for the config file. How about we make "ptr" const void * and add a
new "char *str" field? While it won't _guarantee_ correct code to
be written, it at least allows doing so.

> @@ -1207,9 +1230,13 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>          /* Get the file system interface. */
>          dir_handle = get_parent_handle(loaded_image, &file_name);
>  
> -        /* Read and parse the config file. */
> -        if ( !cfg_file_name )
> +        if ( read_section(loaded_image, ".config", &cfg, NULL) )
> +        {
> +            PrintStr(L"Using unified config file\r\n");
> +        }

Please omit the braces here.

> @@ -1258,29 +1285,39 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>          if ( !name.s )
>              blexit(L"No Dom0 kernel image specified.");
>  
> -        efi_arch_cfg_file_early(dir_handle, section.s);
> +        efi_arch_cfg_file_early(loaded_image, dir_handle, section.s);
>  
>          option_str = split_string(name.s);
> -        read_file(dir_handle, s2w(&name), &kernel, option_str);
> -        efi_bs->FreePool(name.w);
> -
> -        if ( !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL,
> -                        (void **)&shim_lock)) &&
> -             (status = shim_lock->Verify(kernel.ptr, kernel.size)) != EFI_SUCCESS )
> -            PrintErrMesg(L"Dom0 kernel image could not be verified", status);
>  
> -        name.s = get_value(&cfg, section.s, "ramdisk");
> -        if ( name.s )
> +        if ( !read_section(loaded_image, ".kernel", &kernel, option_str) )
>          {
> -            read_file(dir_handle, s2w(&name), &ramdisk, NULL);
> +            read_file(dir_handle, s2w(&name), &kernel, option_str);

As before, I disagree with the idea of taking pieces from disk and
pieces from the unified image. If you continue to think this is a
reasonable thing to do, may I ask that you add a rationale of this
model to the description? And btw, my worry looks to not be without
reason, since ...

>              efi_bs->FreePool(name.w);
> +
> +            if ( !EFI_ERROR(efi_bs->LocateProtocol(&shim_lock_guid, NULL,
> +                            (void **)&shim_lock)) &&
> +                 (status = shim_lock->Verify(kernel.ptr, kernel.size)) != EFI_SUCCESS )
> +                PrintErrMesg(L"Dom0 kernel image could not be verified", status);
>          }
>  
> -        name.s = get_value(&cfg, section.s, "xsm");
> -        if ( name.s )
> +        if ( !read_section(loaded_image, ".ramdisk", &ramdisk, NULL) )
>          {
> -            read_file(dir_handle, s2w(&name), &xsm, NULL);
> -            efi_bs->FreePool(name.w);
> +            name.s = get_value(&cfg, section.s, "ramdisk");
> +            if ( name.s )
> +            {
> +                read_file(dir_handle, s2w(&name), &ramdisk, NULL);
> +                efi_bs->FreePool(name.w);
> +            }
> +        }

... the RAM disk (just taken as example) is optional. How do you express
the unified image's explicit intention to have no RAM disk with this
fallback approach? FAOD, even if objcopy allows to add empty sections
(didn't check), I'd consider an empty section different from an absent
one, i.e. the former meaning "empty RAM disk" while the latter says "no
RAM disk".

> +        if ( !read_section(loaded_image, ".xsm", &xsm, NULL) )
> +        {
> +            name.s = get_value(&cfg, section.s, "xsm");
> +            if ( name.s )
> +            {
> +                read_file(dir_handle, s2w(&name), &xsm, NULL);
> +                efi_bs->FreePool(name.w);
> +            }
>          }

The fallback approach may even have security implications here, as
(afaik) an XSM policy can also be used to increase privileges. The
builder of unified image, in omitting an XSM policy, may certainly
mean "use built-in defaults" rather than intending to allow further
overriding.

> --- /dev/null
> +++ b/xen/common/efi/pe.c
> @@ -0,0 +1,137 @@
> +/*
> + * xen/common/efi/pe.c
> + *
> + * PE executable header parser.
> + *
> + * Derived from https://github.com/systemd/systemd/blob/master/src/boot/efi/pe.c
> + * commit 07d5ed536ec0a76b08229c7a80b910cb9acaf6b1
> + *
> + * Copyright (C) 2015 Kay Sievers <kay@vrfy.org>
> + * Copyright (C) 2020 Trammell Hudson <hudson@trmm.net>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU Lesser General Public License as published by
> + * the Free Software Foundation; either version 2.1 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + */
> +
> +
> +#include "efi.h"
> +
> +struct DosFileHeader {
> +    UINT8   Magic[2];
> +    UINT16  LastSize;
> +    UINT16  nBlocks;
> +    UINT16  nReloc;
> +    UINT16  HdrSize;
> +    UINT16  MinAlloc;
> +    UINT16  MaxAlloc;
> +    UINT16  ss;
> +    UINT16  sp;
> +    UINT16  Checksum;
> +    UINT16  ip;
> +    UINT16  cs;
> +    UINT16  RelocPos;
> +    UINT16  nOverlay;
> +    UINT16  reserved[4];
> +    UINT16  OEMId;
> +    UINT16  OEMInfo;
> +    UINT16  reserved2[10];
> +    UINT32  ExeHeader;
> +} __attribute__((packed));
> +
> +#define PE_HEADER_MACHINE_ARM64         0xaa64
> +#define PE_HEADER_MACHINE_X64           0x8664
> +#define PE_HEADER_MACHINE_I386          0x014c

This list isn't meant to be a complete one anyway, so please omit
the I386 item as it's not needed anywhere.

> +struct PeFileHeader {
> +    UINT16  Machine;
> +    UINT16  NumberOfSections;
> +    UINT32  TimeDateStamp;
> +    UINT32  PointerToSymbolTable;
> +    UINT32  NumberOfSymbols;
> +    UINT16  SizeOfOptionalHeader;
> +    UINT16  Characteristics;
> +} __attribute__((packed));
> +
> +struct PeHeader {
> +    UINT8   Magic[4];
> +    struct PeFileHeader FileHeader;
> +} __attribute__((packed));

At the example of these two (i.e. may extend to others): When the
packed attribute doesn't really have any impact on structure layout
(and will just adversely affect alignof() when applied to the struct
or any of the fields), please omit it. We had a number of such
pointless attributes in the tree, and we had to drop them for one of
the more recent gcc versions to actually still compile our code
without warnings (in fact errors, due to out use of -Werror).

> +struct PeSectionHeader {
> +    UINT8   Name[8];

Better char?

> +const void *__init pe_find_section(const CHAR8 *image, const UINTN image_size,
> +                                   const char *section_name, UINTN *size_out)
> +{
> +    const struct DosFileHeader *dos = (const void*)image;

If the type of "image" was "const void *", there wouldn't be any cast
needed here (and again further down). And I don't think you actually
need "image" to be a pointer to a particular type? Of course ...

> +    const struct PeHeader *pe;
> +    const struct PeSectionHeader *sect;
> +    const UINTN name_len = strlen(section_name);
> +    UINTN offset = 0;

Unnecessary initializer, and please fold ...

> +    UINTN i;

... with this line.

> +    if ( name_len > sizeof(sect->Name) ||
> +         image_size < sizeof(*dos) ||
> +         memcmp(dos->Magic, "MZ", 2) != 0 )
> +        return NULL;
> +
> +    offset = dos->ExeHeader;
> +    pe = (const void *) &image[offset];

... this then needs to become "image + offset".

> +
> +    offset += sizeof(*pe);
> +    if ( image_size < offset ||
> +         memcmp(pe->Magic, "PE\0\0", 4) != 0 )
> +        return NULL;
> +
> +    /* PE32+ Subsystem type */
> +#if defined(__arm__) || defined (__aarch64__)
> +    if ( pe->FileHeader.Machine != PE_HEADER_MACHINE_ARM64 )
> +        return NULL;
> +#elif defined(__x86_64__)
> +    if ( pe->FileHeader.Machine != PE_HEADER_MACHINE_X64 )
> +        return NULL;
> +#else
> +    /* unknown architecture */
> +    return NULL;
> +#endif

Instead of this, further up please #define a single constant (e.g.
PE_HEADER_MACHINE) to check against without any #ifdef-ary here.
This then also should lead to a build error (instead of the
function returning NULL at runtime) when no enabling was done for
a possible future port.

> +    offset += pe->FileHeader.SizeOfOptionalHeader;
> +
> +    for ( i = 0; i < pe->FileHeader.NumberOfSections; i++ )
> +    {
> +        sect = (const void *)&image[offset];

Please limit the scope of sect to the body of this loop, at which
point this assignment can become the initializer.

> +        if ( image_size < offset + sizeof(*sect) )
> +            return NULL;
> +
> +        if ( memcmp(sect->Name, section_name, name_len) != 0 ||
> +             image_size < sect->VirtualSize + sect->VirtualAddress )

Wouldn't this latter part of the condition better be treated as an
error, rather than getting silently ignored? The more if the falling
back to on-disk files got retained?

Jan

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

[Jan Beulich]

On 14.09.2020 13:50, Trammell Hudson wrote:
> If secure boot is enabled, the Xen command line arguments are ignored.
> If a unified Xen image is used, then the bundled configuration, dom0
> kernel, and initrd are prefered over the ones listed in the config file.
> 
> Unlike the shim based verification, the PE signature on a unified image
> covers the all of the Xen+config+kernel+initrd modules linked into the
> unified image. This also ensures that properly configured platforms
> will measure the entire runtime into the TPM for unsealing secrets or
> remote attestation.

The command line may also include a part handed on to the Dom0 kernel.
If the Dom0 kernel image comes from disk, I don't see why that part of
the command line shouldn't be honored. Similarly, if the config file
doesn't come from the unified image, I think Xen's command line options
should also be honored.

> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -949,6 +949,39 @@ static void __init setup_efi_pci(void)
>      efi_bs->FreePool(handles);
>  }
>  
> +/*
> + * Logic should remain sync'ed with linux/arch/x86/xen/efi.c
> + * Secure Boot is enabled iff 'SecureBoot' is set and the system is
> + * not in Setup Mode.
> + */
> +static bool __init efi_secure_boot(void)
> +{
> +    static const __initconst EFI_GUID global_guid = EFI_GLOBAL_VARIABLE;
> +    uint8_t secboot, setupmode;
> +    UINTN secboot_size = sizeof(secboot);
> +    UINTN setupmode_size = sizeof(setupmode);
> +    EFI_STATUS rc;
> +
> +    rc = efi_rs->GetVariable(L"SecureBoot", (EFI_GUID *)&global_guid,

As you need the casts here just to get rid of the const again,
please don't make the variable const in the first place.

> +                             NULL, &secboot_size, &secboot);
> +    if ( rc != EFI_SUCCESS )
> +        return false;
> +
> +    rc = efi_rs->GetVariable(L"SetupMode", (EFI_GUID *)&global_guid,
> +                             NULL, &setupmode_size, &setupmode);
> +    if ( rc != EFI_SUCCESS )
> +        return false;
> +
> +    if ( secboot > 1)
> +    {
> +        PrintStr(L"Invalid SecureBoot variable=0x");
> +        DisplayUint(secboot, 2);
> +        PrintStr(newline);
> +    }
> +
> +    return secboot == 1 && setupmode == 0;

Like for SecureBoot, values other than 0 and 1 also are reserved for
SetupMode and hence would better be logged in the same way. I'm still
unconvinced though that logging is enough.

> @@ -1134,6 +1167,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>      bool base_video = false;
>      char *option_str;
>      bool use_cfg_file;
> +    bool secure = false;

I don't think the initializer is needed here?

> @@ -1152,8 +1186,10 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>          PrintErrMesg(L"No Loaded Image Protocol", status);
>  
>      efi_arch_load_addr_check(loaded_image);
> +    secure = efi_secure_boot();
>  
> -    if ( use_cfg_file )
> +    /* If UEFI Secure Boot is enabled, do not parse the command line */
> +    if ( use_cfg_file && !secure )
>      {

If it is intentional to also change this for the shim case, then
please justify the change in behavior in the description. As per
the comments further up I think the ignoring of the various parts
wants making depend on more than just secure boot mode anyway.

Jan

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

[Trammell Hudson]

On Thursday, September 17, 2020 8:33 AM, Jan Beulich <jbeulich@suse.com> wrote:
> On 14.09.2020 13:50, Trammell Hudson wrote:
> [...]
> > +For all the examples the `.pad` section ended at 0xffff82d041000000.
> > +All the sections are optional (`.config`, `.kernel`, `.ramdisk`, `.xsm`,
> > +`.ucode` (x86) and `.dtb` (ARM)) and the order does not matter.
> > +The virtual addresses do not need to be contiguous, although they should not
> > +be overlapping and should all be greater than the last virtual address of the
> > +hypervisor components.
>
> The .pad section is there really only for padding the image. Its space
> could in principle be used for placing useful stuff (and hence to limit
> overall in-memory image size). That said, there is a plan for a change
> which may involve using the initial part of .pad, but that's not certain
> yet. I'm pointing this out to clarify that there may be a valid reason
> to avoid re-using the .pad space, at least for now.

The instructions show how to append after the .pad region, so there is
no reuse of that space.  I wish objcopy had an --append-region option
so that the user didn't have to do this extra math and adjust sizes.

> > --- a/xen/arch/arm/efi/efi-boot.h
> > +++ b/xen/arch/arm/efi/efi-boot.h
> > @@ -375,27 +375,36 @@ static void __init noreturn efi_arch_post_exit_boot(void)
> > efi_xen_start(fdt, fdt_totalsize(fdt));
> > }
> > -static void __init efi_arch_cfg_file_early(EFI_FILE_HANDLE dir_handle, char *section)
> > +static void __init efi_arch_cfg_file_early(const EFI_LOADED_IMAGE *image,
> >
> > -                                             EFI_FILE_HANDLE dir_handle,
> >
> >
> > -                                             char *section)
> >
> >
>
> Could I talk you into constifying "section" at this occasion - afaics
> there should be no fallout here or in the other three places where
> the same would apply.

I'm always in favor of adding more constness.  Is it ok to have that
as part of this patch since we're changing the signature on the function?

> [...]
> > -   if ( read_section(image, ".ucode", &ucode, NULL) )
> > -          return;
> > -   name.s = get_value(&cfg, section, "ucode");
>
> With the Arm change already in mind and with further similar
> changes further down, may I suggest to consider passing
> 'section' into read_section(), thus guaranteeing consistent
> naming between image section and config file items, not only now
> but also going forward? read_section() would then check for the
> leading dot followed by the specified name.

That could work, I think.  Let me test it out for v5.

> [...]
> > -   file->ptr = (void *)pe_find_section(image->ImageBase, image->ImageSize,
> > -                                          name, &file->size);
>
> This casting away of const-ness worries me. The sole reason why
> the "ptr" member is non-const looks to be the two parsing functions
> for the config file. How about we make "ptr" const void * and add a
> new "char *str" field? While it won't guarantee correct code to
> be written, it at least allows doing so.

That's what I found in the const cleanup patch -- only the config file
parser needed to modify the contents.  It would potentially fail if someone
modified the build to include the config in a read-only text segment,
but they would find out fairly quickly that didn't work...

> [...]
> > -          if ( !read_section(loaded_image, ".kernel", &kernel, option_str) )
> > -              read_file(dir_handle, s2w(&name), &ramdisk, NULL);
> > -              read_file(dir_handle, s2w(&name), &kernel, option_str);
>
> As before, I disagree with the idea of taking pieces from disk and
> pieces from the unified image. If you continue to think this is a
> reasonable thing to do, may I ask that you add a rationale of this
> model to the description?

It allows distributions to continue with the status quo if they want a
signed kernel + config, but allow a user provided initrd (which is what
the shim protocol does today).  Qubes, for instance, has discussed that
as a path forward to allow a trusted kernel, while also allowing users
to rebuild their own initrd since it contains machine specific data.

The config is signed, so an attacker can not add any new lines to it.
So if the config file has no "ramdisk" or "xsm" line then get_value()
will return NULL and the read_file() will not be attempted.
If, however, the config file has an "ramdisk /boot/initrd.gz",
but not ".ramdisk" PE section, then that is an explicit statement
from the signer that they want to load that file from disk, even
though the initrd.gz is not included in the signature.

> > --- /dev/null
> > +++ b/xen/common/efi/pe.c
> > +#define PE_HEADER_MACHINE_ARM64 0xaa64
> > +#define PE_HEADER_MACHINE_X64 0x8664
> > +#define PE_HEADER_MACHINE_I386 0x014c
>
> This list isn't meant to be a complete one anyway, so please omit
> the I386 item as it's not needed anywhere.

Sure.  This file is almost verbatim from systemd-boot, so it has
a few things like that which are not relevant.

> > +struct PeFileHeader {
> > -   UINT16 Machine;
> > -   UINT16 NumberOfSections;
> > -   UINT32 TimeDateStamp;
> > -   UINT32 PointerToSymbolTable;
> > -   UINT32 NumberOfSymbols;
> > -   UINT16 SizeOfOptionalHeader;
> > -   UINT16 Characteristics;
> >     +} attribute((packed));
> >
> > +struct PeHeader {
> > -   UINT8 Magic[4];
> > -   struct PeFileHeader FileHeader;
> >     +} attribute((packed));
>
> At the example of these two (i.e. may extend to others): When the
> packed attribute doesn't really have any impact on structure layout
> (and will just adversely affect alignof() when applied to the struct
> or any of the fields), please omit it.

In this case the packed does not affect the layout, but if PeFileHeader
started with a UINT64, for instance, then padding would be added to
PeHeader to align it without the packed.

> > +struct PeSectionHeader {
> > -   UINT8 Name[8];
>
> Better char?

Maybe? I've heard that some programs use non-7bit ascii in there for things
that are not normal sections (and the names are compared with memcp()).

> > +const void *__init pe_find_section(const CHAR8 *image, const UINTN image_size,
> > -                                     const char *section_name, UINTN *size_out)
> > -   const struct DosFileHeader dos = (const void)image;
>
> If the type of "image" was "const void *", there wouldn't be any cast
> needed here (and again further down). And I don't think you actually
> need "image" to be a pointer to a particular type?

I don't think it needs to be any particular type (and CHAR* is a holdover
from the systemd-boot code).  However, there is quite a bit of pointer
math done on it that avoids casts:

pe = (const void *) &image[offset];

If image were void*, then this would have to be written as something like:

pe = (const void *)((uintptr_t)image + offset);

(unless the gnu extension that allows void* math is enabled)

> > -   const struct PeSectionHeader *sect;
> > -   if ( name_len > sizeof(sect->Name) ||

> > +#elif defined(x86_64)
> >
> > -   if ( pe->FileHeader.Machine != PE_HEADER_MACHINE_X64 )
> > -          return NULL;
> >
> >
> >
> > +#else
> >
> > -   /* unknown architecture */
> > -   return NULL;
> >     +#endif
> >
>
> Instead of this, further up please #define a single constant (e.g.
> PE_HEADER_MACHINE) to check against without any #ifdef-ary here.
> This then also should lead to a build error (instead of the
> function returning NULL at runtime) when no enabling was done for
> a possible future port.

Ok.

> > -   for ( i = 0; i < pe->FileHeader.NumberOfSections; i++ )
> > -   {
> > -          sect = (const void *)&image[offset];
> >
> >
>
> Please limit the scope of sect to the body of this loop, at which
> point this assignment can become the initializer.

sect was used earlier for sizeof math to validate the name.

It's also a little odd that the style guide calls for declaring variable
in limited scopes, while also disallowing for loop scoped variables.

> > -          if ( memcmp(sect->Name, section_name, name_len) != 0 ||
> > -               image_size < sect->VirtualSize + sect->VirtualAddress )
>
> Wouldn't this latter part of the condition better be treated as an
> error, rather than getting silently ignored? The more if the falling
> back to on-disk files got retained?

Possibly.  Since the signature has been validated on the entire image,
this would mean that the signer signed a bogus unified image for some
reason.  Probably should throw an error of some sort; wasn't sure if
it made sense to include that sort of panic behaviour this deep in the
code.

I'll pickup the style guide issues in v5 as well.

--
Trammell

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

[Trammell Hudson]

On Thursday, September 17, 2020 9:04 AM, Trammell Hudson <hudson@trmm.net> wrote:
> On Thursday, September 17, 2020 8:33 AM, Jan Beulich jbeulich@suse.com wrote:
> > [...]
> > > -   if ( read_section(image, ".ucode", &ucode, NULL) )
> > > -            return;
> > >
> > > -   name.s = get_value(&cfg, section, "ucode");
> >
> > With the Arm change already in mind and with further similar
> > changes further down, may I suggest to consider passing
> > 'section' into read_section(), thus guaranteeing consistent
> > naming between image section and config file items, not only now
> > but also going forward? read_section() would then check for the
> > leading dot followed by the specified name.
>
> That could work, I think. Let me test it out for v5.

Or maybe not. section is the "section-name" of the config file
that is being booted:

[global]
default=section-name

Meanwhile, read_section() wants the PE section name, like ".ucode", which might appear as a line item in that section.

--
Trammell

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

[Trammell Hudson]

On Thursday, September 17, 2020 8:51 AM, Jan Beulich <jbeulich@suse.com> wrote:
> On 14.09.2020 13:50, Trammell Hudson wrote:
> > If secure boot is enabled, the Xen command line arguments are ignored.
> > If a unified Xen image is used, then the bundled configuration, dom0
> > kernel, and initrd are prefered over the ones listed in the config file.
> > Unlike the shim based verification, the PE signature on a unified image
> > covers the all of the Xen+config+kernel+initrd modules linked into the
> > unified image. This also ensures that properly configured platforms
> > will measure the entire runtime into the TPM for unsealing secrets or
> > remote attestation.
>
> The command line may also include a part handed on to the Dom0 kernel.
> If the Dom0 kernel image comes from disk, I don't see why that part of
> the command line shouldn't be honored. Similarly, if the config file
> doesn't come from the unified image, I think Xen's command line options
> should also be honored.

Ignoring the command line and breaking the shim behaviour in a
unified image should be ok; that is an explicit decision by the
system owner to sign and configure the new image (and the shim
is not used in a unified image anyway).

If we have a way to detect a unified image early enough, then
we can avoid the backwards incompatibility if it is not unified.
That would require moving the config parsing to above the relocation call.  I'm testing that now to see if it works on x86.

--
Trammell

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

[Jan Beulich]

On 17.09.2020 15:33, Trammell Hudson wrote:
> On Thursday, September 17, 2020 9:04 AM, Trammell Hudson <hudson@trmm.net> wrote:
>> On Thursday, September 17, 2020 8:33 AM, Jan Beulich jbeulich@suse.com wrote:
>>> [...]
>>>> -   if ( read_section(image, ".ucode", &ucode, NULL) )
>>>> -            return;
>>>>
>>>> -   name.s = get_value(&cfg, section, "ucode");
>>>
>>> With the Arm change already in mind and with further similar
>>> changes further down, may I suggest to consider passing
>>> 'section' into read_section(), thus guaranteeing consistent
>>> naming between image section and config file items, not only now
>>> but also going forward? read_section() would then check for the
>>> leading dot followed by the specified name.
>>
>> That could work, I think. Let me test it out for v5.
> 
> Or maybe not. section is the "section-name" of the config file
> that is being booted:
> 
> [global]
> default=section-name
> 
> Meanwhile, read_section() wants the PE section name, like ".ucode", which might appear as a line item in that section.

Oh, yes - looking at just the code fragment left in context I
realize my comment was just rubbish. Sorry.

Jan

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

[Jan Beulich]

On 17.09.2020 15:04, Trammell Hudson wrote:
> On Thursday, September 17, 2020 8:33 AM, Jan Beulich <jbeulich@suse.com> wrote:
>> On 14.09.2020 13:50, Trammell Hudson wrote:
>> [...]
>>> +For all the examples the `.pad` section ended at 0xffff82d041000000.
>>> +All the sections are optional (`.config`, `.kernel`, `.ramdisk`, `.xsm`,
>>> +`.ucode` (x86) and `.dtb` (ARM)) and the order does not matter.
>>> +The virtual addresses do not need to be contiguous, although they should not
>>> +be overlapping and should all be greater than the last virtual address of the
>>> +hypervisor components.
>>
>> The .pad section is there really only for padding the image. Its space
>> could in principle be used for placing useful stuff (and hence to limit
>> overall in-memory image size). That said, there is a plan for a change
>> which may involve using the initial part of .pad, but that's not certain
>> yet. I'm pointing this out to clarify that there may be a valid reason
>> to avoid re-using the .pad space, at least for now.
> 
> The instructions show how to append after the .pad region, so there is
> no reuse of that space.  I wish objcopy had an --append-region option
> so that the user didn't have to do this extra math and adjust sizes.

Well, I've been trying to point out that the .pad space could be made
use of, but there are constraints.

>>> --- a/xen/arch/arm/efi/efi-boot.h
>>> +++ b/xen/arch/arm/efi/efi-boot.h
>>> @@ -375,27 +375,36 @@ static void __init noreturn efi_arch_post_exit_boot(void)
>>> efi_xen_start(fdt, fdt_totalsize(fdt));
>>> }
>>> -static void __init efi_arch_cfg_file_early(EFI_FILE_HANDLE dir_handle, char *section)
>>> +static void __init efi_arch_cfg_file_early(const EFI_LOADED_IMAGE *image,
>>>
>>> -                                             EFI_FILE_HANDLE dir_handle,
>>>
>>>
>>> -                                             char *section)
>>>
>>>
>>
>> Could I talk you into constifying "section" at this occasion - afaics
>> there should be no fallout here or in the other three places where
>> the same would apply.
> 
> I'm always in favor of adding more constness.  Is it ok to have that
> as part of this patch since we're changing the signature on the function?

Yes, if there's no further fallout from it. Please just mention such
"on the side" changes in half a sentence in the description, so it's
clear they're intentional.

>>> -          if ( !read_section(loaded_image, ".kernel", &kernel, option_str) )
>>> -              read_file(dir_handle, s2w(&name), &ramdisk, NULL);
>>> -              read_file(dir_handle, s2w(&name), &kernel, option_str);
>>
>> As before, I disagree with the idea of taking pieces from disk and
>> pieces from the unified image. If you continue to think this is a
>> reasonable thing to do, may I ask that you add a rationale of this
>> model to the description?
> 
> It allows distributions to continue with the status quo if they want a
> signed kernel + config, but allow a user provided initrd (which is what
> the shim protocol does today).  Qubes, for instance, has discussed that
> as a path forward to allow a trusted kernel, while also allowing users
> to rebuild their own initrd since it contains machine specific data.
> 
> The config is signed, so an attacker can not add any new lines to it.
> So if the config file has no "ramdisk" or "xsm" line then get_value()
> will return NULL and the read_file() will not be attempted.
> If, however, the config file has an "ramdisk /boot/initrd.gz",
> but not ".ramdisk" PE section, then that is an explicit statement
> from the signer that they want to load that file from disk, even
> though the initrd.gz is not included in the signature.

Ah yes, I can follow these arguments. Please put this or an abridged
version of it in the description. It'll help me not re-asking the
same question in a couple of week's time, at the very least.

>>> --- /dev/null
>>> +++ b/xen/common/efi/pe.c
>>> +#define PE_HEADER_MACHINE_ARM64 0xaa64
>>> +#define PE_HEADER_MACHINE_X64 0x8664
>>> +#define PE_HEADER_MACHINE_I386 0x014c
>>
>> This list isn't meant to be a complete one anyway, so please omit
>> the I386 item as it's not needed anywhere.
> 
> Sure.  This file is almost verbatim from systemd-boot, so it has
> a few things like that which are not relevant.

Please zap full entities we don't need (and by this I mean please
don't zap e.g. structure members because we only need some of them).

>>> +struct PeFileHeader {
>>> -   UINT16 Machine;
>>> -   UINT16 NumberOfSections;
>>> -   UINT32 TimeDateStamp;
>>> -   UINT32 PointerToSymbolTable;
>>> -   UINT32 NumberOfSymbols;
>>> -   UINT16 SizeOfOptionalHeader;
>>> -   UINT16 Characteristics;
>>>     +} attribute((packed));
>>>
>>> +struct PeHeader {
>>> -   UINT8 Magic[4];
>>> -   struct PeFileHeader FileHeader;
>>>     +} attribute((packed));
>>
>> At the example of these two (i.e. may extend to others): When the
>> packed attribute doesn't really have any impact on structure layout
>> (and will just adversely affect alignof() when applied to the struct
>> or any of the fields), please omit it.
> 
> In this case the packed does not affect the layout, but if PeFileHeader
> started with a UINT64, for instance, then padding would be added to
> PeHeader to align it without the packed.

Of course, in such a case the "packed" would be warranted. But quite
often structures have already got laid out to optimally pack them, in
which case the attribute is pointless, yet still often gets added in
a purely mechanical manner by people. As said - we had to deal with
fallout from the practice in the not so distant past.

>>> +struct PeSectionHeader {
>>> -   UINT8 Name[8];
>>
>> Better char?
> 
> Maybe? I've heard that some programs use non-7bit ascii in there for things
> that are not normal sections (and the names are compared with memcp()).

Which memcmp() is going to happily deal with afaict.

>>> +const void *__init pe_find_section(const CHAR8 *image, const UINTN image_size,
>>> -                                     const char *section_name, UINTN *size_out)
>>> -   const struct DosFileHeader dos = (const void)image;
>>
>> If the type of "image" was "const void *", there wouldn't be any cast
>> needed here (and again further down). And I don't think you actually
>> need "image" to be a pointer to a particular type?
> 
> I don't think it needs to be any particular type (and CHAR* is a holdover
> from the systemd-boot code).  However, there is quite a bit of pointer
> math done on it that avoids casts:
> 
> pe = (const void *) &image[offset];
> 
> If image were void*, then this would have to be written as something like:
> 
> pe = (const void *)((uintptr_t)image + offset);
> 
> (unless the gnu extension that allows void* math is enabled)

We use this extension all over the place.

>>> -   for ( i = 0; i < pe->FileHeader.NumberOfSections; i++ )
>>> -   {
>>> -          sect = (const void *)&image[offset];
>>>
>>>
>>
>> Please limit the scope of sect to the body of this loop, at which
>> point this assignment can become the initializer.
> 
> sect was used earlier for sizeof math to validate the name.
> 
> It's also a little odd that the style guide calls for declaring variable
> in limited scopes, while also disallowing for loop scoped variables.

Because the latter are a newer language feature, originally
coming from C++, which we haven't settled on yet to allow for
general use.

Jan

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

[Jan Beulich]

On 17.09.2020 16:05, Trammell Hudson wrote:
> On Thursday, September 17, 2020 8:51 AM, Jan Beulich <jbeulich@suse.com> wrote:
>> On 14.09.2020 13:50, Trammell Hudson wrote:
>>> If secure boot is enabled, the Xen command line arguments are ignored.
>>> If a unified Xen image is used, then the bundled configuration, dom0
>>> kernel, and initrd are prefered over the ones listed in the config file.
>>> Unlike the shim based verification, the PE signature on a unified image
>>> covers the all of the Xen+config+kernel+initrd modules linked into the
>>> unified image. This also ensures that properly configured platforms
>>> will measure the entire runtime into the TPM for unsealing secrets or
>>> remote attestation.
>>
>> The command line may also include a part handed on to the Dom0 kernel.
>> If the Dom0 kernel image comes from disk, I don't see why that part of
>> the command line shouldn't be honored. Similarly, if the config file
>> doesn't come from the unified image, I think Xen's command line options
>> should also be honored.
> 
> Ignoring the command line and breaking the shim behaviour in a
> unified image should be ok; that is an explicit decision by the
> system owner to sign and configure the new image (and the shim
> is not used in a unified image anyway).
> 
> If we have a way to detect a unified image early enough, then
> we can avoid the backwards incompatibility if it is not unified.

I was assuming this was easily possible, if necessary as about the
first thing we do. If it's not as easy, perhaps something wants
adding to make it so?

> That would require moving the config parsing to above the relocation
> call.

I guess I don't understand why this would be.

Jan

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

[Trammell Hudson]

On Thursday, September 17, 2020 11:26 AM, Jan Beulich <jbeulich@suse.com> wrote:
> On 17.09.2020 16:05, Trammell Hudson wrote:
> > If we have a way to detect a unified image early enough, then
> > we can avoid the backwards incompatibility if it is not unified.
>
> I was assuming this was easily possible, if necessary as about the
> first thing we do. If it's not as easy, perhaps something wants
> adding to make it so?

v5 of the patch (just sent) has changed the logic so that the
config section is searched first thing, and if it is found, then
and only then is the command line ignored.  I believe this
restores the older behaviour.

> > That would require moving the config parsing to above the relocation
> > call.
>
> I guess I don't understand why this would be.

The early command line parsing happens before the call to
efi_arch_relocate_image(), although testing in qemu did not
seem to cause any problems with calling read_section() before
the reloc.

--
Trammell