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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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:
>>>> The format of the buildid is a property of the binary, not a property of 
>>>> how
>>>> it was loaded.  This fixes buildid recognition when starting xen.efi from 
>>>> it's
>>>> MB2 entrypoint.
>>>>
>>>> Suggested-by: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>> I'll pick this up without a Fixes: tag, but I think it ought to have one. (I
>>> didn't check whether MB2 or build-id support came later, hence introducing 
>>> the
>>> issue.)
>> MB2+EFI came long before any buildid support.  If you want a fixes tag,
>> it's eee5909e9d1
> That commit talks of an earlier hack, though. And no, MB2 work came later,
> albeit still in the 4.9 dev window (commit 9180f53655245).

The "previous hack" was embedding note.o (from the livepatch test
infrastructure) back in the main xen binary.

That's still present.  For xen.gz you get an elf note.  For xen.efi, it
may be an elf note or a CodeView region, depending on the toolchain.

>
>>>> --- a/xen/common/version.c
>>>> +++ b/xen/common/version.c
>>>> @@ -203,8 +203,11 @@ void __init xen_build_init(void)
>>>>      rc = xen_build_id_check(n, sz, &build_id_p, &build_id_len);
>>>>  
>>>>  #ifdef CONFIG_X86
>>>> -    /* Alternatively we may have a CodeView record from an EFI build. */
>>>> -    if ( rc && efi_enabled(EFI_LOADER) )
>>>> +    /*
>>>> +     * xen.efi built with a new enough toolchain will have a CodeView 
>>>> record,
>>>> +     * not an ELF note.
>>>> +     */
>>>> +    if ( rc )
>>> Instead of dropping the efi_enabled(), I think you want to replace 
>>> EFI_LOADER
>>> by EFI_BOOT.
>> No - that's differently buggy.  I suppose the commit message wasn't
>> clear enough?
>>
>> We'd still have a CodeView record if we booted xen.efi using it's MB2
>> entrypoint without the EFI extensions.
> Hmm, if that's a possible mode of operation (as said in reply to Marek, I
> wasn't aware of that) - yes.

It's how I found the bug.

There's an awful lot of hackery in the patch-queue holding it together,
but in this case it's not really about MB2 or anything else; it's
xen_build_init() being incorrect in how it determines whether there's a
CodeView region or not.

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

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.

~Andrew