KVM: arm64: Add kvm_for_each_vncr_tlb() helper

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 <[email protected]>
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 open here

Quoted 8 lines

arch/arm64/kvm/nested.c +0 -0

index 883b6c1008fbb..e25d7289aa9e5 100644
@@ -897,9 +897,21 @@ static void invalidate_vncr(struct vncr_tlb *vt)
 		clear_fixmap(vncr_fixmap(vt->cpu));
 }
 

arch/arm64/kvm/nested.c static void invalidate_vncr(struct vncr_tlb *vt) · line 909

Quoted 10 lines

arch/arm64/kvm/nested.c +10 -0

+/*
+ * 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.

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 <[email protected]>

Signed-off-by Marc Zyngier <[email protected]>

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

Fixes 4ffa72ad8f37 ("KVM: arm64: nv: Add S1 TLB invalidation primitive for VNCR_EL2")

Cc [email protected]

---
 arch/arm64/kvm/nested.c | 36 +++++++++++++++---------------------
 1 file changed, 15 insertions(+), 21 deletions(-)

arch/arm64/kvm/nested.c +15 -21

index f8d3f3a723282..690b8e8564166 100644
@@ -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);

Signature · 1 line

2.47.3