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

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

On 7/23/25 17:54, Jan Beulich wrote:
> 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.

Honestly, I don't think peedit is a nice name, also, both peedit and 
editpe already exist.

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

yes why not, but I don't have it in my todo list to make a generalized 
PE header editor. I think this carries me too far away from the original 
bugfix.

> 
>>> 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
> 


-- 


Yann Sionneau | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech