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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.07.2025 15:56, Yann Sionneau wrote:
> 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.

As indicated on Matrix, giving this tool such a specific name doesn't
lend it to use for further editing later on.

Also such an entirely new tool imo wants to use Xen style, not Linux(?)
one. Unless of course it is taken from somewhere, but nothing is being
said along these line.

> --- a/xen/Makefile
> +++ b/xen/Makefile
> @@ -299,10 +299,13 @@ export XEN_HAS_CHECKPOLICY := $(call 
> success,$(CHECKPOLICY) -h 2>&1 | grep -q xe
>  # ===========================================================================
>  # Rules shared between *config targets and build targets
>  
> -PHONY += tools_fixdep
> +PHONY += tools_fixdep tools_keeprelocs
>  tools_fixdep:
>       $(Q)$(MAKE) $(build)=tools tools/fixdep
>  
> +tools_keeprelocs:
> +     $(Q)$(MAKE) $(build)=tools tools/keeprelocs

Hmm, recursing twice into the tools/ subdir isn't quite nice. But
perhaps tolerable for the moment.

> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -236,6 +236,7 @@ endif
>       $(NM) -pa --format=sysv $@ \
>               | $(objtree)/tools/symbols --all-symbols --xensyms --sysv 
> --sort \
>               > $@.map
> +     $(objtree)/tools/keeprelocs -q -i $@

Consider the build being interrupted precisely before this command is
executed, and the target file not being removed. A subsequent build would
find it up-to-date, when the earlier build didn't really finish.

> --- 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 ../include
> \ No newline at end of file

Please don't omit newlines at the ends of files.

> --- /dev/null
> +++ b/xen/tools/keeprelocs.c
> @@ -0,0 +1,119 @@
> +#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 <efi/pe.h>
> +
> +#undef DEBUG
> +
> +static void print_usage(const char *name) {
> +     printf("%s: [-q] [-h] -i xen.efi\n", name);
> +}
> +
> +int main(int argc, char **argv)
> +{
> +     char *filename = NULL;
> +     int fd;
> +     char *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];
> +     int quiet = 0;

bool?

> +     while ((opt = getopt(argc, argv, ":i:qh")) != -1)
> +     {
> +             switch (opt) {
> +             case 'i':
> +                     filename = optarg;
> +                     break;

Is there a particular reason why -i needs to be used to specify the file name?
Can't the file name be the first (and only) non-option argument, as is commonly
done elsewhere?

> +             case 'q':
> +                     quiet = 1;
> +                     break;
> +             case 'h':
> +                     print_usage(prog_name);
> +                     return 0;
> +                     break;

"break" after "return"?

> +             case '?':

Why is this not the same as 'h'?

> +             default:
> +                     print_usage(prog_name);
> +                     return -1;
> +             }
> +     }
> +
> +
> +     if (!filename) {
> +             printf("Error: you must provide a `-i xen.efi` argument\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 = (struct mz_hdr *)mem;

If mem was void * you wouldn't need a cast here.

> +     if (mz->magic != MZ_MAGIC) { // "MZ"
> +             printf("file has incorrect MZ header 0x%02x instead of 
> 0x5a4d\n", mz->magic);
> +             return -1;
> +     }
> +
> +     pe = (struct pe_hdr *)(mem + mz->peaddr);

Nor here.

> +     if (strncmp((char *)&pe->magic, "PE\0\0", 4)) {

I don't think strncmp() can be used here, as it'll stop at the first '\0'.

> +             printf("file has incorrect PE header magic %08x instead of 
> 0x00004550\n", pe->magic);

0x%08x

> +             return -1;
> +     }
> +
> +     if (pe->opt_hdr_size == 0) {
> +             printf("file has empty OptionalHeader\n");
> +             return -1;
> +     }

Code further down doesn't really require this check, as it looks. IOW
either this check wants dropping, or it wants to be more strict than
just checking for zero.

> +     struct section_header *section = (struct section_header *)((uint8_t 
> *)pe + sizeof(*pe) + pe->opt_hdr_size);

"(uint8_t *)pe + sizeof(*pe)" can be easier had as "(uint8_t *)(pe + 1)".
But that won't be sufficient to get the line under 80 chars.

> +     for (unsigned int section_id = 0; section_id < pe->sections; 
> section_id++, section++)
> +     {
> +#ifdef DEBUG
> +             printf("section %s\n", section->Name);
> +#endif

This probably is just a leftover?

> +             if (strncmp(section->name, ".reloc", strlen(".reloc")))

strncmp(..., strlen(...)) is slightly odd. In the specific case, strcmp()
will do I think. Otherwise use memcmp()?

Jan