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

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

[Roger Pau Monne]

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@citrix.com>
---
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;
+
     asm volatile ( "cpuid"
                    : "=a" (*eax), "=b" (*ebx), "=c" (*ecx), "=d" (*edx)
                    : "a" (leaf), "c" (subleaf) );
-- 
2.48.1

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

[Andrew Cooper]

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.

> 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@citrix.com>
> ---
> 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@citrix.com>, preferably with
clarity over the disassembly.

>      asm volatile ( "cpuid"
>                     : "=a" (*eax), "=b" (*ebx), "=c" (*ecx), "=d" (*edx)
>                     : "a" (leaf), "c" (subleaf) );

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

[Roger Pau Monné]

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

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

[Alejandro Vallejo]

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@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;

It clobbers `leaf`, but only after it's no longer relevant.

>      asm volatile ( "cpuid"
>                     : "=a" (*eax), "=b" (*ebx), "=c" (*ecx), "=d" (*edx)
>                     : "a" (leaf), "c" (subleaf) );

Cheers,
Alejandro

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

[Roger Pau Monné]

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@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?

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

Thanks, Roger.

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

[Alejandro Vallejo]

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