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

[Sairaj Kodilkar <sarunkod@amd.com>]

On 11/11/2026 3:47 PM, sashiko-bot@kernel.org wrote:
> 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().
> 

Right, will remove this

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

Right, will do it

Thanks
Sairaj