x86/kvm: Use RCU for svm_vm_data_hash

Thank you for your contribution! Sashiko AI review found 2 potential issues to consider:
- [High] KVM: SVM: UAF in avic_ga_log_notifier() and avic_vm_pre_destroy() with RCU
- [Low] KVM: SVM: unnecessary guard() removal in avic_add_vm_to_ga_log_list()
--

commit 460915a518276bc08e3b21ecfcfa94bc27d07493
Author: Sairaj Kodilkar <[email protected]>

x86/kvm: Use RCU for svm_vm_data_hash

This commit replaces the spinlock-protected lookup of svm_vm_data_hash in avic_ga_log_notifier() with an RCU read-side critical section, and updates insertion and removal to use hash_add_rcu() and hash_del_rcu().

Quoted 47 lines

arch/x86/kvm/svm/avic.c +5 -5

index 3b037e3855234..ba12aa5872255 100644
@@ -250,7 +250,6 @@ static void avic_deactivate_vmcb(struct vcpu_svm *svm)
  */
 static int avic_ga_log_notifier(u32 ga_tag)
 {
-	unsigned long flags;
 	struct kvm_svm *kvm_svm;
 	struct kvm_vcpu *vcpu = NULL;
 	u32 vm_id = AVIC_GATAG_TO_VMID(ga_tag);

@@ -259,14 +258,14 @@ static int avic_ga_log_notifier(u32 ga_tag)
 	pr_debug("SVM: %s: vm_id=%#x, vcpu_idx=%#x\n", __func__, vm_id, vcpu_idx);
 	trace_kvm_avic_ga_log(vm_id, vcpu_idx);
 
-	spin_lock_irqsave(&svm_vm_data_hash_lock, flags);
-	hash_for_each_possible(svm_vm_data_hash, kvm_svm, hnode, vm_id) {
+	rcu_read_lock();
+	hash_for_each_possible_rcu(svm_vm_data_hash, kvm_svm, hnode, vm_id) {
 		if (kvm_svm->avic_vm_id != vm_id)
 			continue;
 		vcpu = kvm_get_vcpu(&kvm_svm->kvm, vcpu_idx);
 		break;
 	}
-	spin_unlock_irqrestore(&svm_vm_data_hash_lock, flags);
+	rcu_read_unlock();
 
 	/* Note:
 	 * At this point, the IOMMU should have already set the pending

@@ -319,13 +318,14 @@ static void avic_add_vm_to_ga_log_list(struct kvm *kvm)
 	struct kvm_svm *kvm_svm = to_kvm_svm(kvm);
 	struct kvm_svm *k2;
 	u32 vm_id;
+	unsigned long flags;
 
 	lockdep_assert_held(&kvm->lock);
 
 	if (kvm_svm->avic_vm_id)
 		return;
 
-	guard(spinlock_irqsave)(&svm_vm_data_hash_lock);
+	spin_lock_irqsave(&svm_vm_data_hash_lock, flags);
  again:
 	vm_id = next_vm_id = (next_vm_id + 1) & AVIC_VM_ID_MASK;
 	if (vm_id == 0) { /* id is 1-based, zero is not okay */

arch/x86/kvm/svm/avic.c static void avic_add_vm_to_ga_log_list(struct kvm *kvm) · line 345

Quoted 7 lines

arch/x86/kvm/svm/avic.c +2 -1

@@ -340,7 +340,8 @@ static void avic_add_vm_to_ga_log_list(struct kvm *kvm)
 		}
 	}
 	kvm_svm->avic_vm_id = vm_id;
-	hash_add(svm_vm_data_hash, &kvm_svm->hnode, kvm_svm->avic_vm_id);
+	hash_add_rcu(svm_vm_data_hash, &kvm_svm->hnode, kvm_svm->avic_vm_id);
+	spin_unlock_irqrestore(&svm_vm_data_hash_lock, flags);
 }

[Severity: Low]
This isn't a bug, but commits 4fa9a3767d1a ("KVM: SVM: Do all per-VM AVIC initialization during vCPU precreation phase") and 78684b65fcc0 ("KVM: SVM:
Remove VM from the GA Log notifier list before VM destruction") switched avic_add_vm_to_ga_log_list() and avic_vm_pre_destroy() to guard(spinlock_irqsave)(&svm_vm_data_hash_lock), which can still be kept here instead of open-coding spin_lock_irqsave() and spin_unlock_irqrestore().

Quoted 7 lines

arch/x86/kvm/svm/avic.c +1 -0

 int avic_vcpu_precreate(struct kvm *kvm)

@@ -369,13 +370,14 @@ int avic_vcpu_precreate(struct kvm *kvm)
 void avic_vm_pre_destroy(struct kvm *kvm)
 {
 	struct kvm_svm *kvm_svm = to_kvm_svm(kvm);
+	unsigned long flags;
 

arch/x86/kvm/svm/avic.c int avic_vcpu_precreate(struct kvm *kvm) · line 381

Quoted 10 lines

arch/x86/kvm/svm/avic.c +3 -3

 	if (WARN_ON_ONCE(!enable_apicv) || !kvm_svm->avic_vm_id)
 		return;
 
-	guard(spinlock_irqsave)(&svm_vm_data_hash_lock);
-
-	hash_del(&kvm_svm->hnode);
+	spin_lock_irqsave(&svm_vm_data_hash_lock, flags);
+	hash_del_rcu(&kvm_svm->hnode);
+	spin_unlock_irqrestore(&svm_vm_data_hash_lock, flags);
 }

[Severity: High]
Can this cause a use-after-free on struct kvm_svm and struct kvm_vcpu when a VM is destroyed concurrently with avic_ga_log_notifier()?

CPU0 CPU1
avic_ga_log_notifier() kvm_destroy_vm()
rcu_read_lock() ...
hash_for_each_possible_rcu(...) avic_vm_pre_destroy()

    vcpu = kvm_get_vcpu(...)          hash_del_rcu(&kvm_svm->hnode)
  rcu_read_unlock()                 ...
                                    kvm_destroy_vcpus()
                                      kmem_cache_free(..., vcpu)
                                    kvm_arch_free_vm()
                                      vfree(kvm)

kvm_vcpu_wake_up(vcpu) <-- use-after-free

avic_vm_pre_destroy() unlinks kvm_svm->hnode with hash_del_rcu(), but nothing waits for an RCU grace period before kvm_destroy_vcpus() frees the vCPUs and kvm_arch_free_vm() frees struct kvm_svm (the intervening SRCU synchronizations in kvm_destroy_vm() only cover private SRCU domains). An in-flight reader on the same hash bucket can therefore dereference freed kvm_svm or vcpu memory.

In addition, avic_ga_log_notifier() calls rcu_read_unlock() before kvm_vcpu_wake_up(vcpu). Because kvm_get_vcpu() does not take a reference, dropping the RCU read lock early leaves vcpu unpinned even if avic_vm_pre_destroy() waits for a grace period. Calling kvm_vcpu_wake_up() inside the rcu_read_lock() critical section and adding synchronize_rcu() after hash_del_rcu() in avic_vm_pre_destroy() would close both windows.