Re: [PATCH 6/6] KVM: arm64: Add VNCR TLB tracking again

[Marc Zyngier <maz@kernel.org>]

On Sat, 01 Aug 2026 14:04:53 +0100,
sashiko-bot@kernel.org wrote:
> 
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [Critical] Cross-CPU invalidation of VNCR TLBs (e.g., via MMU notifiers) clears the fixmap and resets `vt->cpu` but fails to clear the per-CPU `L1_VNCR_MAPPED` flag on the CPU where the vCPU is running. When the vCPU is subsequently scheduled out, `this_cpu_reset_vncr_fixmap()` checks the local flag, finds it set, and hits a `BUG_ON` because `vt->cpu` is `-1`. This allows unprivileged host userspace to panic the host kernel.
> - [High] The fast-path check in `kvm_hyp_handle_tlbi_el2()` skips VNCR TLB invalidation for EL2 TLBI instructions when `E2H` or `TGE` is 0, leaving stale translations in the software VNCR TLB.
> - [High] A TOCTOU race exists between lockless guest page table walks and guest TLBI intercepts, allowing stale VNCR translations to survive invalidation.
> --
> 
> --- Patch [6]: [PATCH 6/6] KVM: arm64: Add VNCR TLB tracking again ---
> commit bde680578e8485472440ba4858d93cb92173897d
> Author: Marc Zyngier <maz@kernel.org>
> Date:   Sat Aug 1 12:48:18 2026 +0000
> 
>     KVM: arm64: Add VNCR TLB tracking again
>     
>     This patch re-introduces VNCR TLB tracking to force EL2 S1 TLBI handling
>     onto the slow path when valid VNCR TLBs exist. It uses a VM-wide atomic
>     counter to track the presence of valid VNCR TLBs, updating it under the
>     MMU lock.
> 
> > diff --git a/arch/arm64/kvm/hyp/vhe/switch.c b/arch/arm64/kvm/hyp/vhe/switch.c
> > index c09b1d411c584..eb59549ec2172 100644
> > --- a/arch/arm64/kvm/hyp/vhe/switch.c
> > +++ b/arch/arm64/kvm/hyp/vhe/switch.c
> > @@ -424,10 +424,11 @@ static bool kvm_hyp_handle_tlbi_el2(struct kvm_vcpu *vcpu, u64 *exit_code)
> >  		return false;
> >  
> >  	/*
> > -	 * If we have to check for any VNCR mapping being invalidated,
> > +	 * If we have to check for any VNCR TLB being invalidated,
> >  	 * go back to the slow path for further processing.
> >  	 */
> > -	if (vcpu_el2_e2h_is_set(vcpu) && vcpu_el2_tge_is_set(vcpu))
> > +	if (vcpu_el2_e2h_is_set(vcpu) && vcpu_el2_tge_is_set(vcpu) &&
> > +	    atomic_read(&vcpu->kvm->arch.vncr_tlb_count))
> >  		return false;
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does this check skip VNCR TLB
> invalidation for EL2 TLBI instructions when the guest is in nVHE
> mode?

nVHE and NV are mutually exclusive in KVM.

[...]

> > diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
> > index 8d5b0208ad728..2a7dd9f9fb597 100644
> > --- a/arch/arm64/kvm/nested.c
> > +++ b/arch/arm64/kvm/nested.c
> [ ... ]
> > @@ -998,9 +999,11 @@ u16 get_asid_by_regime(struct kvm_vcpu *vcpu, enum trans_regime regime)
> >  	return asid;
> >  }
> >  
> > -static void invalidate_vncr(struct vncr_tlb *vt)
> > +static void invalidate_vncr(struct kvm *kvm, struct vncr_tlb *vt)
> >  {
> > +	BUG_ON(!vt->valid);
> >  	vt->valid = false;
> > +	atomic_dec(&kvm->arch.vncr_tlb_count);
> >  	if (vt->cpu != -1)
> >  		unmap_l1_vncr(vt);
> >  }
> 
> [Severity: Critical]
> This is a pre-existing issue, but does this cross-CPU invalidation path leave
> the per-CPU L1_VNCR_MAPPED flag set locally?
> 
> If an MMU notifier on CPU B calls kvm_invalidate_vncr_ipa() which reaches
> invalidate_vncr(), unmap_l1_vncr() will clear the fixmap and set vt->cpu = -1.
> 
> When CPU A (which was running the vCPU) exits the guest and calls
> this_cpu_reset_vncr_fixmap(), won't it see its local L1_VNCR_MAPPED flag still
> set and trigger BUG_ON(vcpu->arch.vncr_tlb->cpu != smp_processor_id()) because
> vt->cpu is now -1?

Addressed in my reply to the comment on the previous patch.

> 
> [ ... ]
> > @@ -1460,6 +1471,8 @@ static int kvm_translate_vncr(struct kvm_vcpu *vcpu, bool *is_gmem)
> >  		vt->valid = true;
> >  		vt->cpu = -1;
> >  
> > +		atomic_inc(&vcpu->kvm->arch.vncr_tlb_count);
> > +
> >  		kvm_make_request(KVM_REQ_MAP_L1_VNCR_EL2, vcpu);
> >  		kvm_release_faultin_page(vcpu->kvm, page, false, vt->wr.pw && vt->hpa_writable);
> >  	}
> 
> [Severity: High]
> This is a pre-existing issue, but is there a time-of-check to time-of-use race
> between the lockless guest page table walk and guest TLBI intercepts here?
> 
> CPU1 locklessly resolves the guest virtual address via __kvm_translate_va()
> earlier in kvm_translate_vncr().
> 
> Before CPU1 acquires the mmu_lock and increments vncr_tlb_count, CPU2 could
> modify the guest page table and issue a TLBI.
> 
> The TLBI on CPU2 would see vncr_tlb_count == 0 (or vt->valid == false) and
> complete. CPU1 would then acquire mmu_lock, pass the mmu_invalidate_retry()
> check (which only tracks host MMU notifiers), and install the stale
> translation.
> 
> Could this result in caching a stale VNCR mapping?

Yup. That's definitely another problem.

I think we need something that like the mmu notifier retry
mechanism. We can hoist the increment of vcpu_tlb_count to the
beginning of the translation process, sample a counter, and recheck
this counter once we've acquired the mmu lock.

I wonder if we can actually use the exact same counter outside of the
MMU notifiers, bumping it up from the TLBI VA code... Something like
the untested hack below.

	M.

diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
index dfdd1b4abb839..73c7bd4223f13 100644
--- a/arch/arm64/kvm/nested.c
+++ b/arch/arm64/kvm/nested.c
@@ -1010,6 +1010,8 @@ static void invalidate_vncr(struct kvm *kvm, struct vncr_tlb *vt)
 	vt->valid = false;
 	atomic_dec(&kvm->arch.vncr_tlb_count);
 	unmap_l1_vncr(vt);
+	kvm->mmu_invalidate_seq++;
+	smp_wmb();
 }
 
 static bool vncr_tlb_intersects(struct vncr_tlb *vt, u64 addr,
@@ -1475,8 +1477,6 @@ static int kvm_translate_vncr(struct kvm_vcpu *vcpu, bool *is_gmem)
 		vt->valid = true;
 		atomic_set(&vt->cpu, -1);
 
-		atomic_inc(&vcpu->kvm->arch.vncr_tlb_count);
-
 		kvm_make_request(KVM_REQ_MAP_L1_VNCR_EL2, vcpu);
 		kvm_release_faultin_page(vcpu->kvm, page, false, vt->wr.pw && vt->hpa_writable);
 	}
@@ -1530,7 +1530,17 @@ int kvm_handle_vncr_abort(struct kvm_vcpu *vcpu)
 		return -EIO;
 	}
 
+	/*
+	 * Speculatively increment the TLB count to make sure concurrent
+	 * TLBIs will take the slow path, and will interact with the retry
+	 * mechanism. Drop it again on error.
+	 */
+	atomic_inc(&vcpu->kvm->arch.vncr_tlb_count);
+
 	ret = kvm_translate_vncr(vcpu, &is_gmem);
+	if (ret)
+		atomic_dec(&vcpu->kvm->arch.vncr_tlb_count);
+
 	switch (ret) {
 	case -EAGAIN:
 		/* Let's try again... */

-- 
Jazz isn't dead. It just smells funny.