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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.07.2025 16:38, Andrew Cooper wrote:
> On 23/07/2025 3:21 pm, Jan Beulich wrote:
>> On 23.07.2025 16:13, Andrew Cooper wrote:
>>> On 23/07/2025 2:56 pm, 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.
>>>>
>>>> Signed-off-by: Yann Sionneau <yann.sionneau@xxxxxxxxxx>
>>>> ---
>>>>  xen/Makefile           |   5 +-
>>>>  xen/arch/x86/Makefile  |   1 +
>>>>  xen/tools/Makefile     |   3 ++
>>>>  xen/tools/keeprelocs.c | 119 +++++++++++++++++++++++++++++++++++++++++
>>>>  4 files changed, 127 insertions(+), 1 deletion(-)
>>>>  create mode 100644 xen/tools/keeprelocs.c
>>> I'm sick and tired of the hoops we have to jump through for broken
>>> tooling.  This is now rewriting the PE+ metadata because apparently the
>>> linker can't do it correctly.
>> The linker is doing it correctly. It is us (and very likely just us) who
>> have special needs here.
>>
>>> Either fix the linker (or the way we drive it/etc), or we're doing away
>>> with PE+ emulation entirely and writing the MZ/PE headers by hand like
>>> literally every other kernel does.
>> "Fixing" the linker was suggested, but with my binutils maintainer hat on
>> I'm inclined to not accept a Xen-only option into the linker.
> 
> Either Xen is doing something wrong, and should be doing it differently,

Well, no-one else would require access to its own executable's .reloc
section. Of course we can do things differently (like duplicate .reloc
into .init.data), but why have the same data in two places?

> or Xen is doing something right and the tooling is
> wrong/incomplete/whatever.
> 
> As a related question, is anyone other than Xen using PE+ emulation in
> anger?  Every other kernel/OS level tool I'm aware of writes the MZ/PE
> header by hand, and frankly, the list of bugs we've found in PE+
> emulation would strongly suggest that noone else is using it.

Both Cygwin and MinGW definitely are using it. And in Windows it's the
PE loader which reads the .reloc section. The executables themselves
wouldn't access the section, at least not normally.

One problem is that Cygwin and MinGW are lacking proper maintainers in
binutils. Hence changes are being made and approved by people (including
me) who may overlook certain aspects. Another problem is that testsuite
coverage is far more scarce for PE than it is for ELF. And then,
specifically for us, we're further making use of an entirely untested
(upstream) code path, linking ELF objects into PE binaries.

Jan