Re: [PATCH] x86/kvm: Use RCU for svm_vm_data_hash

[sashiko-bot@kernel.org]

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 <sarunkod@amd.com>

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().

> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index 3b037e3855234..ba12aa5872255 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -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 */
> @@ -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().

>  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;
>  
>  	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.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111100246.4425-1-sarunkod@amd.com?part=1