Re: [Devel] [PATCH hci-8.1 v5 5/5] selftests/ve: check CPUID override emulation for a prefixed CPUID

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] selftests/ve: do_cpuid_prefixed() 32-bit xchg of ebx truncates callee-saved RBX on x86_64
--

commit a68470c5110f8f0b39dbfe6d2c82d537ef16105e
Author: Konstantin Khorenko <khorenko@virtuozzo.com>

selftests/ve: check CPUID override emulation for a prefixed CPUID

This commit adds a selftest that executes CPUID with an operand-size
prefix (0x66) and expects the overridden table values. It adds a
do_cpuid_prefixed() helper, a prefixed flag in struct child_ctx, and a new
test prefixed_cpuid_is_emulated.

Link: https://virtuozzo.atlassian.net/browse/VSTOR-148843

> diff --git a/tools/testing/selftests/ve/ve_cpuid_override_test.c b/tools/testing/selftests/ve/ve_cpuid_override_test.c
> --- a/tools/testing/selftests/ve/ve_cpuid_override_test.c
> +++ b/tools/testing/selftests/ve/ve_cpuid_override_test.c

[ ... ]

> +/* CPUID with an operand-size prefix, which the CPU accepts and ignores. */
> +static void do_cpuid_prefixed(unsigned int leaf, struct cpuid_regs *r)
> +{
> +	/* ebx is the PIC base on i386; swap it out like <cpuid.h> does. */
> +	asm volatile("xchg %%ebx, %1\n\t"
> +		     ".byte 0x66, 0x0f, 0xa2\n\t"
> +		     "xchg %%ebx, %1"
> +		     : "=a" (r->eax), "=&r" (r->ebx), "=c" (r->ecx), "=d" (r->edx)
> +		     : "a" (leaf), "c" (0));
> +}

[Severity: Medium]
Can this asm corrupt RBX on x86_64?

The i386 PIC workaround is applied unconditionally.  On x86_64, a 32-bit
"xchg %%ebx, %1" zero-extends into RBX, and the closing "xchg %%ebx, %1"
only restores the low 32 bits.

RBX is callee-saved and is not listed as clobbered or tied to an operand in
do_cpuid_prefixed().  If the compiler keeps a 64-bit value there, the upper
half is lost.

Since do_cpuid_prefixed() is static, it is likely inlined into
cpuid_child(), which uses ctx and &r right after the asm:

	if (ctx->prefixed)
		do_cpuid_prefixed(CPUID_LEAF, &r);
	else
		do_cpuid(CPUID_LEAF, &r);

	if (write(ctx->result_pipe[1], &r, sizeof(r)) != sizeof(r))

If ctx or &r lives in RBX, could the write() call or the ctx dereference
fault?  That would show up as a SIGSEGV, which is the same symptom this
test uses to detect missing kernel emulation, so the failure could be
misattributed.

Whether a live value actually sits in RBX depends on the compiler version,
optimization level and register allocation.

Would it make sense to guard the swap with #if defined(__i386__) &&
defined(__PIC__), and use a plain cpuid with a "=b" output otherwise?
Declaring rbx as clobbered or using a 64-bit xchg would also work.

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261007-b4-prep-khorenko-vstor-148656-cpuid-v5-0-652c5c9e56da@virtuozzo.com?part=5

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel