Kernel KVM virtualization development
 help / color / mirror / Atom feed
* [PATCH v2 0/3] KVM: SVM: Fix a (very) unlikely UAF for GA Log IRQs
@ 2026-06-30 21:01 Sean Christopherson
  2026-06-30 21:01 ` [PATCH v2 1/3] KVM: SVM: Make kvm_x86_ops.vcpu_precreate() hook fully AVIC specific Sean Christopherson
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Sean Christopherson @ 2026-06-30 21:01 UTC (permalink / raw)
  To: Sean Christopherson, Paolo Bonzini
  Cc: kvm, linux-kernel, Naveen N Rao, Xiao Wu

Fix a potential UAF due to freeing vCPUs while they're still reachable through
the global hashed list used to handle GA Log notifications.

v2:
 - Defer adding the VM to the GA Log list until a vCPU is created (KVM only
   needs GA Lot notifications to wake blocking vCPUs), so that removing the
   VM from the list can be done during pre-destroy without having to handle
   the scenario where VM creation fails at a later stage. [Sashiko, Xiao]
 - Drop the stable@ tag, as it's not clear to me that the bug can actually
   occur in the wild, whereas the changes themselves are somewhat risky.

v1: https://lore.kernel.org/all/20260625220933.3357733-1-seanjc@google.com

Sean Christopherson (3):
  KVM: SVM: Make kvm_x86_ops.vcpu_precreate() hook fully AVIC specific
  KVM: SVM: Do all per-VM AVIC initialization during vCPU precreation
    phase
  KVM: SVM: Remove VM from the GA Log notifier list before VM
    destruction

 arch/x86/kvm/svm/avic.c | 96 +++++++++++++++++++++++++++--------------
 arch/x86/kvm/svm/svm.c  | 16 ++-----
 arch/x86/kvm/svm/svm.h  |  3 +-
 3 files changed, 69 insertions(+), 46 deletions(-)


base-commit: a204badd8432f93b7e862e7dac6db0fe3d65f370
-- 
2.55.0.rc0.799.gd6f94ed593-goog


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2 1/3] KVM: SVM: Make kvm_x86_ops.vcpu_precreate() hook fully AVIC specific
  2026-06-30 21:01 [PATCH v2 0/3] KVM: SVM: Fix a (very) unlikely UAF for GA Log IRQs Sean Christopherson
@ 2026-06-30 21:01 ` Sean Christopherson
  2026-06-30 21:01 ` [PATCH v2 2/3] KVM: SVM: Do all per-VM AVIC initialization during vCPU precreation phase Sean Christopherson
  2026-06-30 21:01 ` [PATCH v2 3/3] KVM: SVM: Remove VM from the GA Log notifier list before VM destruction Sean Christopherson
  2 siblings, 0 replies; 8+ messages in thread
From: Sean Christopherson @ 2026-06-30 21:01 UTC (permalink / raw)
  To: Sean Christopherson, Paolo Bonzini
  Cc: kvm, linux-kernel, Naveen N Rao, Xiao Wu

In anticipation of deferring all per-VM AVIC initialization until a vCPU
is first created, move SVM's kvm_x86_ops.vcpu_precreate() hook into avic.c
as avic_vcpu_pre_create() and nullify the hook if AVIC is disabled (and
WARN if the hook is somehow invoked without AVIC enabled).

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/avic.c | 13 +++++++++----
 arch/x86/kvm/svm/svm.c  |  8 ++------
 arch/x86/kvm/svm/svm.h  |  2 +-
 3 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index 58e493a80cb0..4a0a2dbd1687 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -293,13 +293,10 @@ static int avic_get_physical_id_table_order(struct kvm *kvm)
 	return get_order((__avic_get_max_physical_id(kvm, NULL) + 1) * sizeof(u64));
 }
 
-int avic_alloc_physical_id_table(struct kvm *kvm)
+static int avic_alloc_physical_id_table(struct kvm *kvm)
 {
 	struct kvm_svm *kvm_svm = to_kvm_svm(kvm);
 
-	if (!irqchip_in_kernel(kvm) || !enable_apicv)
-		return 0;
-
 	if (kvm_svm->avic_physical_id_table)
 		return 0;
 
@@ -311,6 +308,14 @@ int avic_alloc_physical_id_table(struct kvm *kvm)
 	return 0;
 }
 
+int avic_vcpu_precreate(struct kvm *kvm)
+{
+	if (!irqchip_in_kernel(kvm) || WARN_ON_ONCE(!enable_apicv))
+		return 0;
+
+	return avic_alloc_physical_id_table(kvm);
+}
+
 void avic_vm_destroy(struct kvm *kvm)
 {
 	unsigned long flags;
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index ef69a51ab27f..a7d141f7e76c 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -1306,11 +1306,6 @@ void svm_switch_vmcb(struct vcpu_svm *svm, struct kvm_vmcb_info *target_vmcb)
 	svm->vmcb = target_vmcb->ptr;
 }
 
-static int svm_vcpu_precreate(struct kvm *kvm)
-{
-	return avic_alloc_physical_id_table(kvm);
-}
-
 static int svm_vcpu_create(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_svm *svm;
@@ -5333,7 +5328,7 @@ struct kvm_x86_ops svm_x86_ops __initdata = {
 	.emergency_disable_virtualization_cpu = svm_emergency_disable_virtualization_cpu,
 	.has_emulated_msr = svm_has_emulated_msr,
 
-	.vcpu_precreate = svm_vcpu_precreate,
+	.vcpu_precreate = avic_vcpu_precreate,
 	.vcpu_create = svm_vcpu_create,
 	.vcpu_free = svm_vcpu_free,
 	.vcpu_reset = svm_vcpu_reset,
@@ -5712,6 +5707,7 @@ static __init int svm_hardware_setup(void)
 	enable_apicv = avic_hardware_setup();
 	if (!enable_apicv) {
 		enable_ipiv = false;
+		svm_x86_ops.vcpu_precreate = NULL;
 		svm_x86_ops.vcpu_blocking = NULL;
 		svm_x86_ops.vcpu_unblocking = NULL;
 		svm_x86_ops.vcpu_get_apicv_inhibit_reasons = NULL;
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 716be21fba33..3c5459374969 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -945,7 +945,7 @@ extern struct kvm_x86_nested_ops svm_nested_ops;
 
 bool __init avic_hardware_setup(void);
 void avic_hardware_unsetup(void);
-int avic_alloc_physical_id_table(struct kvm *kvm);
+int avic_vcpu_precreate(struct kvm *kvm);
 void avic_vm_destroy(struct kvm *kvm);
 int avic_vm_init(struct kvm *kvm);
 void avic_init_vmcb(struct vcpu_svm *svm, struct vmcb *vmcb);
-- 
2.55.0.rc0.799.gd6f94ed593-goog


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 2/3] KVM: SVM: Do all per-VM AVIC initialization during vCPU precreation phase
  2026-06-30 21:01 [PATCH v2 0/3] KVM: SVM: Fix a (very) unlikely UAF for GA Log IRQs Sean Christopherson
  2026-06-30 21:01 ` [PATCH v2 1/3] KVM: SVM: Make kvm_x86_ops.vcpu_precreate() hook fully AVIC specific Sean Christopherson
@ 2026-06-30 21:01 ` Sean Christopherson
  2026-06-30 21:18   ` sashiko-bot
  2026-06-30 21:01 ` [PATCH v2 3/3] KVM: SVM: Remove VM from the GA Log notifier list before VM destruction Sean Christopherson
  2 siblings, 1 reply; 8+ messages in thread
From: Sean Christopherson @ 2026-06-30 21:01 UTC (permalink / raw)
  To: Sean Christopherson, Paolo Bonzini
  Cc: kvm, linux-kernel, Naveen N Rao, Xiao Wu

Move all per-VM AVIC initialization from VM creation to vCPU pre-creation,
i.e. defer allocating the logical ID table and adding the VM to the GA Log
list until vCPUs are created.  This will allow removing the VM from the GA
Log list before vCPUs are destroyed without needing yet another kvm_x86_ops
hook (.vm_pre_destroy() is very intentionally called if and only if VM
creation fully succeeds).

As a bonus, this re-unites physical and logic table allocation, and avoids
allocating a logical table in the unlikely scenario that userspace creates
a VM without an in-kernel local APIC.

Another bonus to hooking .vcpu_precreate() is that there is no need to
unwind on failure, as the VM has already been created, i.e. KVM will run
through all phases of VM destruction.  In fact, unwinding is undesirable,
as KVM tries to keep VM-wide behavior idempotent/sticky across creaton of
multiple vCPUs.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/avic.c | 94 +++++++++++++++++++++++++----------------
 arch/x86/kvm/svm/svm.c  |  6 ---
 2 files changed, 57 insertions(+), 43 deletions(-)

diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index 4a0a2dbd1687..d71a2fed1a08 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -308,47 +308,32 @@ static int avic_alloc_physical_id_table(struct kvm *kvm)
 	return 0;
 }
 
-int avic_vcpu_precreate(struct kvm *kvm)
+static int avic_alloc_logical_id_table(struct kvm *kvm)
 {
-	if (!irqchip_in_kernel(kvm) || WARN_ON_ONCE(!enable_apicv))
-		return 0;
-
-	return avic_alloc_physical_id_table(kvm);
-}
-
-void avic_vm_destroy(struct kvm *kvm)
-{
-	unsigned long flags;
-	struct kvm_svm *kvm_svm = to_kvm_svm(kvm);
-
-	if (!enable_apicv)
-		return;
-
-	free_page((unsigned long)kvm_svm->avic_logical_id_table);
-	free_pages((unsigned long)kvm_svm->avic_physical_id_table,
-		   avic_get_physical_id_table_order(kvm));
-
-	spin_lock_irqsave(&svm_vm_data_hash_lock, flags);
-	hash_del(&kvm_svm->hnode);
-	spin_unlock_irqrestore(&svm_vm_data_hash_lock, flags);
-}
-
-int avic_vm_init(struct kvm *kvm)
-{
-	unsigned long flags;
-	int err = -ENOMEM;
 	struct kvm_svm *kvm_svm = to_kvm_svm(kvm);
-	struct kvm_svm *k2;
-	u32 vm_id;
 
-	if (!enable_apicv)
+	if (kvm_svm->avic_logical_id_table)
 		return 0;
 
 	kvm_svm->avic_logical_id_table = (void *)get_zeroed_page(GFP_KERNEL_ACCOUNT);
 	if (!kvm_svm->avic_logical_id_table)
-		goto free_avic;
+		return -ENOMEM;
 
-	spin_lock_irqsave(&svm_vm_data_hash_lock, flags);
+	return 0;
+}
+
+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;
+
+	lockdep_assert_held(&kvm->lock);
+
+	if (kvm_svm->avic_vm_id)
+		return;
+
+	guard(spinlock_irqsave)(&svm_vm_data_hash_lock);
  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 */
@@ -364,13 +349,48 @@ int avic_vm_init(struct kvm *kvm)
 	}
 	kvm_svm->avic_vm_id = vm_id;
 	hash_add(svm_vm_data_hash, &kvm_svm->hnode, kvm_svm->avic_vm_id);
-	spin_unlock_irqrestore(&svm_vm_data_hash_lock, flags);
+}
 
+int avic_vcpu_precreate(struct kvm *kvm)
+{
+	int r;
+
+	if (!irqchip_in_kernel(kvm) || WARN_ON_ONCE(!enable_apicv))
+		return 0;
+
+	/*
+	 * Don't unwind on failure, all actions must be idempotent with respect
+	 * to creating multiple vCPUs, i.e. must persist until the VM is destroyed.
+	 */
+	r = avic_alloc_physical_id_table(kvm);
+	if (r)
+		return r;
+
+	r = avic_alloc_logical_id_table(kvm);
+	if (r)
+		return r;
+
+	avic_add_vm_to_ga_log_list(kvm);
 	return 0;
+}
 
-free_avic:
-	avic_vm_destroy(kvm);
-	return err;
+void avic_vm_destroy(struct kvm *kvm)
+{
+	unsigned long flags;
+	struct kvm_svm *kvm_svm = to_kvm_svm(kvm);
+
+	if (!enable_apicv)
+		return;
+
+	free_page((unsigned long)kvm_svm->avic_logical_id_table);
+	free_pages((unsigned long)kvm_svm->avic_physical_id_table,
+		   avic_get_physical_id_table_order(kvm));
+
+	if (kvm_svm->avic_vm_id) {
+		spin_lock_irqsave(&svm_vm_data_hash_lock, flags);
+		hash_del(&kvm_svm->hnode);
+		spin_unlock_irqrestore(&svm_vm_data_hash_lock, flags);
+	}
 }
 
 static phys_addr_t avic_get_backing_page_address(struct vcpu_svm *svm)
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index a7d141f7e76c..7f3a815d737f 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -5297,12 +5297,6 @@ static int svm_vm_init(struct kvm *kvm)
 	if (!pause_filter_count || !pause_filter_thresh)
 		kvm_disable_exits(kvm, KVM_X86_DISABLE_EXITS_PAUSE);
 
-	if (enable_apicv) {
-		int ret = avic_vm_init(kvm);
-		if (ret)
-			return ret;
-	}
-
 	svm_srso_vm_init();
 	return 0;
 }
-- 
2.55.0.rc0.799.gd6f94ed593-goog


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 3/3] KVM: SVM: Remove VM from the GA Log notifier list before VM destruction
  2026-06-30 21:01 [PATCH v2 0/3] KVM: SVM: Fix a (very) unlikely UAF for GA Log IRQs Sean Christopherson
  2026-06-30 21:01 ` [PATCH v2 1/3] KVM: SVM: Make kvm_x86_ops.vcpu_precreate() hook fully AVIC specific Sean Christopherson
  2026-06-30 21:01 ` [PATCH v2 2/3] KVM: SVM: Do all per-VM AVIC initialization during vCPU precreation phase Sean Christopherson
@ 2026-06-30 21:01 ` Sean Christopherson
  2026-06-30 21:15   ` sashiko-bot
  2 siblings, 1 reply; 8+ messages in thread
From: Sean Christopherson @ 2026-06-30 21:01 UTC (permalink / raw)
  To: Sean Christopherson, Paolo Bonzini
  Cc: kvm, linux-kernel, Naveen N Rao, Xiao Wu

When a VM is being destroyed, delete it from the list used to process GA
Log interrupts before vCPUs are freed, otherwise avic_ga_log_notifier()
could theoretically hit a use-after-free if a GA Log notification arrives
for a vCPU after the last reference to the VM has been put.

Note, in practice, it's likely all but impossible to trigger UAF, as all
all irqfds and thus all IRTEs are cleaned up by:

  kvm_irqfd_release()
  |
  |-> irqfd_deactivate()
      |
      |-> irqfd_shutdown()
          |
          |-> irq_bypass_unregister_consumer()

And kvm_irqfd_release() is guaranteed to run before the last reference to
the VM is put.  KVM also configures GA Log interrupts only when a vCPU is
blocking (older versions of KVM configre GA Log interrupts at all times,
but AVIC is off by default on those kernels).  Hitting UAF would require
tearing down a VM shortly after a vCPU stopped blocking, and with a very,
very delayed IRQ from hardware.

Opportunistically use guard() to avoid a local "flags" variable.

Fixes: 5881f73757cc ("svm: Introduce AMD IOMMU avic_ga_log_notifier")
Cc: Naveen N Rao (AMD) <naveen@kernel.org>
Cc: Xiao Wu <xiaowu.417@qq.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/avic.c | 19 ++++++++++++-------
 arch/x86/kvm/svm/svm.c  |  2 ++
 arch/x86/kvm/svm/svm.h  |  1 +
 3 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index d71a2fed1a08..c5b1d294b15a 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);
+}
+
 void avic_vm_destroy(struct kvm *kvm)
 {
-	unsigned long flags;
 	struct kvm_svm *kvm_svm = to_kvm_svm(kvm);
 
 	if (!enable_apicv)
@@ -385,12 +396,6 @@ void avic_vm_destroy(struct kvm *kvm)
 	free_page((unsigned long)kvm_svm->avic_logical_id_table);
 	free_pages((unsigned long)kvm_svm->avic_physical_id_table,
 		   avic_get_physical_id_table_order(kvm));
-
-	if (kvm_svm->avic_vm_id) {
-		spin_lock_irqsave(&svm_vm_data_hash_lock, flags);
-		hash_del(&kvm_svm->hnode);
-		spin_unlock_irqrestore(&svm_vm_data_hash_lock, flags);
-	}
 }
 
 static phys_addr_t avic_get_backing_page_address(struct vcpu_svm *svm)
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 7f3a815d737f..0e0dd9618750 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -5329,6 +5329,7 @@ struct kvm_x86_ops svm_x86_ops __initdata = {
 
 	.vm_size = sizeof(struct kvm_svm),
 	.vm_init = svm_vm_init,
+	.vm_pre_destroy = avic_vm_pre_destroy,
 	.vm_destroy = svm_vm_destroy,
 
 	.prepare_switch_to_guest = svm_prepare_switch_to_guest,
@@ -5702,6 +5703,7 @@ static __init int svm_hardware_setup(void)
 	if (!enable_apicv) {
 		enable_ipiv = false;
 		svm_x86_ops.vcpu_precreate = NULL;
+		svm_x86_ops.vm_pre_destroy = NULL;
 		svm_x86_ops.vcpu_blocking = NULL;
 		svm_x86_ops.vcpu_unblocking = NULL;
 		svm_x86_ops.vcpu_get_apicv_inhibit_reasons = NULL;
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 3c5459374969..616e45624f4c 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -946,6 +946,7 @@ extern struct kvm_x86_nested_ops svm_nested_ops;
 bool __init avic_hardware_setup(void);
 void avic_hardware_unsetup(void);
 int avic_vcpu_precreate(struct kvm *kvm);
+void avic_vm_pre_destroy(struct kvm *kvm);
 void avic_vm_destroy(struct kvm *kvm);
 int avic_vm_init(struct kvm *kvm);
 void avic_init_vmcb(struct vcpu_svm *svm, struct vmcb *vmcb);
-- 
2.55.0.rc0.799.gd6f94ed593-goog


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 3/3] KVM: SVM: Remove VM from the GA Log notifier list before VM destruction
  2026-06-30 21:01 ` [PATCH v2 3/3] KVM: SVM: Remove VM from the GA Log notifier list before VM destruction Sean Christopherson
@ 2026-06-30 21:15   ` sashiko-bot
  2026-06-30 21:27     ` Sean Christopherson
  0 siblings, 1 reply; 8+ messages in thread
From: sashiko-bot @ 2026-06-30 21:15 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: kvm

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 2/3] KVM: SVM: Do all per-VM AVIC initialization during vCPU precreation phase
  2026-06-30 21:01 ` [PATCH v2 2/3] KVM: SVM: Do all per-VM AVIC initialization during vCPU precreation phase Sean Christopherson
@ 2026-06-30 21:18   ` sashiko-bot
  2026-06-30 21:20     ` Sean Christopherson
  0 siblings, 1 reply; 8+ messages in thread
From: sashiko-bot @ 2026-06-30 21:18 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: kvm

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The function prototype `avic_vm_init(struct kvm *kvm)` remains in `arch/x86/kvm/svm/svm.h` despite its implementation being removed.
--

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

    KVM: SVM: Do all per-VM AVIC initialization during vCPU precreation phase
    
    Move all per-VM AVIC initialization from VM creation to vCPU pre-creation,
    i.e. defer allocating the logical ID table and adding the VM to the GA Log
    list until vCPUs are created.  This will allow removing the VM from the GA
    Log list before vCPUs are destroyed without needing yet another kvm_x86_ops
    hook (.vm_pre_destroy() is very intentionally called if and only if VM
    creation fully succeeds).

> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index 4a0a2dbd1687f..d71a2fed1a08b 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c

[ ... ]

> @@ -308,47 +308,32 @@ static int avic_alloc_physical_id_table(struct kvm *kvm)

[ ... ]

> -int avic_vm_init(struct kvm *kvm)
> +static void avic_add_vm_to_ga_log_list(struct kvm *kvm)
>  {

[Severity: Low]
Does the function prototype for avic_vm_init() in arch/x86/kvm/svm/svm.h also
need to be removed? The declaration appears to still be present even though
the implementation is removed here.

> -	unsigned long flags;
> -	int err = -ENOMEM;
>  	struct kvm_svm *kvm_svm = to_kvm_svm(kvm);

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 2/3] KVM: SVM: Do all per-VM AVIC initialization during vCPU precreation phase
  2026-06-30 21:18   ` sashiko-bot
@ 2026-06-30 21:20     ` Sean Christopherson
  0 siblings, 0 replies; 8+ messages in thread
From: Sean Christopherson @ 2026-06-30 21:20 UTC (permalink / raw)
  To: sashiko-reviews; +Cc: kvm

On Tue, Jun 30, 2026, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> > diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> > index 4a0a2dbd1687f..d71a2fed1a08b 100644
> > --- a/arch/x86/kvm/svm/avic.c
> > +++ b/arch/x86/kvm/svm/avic.c
> 
> [ ... ]
> 
> > @@ -308,47 +308,32 @@ static int avic_alloc_physical_id_table(struct kvm *kvm)
> 
> [ ... ]
> 
> > -int avic_vm_init(struct kvm *kvm)
> > +static void avic_add_vm_to_ga_log_list(struct kvm *kvm)
> >  {
> 
> [Severity: Low]
> Does the function prototype for avic_vm_init() in arch/x86/kvm/svm/svm.h also
> need to be removed? The declaration appears to still be present even though
> the implementation is removed here.

Doh.  Assuming no other issues, I'll fixup when applying.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 3/3] KVM: SVM: Remove VM from the GA Log notifier list before VM destruction
  2026-06-30 21:15   ` sashiko-bot
@ 2026-06-30 21:27     ` Sean Christopherson
  0 siblings, 0 replies; 8+ messages in thread
From: Sean Christopherson @ 2026-06-30 21:27 UTC (permalink / raw)
  To: sashiko-reviews; +Cc: kvm

On Tue, Jun 30, 2026, sashiko-bot@kernel.org wrote:
> > 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?

Ugh, yes, that's theoretically possible.  I *think* we can simply move the call
to kvm_vcpu_wake_up() inside the spinlock.  There is a potential deadlock that's
near this code[*], but I don't think svm_vm_data_hash_lock will have the same
problem as ir_list_lock?  Regardless, I'll tackle this one in a separate patch,
unless it looks like we'll have to completely redo the entire scheme.

[*] https://lore.kernel.org/all/aUmdSb3d7Z5REMLk@google.com

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2026-06-30 21:27 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-06-30 21:01 [PATCH v2 0/3] KVM: SVM: Fix a (very) unlikely UAF for GA Log IRQs Sean Christopherson
2026-06-30 21:01 ` [PATCH v2 1/3] KVM: SVM: Make kvm_x86_ops.vcpu_precreate() hook fully AVIC specific Sean Christopherson
2026-06-30 21:01 ` [PATCH v2 2/3] KVM: SVM: Do all per-VM AVIC initialization during vCPU precreation phase Sean Christopherson
2026-06-30 21:18   ` sashiko-bot
2026-06-30 21:20     ` Sean Christopherson
2026-06-30 21:01 ` [PATCH v2 3/3] KVM: SVM: Remove VM from the GA Log notifier list before VM destruction Sean Christopherson
2026-06-30 21:15   ` sashiko-bot
2026-06-30 21:27     ` Sean Christopherson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox