[PATCH v2 0/3] KVM: SVM: Fix AVIC Physical ID table UAF

[PATCH v2 0/3] KVM: SVM: Fix AVIC Physical ID table UAF

[Sean Christopherson]

Naveen's fix to clear AVIC Physical ID table entries if vCPU creation fails,
to ensure the CPU doesn't access the freed virtual  APIC state.

v2: Add prep patches to consolidate the table bounds checks.

v1: https://lore.kernel.org/all/20260903062856.2090499-1-naveen@kernel.org

Naveen N Rao (1):
  KVM: SVM: Clear AVIC Physical ID table entry if vCPU creation fails

Sean Christopherson (2):
  KVM: SVM: Add paranoid helper for checking if vCPU is AVIC-addressable
  KVM: SVM: Use "is AVIC-addressable" helper to sanity check
    load()/put()

 arch/x86/kvm/svm/avic.c | 22 ++++++++++++++++------
 arch/x86/kvm/svm/svm.c  |  6 +++++-
 arch/x86/kvm/svm/svm.h  |  1 +
 3 files changed, 22 insertions(+), 7 deletions(-)


base-commit: d4b7fb647204f0c81dfeae2d1a708e4d858e0c94
-- 
2.55.0.1082.g2b9226bbc0-goog

[PATCH v2 1/3] KVM: SVM: Add paranoid helper for checking if vCPU is AVIC-addressable

[Sean Christopherson]

Add a helper to check if a vCPU is addressable by AVIC hardware, i.e. has
an APIC ID that fits in the physical ID table, and use the more paranoid
helper when determining if a vCPU is compatible with AVIC when initializing
the vCPU.  KVM is supposed to reject vCPU creation if the vCPU's ID is
greater than or equal to max_vcpu_ids, i.e. simply checking the
architectural maximum *should* suffice.  But piecing together why this is
safe is unnecessarily difficult, and there is no meaningful downside to
being extra cautious.

Suggested-by: Naveen N Rao (AMD) <naveen@kernel.org>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/avic.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index 3b037e385523..0e4b5eb6ac82 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -395,6 +395,11 @@ static phys_addr_t avic_get_backing_page_address(struct vcpu_svm *svm)
 	return __sme_set(__pa(svm->vcpu.arch.apic->regs));
 }
 
+static bool avic_is_addressable_vcpu(struct kvm_vcpu *vcpu)
+{
+	return vcpu->vcpu_id <= __avic_get_max_physical_id(vcpu->kvm, NULL);
+}
+
 void avic_init_vmcb(struct vcpu_svm *svm, struct vmcb *vmcb)
 {
 	struct kvm_svm *kvm_svm = to_kvm_svm(svm->vcpu.kvm);
@@ -412,7 +417,6 @@ void avic_init_vmcb(struct vcpu_svm *svm, struct vmcb *vmcb)
 
 static int avic_init_backing_page(struct kvm_vcpu *vcpu)
 {
-	u32 max_id = x2avic_enabled ? x2avic_max_physical_id : AVIC_MAX_PHYSICAL_ID;
 	struct kvm_svm *kvm_svm = to_kvm_svm(vcpu->kvm);
 	struct vcpu_svm *svm = to_svm(vcpu);
 	u32 id = vcpu->vcpu_id;
@@ -425,7 +429,7 @@ static int avic_init_backing_page(struct kvm_vcpu *vcpu)
 	 * avic_vcpu_load() expects to be called if and only if the vCPU has
 	 * fully initialized AVIC.
 	 */
-	if (id > max_id) {
+	if (!avic_is_addressable_vcpu(vcpu)) {
 		kvm_set_apicv_inhibit(vcpu->kvm, APICV_INHIBIT_REASON_PHYSICAL_ID_TOO_BIG);
 		vcpu->arch.apic->apicv_active = false;
 		return 0;
-- 
2.55.0.1082.g2b9226bbc0-goog

[PATCH v2 2/3] KVM: SVM: Use "is AVIC-addressable" helper to sanity check load()/put()

[Sean Christopherson]

Use avic_is_addressable_vcpu() instead of open coding a check on the bounds
of the allocated table for the sanity checks when loading/putting AVIC
state for a vCPU.  If KVM botches the allocation, then KVM will already
have performed an OOB write in avic_init_backing_page(), i.e. being super
paranoid in load()/put() doesn't provide meaningful protection in practice.

Cc: Naveen N Rao (AMD) <naveen@kernel.org>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/avic.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index 0e4b5eb6ac82..4173a30dfe60 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -1049,8 +1049,7 @@ static void __avic_vcpu_load(struct kvm_vcpu *vcpu, int cpu,
 	if (WARN_ON(h_physical_id & ~AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK))
 		return;
 
-	if (WARN_ON_ONCE(vcpu->vcpu_id * sizeof(entry) >=
-			 PAGE_SIZE << avic_get_physical_id_table_order(vcpu->kvm)))
+	if (WARN_ON_ONCE(!avic_is_addressable_vcpu(vcpu)))
 		return;
 
 	/*
@@ -1112,8 +1111,7 @@ static void __avic_vcpu_put(struct kvm_vcpu *vcpu, enum avic_vcpu_action action)
 
 	lockdep_assert_preemption_disabled();
 
-	if (WARN_ON_ONCE(vcpu->vcpu_id * sizeof(entry) >=
-			 PAGE_SIZE << avic_get_physical_id_table_order(vcpu->kvm)))
+	if (WARN_ON_ONCE(!avic_is_addressable_vcpu(vcpu)))
 		return;
 
 	/*
-- 
2.55.0.1082.g2b9226bbc0-goog

[PATCH v2 3/3] KVM: SVM: Clear AVIC Physical ID table entry if vCPU creation fails

[Sean Christopherson]

From: Naveen N Rao <naveen@kernel.org>

If vCPU creation fails after kvm_arch_vcpu_create(), the AVIC Physical
ID table entry corresponding to that vCPU continues to point to the
freed APIC backing page which can result in UAF. Address this by
clearing out the corresponding AVIC Physical ID table entry in the
vcpu_free() callback, similar to the VMX commit b41f2ca6c060 ("KVM: VMX:
Fix stale PID-pointer table entry left after vCPU free").

Though unlikely, it is also possible that svm_vcpu_create() itself fails
after the AVIC Physical ID table entry has been setup if memory
allocation fails in svm_vcpu_alloc_msrpm(). Clear the entry in this path
as well.

Note: this change depends on commit 97d65b544f48 ("KVM: Check for
duplicate vcpu_id as early as possible"), which ensures that a vCPU with
a duplicate ID is never created. Otherwise, a valid AVIC Physical ID
table entry for an existing vCPU will be cleared.

Fixes: 44a95dae1d22 ("KVM: x86: Detect and Initialize AVIC support")
Signed-off-by: Naveen N Rao (AMD) <naveen@kernel.org>
Tested-by: Atish Patra <atishp@meta.com>
[sean: use avic_is_addressable_vcpu()]
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/avic.c | 8 ++++++++
 arch/x86/kvm/svm/svm.c  | 6 +++++-
 arch/x86/kvm/svm/svm.h  | 1 +
 3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index 4173a30dfe60..96ca39c0045f 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -889,6 +889,14 @@ int avic_init_vcpu(struct vcpu_svm *svm)
 	return ret;
 }
 
+void avic_vcpu_free(struct kvm_vcpu *vcpu)
+{
+	struct kvm_svm *kvm_svm = to_kvm_svm(vcpu->kvm);
+
+	if (kvm_svm->avic_physical_id_table && avic_is_addressable_vcpu(vcpu))
+		WRITE_ONCE(kvm_svm->avic_physical_id_table[vcpu->vcpu_id], 0);
+}
+
 void avic_apicv_post_state_restore(struct kvm_vcpu *vcpu)
 {
 	avic_handle_dfr_update(vcpu);
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 7d59d301e1e5..686e4c560fec 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -1337,7 +1337,7 @@ static int svm_vcpu_create(struct kvm_vcpu *vcpu)
 	svm->msrpm = svm_vcpu_alloc_msrpm();
 	if (!svm->msrpm) {
 		err = -ENOMEM;
-		goto error_free_sev;
+		goto error_free_avic;
 	}
 
 	svm->x2avic_msrs_intercepted = true;
@@ -1351,6 +1351,8 @@ static int svm_vcpu_create(struct kvm_vcpu *vcpu)
 
 	return 0;
 
+error_free_avic:
+	avic_vcpu_free(vcpu);
 error_free_sev:
 	sev_free_vcpu(vcpu);
 error_free_vmcb_page:
@@ -1365,6 +1367,8 @@ static void svm_vcpu_free(struct kvm_vcpu *vcpu)
 
 	WARN_ON_ONCE(!list_empty(&svm->ir_list));
 
+	avic_vcpu_free(vcpu);
+
 	svm_leave_nested(vcpu);
 	svm_free_nested(svm);
 
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index e958943b8162..790bd96a9791 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -954,6 +954,7 @@ void avic_init_vmcb(struct vcpu_svm *svm, struct vmcb *vmcb);
 int avic_incomplete_ipi_interception(struct kvm_vcpu *vcpu);
 int avic_unaccelerated_access_interception(struct kvm_vcpu *vcpu);
 int avic_init_vcpu(struct vcpu_svm *svm);
+void avic_vcpu_free(struct kvm_vcpu *vcpu);
 void avic_vcpu_load(struct kvm_vcpu *vcpu, int cpu);
 void avic_vcpu_put(struct kvm_vcpu *vcpu);
 void avic_apicv_post_state_restore(struct kvm_vcpu *vcpu);
-- 
2.55.0.1082.g2b9226bbc0-goog