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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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

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