[PATCH v2] xen/x86: fix xen.efi boot crash from some bootloaders

["Yann Sionneau" <yann.sionneau@xxxxxxxxxx>]

xen.efi PE does not boot when loaded from shim or some patched
downstream grub2.

What happens is the bootloader would honour the MEM_DISCARDABLE
flag of the .reloc section meaning it would not load its content
into memory.

But Xen is parsing the .reloc section content twice at boot:
* https://elixir.bootlin.com/xen/v4.20.1/source/xen/common/efi/boot.c#L1362
* https://elixir.bootlin.com/xen/v4.20.1/source/xen/arch/x86/efi/efi-boot.h#L237

Therefore it would crash with the following message:
"Unsupported relocation type" as reported there:

* https://github.com/QubesOS/qubes-issues/issues/8206#issuecomment-2619048838
* 
https://lore.kernel.org/xen-devel/7e039262-1f54-46e1-8f70-ac3f03607d5a@xxxxxxxx/T/#me122b9e6c27cd98db917da2c9f67e74a2c6ad7a5

This commit adds a small C host tool named keeprelocs
that is called after xen.efi is produced by the build system
in order to remove this bit from its .reloc section header.

Signed-off-by: Yann Sionneau <yann.sionneau@xxxxxxxxxx>
---
Changes since v1:
- use xen coding style instead of Linux kernel one
- use void * to store the return value from mmap()
- PE file is passed as non-optional argument instead of -i FILE
- use bool instead of int type for "quiet" flag
- remove useless break after return
- add missing 0x before %08x conversion specifier
- add SPDX header
- remove DEBUG stuff
- improve opt_hdr_size checks
- removed useless tools_keeprelocs Makefile target
- fixed -I include path for keeprelocs tool: use $(srctree)
- use memcmp instead of strncmp
- produce an intermediate xen.efi.1 before running keeprelocs
  so that an interrupted build would not end up having a bad
  xen.efi (with MEM_DISCARDABLE flag in .reloc section).
  In other words: make sure the final xen.efi is produced only
  after all modifications are done.

Link to v1: 
https://lore.kernel.org/xen-devel/20250723135620.1327914-1-yann.sionneau@xxxxxxxxxx/
---
 xen/arch/x86/Makefile  |   6 +-
 xen/tools/Makefile     |   3 +
 xen/tools/keeprelocs.c | 165 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 172 insertions(+), 2 deletions(-)
 create mode 100644 xen/tools/keeprelocs.c

diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index ce724a9daa..91af6ae214 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -232,10 +232,12 @@ endif
        $(MAKE) $(build)=$(@D) .$(@F).1r.o .$(@F).1s.o
        $(LD) $(call EFI_LDFLAGS,$(VIRT_BASE)) -T $(obj)/efi.lds $< \
              $(dot-target).1r.o $(dot-target).1s.o $(orphan-handling-y) \
-             $(note_file_option) -o $@
-       $(NM) -pa --format=sysv $@ \
+             $(note_file_option) -o $@.1
+       $(NM) -pa --format=sysv $@.1 \
                | $(objtree)/tools/symbols --all-symbols --xensyms --sysv 
--sort \
                > $@.map
+       $(objtree)/tools/keeprelocs -q $@.1
+       mv $@.1 $@
 ifeq ($(CONFIG_DEBUG_INFO),y)
        $(if $(filter --strip-debug,$(EFI_LDFLAGS)),:$(space))$(OBJCOPY) -O 
elf64-x86-64 $@ $@.elf
 endif
diff --git a/xen/tools/Makefile b/xen/tools/Makefile
index a5078b7cb8..620063ab1e 100644
--- a/xen/tools/Makefile
+++ b/xen/tools/Makefile
@@ -1,2 +1,5 @@
 hostprogs-always-y += symbols
 hostprogs-always-y += fixdep
+hostprogs-always-$(XEN_BUILD_PE) += keeprelocs
+# next line is to allow including include/efi/pe.h
+HOSTCFLAGS_keeprelocs.o := -I $(srctree)/include
\ No newline at end of file
diff --git a/xen/tools/keeprelocs.c b/xen/tools/keeprelocs.c
new file mode 100644
index 0000000000..284378564a
--- /dev/null
+++ b/xen/tools/keeprelocs.c
@@ -0,0 +1,165 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#include <stdio.h>
+#include <fcntl.h>
+#include <errno.h>
+#include <string.h>
+#include <sys/mman.h>
+#include <sys/stat.h>
+#include <stdint.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <stdbool.h>
+#include <efi/pe.h>
+
+static void print_usage(const char *name)
+{
+    printf("%s: [-q] [-h] xen.efi\n", name);
+}
+
+static int get_minimum_opt_hdr_size(uint16_t opt_hdr_magic)
+{
+    switch ( opt_hdr_magic )
+    {
+        case PE_OPT_MAGIC_PE32:
+            return sizeof(struct pe32_opt_hdr);
+        case PE_OPT_MAGIC_PE32PLUS:
+            return sizeof(struct pe32plus_opt_hdr);
+        default:
+            return -1;
+    }
+}
+
+int main(int argc, char **argv)
+{
+    char *filename = NULL;
+    int fd;
+    void *mem;
+    struct stat st;
+    off_t len;
+    int ret;
+    struct mz_hdr *mz;
+    struct pe_hdr *pe;
+    int opt;
+    const char *prog_name = argv[0];
+    bool quiet = false;
+    int min_opt_hdr_size;
+    unsigned int section_id;
+
+    while ( (opt = getopt(argc, argv, ":qh")) != -1 )
+    {
+        switch ( opt )
+        {
+            case 'q':
+                quiet = true;
+                break;
+            case 'h':
+                print_usage(prog_name);
+                return 0;
+            case '?':
+            default:
+                print_usage(prog_name);
+                return -1;
+            }
+    }
+
+    if ( optind < argc )
+        filename = argv[optind++];
+    else
+    {
+        printf("Error: you must provide the PE file name as first and only 
non-optional argument\n");
+        return -1;
+    }
+
+    if ( optind != argc )
+    {
+        printf("Only one non-optional argument should be supplied: the PE file 
name\n");
+        return -1;
+    }
+
+    fd = open(filename, O_RDWR);
+    if ( fd < 0 )
+    {
+        printf("Could not open file %s: %s\n", filename, strerror(errno));
+        return -1;
+    }
+
+    ret = fstat(fd, &st);
+    if ( ret < 0 )
+    {
+        perror("Error while getting PE file length");
+        return -1;
+    }
+
+    len = st.st_size;
+    mem = mmap(NULL, len, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
+
+    if ( mem == MAP_FAILED )
+    {
+        perror("Failed to mmap PE file");
+        return -1;
+    }
+
+    mz = mem;
+    if ( mz->magic != MZ_MAGIC )
+    {
+        printf("file has incorrect MZ header 0x%02x instead of 0x5a4d\n",
+                mz->magic);
+        return -1;
+    }
+
+    pe = mem + mz->peaddr;
+    if ( memcmp((char *)&pe->magic, "PE\0\0", 4) )
+    {
+        printf("file has incorrect PE header magic 0x%08x instead of 
0x00004550\n",
+                pe->magic);
+        return -1;
+    }
+
+    if ( pe->opt_hdr_size < 2 )
+    {
+        printf("file has too small OptionalHeaderSize: %d\n", 
pe->opt_hdr_size);
+        return -1;
+    }
+
+    /* Compute minimum optional header size, based on its
+     * first field (uint16_t magic).
+     */
+    min_opt_hdr_size = get_minimum_opt_hdr_size(*(uint16_t *)((void *)pe
+                                                + sizeof(*pe)));
+    if ( min_opt_hdr_size < 0 )
+    {
+        printf("Incorrect binary type, should be PE32 or PE32+\n");
+        return -1;
+    }
+
+    if ( pe->opt_hdr_size < min_opt_hdr_size )
+    {
+        printf("file has too small OptionalHeaderSize: %d\n", 
pe->opt_hdr_size);
+        return -1;
+    }
+
+    struct section_header *section = (struct section_header *)((uint8_t *)pe
+                                     + sizeof(*pe) + pe->opt_hdr_size);
+    for ( section_id = 0; section_id < pe->sections; section_id++, section++ )
+    {
+        if ( memcmp(section->name, ".reloc", strlen(".reloc") + 1) )
+            continue;
+
+        if ( !quiet )
+            printf(".reloc section characteristics: %08x\n", section->flags);
+        if ( section->flags & IMAGE_SCN_MEM_DISCARDABLE )
+        {
+            if ( !quiet )
+                printf("MEM_DISCARDABLE flag found! Dropping it.\n");
+            section->flags &= ~(IMAGE_SCN_MEM_DISCARDABLE);
+        }
+    }
+
+    munmap(mem, len);
+    close(fd);
+
+    if ( !quiet )
+        printf("Ok!\n");
+    return 0;
+}
-- 
2.43.0



Yann Sionneau | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech