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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.07.2025 17:39, Yann Sionneau wrote:
> On 7/23/25 16:18, Jan Beulich wrote:
>> 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.
> 
> What would you like to call it?

peedit or editpe or some such? And then of course have it have a command
line option indicating to remove the one flag from the one section.

Thinking of it, binutils having elfedit, it may be an option to actually
have peedit there, in sufficiently generalized form.

>> 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.
> 
> Ah, sorry I didn't know about the coding style, I'll reformat it then.
> Is there a correct .clang-format file somewhere or a checkpatch.pl 
> equivalent?

Sadly not. All we have is ./CODING_STYLE and a lot of unwritten rules.

>>> +           case 'q':
>>> +                   quiet = 1;
>>> +                   break;
>>> +           case 'h':
>>> +                   print_usage(prog_name);
>>> +                   return 0;
>>> +                   break;
>>
>> "break" after "return"?
> This needs to go.
>>
>>> +           case '?':
>>
>> Why is this not the same as 'h'?
> One returns 0 because help is asked for so it's not an error.
> The other one is when using non-existing argument which is an error.

But a user passing -? deserves to be shown help output, just like you
do for -h?

>>> +   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.
> 
> Based on 
> https://learn.microsoft.com/en-us/windows/win32/debug/pe-format#coff-file-header-object-and-image
> My understanding is that SizeOfOptionalHeader member can be 0, for 
> object files.
> But we don't want an object file here, we want an image file.
> However, the optional header is required for image files (thus the != 0 
> check):
> 
> "Every image file has an optional header that provides information to 
> the loader."
> 
> But, we really don't know its size, moreover it's even different for 
> PE32 vs PE32+.

Yet surely we know 1 is still too little, for example?

Jan