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