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

[Alejandro Vallejo <agarciav@xxxxxxx>]

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;

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