[PATCH] KVM: arm64: Add kvm_for_each_vncr_tlb() helper

[PATCH] KVM: arm64: Add kvm_for_each_vncr_tlb() helper

[Marc Zyngier]

VNCR TLB invalidation occurs from MMU notifiers or TLBI instructions,
and either can race against a vcpu not being onlined yet (no pseudo-TLB
allocated). Similarly, the TLB might be invalid, and the invalidation
should be skipped in this case.

Both kvm_invalidate_vncr_ipa() and kvm_invalidate_vncr_va() are
expected to perform the same checks, except that the latter doesn't
check for the allocation and blindly dereferences the pointer.

Solve this by introducing a new iterator built on top of the usual
kvm_for_each_vcpu() that checks for both of the above conditions,
and convert the two users to it.

Reported-by: Hyunwoo Kim <imv4bel@gmail.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
Link: https://lore.kernel.org/r/aiUvSbrWndQeUPc8@v4bel
Fixes: 4ffa72ad8f37 ("KVM: arm64: nv: Add S1 TLB invalidation primitive for VNCR_EL2")
Cc: stable@vger.kernel.org
---
 arch/arm64/kvm/nested.c | 36 +++++++++++++++---------------------
 1 file changed, 15 insertions(+), 21 deletions(-)

diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
index f8d3f3a723282..690b8e8564166 100644
--- a/arch/arm64/kvm/nested.c
+++ b/arch/arm64/kvm/nested.c
@@ -908,9 +908,21 @@ static void invalidate_vncr(struct vncr_tlb *vt)
 		clear_fixmap(vncr_fixmap(vt->cpu));
 }
 
+/*
+ * VNCR TLB invalidation occurs from MMU notifiers or TLBI instructions, and
+ * either can race against a vcpu not being onlined yet (no pseudo-TLB
+ * allocated). Similarly, the TLB might be invalid.  Skip those, as they
+ * obviously don't participate in the invalidation at this stage.
+ */
+#define kvm_for_each_vncr_tlb(idx, vcpup, tlbp, kvm)	\
+	kvm_for_each_vcpu(idx, vcpup, kvm)		\
+		if (((tlbp) = vcpup->arch.vncr_tlb) &&	\
+		    (tlbp)->valid)
+
 static void kvm_invalidate_vncr_ipa(struct kvm *kvm, u64 start, u64 end)
 {
 	struct kvm_vcpu *vcpu;
+	struct vncr_tlb *vt;
 	unsigned long i;
 
 	lockdep_assert_held_write(&kvm->mmu_lock);
@@ -918,24 +930,9 @@ static void kvm_invalidate_vncr_ipa(struct kvm *kvm, u64 start, u64 end)
 	if (!kvm_has_feat(kvm, ID_AA64MMFR4_EL1, NV_frac, NV2_ONLY))
 		return;
 
-	kvm_for_each_vcpu(i, vcpu, kvm) {
-		struct vncr_tlb *vt = vcpu->arch.vncr_tlb;
+	kvm_for_each_vncr_tlb(i, vcpu, vt, kvm) {
 		u64 ipa_start, ipa_end, ipa_size;
 
-		/*
-		 * Careful here: We end-up here from an MMU notifier,
-		 * and this can race against a vcpu not being onlined
-		 * yet, without the pseudo-TLB being allocated.
-		 *
-		 * Skip those, as they obviously don't participate in
-		 * the invalidation at this stage.
-		 */
-		if (!vt)
-			continue;
-
-		if (!vt->valid)
-			continue;
-
 		ipa_size = ttl_to_size(pgshift_level_to_ttl(vt->wi.pgshift,
 							    vt->wr.level));
 		ipa_start = vt->wr.pa & ~(ipa_size - 1);
@@ -965,17 +962,14 @@ static void invalidate_vncr_va(struct kvm *kvm,
 			       struct s1e2_tlbi_scope *scope)
 {
 	struct kvm_vcpu *vcpu;
+	struct vncr_tlb *vt;
 	unsigned long i;
 
 	lockdep_assert_held_write(&kvm->mmu_lock);
 
-	kvm_for_each_vcpu(i, vcpu, kvm) {
-		struct vncr_tlb *vt = vcpu->arch.vncr_tlb;
+	kvm_for_each_vncr_tlb(i, vcpu, vt, kvm) {
 		u64 va_start, va_end, va_size;
 
-		if (!vt->valid)
-			continue;
-
 		va_size = ttl_to_size(pgshift_level_to_ttl(vt->wi.pgshift,
 							   vt->wr.level));
 		va_start = vt->gva & ~(va_size - 1);
-- 
2.47.3

Re: [PATCH] KVM: arm64: Add kvm_for_each_vncr_tlb() helper

[sashiko-bot]

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

New issues:
- [Low] The `kvm_for_each_vncr_tlb` macro introduces an unbraced `if` statement, exposing callers to a dangling `else` vulnerability.

Pre-existing issues:
- [High] Concurrent lockless publication and observation of `vcpu->arch.vncr_tlb` lacks memory barriers and `READ_ONCE()`, risking uninitialized memory access.
- [High] Unconditional allocation in `kvm_vcpu_allocate_vncr_tlb` leaks memory if `kvm_arch_vcpu_run_pid_change` is restarted.
--

commit 03a7f368fcb61f7b51d0623ed1c15d57166b377e
Author: Marc Zyngier <maz@kernel.org>
Subject: KVM: arm64: Add kvm_for_each_vncr_tlb() helper

This commit introduces a new helper macro kvm_for_each_vncr_tlb() to 
iterate over vCPUs that have a valid vncr_tlb allocated. It then
converts kvm_invalidate_vncr_ipa() and invalidate_vncr_va() to use
this new iterator.

Link: https://lore.kernel.org/r/aiUvSbrWndQeUPc8@v4bel

> diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
> index 883b6c1008fbb..e25d7289aa9e5 100644
> --- a/arch/arm64/kvm/nested.c
> +++ b/arch/arm64/kvm/nested.c
> @@ -897,9 +897,21 @@ static void invalidate_vncr(struct vncr_tlb *vt)
>  		clear_fixmap(vncr_fixmap(vt->cpu));
>  }
>  
> +/*
> + * VNCR TLB invalidation occurs from MMU notifiers or TLBI instructions, and
> + * either can race against a vcpu not being onlined yet (no pseudo-TLB
> + * allocated). Similarly, the TLB might be invalid.  Skip those, as they
> + * obviously don't participate in the invalidation at this stage.
> + */
> +#define kvm_for_each_vncr_tlb(idx, vcpup, tlbp, kvm)	\
> +	kvm_for_each_vcpu(idx, vcpup, kvm)		\
> +		if (((tlbp) = vcpup->arch.vncr_tlb) &&	\
> +		    (tlbp)->valid)

[Severity: High]
This isn't a bug introduced by this patch, but can we read uninitialized
memory here due to a missing memory barrier?

When vcpu->arch.vncr_tlb is allocated in kvm_vcpu_allocate_vncr_tlb(),
it is assigned via a plain store without smp_store_release(), and not
under kvm->mmu_lock.

Concurrently, the MMU notifier path (which holds kvm->mmu_lock) iterates
over vCPUs and reads this pointer locklessly using a plain load here without
READ_ONCE() or smp_load_acquire(). 

On weakly-ordered architectures, could the observing CPU see a non-NULL
pointer before the zero-initialization of kzalloc_obj() is visible?

If so, tlbp->valid could be evaluated using uninitialized memory.
If it evaluates to true, invalidate_vncr() could read garbage from
vt->cpu and trigger the BUG_ON() in vncr_fixmap(), causing a host panic.

[Severity: High]
This is a pre-existing issue, but does kvm_vcpu_allocate_vncr_tlb() leak
memory if KVM_RUN is restarted?

In kvm_arch_vcpu_run_pid_change(), kvm_vcpu_allocate_vncr_tlb() is
called during the first KVM_RUN and unconditionally allocates the pointer
with kzalloc_obj().

If subsequent initialization steps in kvm_arch_vcpu_run_pid_change() fail
(like kvm_timer_enable()), it returns an error to userspace without
marking the vCPU as having run.

If userspace repeatedly invokes the KVM_RUN ioctl after a failure,
could kvm_vcpu_allocate_vncr_tlb() be executed again, overwriting the
previous vncr_tlb allocation without freeing it and leaking memory?

[Severity: Low]
Does the unbraced if in this macro expose callers to a dangling else
vulnerability?

While current usages are safely enclosed in braces, if a future caller uses
this macro inside an unbraced if block and attempts to attach an else,
the compiler could incorrectly bind it to this hidden internal if.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260607175745.297793-1-maz@kernel.org?part=1

Re: [PATCH] KVM: arm64: Add kvm_for_each_vncr_tlb() helper

[Marc Zyngier]

On Sun, 07 Jun 2026 19:08:15 +0100,
sashiko-bot@kernel.org wrote:
> 
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> 
> New issues:
> - [Low] The `kvm_for_each_vncr_tlb` macro introduces an unbraced `if` statement, exposing callers to a dangling `else` vulnerability.
> 
> Pre-existing issues:
> - [High] Concurrent lockless publication and observation of `vcpu->arch.vncr_tlb` lacks memory barriers and `READ_ONCE()`, risking uninitialized memory access.
> - [High] Unconditional allocation in `kvm_vcpu_allocate_vncr_tlb` leaks memory if `kvm_arch_vcpu_run_pid_change` is restarted.
> --
> 
> commit 03a7f368fcb61f7b51d0623ed1c15d57166b377e
> Author: Marc Zyngier <maz@kernel.org>
> Subject: KVM: arm64: Add kvm_for_each_vncr_tlb() helper
> 
> This commit introduces a new helper macro kvm_for_each_vncr_tlb() to 
> iterate over vCPUs that have a valid vncr_tlb allocated. It then
> converts kvm_invalidate_vncr_ipa() and invalidate_vncr_va() to use
> this new iterator.
> 
> Link: https://lore.kernel.org/r/aiUvSbrWndQeUPc8@v4bel
> 
> > diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
> > index 883b6c1008fbb..e25d7289aa9e5 100644
> > --- a/arch/arm64/kvm/nested.c
> > +++ b/arch/arm64/kvm/nested.c
> > @@ -897,9 +897,21 @@ static void invalidate_vncr(struct vncr_tlb *vt)
> >  		clear_fixmap(vncr_fixmap(vt->cpu));
> >  }
> >  
> > +/*
> > + * VNCR TLB invalidation occurs from MMU notifiers or TLBI instructions, and
> > + * either can race against a vcpu not being onlined yet (no pseudo-TLB
> > + * allocated). Similarly, the TLB might be invalid.  Skip those, as they
> > + * obviously don't participate in the invalidation at this stage.
> > + */
> > +#define kvm_for_each_vncr_tlb(idx, vcpup, tlbp, kvm)	\
> > +	kvm_for_each_vcpu(idx, vcpup, kvm)		\
> > +		if (((tlbp) = vcpup->arch.vncr_tlb) &&	\
> > +		    (tlbp)->valid)
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but can we read uninitialized
> memory here due to a missing memory barrier?
> 
> When vcpu->arch.vncr_tlb is allocated in kvm_vcpu_allocate_vncr_tlb(),
> it is assigned via a plain store without smp_store_release(), and not
> under kvm->mmu_lock.
> 
> Concurrently, the MMU notifier path (which holds kvm->mmu_lock) iterates
> over vCPUs and reads this pointer locklessly using a plain load here without
> READ_ONCE() or smp_load_acquire(). 
> 
> On weakly-ordered architectures, could the observing CPU see a non-NULL
> pointer before the zero-initialization of kzalloc_obj() is visible?
> 
> If so, tlbp->valid could be evaluated using uninitialized memory.
> If it evaluates to true, invalidate_vncr() could read garbage from
> vt->cpu and trigger the BUG_ON() in vncr_fixmap(), causing a host panic.
> 
> [Severity: High]
> This is a pre-existing issue, but does kvm_vcpu_allocate_vncr_tlb() leak
> memory if KVM_RUN is restarted?
> 
> In kvm_arch_vcpu_run_pid_change(), kvm_vcpu_allocate_vncr_tlb() is
> called during the first KVM_RUN and unconditionally allocates the pointer
> with kzalloc_obj().
> 
> If subsequent initialization steps in kvm_arch_vcpu_run_pid_change() fail
> (like kvm_timer_enable()), it returns an error to userspace without
> marking the vCPU as having run.
> 
> If userspace repeatedly invokes the KVM_RUN ioctl after a failure,
> could kvm_vcpu_allocate_vncr_tlb() be executed again, overwriting the
> previous vncr_tlb allocation without freeing it and leaking memory?
>

Both issues can be solved in one go by taking the kvm->mmu_lock when
updating vncr_tlb, relying on the release effects of the unlock, and
testing for vncr_tlb before assignment. I'll hack something.

> [Severity: Low]
> Does the unbraced if in this macro expose callers to a dangling else
> vulnerability?
> 
> While current usages are safely enclosed in braces, if a future caller uses
> this macro inside an unbraced if block and attempts to attach an else,
> the compiler could incorrectly bind it to this hidden internal if.

This is C, and more importantly Linux-C. It's out to get you, on
purpose, and by design. Try a scoped_guard() with a continue statement
within a for loop to have some real fun, rather than this hypothetical
'else' which would be most bizarre. Given the above and how its use is
wildly promoted within the kernel, I'm not exactly worried.

	M.

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