Re: [PATCH v2 3/3] KVM: SVM: Remove VM from the GA Log notifier list before VM destruction

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The patch fails to fully resolve the Use-After-Free race in `avic_ga_log_notifier()`. It leaves a Time-Of-Check to Time-Of-Use (TOCTOU) vulnerability where an already-found vCPU pointer can be dereferenced after it is freed.
--

commit a94e2dbde8bab766023c731fa2628928c6d8e750
Author: Sean Christopherson <seanjc@google.com>

KVM: SVM: Remove VM from the GA Log notifier list before VM destruction

This patch changes when a VM is deleted from the GA Log interrupt list. It
now removes the VM during avic_vm_pre_destroy() instead of waiting for
avic_vm_destroy(). This attempts to prevent a use-after-free if a GA Log
notification arrives for a vCPU after the last reference to the VM is put.

> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index d71a2fed1a08b..c5b1d294b15a2 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -374,9 +374,20 @@ int avic_vcpu_precreate(struct kvm *kvm)
>  	return 0;
>  }
>  
> +void avic_vm_pre_destroy(struct kvm *kvm)
> +{
> +	struct kvm_svm *kvm_svm = to_kvm_svm(kvm);
> +
> +	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);
> +}

[Severity: High]
This isn't a bug introduced by this patch, but could there still be a
Time-Of-Check to Time-Of-Use race in avic_ga_log_notifier()?

If a notification arrives and avic_ga_log_notifier() finds the VM and its
vCPU pointer under svm_vm_data_hash_lock, it drops the lock before calling
kvm_vcpu_wake_up():

arch/x86/kvm/svm/avic.c:avic_ga_log_notifier() {
    ...
    spin_unlock_irqrestore(&svm_vm_data_hash_lock, flags);

    if (vcpu)
        kvm_vcpu_wake_up(vcpu);
}

If avic_ga_log_notifier() is preempted right after releasing the lock,
couldn't avic_vm_pre_destroy() still run concurrently, remove the VM from
the hash table here, and allow the remaining teardown process to free the
vCPUs?

Would this result in avic_ga_log_notifier() calling kvm_vcpu_wake_up()
on a freed vCPU pointer once it resumes execution?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260630210156.457151-1-seanjc@google.com?part=3