Re: [PATCH 1/3] x86/EFI: Fix detection of buildid

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 10/06/2025 9:01 am, Jan Beulich wrote:
> On 06.06.2025 17:01, Andrew Cooper wrote:
>> On 06/06/2025 8:22 am, Jan Beulich wrote:
>>> On 05.06.2025 19:01, Andrew Cooper wrote:
>>>> On 05/06/2025 2:24 pm, Jan Beulich wrote:
>>>>> On 05.06.2025 14:14, Andrew Cooper wrote:
>>>>>> On 05/06/2025 1:02 pm, Jan Beulich wrote:
>>>>>>> On 05.06.2025 13:16, Andrew Cooper wrote:
>>>>>> This really is a property of being a PE32+ binary, and nothing to do
>>>>>> with EFI.
>>>>> Which still can be checked for without having this code path being taken
>>>>> for xen.gz, too: You could e.g. check for &efi > &_end. That's firmly an
>>>>> image property (yet I expect you're going to sigh about yet another hack).
>>>> It's all hacks, but no.
>>>>
>>>> I'm amazed MISRA hasn't spotted that we've got a global `struct efi
>>>> efi;` and a label named efi, creating an alias for the object with it
>>>> out of bounds in the compiled image.  But even then, it's based on
>>>> XEN_BUILD_EFI not XEN_BUILD_PE and does not distinguish the property
>>>> that matters.
>>> The use of XEN_BUILD_EFI in the linker script should have been switched
>>> to XEN_BUILD_PE when the split was introduced.
>> That doesn't build.  As I already explained, the stubs aren't split in a
>> way that allows that.
> Which then is a pretty clear indication that the split was wrong to do in
> the first place, don't you agree?

I think my feelings on how xen.efi was done are quite well known, but so
what?

I've spent longer than I can afford trying to untangle this, and its an
impenetrable mess.

>>>> But the argument I'm going to make this this:  Why do you want a check,
>>>> even if you can find a correct one (and as said before, I cannot)?
>>>>
>>>> This function is run exactly once.  We've excluded "nothing given by the
>>>> toolchain", and excluded "what the toolchain gave us was not the
>>>> expected ELF note".  The only thing left (modulo toolchain bugs) is the
>>>> CodeView region, and if it's not a valid CodeView region then we've
>>>> wasted a handful of cycles.
>>> Two reasons: Having code which cannot possibly do anything useful isn't
>>> good. Misra calls the latest the body of the inner if() "unreachable code"
>>> and objects to the presence of such in a build.
>> It's not unreachable code, not even theoretically.
> How is it not? If we build without this CodeView record, it very much is
> unreachable.

Compiling without a CodeView record doesn't magically cause the prior
logic to guarantee to succeed.

The compiler is forced to emit real conditional logic, and there's
almost 2^96 bit-pattens the toolchain could put into memory which will
very literally reach the CodeView check.

"The toolchain shouldn't cause this path to be executed" is not the same
as genuinely unreachable.

What safety certification is liable to complain about is the inability
to construct a test that demonstrates coverage, but I'm not changing
that property with this patch.
>>> And then, based on your reasoning above, why don't you also drop the
>>> #ifdef CONFIG_X86?
>> Because that's the one non-buggy way of excluding an impossible case.
>>
>> x86 is the only architecture possibly linking with pep emulation, and
>> therefore the only architecture to possibly have a CodeView record.
> And how's the, say, Arm case different from the x86 case with no such
> record built in? 

Because its currently impossible for ARM to have a codeview record.

Remember that ARM writes a MZ/PE header by hand in a flat binary.  It
does not use a PEP linker.

~Andrew