Re: [PATCH 07/17] KVM: pfncache: Add guest-mode pinning (GUEST_USES_PFN successor)

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] kvm: uaf in pfncache due to bypassed vcpu kick on concurrent invalidation
- [Low] kvm: build failure due to non-existent synchronize_srcu_atomic API
--

commit f94174d271f1e35cea5e589d0bf75a4374e505a9
Author: David Woodhouse <dwmw@xxxxxxxxxxxx>

KVM: pfncache: Add guest-mode pinning (GUEST_USES_PFN successor)
    
This commit reinstates support for pfn caches used directly from guest
mode. It attempts to provide guarantees that the vCPU is kicked out
of guest mode and waits when the cache is invalidated, ensuring that
a stale mapping is not used.

Link: https://lore.kernel.org/all/ZBEEQtmtNPaEqU1i@xxxxxxxxxx
Link: https://lore.kernel.org/all/ZQiR8IpqOZrOpzHC@xxxxxxxxxx
Link: https://lore.kernel.org/all/agJnqh7ACEkB-ftx@xxxxxxxxxx
Link: https://lore.kernel.org/all/20260102142429.896101-1-griffoul@xxxxxxxxx

> diff --git a/virt/kvm/pfncache.c b/virt/kvm/pfncache.c
> index 2fe50dfa3fc16..e15d9a475fed7 100644
> --- a/virt/kvm/pfncache.c
> +++ b/virt/kvm/pfncache.c

[ ... ]

> @@ -91,10 +93,60 @@ void gfn_to_pfn_cache_invalidate_start(struct kvm *kvm, unsigned long start,
>  
>  			if (old & GPC_VALID)
>  				cleared = true;
> +
> +			/*
> +			 * If a vCPU has the pfn pinned for direct use from
> +			 * guest mode, it must be forced out of guest mode
> +			 * before the invalidation may complete. The pin bit
> +			 * was set with a cmpxchg only while GPC_VALID held,
> +			 * so this same read of 'old' cannot miss it. It is
> +			 * NOT cleared here: only the vCPU itself (on request
> +			 * service, before re-pinning) or a mutator's xchg
> +			 * (which inherits the kick duty) may clear it. A
> +			 * stale pin costs at most a spurious request bit:
> +			 * kvm_make_vcpus_request_mask() only IPIs and waits
> +			 * for targets actually in guest mode.
> +			 */
> +			if (old & GPC_GUEST_USING) {

[Severity: Critical]
Does the early skip condition in this loop fail to check for GPC_GUEST_USING?

Looking at the existing context just before this hunk in
gfn_to_pfn_cache_invalidate_start():

        old = atomic_read_acquire(&gpc->state);
        if (!(old & (GPC_VALID | GPC_BECOMING_VALID)))
                continue;

If Thread A clears GPC_VALID (but leaves GPC_GUEST_USING set), and
concurrently Thread B runs mmu_notifier_invalidate_range_start(), it seems
Thread B would evaluate the above condition as true, skipping the cache
entirely and ignoring the pin.

Could this allow core MM to free the physical page while the vCPU is still
running in guest mode and using the freed page in its hardware control
structures, leading to a use-after-free?

[ ... ]

> @@ -122,7 +174,7 @@ void gfn_to_pfn_cache_invalidate_start(struct kvm *kvm, unsigned long start,
>  	 * so the grace period never blocks in reclaim either.)
>  	 */
>  	if (cleared)
> -		synchronize_srcu_expedited(&kvm->gpc_srcu);
> +		synchronize_srcu_atomic(&kvm->gpc_srcu);

[Severity: Low]
Is synchronize_srcu_atomic() a defined API in the kernel? It looks like
this patch replaces synchronize_srcu_expedited() with this function, but
it does not appear to exist, which could cause build failures.

[ ... ]

> @@ -498,10 +644,10 @@ static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned l
>  	 * reader-visible fields (uhva, gpa, memslot, and later pfn/khva) be
>  	 * mutated or the old mapping torn down.
>  	 */
> -	must_drain = atomic_xchg(&gpc->state, 0) &
> -		     (GPC_VALID | GPC_INVALIDATING);
> -	if (must_drain)
> -		synchronize_srcu_expedited(&gpc->kvm->gpc_srcu);
> +	old_state = atomic_xchg(&gpc->state, 0);
> +	gpc_kick_pinned_vcpu(gpc, old_state);

[Severity: Critical]
Does clearing the state to 0 here prematurely hide the GPC_GUEST_USING pin
from concurrent MMU notifiers?

It appears this completely wipes the pin before waiting for the vCPU. If a
concurrent invalidation occurs before the vCPU exits, it might bypass the
vCPU kick because the pin was erased, potentially exposing the same
use-after-free window.

> +	if (old_state & (GPC_VALID | GPC_INVALIDATING))
> +		synchronize_srcu_atomic(&gpc->kvm->gpc_srcu);

[Severity: Low]
As noted above, synchronize_srcu_atomic() does not appear to exist.

[ ... ]

> @@ -687,8 +836,9 @@ void kvm_gpc_deactivate(struct gfn_to_pfn_cache *gpc)
>  		 * readers outstanding — it left GPC_INVALIDATING set to say
>  		 * so — and the unmap below must not run under those readers.
>  		 */
> -		if (atomic_xchg(&gpc->state, 0) &
> -		    (GPC_VALID | GPC_INVALIDATING))
> +		old_state = atomic_xchg(&gpc->state, 0);
> +		gpc_kick_pinned_vcpu(gpc, old_state);

[Severity: Critical]
Similarly, does this zeroing of the state in kvm_gpc_deactivate() hide the
GPC_GUEST_USING pin from concurrent invalidators before the vCPU actually
exits?

> +		if (old_state & (GPC_VALID | GPC_INVALIDATING))
>  			synchronize_srcu_atomic(&kvm->gpc_srcu);

[Severity: Low]
This also uses synchronize_srcu_atomic() which does not seem to be defined.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260920211920.928306-1-dwmw2@xxxxxxxxxxxxx?part=7