Re: [PATCH v5 04/34] KVM: x86: Add KVM_[GS]ET_CLOCK_GUEST for accurate KVM clock migration

[sashiko-bot@kernel.org]

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

New issues:
- [High] Unbounded bitwise shift in `hvclock_to_hz()` triggered by user-controlled input can cause Undefined Behavior (shift out-of-bounds) and a kernel panic (DoS).
- [Low] The KVM_[GS]ET_CLOCK_GUEST ioctls cause compilation failures in userspace due to an incomplete type in the generic UAPI header.

Pre-existing issues:
- [High] Race condition in `kvm_guest_time_update` reads `kvmclock_offset` unsynchronized, leading to corrupt guest `system_time`.
--

commit fdd3f3e2e20605997654a24ac2e941cc677cf87b
Author: Jack Allister <jalliste@amazon.com>

KVM: x86: Add KVM_[GS]ET_CLOCK_GUEST for accurate KVM clock migration

This commit adds per-vCPU ioctls (KVM_GET_CLOCK_GUEST and KVM_SET_CLOCK_GUEST)
to allow userspace to precisely save and restore the arithmetic relationship
between the guest TSC and the KVM clock across live migrations.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index d9ef165df6a14..b7e5f6e3dc6c1 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c

[ ... ]

> +static u64 hvclock_to_hz(u32 mul, s8 shift)
> +{
> +	u64 tm = NSEC_PER_SEC << 32;
> +
> +	/* Maximise precision. Shift right until the top bit is set */
> +	tm <<= 2;
> +	shift += 2;
> +
> +	/* While 'mul' is even, increase the shift *after* the division */
> +	while (!(mul & 1)) {
> +		shift++;
> +		mul >>= 1;
> +	}
> +
> +	tm /= mul;
> +
> +	if (shift > 0)
> +		return tm >> shift;

[Severity: High]
Can this result in an undefined bitwise shift? If userspace provides a
tsc_shift of 32 and a tsc_to_system_mul with 31 trailing zeroes (like
0x80000000), the loop will increment the shift 31 times. This results in a
total shift amount of 65. Since tm is a 64-bit integer, shifting it by 65
bits triggers undefined behavior. Does the validation in
kvm_vcpu_ioctl_set_clock_guest() need to enforce a limit on the combined
shift amount?

> +	else
> +		return tm << -shift;
> +}

[ ... ]

> +static int kvm_vcpu_ioctl_set_clock_guest(struct kvm_vcpu *v, void __user *argp)
> +{

[ ... ]

> +	/*
> +	 * Calculate the guest TSC at the new reference point, and the
> +	 * corresponding KVM clock value according to user_hv_clock.
> +	 * Adjust kvmclock_offset so both definitions agree.
> +	 */
> +	guest_tsc = kvm_read_l1_tsc(v, ka->master_cycle_now);
> +	user_clk_ns = __pvclock_read_cycles(&user_hv_clock, guest_tsc);
> +	ka->kvmclock_offset = user_clk_ns - ka->master_kernel_ns;

[Severity: High]
This is a pre-existing issue, but is it possible for concurrent updates to
cause a torn read in kvm_guest_time_update() now that this ioctl modifies
kvmclock_offset?

In kvm_guest_time_update(), master_kernel_ns is read safely inside the
pvclock_sc seqcount loop, but kvmclock_offset is read outside of it:

kvm_guest_time_update() {
        do {
                seq = read_seqcount_begin(&ka->pvclock_sc);
                ...
                master_kernel_ns = ka->master_kernel_ns;
        } while (read_seqcount_retry(&ka->pvclock_sc, seq));
        ...
        hv_clock.system_time = kernel_ns + v->kvm->arch.kvmclock_offset;
}

If kvm_vcpu_ioctl_set_clock_guest() modifies both variables under the seqcount
write lock after the reader finishes the loop but before reading
kvmclock_offset, couldn't the guest receive a stale kernel_ns combined with a
new kvmclock_offset?

> +
> +out:
> +	kvm_end_pvclock_update(kvm);
> +	return rc;
> +}

[ ... ]

> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 6c8afa2047bf3..9b50191b859cd 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1669,4 +1669,7 @@ struct kvm_pre_fault_memory {
>  	__u64 padding[5];
>  };
>  
> +#define KVM_SET_CLOCK_GUEST	_IOW(KVMIO, 0xd6, struct pvclock_vcpu_time_info)
> +#define KVM_GET_CLOCK_GUEST	_IOR(KVMIO, 0xd7, struct pvclock_vcpu_time_info)
> +

[Severity: Low]
Does this cause compilation failures for userspace applications? The ioctls
use struct pvclock_vcpu_time_info which is defined in asm/pvclock-abi.h, but
that header is not included by linux/kvm.h. Since UAPI headers should be
self-contained, could this result in an 'invalid application of sizeof to
incomplete type' error?

>  #endif /* __LINUX_KVM_H */

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