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....@citrix.com>
>>
>> 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
