Re: [PATCH v6 51/51] x86/kvm: Get local APIC bus frequency from PV CPUID Timing Info

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing validation of the APIC timer frequency provided by an untrusted CPUID leaf allows a malicious hypervisor to cause a guest interrupt storm or timer freeze.
--

--- Patch [51]: [PATCH v6 51/51] x86/kvm: Get local APIC bus frequency from PV CPUID Timing Info ---
commit f6ea787ae688819e70ecd70def270210b8842a87
Author: Sean Christopherson <[email protected]>

x86/kvm: Get local APIC bus frequency from PV CPUID Timing Info

When running as a KVM guest with PV timing info provided by the host, this
commit updates the code to use the local APIC bus frequency reported in
CPUID.0x40000010.EBX for the APIC timer period instead of calibrating it.

> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index b954238b09fc0..edeff41c03c2a 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
[ ... ]
> @@ -1054,6 +1054,11 @@ static void __init kvm_init_platform(void)
>  			x86_init.hyper.get_tsc_khz = kvm_get_tsc_khz;
>  			x86_init.hyper.get_cpu_khz = kvm_get_tsc_khz;
>  		}
> +
> +		/* The leaf also includes the local APIC bus/timer frequency.*/
> +		apic_khz = cpuid_ebx(timing_info_leaf);
> +		if (apic_khz)
> +			apic_set_timer_frequency_khz(apic_khz, "KVM hypervisor");

[Severity: High]
Does this code allow an untrusted hypervisor to cause a guest interrupt storm
or timer freeze?

In a Confidential Computing (CoCo) environment, the hypervisor could provide a
maliciously small value for apic_khz in CPUID leaf 0x40000010.EBX. When this
value is passed to apic_set_timer_frequency_khz(), it calculates and stores
lapic_timer_period.

Later, calibrate_APIC_clock() bypasses its normal safety checks when
lapic_timer_period is already set:

arch/x86/kernel/apic/apic.c:calibrate_APIC_clock() {
    ...
	if (!lapic_init_clockevent()) {
		apic_pr_verbose("lapic timer already calibrated %d\n", lapic_timer_period);
		/*
		 * Direct calibration methods must have an always running
		 * local APIC timer, no need for broadcast timer.
		 */
		lapic_clockevent.features &= ~CLOCK_EVT_FEAT_DUMMY;
		return 0;
	}
    ...
}

This early return skips the subsequent check:
if (lapic_timer_period < (1000000 / HZ))

Could a malicious hypervisor provide a small enough value that causes the
timer interval (TMICT) to become 1 or 0, stopping the local timer entirely
or causing an endless interrupt storm?

>  	}
>  
>  	/*

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=51