Re: [PATCH v5 14/34] KVM: x86: Fix compute_guest_tsc() to handle negative time deltas

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Unsafe unsigned comparison in `kvm_guest_time_update` defeats the monotonicity guard when `compute_guest_tsc` mathematically wraps below zero.
- [High] `__pvclock_read_cycles` passes a wrapped negative delta to `pvclock_scale_delta()`, resulting in a wildly corrupted scaled clock offset.
--

commit 2283f250a7b73885318539a7dbf8a833e3f3f4ce
Author: David Woodhouse <dwmw@amazon.co.uk>

KVM: x86: handle negative deltas in compute_guest_tsc()

The compute_guest_tsc() function computes the guest TSC at a given
kernel_ns timestamp. When the master clock reference point is earlier
than vcpu->arch.this_tsc_nsec, the delta wraps to a huge positive number.
This commit handles negative deltas explicitly by negating, scaling,
and subtracting from this_tsc_write.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index fc9366b83912..8aae22401046 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2588,11 +2588,21 @@ static int kvm_set_tsc_khz(struct kvm_vcpu *vcpu, u32 user_tsc_khz)
>  
>  static u64 compute_guest_tsc(struct kvm_vcpu *vcpu, s64 kernel_ns)
>  {
> -	u64 tsc = pvclock_scale_delta(kernel_ns-vcpu->arch.this_tsc_nsec,
> -				      vcpu->arch.virtual_tsc_mult,
> -				      vcpu->arch.virtual_tsc_shift);
> -	tsc += vcpu->arch.this_tsc_write;
> -	return tsc;
> +	s64 delta_ns = kernel_ns - vcpu->arch.this_tsc_nsec;
> +	u64 tsc;
> +
> +	/* Handle negative deltas gracefully (master clock ref may be earlier) */
> +	if (delta_ns < 0) {
> +		tsc = pvclock_scale_delta(-delta_ns,
> +					  vcpu->arch.virtual_tsc_mult,
> +					  vcpu->arch.virtual_tsc_shift);
> +		return vcpu->arch.this_tsc_write - tsc;

[Severity: High]
This isn't a bug introduced by this patch, but does this incomplete fix expose
a logic error in the caller when the subtraction mathematically wraps below
zero?

If this_tsc_write is smaller than the scaled tsc (for example, if the guest
recently wrote 0 to the TSC), the return value wraps to a huge unsigned u64.

In the caller kvm_guest_time_update(), we see this unsigned comparison:

arch/x86/kvm/x86.c:kvm_guest_time_update() {
    ...
    u64 tsc = compute_guest_tsc(v, kernel_ns);
    if (tsc > tsc_timestamp) {
        adjust_tsc_offset_guest(v, tsc - tsc_timestamp);
        tsc_timestamp = tsc;
    }
    ...
}

Because the wrapped negative tsc appears as a huge positive number, this
evaluates to true. adjust_tsc_offset_guest() is then called with the wrapped
value, forcing the guest TSC to leap backwards.

Can this break the strict monotonicity guarantee and cause time anomalies or
guest kernel panics?

> +	}
> +
> +	tsc = pvclock_scale_delta(delta_ns,
> +				  vcpu->arch.virtual_tsc_mult,
> +				  vcpu->arch.virtual_tsc_shift);

[Severity: High]
This isn't a bug introduced by this patch, but does __pvclock_read_cycles()
suffer from the exact same scaling corruption?

When __get_kvmclock() calls __pvclock_read_cycles(), it uses rdtsc() for the
local TSC and ka->master_cycle_now for the timestamp:

arch/x86/kvm/x86.c:__get_kvmclock() {
    ...
    data->clock = __pvclock_read_cycles(&hv_clock, data->host_tsc);
    ...
}

If the local CPU's TSC is slightly behind the CPU that recorded the master
clock due to normal cross-CPU TSC skew, tsc < src->tsc_timestamp can occur:

arch/x86/include/asm/pvclock.h:__pvclock_read_cycles() {
    ...
    u64 delta = tsc - src->tsc_timestamp;
    u64 offset = pvclock_scale_delta(delta, src->tsc_to_system_mul,
                                     src->tsc_shift);
    ...
}

This causes delta to mathematically wrap to a huge unsigned number, which is
then passed directly to pvclock_scale_delta().

Could this produce an astronomical clock offset that gets returned to
userspace via the KVM_GET_CLOCK ioctl?

> +	return vcpu->arch.this_tsc_write + tsc;
>  }

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