Re: [PATCH] x86/hvmloader: fix usage of NULL with cpuid_count()

[Alejandro Vallejo <agarciav@xxxxxxx>]

On Fri Apr 25, 2025 at 1:44 PM BST, Roger Pau Monné wrote:
> On Fri, Apr 25, 2025 at 01:23:30PM +0100, Alejandro Vallejo wrote:
>> On Thu Apr 24, 2025 at 1:58 PM BST, Roger Pau Monne wrote:
>> > The commit that added support for retrieving the APIC IDs from the APs
>> > introduced several usages of cpuid() with NULL parameters, which is not
>> > handled by the underlying implementation.  For GCC I expect this results in
>> > writes to the physical address at 0, however for Clang the generated code
>> > in smp.o is:
>> >
>> > tools/firmware/hvmloader/smp.o: file format elf32-i386
>> >
>> > Disassembly of section .text:
>> >
>> > 00000000 <smp_initialise>:
>> >        0: 55                            pushl   %ebp
>> >        1: 89 e5                         movl    %esp, %ebp
>> >        3: 53                            pushl   %ebx
>> >        4: 31 c0                         xorl    %eax, %eax
>> >        6: 31 c9                         xorl    %ecx, %ecx
>> >        8: 0f a2                         cpuid
>> >
>> > Showing the usage of a NULL pointer results in undefined behavior, and
>> > clang refusing to generate further code after it.
>> >
>> > Fix by using a temporary variable in cpuid_count() in place for any NULL
>> > parameter.
>> >
>> > Fixes: 9ad0db58c7e2 ('tools/hvmloader: Retrieve APIC IDs from the APs 
>> > themselves')
>> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>> 
>> Ugh, that's on me. I was sure I saw the pattern in Xen (from where the
>> code came from), but clearly I hallucinated.
>> 
>> > ---
>> > Could also be fixed by using the temporary variable in the call sites,
>> > however that's more code in the call sites at the expense of less checking.
>> > I don't think the extra NULL check logic in cpuid_count() is that bad.
>> >
>> > Overall the solution proposed in this patch is safer going forward, as it
>> > prevent issues like this from being introduced in the first place.
>> 
>> Might be worth moving this same extra checks onto Xen's cpuid. There's
>> no shortage of `junk` variables at the callsites.
>> 
>> > ---
>> >  tools/firmware/hvmloader/util.h | 11 +++++++++++
>> >  1 file changed, 11 insertions(+)
>> >
>> > diff --git a/tools/firmware/hvmloader/util.h 
>> > b/tools/firmware/hvmloader/util.h
>> > index 644450c51ceb..765a013ddd9e 100644
>> > --- a/tools/firmware/hvmloader/util.h
>> > +++ b/tools/firmware/hvmloader/util.h
>> > @@ -190,6 +190,17 @@ static inline void cpuid_count(
>> >      uint32_t *ecx,
>> >      uint32_t *edx)
>> >  {
>> > +    uint32_t tmp;
>> > +
>> > +    if ( !eax )
>> > +        eax = &tmp;
>> > +    if ( !ebx )
>> > +        ebx = &tmp;
>> > +    if ( !ecx )
>> > +        ecx = &tmp;
>> > +    if ( !edx )
>> > +        edx = &tmp;
>> > +
>> 
>> A somewhat more compact alternative that doesn't require tmp would be:
>> 
>>   eax = eax ?: &leaf;
>>   ebx = ebx ?: &leaf;
>>   ecx = ecx ?: &leaf;
>>   edx = edx ?: &leaf;
>
> But that performs the write unconditionally?  It might be more compact
> code-wise, but might incur in an unneeded store?

Pretty sure it would all optimise to very similar, if not identical code.

>
>> It clobbers `leaf`, but only after it's no longer relevant.
>
> My preference was to use a explicitly tmp variable, but I could switch
> to using leaf if that's the preference.  Given that Andrew has already
> Acked the current version I'm tempted to just go with what has already
> been Acked.

That's perfectly fine. It was merely a cosmetic nit. LGTM as it is too.

>
> Thanks, Roger.

Cheers,
Alejandro