Re: [PATCH v7 35/36] KVM: selftests: Add Xen/generic CPUID timing leaf test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The TSC scale ratio verification test will spuriously fail if the host TSC frequency is within KVM's 250 ppm tolerance of a target frequency.
- [Medium] Unsafe `realloc` pattern overwrites the original pointer, leading to a memory leak and immediate NULL pointer dereference on allocation failure.
- [Low] Commit message typo: duplicates 'KVM_VCPU_TSC_SCALE'.
--

--- Patch [35]: [PATCH v7 35/36] KVM: selftests: Add Xen/generic CPUID timing leaf test ---
commit f482c86f3dcfdbeb8ea33ae05535bd28d7cd5656
Author: David Woodhouse <dwmw@amazon.co.uk>

KVM: selftests: Add Xen/generic CPUID timing leaf test

This commit adds a test to verify that userspace can correctly populate Xen
and generic CPUID timing leaves. It validates this using native and scaled
TSC frequencies.

> Verify that userspace can correctly populate Xen and generic CPUID
> timing leaves using the KVM_VCPU_TSC_SCALE and
> KVM_VCPU_TSC_SCALE attributes.

[Severity: Low]
This isn't a bug, but there's a typo in the commit message where
"KVM_VCPU_TSC_SCALE" is duplicated instead of mentioning both
KVM_GET_TSC_KHZ and KVM_VCPU_TSC_SCALE.

> diff --git a/tools/testing/selftests/kvm/x86/xen_cpuid_timing_test.c b/tools/testing/selftests/kvm/x86/xen_cpuid_timing_test.c
> new file mode 100644
> index 0000000000000..2098cb511ef02
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/x86/xen_cpuid_timing_test.c

[ ... ]

> +static void add_cpuid_entry(struct kvm_vcpu *vcpu, uint32_t function,
> +			    uint32_t index, uint32_t eax, uint32_t ebx,
> +			    uint32_t ecx, uint32_t edx)
> +{
> +	struct kvm_cpuid2 *cpuid = vcpu->cpuid;
> +	struct kvm_cpuid_entry2 *entry;
> +	int n = cpuid->nent;
> +
> +	vcpu->cpuid = realloc(vcpu->cpuid,
> +			      sizeof(*cpuid) + (n + 1) * sizeof(*entry));
> +	cpuid = vcpu->cpuid;
> +	cpuid->nent = n + 1;

[Severity: Medium]
Will this cause a memory leak and immediate NULL pointer dereference if
realloc() fails?

If realloc() returns NULL due to memory exhaustion, the original pointer in
vcpu->cpuid is overwritten, leaking the memory. Then cpuid->nent = n + 1
will immediately dereference that NULL pointer.

[ ... ]

> +static void run_test(uint64_t tsc_khz)
> +{

[ ... ]

> +	effective_tsc_khz = __vcpu_ioctl(vcpu, KVM_GET_TSC_KHZ, NULL);
> +	bus_cycle_ns = vm_check_cap(vm, KVM_CAP_X86_APIC_BUS_CYCLES_NS);
> +	effective_bus_khz = bus_cycle_ns > 0 ? 1000000 / bus_cycle_ns : 1000000;
> +
> +	/* If scaling wasn't applied, skip this frequency */
> +	if (tsc_khz && effective_tsc_khz == host_khz) {

[Severity: Medium]
Could this check fail to skip properly if the requested frequency is within 
KVM's 250 ppm tolerance of the host TSC? 

If the target frequency is close to the host frequency (within tolerance), 
KVM disables TSC scaling (setting the ratio to 1.0) but still returns the 
requested frequency in effective_tsc_khz. Since effective_tsc_khz differs
from host_khz, this check evaluates to false even though scaling wasn't 
actually applied.

> +		pr_info("  TSC scaling not available, skipping\n");
> +		kvm_vm_free(vm);
> +		return;
> +	}

[ ... ]

> +			if (tsc_khz) {
> +				uint64_t computed = ((__uint128_t)host_khz * scale.ratio) >> scale.frac_bits;
> +				int64_t diff = (int64_t)computed - (int64_t)effective_tsc_khz;
> +
> +				TEST_ASSERT(diff >= -1 && diff <= 1,
> +					    "TSC_SCALE ratio mismatch: computed %lu vs effective %u (diff %ld)",
> +					    computed, effective_tsc_khz, diff);

[Severity: Medium]
Does this cause a spurious test failure due to the unskipped ppm tolerance 
case above?

If scaling wasn't applied but the test didn't skip (because the ratio is 1.0), 
computed will equal host_khz. The assertion will then check if 
host_khz is within 1 kHz of effective_tsc_khz. If the difference 
between them was just large enough (e.g., 100 kHz) but still within KVM's 
ppm tolerance, this assertion will fail.

> +			}
> +		}
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728144954.355376-1-dwmw2@infradead.org?part=35