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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Apr 24, 2025 at 03:21:05PM +0100, Andrew Cooper wrote:
> On 24/04/2025 1:58 pm, 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:
> 
> Oh lovely.  I guess we need full VM testing for the Clang builds too.
> 
> > 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
> 
> I get the hint that this is the whole file?  But you don't say that
> explicitly.

Yeah, realized later while reading the message myself.  This is indeed
the whole file.

> > 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>
> > ---
> > 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.
> > ---
> >  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;
> > +
> 
> Personally I dislike this pattern, and some of that is definitely PTSD
> from Xen's original hvm_cpuid() function.
> 
> hvmloader is a small enough codebase that I don't think it matters
> either way.
> 
> Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, preferably with
> clarity over the disassembly.

I've adjusted to:

"""
For GCC I expect this results in writes to the physical address at 0,
however for Clang the generated code in smp.o for the whole file is:

tools/firmware/hvmloader/smp.o: file format elf32-i386
...
"""

Thanks, Roger.