[PATCH v2 0/2] x86: Multiboot PE support

[PATCH v2 0/2] x86: Multiboot PE support

[Ross Lagerwall]

Hi,

This patches series implements support for building a multiboot-capable
PE binary in addition to the existing xen.efi and xen.gz. The purpose of
this is to allow the same binary to be booted using BIOS, UEFI, and UEFI
with Secure Boot verification just like it can be done with a Linux
kernel. It also means that it is possible to enable Secure Boot while
still retaining the flexibility of a full bootloader like GRUB2 - not
currently possible when using xen.efi.

This requires a multiboot2 loader that supports loading PE binaries.
Changes to implement this in GRUB will be sent in a separate series.

Ross

Changed in v2:

* Adjusted for changes to the proposed multiboot2 spec changes. In
  particular, there are no new multiboot2 tags needed.
* Unconditionally build a new binary rather than adding a build option.
* Avoid compressing it since this makes verification more difficult.
* Build the new binary as a modification of xen.efi rather than
  relinking from scratch.

Ross Lagerwall (2):
  x86: Add support for building a multiboot2 PE binary
  x86: Call Shim Verify in the multiboot2 path

 .gitignore                        |  2 +
 xen/Makefile                      |  1 +
 xen/arch/x86/Makefile             | 16 ++++++-
 xen/arch/x86/boot/head.S          |  4 +-
 xen/arch/x86/efi/efi-boot.h       | 65 +++++++++++++++++++++++++-
 xen/arch/x86/efi/modify-mbi-exe.c | 77 +++++++++++++++++++++++++++++++
 6 files changed, 162 insertions(+), 3 deletions(-)
 create mode 100644 xen/arch/x86/efi/modify-mbi-exe.c

-- 
2.43.0

[PATCH v2 1/2] x86: Add support for building a multiboot2 PE binary

[Ross Lagerwall]

In addition to building xen.efi and xen.gz, build xen-mbi.exe. The
latter is a PE binary that can be used with a multiboot2 loader that
supports loading PE binaries.

Using this option allows the binary to be signed and verified by Shim.
This means the same xen-mbi.exe binary can then be used for BIOS boot,
UEFI Boot and UEFI boot with Secure Boot verification (all with the
convenience of GRUB2 as a bootloader).

The new binary is created by modifying xen.efi:
* Relocations are stripped since they are not needed.
* The image base address is set to 0 since it must necessarily be below
  4 GiB and the loader will relocate it anyway.
* The PE entry point is set to the multiboot2 entry point rather than
  the normal EFI entry point. This is only relevant for BIOS boot since
  for EFI boot the entry point is specified via a multiboot2 tag.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 .gitignore                        |  2 +
 xen/Makefile                      |  1 +
 xen/arch/x86/Makefile             | 16 ++++++-
 xen/arch/x86/efi/modify-mbi-exe.c | 77 +++++++++++++++++++++++++++++++
 4 files changed, 95 insertions(+), 1 deletion(-)
 create mode 100644 xen/arch/x86/efi/modify-mbi-exe.c

diff --git a/.gitignore b/.gitignore
index d8b57e32f888..e61acd574b44 100644
--- a/.gitignore
+++ b/.gitignore
@@ -256,6 +256,7 @@ xen/arch/x86/boot/*.lnk
 xen/arch/x86/efi.lds
 xen/arch/x86/efi/check.efi
 xen/arch/x86/efi/mkreloc
+xen/arch/x86/efi/modify-mbi-exe
 xen/arch/x86/include/asm/asm-macros.h
 xen/arch/*/xen.lds
 xen/arch/*/efi/boot.c
@@ -304,6 +305,7 @@ xen/suppression-list.txt
 xen/xen-syms
 xen/xen-syms.map
 xen/xen.*
+xen/xen-mbi.*
 LibVNCServer*
 
 tools/qemu-xen-dir-remote
diff --git a/xen/Makefile b/xen/Makefile
index 21832d640225..1955e1d687df 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -581,6 +581,7 @@ _clean:
 		-o -name ".*.cmd" -o -name "lib.a" \) -exec rm -f {} \;
 	rm -f include/asm $(TARGET) $(TARGET).gz $(TARGET)-syms $(TARGET)-syms.map
 	rm -f $(TARGET).efi $(TARGET).efi.map $(TARGET).efi.elf $(TARGET).efi.stripped
+	rm -f $(TARGET)-mbi.exe
 	rm -f asm-offsets.s arch/*/include/asm/asm-offsets.h
 	rm -f .banner .allconfig.tmp include/xen/compile.h
 	rm -rf $(objtree)/arch/*/include/generated
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 26d87405297b..5b6b8911f1f8 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -86,6 +86,7 @@ extra-y += xen.lds
 
 hostprogs-y += boot/mkelf32
 hostprogs-y += efi/mkreloc
+hostprogs-y += efi/modify-mbi-exe
 
 # Allows usercopy.c to include itself
 $(obj)/usercopy.o: CFLAGS-y += -iquote .
@@ -96,7 +97,7 @@ endif
 
 efi-y := $(shell if [ ! -r $(objtree)/include/xen/compile.h -o \
                       -O $(objtree)/include/xen/compile.h ]; then \
-                         echo '$(TARGET).efi'; fi) \
+                         echo '$(TARGET).efi $(TARGET)-mbi.exe'; fi) \
          $(space)
 efi-$(CONFIG_PV_SHIM_EXCLUSIVE) :=
 
@@ -123,6 +124,19 @@ syms-warn-dup-$(CONFIG_ENFORCE_UNIQUE_SYMBOLS) := --error-dup
 
 orphan-handling-$(call ld-option,--orphan-handling=warn) += --orphan-handling=warn
 
+ifeq ($(XEN_BUILD_PE),y)
+$(TARGET)-mbi.exe: $(TARGET).efi $(obj)/efi/modify-mbi-exe
+	$(OBJCOPY) --remove-section=.reloc $< $@.tmp
+	$(obj)/efi/modify-mbi-exe $@.tmp
+	$(OBJCOPY) --set-start=0x$$($(NM) -pa $@.tmp | awk '/T start$$/{print $$1}') $@.tmp $@.tmp2
+	mv $@.tmp2 $@
+	rm -f $@.tmp
+else
+$(TARGET)-mb.exe: FORCE
+	rm -f $@
+	echo 'PE build not supported'
+endif
+
 $(TARGET): TMP = $(dot-target).elf32
 $(TARGET): $(TARGET)-syms $(efi-y) $(obj)/boot/mkelf32
 	$(obj)/boot/mkelf32 $(notes_phdrs) $(TARGET)-syms $(TMP) $(XEN_IMG_OFFSET) \
diff --git a/xen/arch/x86/efi/modify-mbi-exe.c b/xen/arch/x86/efi/modify-mbi-exe.c
new file mode 100644
index 000000000000..57af382cab4d
--- /dev/null
+++ b/xen/arch/x86/efi/modify-mbi-exe.c
@@ -0,0 +1,77 @@
+#include <stdio.h>
+#include <stdint.h>
+#include <unistd.h>
+#include <fcntl.h>
+
+struct mz_hdr {
+    uint16_t signature;
+#define MZ_SIGNATURE 0x5a4d
+    uint16_t last_page_size;
+    uint16_t page_count;
+    uint16_t relocation_count;
+    uint16_t header_paras;
+    uint16_t min_paras;
+    uint16_t max_paras;
+    uint16_t entry_ss;
+    uint16_t entry_sp;
+    uint16_t checksum;
+    uint16_t entry_ip;
+    uint16_t entry_cs;
+    uint16_t relocations;
+    uint16_t overlay;
+    uint8_t reserved[32];
+    uint32_t extended_header_base;
+};
+
+struct coff_hdr {
+    uint32_t signature;
+    uint16_t cpu;
+    uint16_t section_count;
+    int32_t timestamp;
+    uint32_t symbols_file_offset;
+    uint32_t symbol_count;
+    uint16_t opt_hdr_size;
+    uint16_t flags;
+};
+
+#define IMAGE_BASE_OFFSET 48
+#define NEW_IMAGE_BASE 0x0
+
+int main(int argc, char **argv)
+{
+    int fd;
+    struct mz_hdr mz_hdr;
+    const uint64_t base_addr = NEW_IMAGE_BASE;
+
+    if ( argc != 2 )
+    {
+        fprintf(stderr, "usage: %s <image>\n", argv[0]);
+        return 1;
+    }
+
+    fd = open(argv[1], O_RDWR);
+    if ( fd < 0 ||
+         read(fd, &mz_hdr, sizeof(mz_hdr)) != sizeof(mz_hdr) )
+    {
+        perror(argv[1]);
+        return 2;
+    }
+
+    if ( mz_hdr.signature != MZ_SIGNATURE ||
+         !mz_hdr.extended_header_base )
+    {
+        fprintf(stderr, "%s: Wrong DOS file format\n", argv[1]);
+        return 2;
+    }
+
+    if ( lseek(fd, mz_hdr.extended_header_base + IMAGE_BASE_OFFSET, SEEK_SET) < 0 ||
+         write(fd, &base_addr, sizeof(base_addr)) != sizeof(base_addr) )
+    {
+        perror(argv[1]);
+        return 3;
+    }
+
+    close(fd);
+
+    return 0;
+}
-- 
2.43.0

[PATCH v2 2/2] x86: Call Shim Verify in the multiboot2 path

[Ross Lagerwall]

Now that the multiboot2 binary can be verified by Shim, ensure that the
dom0 kernel is verified when using the multiboot2 path. If the Shim
protocol is not available and the SecureBoot variable is not set to 0
(or the state cannot be determined), abort the boot.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 xen/arch/x86/boot/head.S    |  4 ++-
 xen/arch/x86/efi/efi-boot.h | 65 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 67 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index d8ac0f0494db..e03ae19bdafb 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -349,10 +349,12 @@ __efi64_mb2_start:
         /* Keep the stack aligned. Do not pop a single item off it. */
         mov     (%rsp),%rdi
 
+        mov     %rbx, %rcx
+
         /*
          * efi_multiboot2() is called according to System V AMD64 ABI:
          *   - IN:  %rdi - EFI ImageHandle, %rsi - EFI SystemTable,
-         *          %rdx - MB2 cmdline
+         *          %rdx - MB2 cmdline, %rcx - Multiboot information.
          */
         call    efi_multiboot2
 
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 8ea64e31cdc2..a9569e150e08 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -3,6 +3,7 @@
  * is intended to be included by common/efi/boot.c _only_, and
  * therefore can define arch specific global variables.
  */
+#include <xen/multiboot2.h>
 #include <xen/vga.h>
 #include <asm/e820.h>
 #include <asm/edd.h>
@@ -808,9 +809,69 @@ static const char *__init get_option(const char *cmd, const char *opt)
     return o;
 }
 
+#define ALIGN_UP(arg, align) \
+                (((arg) + (align) - 1) & ~((typeof(arg))(align) - 1))
+
+static void __init efi_verify_dom0(uint64_t mbi_in)
+{
+    uint64_t ptr;
+    const multiboot2_tag_t *tag;
+    EFI_SHIM_LOCK_PROTOCOL *shim_lock;
+    EFI_STATUS status;
+    const multiboot2_tag_module_t *kernel = NULL;
+    const multiboot2_fixed_t *mbi_fix = _p(mbi_in);
+    static EFI_GUID __initdata shim_lock_guid = SHIM_LOCK_PROTOCOL_GUID;
+    static EFI_GUID __initdata global_variable_guid = EFI_GLOBAL_VARIABLE;
+
+    ptr = ALIGN_UP(mbi_in + sizeof(*mbi_fix), MULTIBOOT2_TAG_ALIGN);
+
+    for ( tag = _p(ptr); (uint64_t)tag - mbi_in < mbi_fix->total_size;
+          tag = _p(ALIGN_UP((uint64_t)tag + tag->size, MULTIBOOT2_TAG_ALIGN)) )
+    {
+        if ( tag->type == MULTIBOOT2_TAG_TYPE_MODULE )
+        {
+            kernel = (const multiboot2_tag_module_t *)tag;
+            break;
+        }
+        else if ( tag->type == MULTIBOOT2_TAG_TYPE_END )
+            break;
+    }
+
+    if ( !kernel )
+        return;
+
+    if ( (status = efi_bs->LocateProtocol(&shim_lock_guid, NULL,
+                                          (void **)&shim_lock)) != EFI_SUCCESS )
+    {
+        UINT32 attr;
+        UINT8 data;
+        UINTN size = sizeof(data);
+
+        status = efi_rs->GetVariable((CHAR16 *)L"SecureBoot", &global_variable_guid,
+                                     &attr, &size, &data);
+        if ( status == EFI_NOT_FOUND )
+            return;
+
+        if ( EFI_ERROR(status) )
+            PrintErrMesg(L"Could not get SecureBoot variable", status);
+
+        if ( attr != (EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS) )
+            PrintErrMesg(L"Unexpected SecureBoot attributes", attr);
+
+        if ( size == 1 && data == 0 )
+            return;
+
+        blexit(L"Could not locate shim but Secure Boot is enabled");
+    }
+
+    if ( (status = shim_lock->Verify(_p(kernel->mod_start),
+                                     kernel->mod_end - kernel->mod_start)) != EFI_SUCCESS )
+        PrintErrMesg(L"Dom0 kernel image could not be verified", status);
+}
+
 void asmlinkage __init efi_multiboot2(EFI_HANDLE ImageHandle,
                                       EFI_SYSTEM_TABLE *SystemTable,
-                                      const char *cmdline)
+                                      const char *cmdline, uint64_t mbi_in)
 {
     EFI_GRAPHICS_OUTPUT_PROTOCOL *gop;
     EFI_HANDLE gop_handle;
@@ -902,6 +963,8 @@ void asmlinkage __init efi_multiboot2(EFI_HANDLE ImageHandle,
 
     efi_relocate_esrt(SystemTable);
 
+    efi_verify_dom0(mbi_in);
+
     efi_exit_boot(ImageHandle, SystemTable);
 }
 
-- 
2.43.0

Re: [PATCH v2 1/2] x86: Add support for building a multiboot2 PE binary

[Jan Beulich]

On 28.03.2024 16:11, Ross Lagerwall wrote:
> In addition to building xen.efi and xen.gz, build xen-mbi.exe. The
> latter is a PE binary that can be used with a multiboot2 loader that
> supports loading PE binaries.

I have to admit I find .exe a strange extension outside of the Windows
world. Would it be an option to have no extension at all (xen-mbi), or
use xen.mbi?

> Using this option allows the binary to be signed and verified by Shim.
> This means the same xen-mbi.exe binary can then be used for BIOS boot,
> UEFI Boot and UEFI boot with Secure Boot verification (all with the
> convenience of GRUB2 as a bootloader).

With which "UEFI boot" really means "chainloader" from grub? That isn't
required though, is it? I.e. "UEFI boot" ought to work also without
involving grub?

> The new binary is created by modifying xen.efi:
> * Relocations are stripped since they are not needed.

Because of the changed entry point, aiui. What hasn't become clear to
me is what difference in functionality there's going to be between
booting this new image in "UEFI boot" mode vs using xen.efi. Clearly
xen.efi's own (EFI-level) command line options won't be available. But
it feels like there might be more.

> * The image base address is set to 0 since it must necessarily be below
>   4 GiB and the loader will relocate it anyway.

While technically okay, what is the reason for this adjustment? 

> * The PE entry point is set to the multiboot2 entry point rather than
>   the normal EFI entry point. This is only relevant for BIOS boot since
>   for EFI boot the entry point is specified via a multiboot2 tag.

Hmm, I may then be confused about the terminology further up: What is
"BIOS boot" then? So far I've taken that as the equivalent of xen.gz's
way of being booted (i.e. grub without EFI underneath).

> @@ -123,6 +124,19 @@ syms-warn-dup-$(CONFIG_ENFORCE_UNIQUE_SYMBOLS) := --error-dup
>  
>  orphan-handling-$(call ld-option,--orphan-handling=warn) += --orphan-handling=warn
>  
> +ifeq ($(XEN_BUILD_PE),y)
> +$(TARGET)-mbi.exe: $(TARGET).efi $(obj)/efi/modify-mbi-exe
> +	$(OBJCOPY) --remove-section=.reloc $< $@.tmp
> +	$(obj)/efi/modify-mbi-exe $@.tmp
> +	$(OBJCOPY) --set-start=0x$$($(NM) -pa $@.tmp | awk '/T start$$/{print $$1}') $@.tmp $@.tmp2

I understand section removal is better done by objcopy. Changing the entry
point could be done by the new tool, though (by passing it the designated
address)?

As to stripping .reloc - generally this needs accompanying by setting the
"relocations stripped" flag in the COFF(?) header. Here, however, I take
it this is not only not needed, but actually not wanted. This imo wants
saying somewhere; the individual steps done here could do with brief
comments anyway, imo.

> --- /dev/null
> +++ b/xen/arch/x86/efi/modify-mbi-exe.c
> @@ -0,0 +1,77 @@
> +#include <stdio.h>
> +#include <stdint.h>
> +#include <unistd.h>
> +#include <fcntl.h>
> +
> +struct mz_hdr {
> +    uint16_t signature;
> +#define MZ_SIGNATURE 0x5a4d
> +    uint16_t last_page_size;
> +    uint16_t page_count;
> +    uint16_t relocation_count;
> +    uint16_t header_paras;
> +    uint16_t min_paras;
> +    uint16_t max_paras;
> +    uint16_t entry_ss;
> +    uint16_t entry_sp;
> +    uint16_t checksum;
> +    uint16_t entry_ip;
> +    uint16_t entry_cs;
> +    uint16_t relocations;
> +    uint16_t overlay;
> +    uint8_t reserved[32];
> +    uint32_t extended_header_base;
> +};
> +
> +struct coff_hdr {
> +    uint32_t signature;
> +    uint16_t cpu;
> +    uint16_t section_count;
> +    int32_t timestamp;
> +    uint32_t symbols_file_offset;
> +    uint32_t symbol_count;
> +    uint16_t opt_hdr_size;
> +    uint16_t flags;
> +};

I can't spot any use of this struct.

Jan

> +#define IMAGE_BASE_OFFSET 48
> +#define NEW_IMAGE_BASE 0x0
> +
> +int main(int argc, char **argv)
> +{
> +    int fd;
> +    struct mz_hdr mz_hdr;
> +    const uint64_t base_addr = NEW_IMAGE_BASE;
> +
> +    if ( argc != 2 )
> +    {
> +        fprintf(stderr, "usage: %s <image>\n", argv[0]);
> +        return 1;
> +    }
> +
> +    fd = open(argv[1], O_RDWR);
> +    if ( fd < 0 ||
> +         read(fd, &mz_hdr, sizeof(mz_hdr)) != sizeof(mz_hdr) )
> +    {
> +        perror(argv[1]);
> +        return 2;
> +    }
> +
> +    if ( mz_hdr.signature != MZ_SIGNATURE ||
> +         !mz_hdr.extended_header_base )
> +    {
> +        fprintf(stderr, "%s: Wrong DOS file format\n", argv[1]);
> +        return 2;
> +    }
> +
> +    if ( lseek(fd, mz_hdr.extended_header_base + IMAGE_BASE_OFFSET, SEEK_SET) < 0 ||
> +         write(fd, &base_addr, sizeof(base_addr)) != sizeof(base_addr) )
> +    {
> +        perror(argv[1]);
> +        return 3;
> +    }
> +
> +    close(fd);
> +
> +    return 0;
> +}

Re: [PATCH v2 2/2] x86: Call Shim Verify in the multiboot2 path

[Jan Beulich]

On 28.03.2024 16:11, Ross Lagerwall wrote:
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -3,6 +3,7 @@
>   * is intended to be included by common/efi/boot.c _only_, and
>   * therefore can define arch specific global variables.
>   */
> +#include <xen/multiboot2.h>
>  #include <xen/vga.h>
>  #include <asm/e820.h>
>  #include <asm/edd.h>
> @@ -808,9 +809,69 @@ static const char *__init get_option(const char *cmd, const char *opt)
>      return o;
>  }
>  
> +#define ALIGN_UP(arg, align) \
> +                (((arg) + (align) - 1) & ~((typeof(arg))(align) - 1))

Nit: I don't think aligning the opening parentheses is an appropriate
criteria here. Imo either

#define ALIGN_UP(arg, align) \
            (((arg) + (align) - 1) & ~((typeof(arg))(align) - 1))

or

#define ALIGN_UP(arg, align) \
        (((arg) + (align) - 1) & ~((typeof(arg))(align) - 1))

or

#define ALIGN_UP(arg, align) \
    (((arg) + (align) - 1) & ~((typeof(arg))(align) - 1))

.

> +static void __init efi_verify_dom0(uint64_t mbi_in)
> +{
> +    uint64_t ptr;
> +    const multiboot2_tag_t *tag;
> +    EFI_SHIM_LOCK_PROTOCOL *shim_lock;
> +    EFI_STATUS status;
> +    const multiboot2_tag_module_t *kernel = NULL;
> +    const multiboot2_fixed_t *mbi_fix = _p(mbi_in);
> +    static EFI_GUID __initdata shim_lock_guid = SHIM_LOCK_PROTOCOL_GUID;
> +    static EFI_GUID __initdata global_variable_guid = EFI_GLOBAL_VARIABLE;
> +
> +    ptr = ALIGN_UP(mbi_in + sizeof(*mbi_fix), MULTIBOOT2_TAG_ALIGN);
> +
> +    for ( tag = _p(ptr); (uint64_t)tag - mbi_in < mbi_fix->total_size;
> +          tag = _p(ALIGN_UP((uint64_t)tag + tag->size, MULTIBOOT2_TAG_ALIGN)) )
> +    {
> +        if ( tag->type == MULTIBOOT2_TAG_TYPE_MODULE )
> +        {
> +            kernel = (const multiboot2_tag_module_t *)tag;
> +            break;

This could do with a comment along the lines of what __start_xen() has
("Dom0 kernel is always first").

> +        }
> +        else if ( tag->type == MULTIBOOT2_TAG_TYPE_END )

Not need for "else" here (personally I find such irritating).

> +            break;
> +    }
> +
> +    if ( !kernel )
> +        return;
> +
> +    if ( (status = efi_bs->LocateProtocol(&shim_lock_guid, NULL,
> +                                          (void **)&shim_lock)) != EFI_SUCCESS )
> +    {
> +        UINT32 attr;
> +        UINT8 data;
> +        UINTN size = sizeof(data);
> +
> +        status = efi_rs->GetVariable((CHAR16 *)L"SecureBoot", &global_variable_guid,
> +                                     &attr, &size, &data);
> +        if ( status == EFI_NOT_FOUND )
> +            return;
> +
> +        if ( EFI_ERROR(status) )
> +            PrintErrMesg(L"Could not get SecureBoot variable", status);
> +
> +        if ( attr != (EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS) )
> +            PrintErrMesg(L"Unexpected SecureBoot attributes", attr);

This wants to be blexit(), not PrintErrMesg().

> +        if ( size == 1 && data == 0 )
> +            return;
> +
> +        blexit(L"Could not locate shim but Secure Boot is enabled");
> +    }
> +
> +    if ( (status = shim_lock->Verify(_p(kernel->mod_start),
> +                                     kernel->mod_end - kernel->mod_start)) != EFI_SUCCESS )
> +        PrintErrMesg(L"Dom0 kernel image could not be verified", status);
> +}

Overall this is a superset of what efi_start() does. What I'm missing from
the description is some discussion of why what's done there is not
sufficient (beyond the env var check, which iirc there once was a patch to
add it). One could only then judge whether it wouldn't make sense to make
this function uniformly used by both paths (with mbi_in suitably dealt with
for the other case).

Jan

Re: [PATCH v2 1/2] x86: Add support for building a multiboot2 PE binary

[Ross Lagerwall]

On Mon, Apr 8, 2024 at 11:25 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 28.03.2024 16:11, Ross Lagerwall wrote:
> > In addition to building xen.efi and xen.gz, build xen-mbi.exe. The
> > latter is a PE binary that can be used with a multiboot2 loader that
> > supports loading PE binaries.
>
> I have to admit I find .exe a strange extension outside of the Windows
> world. Would it be an option to have no extension at all (xen-mbi), or
> use xen.mbi?

Sure, I have no strong preference on the name. I'll change it to
xen-mbi.

>
> > Using this option allows the binary to be signed and verified by Shim.
> > This means the same xen-mbi.exe binary can then be used for BIOS boot,
> > UEFI Boot and UEFI boot with Secure Boot verification (all with the
> > convenience of GRUB2 as a bootloader).
>
> With which "UEFI boot" really means "chainloader" from grub? That isn't
> required though, is it? I.e. "UEFI boot" ought to work also without
> involving grub?

There are a few comments here related to terminology and purpose so let
me try and clarify them in one place...

These are the existing methods of booting Xen on x86 that I know of:

* UEFI firmware boots xen.efi directly. This can be used with
  Secure Boot enabled.

* UEFI GRUB2 chainloads xen.efi by calling the firmware's
  LoadImage/StartImage. This can be used with Secure Boot enabled.

* BIOS GRUB2 boots xen.gz via multiboot1 protocol.

* BIOS GRUB2 boots xen.gz via multiboot2 protocol.

* UEFI GRUB2 boots xen.gz via multiboot2 protocol. This is much the
  same as the previous ,ethod but it does use a different entry point.
  This cannot be used with Secure Boot because Shim can only verify PE
  binaries.

These methods _do not_ work:

* BIOS GRUB2 boots xen.efi via multiboot2 protocol.

* UEFI GRUB2 boots xen.efi via multiboot2 protocol (despite what is
  implied by https://xenbits.xen.org/docs/unstable/misc/efi.html).

* Shim chainloads xen.efi. AFAICT this may have worked some time in
  the past but it doesn't currently work in my testing.

* GRUB2 verifies xen.efi using Shim and then chainloads it using an
  internal PE loader. This functionality doesn't exist in upstream
  GRUB. There are some distro patches to implement this functionality
  but they did not work with Xen when I tested them (probably for the
  same reason as the previous method).

This patch series adds 2 new methods of booting Xen:

* BIOS GRUB2 boots xen-mbi via multiboot2 protocol.

* UEFI GRUB2 boots xen-mbi via multiboot2 protocol. This supports
  Secure Boot by making it possible to call back to Shim to verify
  Xen.

We want to add Secure Boot support to XenServer and to that end, the
boot methods added by this patch are preferable to the existing boot
methods for a few reasons:

* We want a similar user experience regardless of whether BIOS or UEFI
  is used.
* When using GRUB2/multiboot, the boot files (xen.gz, vmlinuz, initrd)
  can be stored outside of the ESP which is preferable since the ESP
  is less flexible and is often somewhat limited in size.
* Using GRUB2/multiboot makes it easier to edit the Xen/Dom0
  command-line while booting / recovering a host compared with the
  config files used by the direct EFI boot.
* Using GRUB2 makes it easier to choose different boot options rather
  than having to use the firmware boot menu which is often quite
  unpleasant. Yes, we could use a UEFI bootloader like rEFInd to help
  with this but that then goes back to the first point about user
  experience.
* For development it makes life easier to always have a single Xen
  binary that is used regardless of whether BIOS/UEFI is used.

The terminology used in the patch description was not particularly
precise. To clarify, "BIOS boot" means booting xen-mbi via the
multiboot2 protocol with a BIOS-booted bootloader (like GRUB2).
"(U)EFI boot" means booting xen-mbi via the multiboot2 protocol with
a UEFI bootloader (like GRUB2).

>
> > The new binary is created by modifying xen.efi:
> > * Relocations are stripped since they are not needed.
>
> Because of the changed entry point, aiui. What hasn't become clear to
> me is what difference in functionality there's going to be between
> booting this new image in "UEFI boot" mode vs using xen.efi. Clearly
> xen.efi's own (EFI-level) command line options won't be available. But
> it feels like there might be more.

Indeed, relocations are not needed because we're using the multiboot2
entry point which handles everything without the need for relocations.

Hopefully the long comment above addresses why this patch is useful.

>
> > * The image base address is set to 0 since it must necessarily be below
> >   4 GiB and the loader will relocate it anyway.
>
> While technically okay, what is the reason for this adjustment?

The multiboot2 spec generally uses 32 bit addresses for everything and
says:

"The bootloader must not load any part of the kernel, the modules, the
Multiboot2 information structure, etc. higher than 4 GiB - 1."

An image base address above 4 GiB causes trouble because multiboot2
wasn't designed for this.

>
> > * The PE entry point is set to the multiboot2 entry point rather than
> >   the normal EFI entry point. This is only relevant for BIOS boot since
> >   for EFI boot the entry point is specified via a multiboot2 tag.
>
> Hmm, I may then be confused about the terminology further up: What is
> "BIOS boot" then? So far I've taken that as the equivalent of xen.gz's
> way of being booted (i.e. grub without EFI underneath).

Hopefully the long comment above clarifies the terminology.

>
> > @@ -123,6 +124,19 @@ syms-warn-dup-$(CONFIG_ENFORCE_UNIQUE_SYMBOLS) := --error-dup
> >
> >  orphan-handling-$(call ld-option,--orphan-handling=warn) += --orphan-handling=warn
> >
> > +ifeq ($(XEN_BUILD_PE),y)
> > +$(TARGET)-mbi.exe: $(TARGET).efi $(obj)/efi/modify-mbi-exe
> > +     $(OBJCOPY) --remove-section=.reloc $< $@.tmp
> > +     $(obj)/efi/modify-mbi-exe $@.tmp
> > +     $(OBJCOPY) --set-start=0x$$($(NM) -pa $@.tmp | awk '/T start$$/{print $$1}') $@.tmp $@.tmp2
>
> I understand section removal is better done by objcopy. Changing the entry
> point could be done by the new tool, though (by passing it the designated
> address)?

Sure, I can do that if you would prefer.

>
> As to stripping .reloc - generally this needs accompanying by setting the
> "relocations stripped" flag in the COFF(?) header. Here, however, I take
> it this is not only not needed, but actually not wanted. This imo wants
> saying somewhere; the individual steps done here could do with brief
> comments anyway, imo.

Correct, it is not wanted. I can add some descriptive comments.

>
> > --- /dev/null
> > +++ b/xen/arch/x86/efi/modify-mbi-exe.c
> > @@ -0,0 +1,77 @@
> > +#include <stdio.h>
> > +#include <stdint.h>
> > +#include <unistd.h>
> > +#include <fcntl.h>
> > +
> > +struct mz_hdr {
> > +    uint16_t signature;
> > +#define MZ_SIGNATURE 0x5a4d
> > +    uint16_t last_page_size;
> > +    uint16_t page_count;
> > +    uint16_t relocation_count;
> > +    uint16_t header_paras;
> > +    uint16_t min_paras;
> > +    uint16_t max_paras;
> > +    uint16_t entry_ss;
> > +    uint16_t entry_sp;
> > +    uint16_t checksum;
> > +    uint16_t entry_ip;
> > +    uint16_t entry_cs;
> > +    uint16_t relocations;
> > +    uint16_t overlay;
> > +    uint8_t reserved[32];
> > +    uint32_t extended_header_base;
> > +};
> > +
> > +struct coff_hdr {
> > +    uint32_t signature;
> > +    uint16_t cpu;
> > +    uint16_t section_count;
> > +    int32_t timestamp;
> > +    uint32_t symbols_file_offset;
> > +    uint32_t symbol_count;
> > +    uint16_t opt_hdr_size;
> > +    uint16_t flags;
> > +};
>
> I can't spot any use of this struct.
>

Indeed, though this will actually be used if changing the entry point
is done with this tool so I'll probably keep it.

Thanks for your review,
Ross

Re: [PATCH v2 2/2] x86: Call Shim Verify in the multiboot2 path

[Ross Lagerwall]

On Mon, Apr 8, 2024 at 11:42 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 28.03.2024 16:11, Ross Lagerwall wrote:
> > --- a/xen/arch/x86/efi/efi-boot.h
> > +++ b/xen/arch/x86/efi/efi-boot.h
> > @@ -3,6 +3,7 @@
> >   * is intended to be included by common/efi/boot.c _only_, and
> >   * therefore can define arch specific global variables.
> >   */
> > +#include <xen/multiboot2.h>
> >  #include <xen/vga.h>
> >  #include <asm/e820.h>
> >  #include <asm/edd.h>
> > @@ -808,9 +809,69 @@ static const char *__init get_option(const char *cmd, const char *opt)
> >      return o;
> >  }
> >
> > +#define ALIGN_UP(arg, align) \
> > +                (((arg) + (align) - 1) & ~((typeof(arg))(align) - 1))
>
> Nit: I don't think aligning the opening parentheses is an appropriate
> criteria here. Imo either
>
> #define ALIGN_UP(arg, align) \
>             (((arg) + (align) - 1) & ~((typeof(arg))(align) - 1))
>
> or
>
> #define ALIGN_UP(arg, align) \
>         (((arg) + (align) - 1) & ~((typeof(arg))(align) - 1))
>
> or
>
> #define ALIGN_UP(arg, align) \
>     (((arg) + (align) - 1) & ~((typeof(arg))(align) - 1))
>
> .

OK, will fix.

>
> > +static void __init efi_verify_dom0(uint64_t mbi_in)
> > +{
> > +    uint64_t ptr;
> > +    const multiboot2_tag_t *tag;
> > +    EFI_SHIM_LOCK_PROTOCOL *shim_lock;
> > +    EFI_STATUS status;
> > +    const multiboot2_tag_module_t *kernel = NULL;
> > +    const multiboot2_fixed_t *mbi_fix = _p(mbi_in);
> > +    static EFI_GUID __initdata shim_lock_guid = SHIM_LOCK_PROTOCOL_GUID;
> > +    static EFI_GUID __initdata global_variable_guid = EFI_GLOBAL_VARIABLE;
> > +
> > +    ptr = ALIGN_UP(mbi_in + sizeof(*mbi_fix), MULTIBOOT2_TAG_ALIGN);
> > +
> > +    for ( tag = _p(ptr); (uint64_t)tag - mbi_in < mbi_fix->total_size;
> > +          tag = _p(ALIGN_UP((uint64_t)tag + tag->size, MULTIBOOT2_TAG_ALIGN)) )
> > +    {
> > +        if ( tag->type == MULTIBOOT2_TAG_TYPE_MODULE )
> > +        {
> > +            kernel = (const multiboot2_tag_module_t *)tag;
> > +            break;
>
> This could do with a comment along the lines of what __start_xen() has
> ("Dom0 kernel is always first").

Will add.

>
> > +        }
> > +        else if ( tag->type == MULTIBOOT2_TAG_TYPE_END )
>
> Not need for "else" here (personally I find such irritating).

OK, I'll remove it.

>
> > +            break;
> > +    }
> > +
> > +    if ( !kernel )
> > +        return;
> > +
> > +    if ( (status = efi_bs->LocateProtocol(&shim_lock_guid, NULL,
> > +                                          (void **)&shim_lock)) != EFI_SUCCESS )
> > +    {
> > +        UINT32 attr;
> > +        UINT8 data;
> > +        UINTN size = sizeof(data);
> > +
> > +        status = efi_rs->GetVariable((CHAR16 *)L"SecureBoot", &global_variable_guid,
> > +                                     &attr, &size, &data);
> > +        if ( status == EFI_NOT_FOUND )
> > +            return;
> > +
> > +        if ( EFI_ERROR(status) )
> > +            PrintErrMesg(L"Could not get SecureBoot variable", status);
> > +
> > +        if ( attr != (EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS) )
> > +            PrintErrMesg(L"Unexpected SecureBoot attributes", attr);
>
> This wants to be blexit(), not PrintErrMesg().

blexit() doesn't allow printing the attributes but I can call some
other function like DisplayUint() to do that before calling blexit().

>
> > +        if ( size == 1 && data == 0 )
> > +            return;
> > +
> > +        blexit(L"Could not locate shim but Secure Boot is enabled");
> > +    }
> > +
> > +    if ( (status = shim_lock->Verify(_p(kernel->mod_start),
> > +                                     kernel->mod_end - kernel->mod_start)) != EFI_SUCCESS )
> > +        PrintErrMesg(L"Dom0 kernel image could not be verified", status);
> > +}
>
> Overall this is a superset of what efi_start() does. What I'm missing from
> the description is some discussion of why what's done there is not
> sufficient (beyond the env var check, which iirc there once was a patch to
> add it). One could only then judge whether it wouldn't make sense to make
> this function uniformly used by both paths (with mbi_in suitably dealt with
> for the other case).
>

Hmm, I wasn't really looking at efi_start() verification for the
purpose of this patch series. I can update the patch so that
both paths use a common verification function and also describe
how it differs from the simple call to shim verify that currently
exists in efi_start().

Thanks,
Ross

Re: [PATCH v2 1/2] x86: Add support for building a multiboot2 PE binary

[Jan Beulich]

On 10.04.2024 11:41, Ross Lagerwall wrote:
> On Mon, Apr 8, 2024 at 11:25 AM Jan Beulich <jbeulich@suse.com> wrote:
>> On 28.03.2024 16:11, Ross Lagerwall wrote:
>>> * The image base address is set to 0 since it must necessarily be below
>>>   4 GiB and the loader will relocate it anyway.
>>
>> While technically okay, what is the reason for this adjustment?
> 
> The multiboot2 spec generally uses 32 bit addresses for everything and
> says:
> 
> "The bootloader must not load any part of the kernel, the modules, the
> Multiboot2 information structure, etc. higher than 4 GiB - 1."
> 
> An image base address above 4 GiB causes trouble because multiboot2
> wasn't designed for this.

Yet mb2 doesn't care about that PE header field at all, does it? In which
case my question remains: What purpose does this particular modification
of the image have?

Jan

Re: [PATCH v2 1/2] x86: Add support for building a multiboot2 PE binary

[Ross Lagerwall]

On Wed, Apr 17, 2024 at 3:15 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 10.04.2024 11:41, Ross Lagerwall wrote:
> > On Mon, Apr 8, 2024 at 11:25 AM Jan Beulich <jbeulich@suse.com> wrote:
> >> On 28.03.2024 16:11, Ross Lagerwall wrote:
> >>> * The image base address is set to 0 since it must necessarily be below
> >>>   4 GiB and the loader will relocate it anyway.
> >>
> >> While technically okay, what is the reason for this adjustment?
> >
> > The multiboot2 spec generally uses 32 bit addresses for everything and
> > says:
> >
> > "The bootloader must not load any part of the kernel, the modules, the
> > Multiboot2 information structure, etc. higher than 4 GiB - 1."
> >
> > An image base address above 4 GiB causes trouble because multiboot2
> > wasn't designed for this.
>
> Yet mb2 doesn't care about that PE header field at all, does it? In which
> case my question remains: What purpose does this particular modification
> of the image have?
>

With the currently published version of mb2, it doesn't look at the PE
header field since it has no knowledge about PE binaries.

With the proposal on the grub-devel list [1], mb2 would use the PE
header to load the new xen-mbi binary in which case, the image base
address is indeed relevant.

Ross

[1] https://lists.gnu.org/archive/html/grub-devel/2024-03/msg00081.html

Re: [PATCH v2 1/2] x86: Add support for building a multiboot2 PE binary

[Jan Beulich]

On 17.04.2024 17:05, Ross Lagerwall wrote:
> On Wed, Apr 17, 2024 at 3:15 PM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 10.04.2024 11:41, Ross Lagerwall wrote:
>>> On Mon, Apr 8, 2024 at 11:25 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>> On 28.03.2024 16:11, Ross Lagerwall wrote:
>>>>> * The image base address is set to 0 since it must necessarily be below
>>>>>   4 GiB and the loader will relocate it anyway.
>>>>
>>>> While technically okay, what is the reason for this adjustment?
>>>
>>> The multiboot2 spec generally uses 32 bit addresses for everything and
>>> says:
>>>
>>> "The bootloader must not load any part of the kernel, the modules, the
>>> Multiboot2 information structure, etc. higher than 4 GiB - 1."
>>>
>>> An image base address above 4 GiB causes trouble because multiboot2
>>> wasn't designed for this.
>>
>> Yet mb2 doesn't care about that PE header field at all, does it? In which
>> case my question remains: What purpose does this particular modification
>> of the image have?
>>
> 
> With the currently published version of mb2, it doesn't look at the PE
> header field since it has no knowledge about PE binaries.
> 
> With the proposal on the grub-devel list [1], mb2 would use the PE
> header to load the new xen-mbi binary in which case, the image base
> address is indeed relevant.

But then how can you strip .reloc? If the image base field is to be used,
and if the image can't be placed there, relocation needs to happen. (As
an aside, [1] looks to be talking of the entry point only, not the image
base?)

Jan

> [1] https://lists.gnu.org/archive/html/grub-devel/2024-03/msg00081.html