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

Yann Sionneau posted 1 patch 3 months, 1 week ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20250724140731.1502774-1-yann.sionneau@vates.tech
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
[PATCH v2] xen/x86: fix xen.efi boot crash from some bootloaders
Posted by Yann Sionneau 3 months, 1 week ago
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@suse.com/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@vates.tech>
---
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@vates.tech/
---
 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
Re: [PATCH v2] xen/x86: fix xen.efi boot crash from some bootloaders
Posted by Jan Beulich 2 months, 3 weeks ago
On 24.07.2025 16:07, 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@suse.com/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@vates.tech>

So I found a way to deal with this at the linker side, without any new command
line options. Behavior is solely driven by the attributes of any incoming .reloc
sections (of which there would be none by default, retaining original behavior).
The important patch is [1], but at least the first patch of the series [2] would
in most cases also be wanted/needed (patch 04 is obviously a mechanical prereq
for the main patch). Need for other of the prereqs there depends on the scope
and purpose of one's binutils build(s).

Jan

[1] https://sourceware.org/pipermail/binutils/2025-August/143153.html
[2] https://sourceware.org/pipermail/binutils/2025-August/143141.html
Re: [PATCH v2] xen/x86: fix xen.efi boot crash from some bootloaders
Posted by Yann Sionneau 2 months ago
On 8/4/25 11:34, Jan Beulich wrote:
> On 24.07.2025 16:07, 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@suse.com/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@vates.tech>
> 
> So I found a way to deal with this at the linker side, without any new command
> line options. Behavior is solely driven by the attributes of any incoming .reloc
> sections (of which there would be none by default, retaining original behavior).
> The important patch is [1], but at least the first patch of the series [2] would
> in most cases also be wanted/needed (patch 04 is obviously a mechanical prereq
> for the main patch). Need for other of the prereqs there depends on the scope
> and purpose of one's binutils build(s).
> 
> Jan
> 
> [1] https://sourceware.org/pipermail/binutils/2025-August/143153.html
> [2] https://sourceware.org/pipermail/binutils/2025-August/143141.html

That sounds great!
It's clearly better to fix the issue by changing/improving binutils.
Let's drop my patch in Xen if this gets accepted in binutils!
It would be nice if you could keep us posted in xen-devel of the 
status/progress of the binutils patches.

By the number of patches needed for binutils it seems you opened a can 
of worms/pandora box with this issue ^^

Also, in patch 12/17, you state that the logic would be that if .reloc 
is generated partly by the code itself instead of solely by the linker 
this means we want to use the section at runtime.
While I kind of understand this idea, it also feels a bit as a hack, 
doesn't it?
One could argue that even if .reloc is just generated by the linker, the
program could still want to access it at runtime.
I've looked at Xen code to see if it does put something in .reloc itself 
and it seems so: 
https://elixir.bootlin.com/xen/v4.20.1/source/xen/arch/x86/efi/relocs-dummy.S
The "code puts data in .reloc section" would just serve as a "hint" for 
the linker if I understand your patch well, just as well as a 
`--keep-reloc` command line option would.

Anyway, I won't comment much further on the binutils patchset since I'm 
not well versed in linker black magic.

Thanks for the patchset!

Yann

-- 


Yann Sionneau | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech
Re: [PATCH v2] xen/x86: fix xen.efi boot crash from some bootloaders
Posted by Jan Beulich 2 months ago
On 25.08.2025 16:17, Yann Sionneau wrote:
> On 8/4/25 11:34, Jan Beulich wrote:
>> On 24.07.2025 16:07, 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@suse.com/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@vates.tech>
>>
>> So I found a way to deal with this at the linker side, without any new command
>> line options. Behavior is solely driven by the attributes of any incoming .reloc
>> sections (of which there would be none by default, retaining original behavior).
>> The important patch is [1], but at least the first patch of the series [2] would
>> in most cases also be wanted/needed (patch 04 is obviously a mechanical prereq
>> for the main patch). Need for other of the prereqs there depends on the scope
>> and purpose of one's binutils build(s).
>>
>> [1] https://sourceware.org/pipermail/binutils/2025-August/143153.html
>> [2] https://sourceware.org/pipermail/binutils/2025-August/143141.html
> 
> That sounds great!
> It's clearly better to fix the issue by changing/improving binutils.
> Let's drop my patch in Xen if this gets accepted in binutils!

Luckily I'm in a position where I don't need "acceptance", but merely
"absence of objections". The sole reason for the present delay is with
a colliding MIPS patch, which I'd rather see go in first.

> It would be nice if you could keep us posted in xen-devel of the 
> status/progress of the binutils patches.

I'll try to remember.

> By the number of patches needed for binutils it seems you opened a can 
> of worms/pandora box with this issue ^^

Well, that's been only one of the tinier cans.

> Also, in patch 12/17, you state that the logic would be that if .reloc 
> is generated partly by the code itself instead of solely by the linker 
> this means we want to use the section at runtime.
> While I kind of understand this idea, it also feels a bit as a hack, 
> doesn't it?

Yes and no. Assigning purpose to sections merely from their names is
already a hack. Yet since we need to live with that concept on PE/COFF
(and even ELF is quite far from being free of such), making this small
extra distinction feels quite acceptable to me.

> One could argue that even if .reloc is just generated by the linker, the
> program could still want to access it at runtime.

Not really, no. Ordinary programs hardly have a need to access their
own .reloc. And if so, having a simple, command-line-option-less way
to distinguish both intentions is probably the best we can have for
both worlds.

> I've looked at Xen code to see if it does put something in .reloc itself 
> and it seems so: 
> https://elixir.bootlin.com/xen/v4.20.1/source/xen/arch/x86/efi/relocs-dummy.S
> The "code puts data in .reloc section" would just serve as a "hint" for 
> the linker if I understand your patch well, just as well as a 
> `--keep-reloc` command line option would.

And something similar would then be needed for the other two linking
steps, just that there would be no actual data in that .reloc section
"contribution".

Jan
Re: [PATCH v2] xen/x86: fix xen.efi boot crash from some bootloaders
Posted by Jan Beulich 2 weeks, 3 days ago
On 25.08.2025 16:29, Jan Beulich wrote:
> On 25.08.2025 16:17, Yann Sionneau wrote:
>> On 8/4/25 11:34, Jan Beulich wrote:
>>> On 24.07.2025 16:07, 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@suse.com/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@vates.tech>
>>>
>>> So I found a way to deal with this at the linker side, without any new command
>>> line options. Behavior is solely driven by the attributes of any incoming .reloc
>>> sections (of which there would be none by default, retaining original behavior).
>>> The important patch is [1], but at least the first patch of the series [2] would
>>> in most cases also be wanted/needed (patch 04 is obviously a mechanical prereq
>>> for the main patch). Need for other of the prereqs there depends on the scope
>>> and purpose of one's binutils build(s).
>>>
>>> [1] https://sourceware.org/pipermail/binutils/2025-August/143153.html
>>> [2] https://sourceware.org/pipermail/binutils/2025-August/143141.html
>>
>> That sounds great!
>> It's clearly better to fix the issue by changing/improving binutils.
>> Let's drop my patch in Xen if this gets accepted in binutils!
> 
> Luckily I'm in a position where I don't need "acceptance", but merely
> "absence of objections". The sole reason for the present delay is with
> a colliding MIPS patch, which I'd rather see go in first.
> 
>> It would be nice if you could keep us posted in xen-devel of the 
>> status/progress of the binutils patches.
> 
> I'll try to remember.

Here you go - the series went in late last week.

Jan
Re: [PATCH v2] xen/x86: fix xen.efi boot crash from some bootloaders
Posted by Yann Sionneau 2 weeks, 2 days ago
On 10/13/25 13:41, Jan Beulich wrote:
> On 25.08.2025 16:29, Jan Beulich wrote:
>> On 25.08.2025 16:17, Yann Sionneau wrote:
>>> On 8/4/25 11:34, Jan Beulich wrote:
>>>> On 24.07.2025 16:07, 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@suse.com/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@vates.tech>
>>>>
>>>> So I found a way to deal with this at the linker side, without any new command
>>>> line options. Behavior is solely driven by the attributes of any incoming .reloc
>>>> sections (of which there would be none by default, retaining original behavior).
>>>> The important patch is [1], but at least the first patch of the series [2] would
>>>> in most cases also be wanted/needed (patch 04 is obviously a mechanical prereq
>>>> for the main patch). Need for other of the prereqs there depends on the scope
>>>> and purpose of one's binutils build(s).
>>>>
>>>> [1] https://sourceware.org/pipermail/binutils/2025-August/143153.html
>>>> [2] https://sourceware.org/pipermail/binutils/2025-August/143141.html
>>>
>>> That sounds great!
>>> It's clearly better to fix the issue by changing/improving binutils.
>>> Let's drop my patch in Xen if this gets accepted in binutils!
>>
>> Luckily I'm in a position where I don't need "acceptance", but merely
>> "absence of objections". The sole reason for the present delay is with
>> a colliding MIPS patch, which I'd rather see go in first.
>>
>>> It would be nice if you could keep us posted in xen-devel of the
>>> status/progress of the binutils patches.
>>
>> I'll try to remember.
> 
> Here you go - the series went in late last week.
> 
> Jan
> 

Thanks Jan :)

-- 


--
Yann Sionneau | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech
Re: [PATCH v2] xen/x86: fix xen.efi boot crash from some bootloaders
Posted by Andrew Cooper 2 months, 3 weeks ago
On 04/08/2025 10:34 am, Jan Beulich wrote:
> On 24.07.2025 16:07, 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@suse.com/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@vates.tech>
> So I found a way to deal with this at the linker side, without any new command
> line options. Behavior is solely driven by the attributes of any incoming .reloc
> sections (of which there would be none by default, retaining original behavior).
> The important patch is [1], but at least the first patch of the series [2] would
> in most cases also be wanted/needed (patch 04 is obviously a mechanical prereq
> for the main patch). Need for other of the prereqs there depends on the scope
> and purpose of one's binutils build(s).
>
> Jan
>
> [1] https://sourceware.org/pipermail/binutils/2025-August/143153.html
> [2] https://sourceware.org/pipermail/binutils/2025-August/143141.html

That's good to see.  Do we need any matching changes in Xen?

~Andrew

Re: [PATCH v2] xen/x86: fix xen.efi boot crash from some bootloaders
Posted by Jan Beulich 2 months, 3 weeks ago
On 04.08.2025 13:00, Andrew Cooper wrote:
> On 04/08/2025 10:34 am, Jan Beulich wrote:
>> On 24.07.2025 16:07, 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@suse.com/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@vates.tech>
>> So I found a way to deal with this at the linker side, without any new command
>> line options. Behavior is solely driven by the attributes of any incoming .reloc
>> sections (of which there would be none by default, retaining original behavior).
>> The important patch is [1], but at least the first patch of the series [2] would
>> in most cases also be wanted/needed (patch 04 is obviously a mechanical prereq
>> for the main patch). Need for other of the prereqs there depends on the scope
>> and purpose of one's binutils build(s).
>>
>> [1] https://sourceware.org/pipermail/binutils/2025-August/143153.html
>> [2] https://sourceware.org/pipermail/binutils/2025-August/143141.html
> 
> That's good to see.  Do we need any matching changes in Xen?

Well, we'll need some (zero-size) contribution to .reloc then. For my
testing I hacked this into the linking rule (which is a mess already
anyway), but I hope / expect to find a cleaner solution for upstreaming.

Jan