Re: [PATCH 1/2] 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 2 potential issue(s) to consider:

New issues:
- [High] Moving `hash_del()` from `avic_vm_destroy()` to `avic_vm_pre_destroy()` causes a Use-After-Free if VM creation fails.

Pre-existing issues:
- [High] Time-Of-Check to Time-Of-Use (TOCTOU) race in `avic_ga_log_notifier()` leaves the vCPU pointer vulnerable to Use-After-Free.
--

(Note: The format of this report is altered to provide a free-form summary
without quoting the original patch code due to recitation restrictions.)

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

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

This commit moves the removal of the VM from the GA log notifier list
from avic_vm_destroy() to avic_vm_pre_destroy(). It also opportunistically
adopts guard() for locking in the pre-destroy hook.

[Severity: High]
Does this change introduce a use-after-free on the VM creation error path?

When kvm_create_vm() in virt/kvm/kvm_main.c encounters an error after
kvm_arch_init_vm() has already succeeded (for instance, if
kvm_init_mmu_notifier() fails due to memory exhaustion), the error unwind
path calls kvm_arch_destroy_vm(), but it completely bypasses
kvm_arch_pre_destroy_vm().

Since the removal of the hash node from svm_vm_data_hash was moved from
avic_vm_destroy() into avic_vm_pre_destroy(), this early failure path will
no longer execute the hash removal. Will this leave a dangling pointer in
the global hash table once the VM struct is freed by kvm_arch_free_vm(),
potentially crashing the host on subsequent hash table traversals?


[Severity: High]
This is a pre-existing issue, but does a time-of-check to time-of-use race
still exist in avic_ga_log_notifier() even with this patch applied?

In arch/x86/kvm/svm/avic.c, avic_ga_log_notifier() looks up the vCPU while
holding svm_vm_data_hash_lock. It then drops the lock before passing the
vCPU to kvm_vcpu_wake_up(). There does not appear to be any rcu_read_lock()
or reference counting protecting the vCPU memory during this window.

If the threaded IOMMU IRQ handler is preempted immediately after unlocking
but before kvm_vcpu_wake_up() executes, could another CPU finish destroying
the VM and free the vCPU via kvm_destroy_vcpus()? When the IRQ thread
resumes, it would attempt to wake up a freed vCPU pointer. Because the
active notifier thread isn't blocked by VM destruction, does moving the
hash removal earlier fully resolve the use-after-free risk?

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