Re: [PATCH v5 31/34] KVM: x86: Use ktime_get_snapshot_id() for master clock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Uninitialized stack memory used on failure path in KVM timekeeping.
- [Low] Unused static functions left in the code, causing `-Wunused-function` warnings.
--

commit 25989314a763f4ae38067d7da21492c86ffa8fa4
Author: David Woodhouse <dwmw@amazon.co.uk>

Replace the KVM-private vgettsc()/do_kvmclock_base()/do_monotonic()/
do_realtime() timekeeping reimplementation with calls to the generic
ktime_get_snapshot_id() interface.

This patch replaces the KVM-private timekeeping functions with generic
ktime_get_snapshot_id() calls. It provides system time and raw cycles 
atomically paired to simplify the timekeeping state.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 96250264d403..2713aebb96ae 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
[ ... ]
> @@ -3162,14 +3163,32 @@ static int do_realtime(struct timespec64 *ts, u64 *tsc_timestamp)

[Severity: Low]
The patch description mentions replacing the private timekeeping functions
(vgettsc, do_kvmclock_base, do_monotonic, and do_realtime), but it appears
they were not removed from the source code.

Does leaving these unused static functions trigger -Wunused-function
compiler warnings during the build?

>   * reports the TSC value from which it do so. Returns true if host is
>   * using TSC based clocksource.
>   */
> +static bool kvm_snapshot_has_tsc(struct system_time_snapshot *snap,
> +				u64 *tsc_timestamp)
> +{
> +	if (snap->cs_id == CSID_X86_TSC) {
> +		*tsc_timestamp = snap->cycles;
> +		return true;
> +	}
> +
> +	if (snap->hw_csid == CSID_X86_TSC && snap->hw_cycles) {
> +		*tsc_timestamp = snap->hw_cycles;
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
>  static bool kvm_get_time_and_clockread(s64 *kernel_ns, u64 *tsc_timestamp)
>  {
> -	/* checked again under seqlock below */
> -	if (!gtod_is_based_on_tsc(pvclock_gtod_data.clock.vclock_mode))
> +	struct system_time_snapshot snap;
> +
> +	ktime_get_snapshot_id(CLOCK_MONOTONIC_RAW, &snap);
> +	if (!kvm_snapshot_has_tsc(&snap, tsc_timestamp))

[Severity: Critical]
If ktime_get_snapshot_id() encounters a failure (such as when timekeeping is
suspended), it only sets snap.valid = false and returns immediately, leaving
the rest of the struct uninitialized.

Since snap.valid is not checked before calling kvm_snapshot_has_tsc(), could
this pass uninitialized stack memory into the helper?

If the uninitialized snap->cs_id happens to match CSID_X86_TSC, could KVM
expose uninitialized host kernel stack data to the guest VM?

>  		return false;
>  
> -	return gtod_is_based_on_tsc(do_kvmclock_base(kernel_ns,
> -						     tsc_timestamp));
> +	*kernel_ns = ktime_to_ns(ktime_mono_to_any(snap.systime, TK_OFFS_BOOT));
> +	return true;
>  }

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