Re: [PATCH] KVM: x86: Guard against division by zero in adjust_lapic_timer_advance()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] kvm: x86: divide by zero in __wait_lapic_expire() with nested TSC scaling
--

commit 615a870f84e9397c4bfba8db0f187034794e3d9e
Author: Peng Hao <flyingpenghao@gmail.com>

KVM: x86: Guard against division by zero in adjust_lapic_timer_advance()

When TSC calibration fails, vcpu->arch.virtual_tsc_khz is zero. This patch
adds a check to avoid division by zero in adjust_lapic_timer_advance().

>     When TSC calibration fails and userspace never programs a tsc khz,
>     vcpu->arch.virtual_tsc_khz stays zero, but the lapic timer's
>     software-mode expiry path still calls adjust_lapic_timer_advance()
>     unconditionally on every timer expiration (unlike
>     __wait_lapic_expire(), this path is not gated on tsc_scaling_ratio).

[ ... ]

> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 48b019114c196..dc4a669d1d460 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -1978,6 +1978,9 @@ static inline void adjust_lapic_timer_advance(struct kvm_vcpu *vcpu,
>  	    abs(advance_expire_delta) < LAPIC_TIMER_ADVANCE_ADJUST_MIN)
>  		return;
>  
> +	if (unlikely(!vcpu->arch.virtual_tsc_khz))
> +		return;
> +

[Severity: High]
Does __wait_lapic_expire() need this same check?

The commit message mentions __wait_lapic_expire() is safely gated by
tsc_scaling_ratio, but is that description accurate when nested
virtualization is enabled?

An L1 guest can enable nested virtualization and set a custom tsc_multiplier
in VMCS12 for its L2 guest. KVM computes vcpu->arch.tsc_scaling_ratio to this
non-default value:

arch/x86/kvm/vmx/nested.c:prepare_vmcs02_early() {
    ...
	vcpu->arch.tsc_scaling_ratio = kvm_calc_nested_tsc_multiplier(
			vcpu->arch.l1_tsc_scaling_ratio,
			vmx_get_l2_tsc_multiplier(vcpu));
    ...
}

If host TSC calibration failed and virtual_tsc_khz is 0, when the LAPIC timer
expires during L2 execution, __wait_lapic_expire() takes the else branch
because tsc_scaling_ratio != default_tsc_scaling_ratio:

arch/x86/kvm/lapic.c:__wait_lapic_expire() {
    ...
	if (vcpu->arch.tsc_scaling_ratio == kvm_caps.default_tsc_scaling_ratio) {
		__delay(min(guest_cycles,
			nsec_to_cycles(vcpu, timer_advance_ns)));
	} else {
		u64 delay_ns = guest_cycles * 1000000ULL;
		do_div(delay_ns, vcpu->arch.virtual_tsc_khz);
		ndelay(min_t(u32, delay_ns, timer_advance_ns));
	}
    ...
}

Can an untrusted L1 guest use this path to reliably crash the host with a
divide-by-zero panic?

>  	/* too early */
>  	if (advance_expire_delta < 0) {
>  		ns = -advance_expire_delta * 1000000ULL;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008090037.86931-1-flyingpeng@tencent.com?part=1