public inbox for kvm@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 00/10] KVM: SVM: Attempt to cleanup SEV_FEATURES
@ 2025-02-27  1:25 Sean Christopherson
  2025-02-27  1:25 ` [PATCH v2 01/10] KVM: SVM: Save host DR masks on CPUs with DebugSwap Sean Christopherson
                   ` (10 more replies)
  0 siblings, 11 replies; 20+ messages in thread
From: Sean Christopherson @ 2025-02-27  1:25 UTC (permalink / raw)
  To: Sean Christopherson, Paolo Bonzini
  Cc: kvm, linux-kernel, Naveen N Rao, Kim Phillips, Tom Lendacky,
	Alexey Kardashevskiy, Pankaj Gupta

Try to address the worst of the issues that arise with guest controlled SEV
features (thanks AP creation)[1].  The most pressing issue is with DebugSwap,
as a misbehaving guest could clobber host DR masks (which should be relatively
benign?).

The other notable issue is that KVM doesn't guard against userspace manually
making a vCPU RUNNABLE after it has been DESTROYED (or after a failed CREATE).
This shouldn't be super problematic, as VMRUN is supposed to "only" fail if
the VMSA page is invalid, but passing a known bad PA to hardware isn't exactly
desirable.

[1] https://lore.kernel.org/all/Z7TSef290IQxQhT2@google.com

v2:
 - Collect reviews. [Tom, Pankaj]
 - Fix a changelog typo. [Tom]
 - Reject KVM_RUN, but don't terminate the guest if KVM attempts VMRUN
   with a bad VMSA. [Tom]
 - Fix a commment where DRs were incorreclty listed as Type-A when DebugSwap
   is disabled/unsupported. [Tom]

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

Sean Christopherson (10):
  KVM: SVM: Save host DR masks on CPUs with DebugSwap
  KVM: SVM: Don't rely on DebugSwap to restore host DR0..DR3
  KVM: SVM: Refuse to attempt VRMUN if an SEV-ES+ guest has an invalid
    VMSA
  KVM: SVM: Don't change target vCPU state on AP Creation VMGEXIT error
  KVM: SVM: Require AP's "requested" SEV_FEATURES to match KVM's view
  KVM: SVM: Simplify request+kick logic in SNP AP Creation handling
  KVM: SVM: Use guard(mutex) to simplify SNP AP Creation error handling
  KVM: SVM: Mark VMCB dirty before processing incoming snp_vmsa_gpa
  KVM: SVM: Use guard(mutex) to simplify SNP vCPU state updates
  KVM: SVM: Invalidate "next" SNP VMSA GPA even on failure

 arch/x86/kvm/svm/sev.c | 218 +++++++++++++++++++----------------------
 arch/x86/kvm/svm/svm.c |  11 ++-
 arch/x86/kvm/svm/svm.h |   2 +-
 3 files changed, 109 insertions(+), 122 deletions(-)


base-commit: fed48e2967f402f561d80075a20c5c9e16866e53
-- 
2.48.1.711.g2feabab25a-goog


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

* [PATCH v2 01/10] KVM: SVM: Save host DR masks on CPUs with DebugSwap
  2025-02-27  1:25 [PATCH v2 00/10] KVM: SVM: Attempt to cleanup SEV_FEATURES Sean Christopherson
@ 2025-02-27  1:25 ` Sean Christopherson
  2025-02-27  1:25 ` [PATCH v2 02/10] KVM: SVM: Don't rely on DebugSwap to restore host DR0..DR3 Sean Christopherson
                   ` (9 subsequent siblings)
  10 siblings, 0 replies; 20+ messages in thread
From: Sean Christopherson @ 2025-02-27  1:25 UTC (permalink / raw)
  To: Sean Christopherson, Paolo Bonzini
  Cc: kvm, linux-kernel, Naveen N Rao, Kim Phillips, Tom Lendacky,
	Alexey Kardashevskiy, Pankaj Gupta

When running SEV-SNP guests on a CPU that supports DebugSwap, always save
the host's DR0..DR3 mask MSR values irrespective of whether or not
DebugSwap is enabled, to ensure the host values aren't clobbered by the
CPU.  And for now, also save DR0..DR3, even though doing so isn't
necessary (see below).

SVM_VMGEXIT_AP_CREATE is deeply flawed in that it allows the *guest* to
create a VMSA with guest-controlled SEV_FEATURES.  A well behaved guest
can inform the hypervisor, i.e. KVM, of its "requested" features, but on
CPUs without ALLOWED_SEV_FEATURES support, nothing prevents the guest from
lying about which SEV features are being enabled (or not!).

If a misbehaving guest enables DebugSwap in a secondary vCPU's VMSA, the
CPU will load the DR0..DR3 mask MSRs on #VMEXIT, i.e. will clobber the
MSRs with '0' if KVM doesn't save its desired value.

Note, DR0..DR3 themselves are "ok", as DR7 is reset on #VMEXIT, and KVM
restores all DRs in common x86 code as needed via hw_breakpoint_restore().
I.e. there is no risk of host DR0..DR3 being clobbered (when it matters).
However, there is a flaw in the opposite direction; because the guest can
lie about enabling DebugSwap, i.e. can *disable* DebugSwap without KVM's
knowledge, KVM must not rely on the CPU to restore DRs.  Defer fixing
that wart, as it's more of a documentation issue than a bug in the code.

Note, KVM added support for DebugSwap on commit d1f85fbe836e ("KVM: SEV:
Enable data breakpoints in SEV-ES"), but that is not an appropriate Fixes,
as the underlying flaw exists in hardware, not in KVM.  I.e. all kernels
that support SEV-SNP need to be patched, not just kernels with KVM's full
support for DebugSwap (ignoring that DebugSwap support landed first).

Opportunistically fix an incorrect statement in the comment; on CPUs
without DebugSwap, the CPU does NOT save or load debug registers, i.e.

Fixes: e366f92ea99e ("KVM: SEV: Support SEV-SNP AP Creation NAE event")
Cc: stable@vger.kernel.org
Cc: Naveen N Rao <naveen@kernel.org>
Cc: Kim Phillips <kim.phillips@amd.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Alexey Kardashevskiy <aik@amd.com>
Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/sev.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 74525651770a..5c3d8618b722 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -4568,6 +4568,8 @@ void sev_es_vcpu_reset(struct vcpu_svm *svm)
 
 void sev_es_prepare_switch_to_guest(struct vcpu_svm *svm, struct sev_es_save_area *hostsa)
 {
+	struct kvm *kvm = svm->vcpu.kvm;
+
 	/*
 	 * All host state for SEV-ES guests is categorized into three swap types
 	 * based on how it is handled by hardware during a world switch:
@@ -4591,10 +4593,15 @@ void sev_es_prepare_switch_to_guest(struct vcpu_svm *svm, struct sev_es_save_are
 
 	/*
 	 * If DebugSwap is enabled, debug registers are loaded but NOT saved by
-	 * the CPU (Type-B). If DebugSwap is disabled/unsupported, the CPU both
-	 * saves and loads debug registers (Type-A).
+	 * the CPU (Type-B). If DebugSwap is disabled/unsupported, the CPU does
+	 * not save or load debug registers.  Sadly, on CPUs without
+	 * ALLOWED_SEV_FEATURES, KVM can't prevent SNP guests from enabling
+	 * DebugSwap on secondary vCPUs without KVM's knowledge via "AP Create".
+	 * Save all registers if DebugSwap is supported to prevent host state
+	 * from being clobbered by a misbehaving guest.
 	 */
-	if (sev_vcpu_has_debug_swap(svm)) {
+	if (sev_vcpu_has_debug_swap(svm) ||
+	    (sev_snp_guest(kvm) && cpu_feature_enabled(X86_FEATURE_DEBUG_SWAP))) {
 		hostsa->dr0 = native_get_debugreg(0);
 		hostsa->dr1 = native_get_debugreg(1);
 		hostsa->dr2 = native_get_debugreg(2);
-- 
2.48.1.711.g2feabab25a-goog


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

* [PATCH v2 02/10] KVM: SVM: Don't rely on DebugSwap to restore host DR0..DR3
  2025-02-27  1:25 [PATCH v2 00/10] KVM: SVM: Attempt to cleanup SEV_FEATURES Sean Christopherson
  2025-02-27  1:25 ` [PATCH v2 01/10] KVM: SVM: Save host DR masks on CPUs with DebugSwap Sean Christopherson
@ 2025-02-27  1:25 ` Sean Christopherson
  2025-02-27  1:25 ` [PATCH v2 03/10] KVM: SVM: Refuse to attempt VRMUN if an SEV-ES+ guest has an invalid VMSA Sean Christopherson
                   ` (8 subsequent siblings)
  10 siblings, 0 replies; 20+ messages in thread
From: Sean Christopherson @ 2025-02-27  1:25 UTC (permalink / raw)
  To: Sean Christopherson, Paolo Bonzini
  Cc: kvm, linux-kernel, Naveen N Rao, Kim Phillips, Tom Lendacky,
	Alexey Kardashevskiy, Pankaj Gupta

Never rely on the CPU to restore/load host DR0..DR3 values, even if the
CPU supports DebugSwap, as there are no guarantees that SNP guests will
actually enable DebugSwap on APs.  E.g. if KVM were to rely on the CPU to
load DR0..DR3 and skipped them during hw_breakpoint_restore(), KVM would
run with clobbered-to-zero DRs if an SNP guest created APs without
DebugSwap enabled.

Update the comment to explain the dangers, and hopefully prevent breaking
KVM in the future.

Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/sev.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 5c3d8618b722..719cd48330f1 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -4594,18 +4594,21 @@ void sev_es_prepare_switch_to_guest(struct vcpu_svm *svm, struct sev_es_save_are
 	/*
 	 * If DebugSwap is enabled, debug registers are loaded but NOT saved by
 	 * the CPU (Type-B). If DebugSwap is disabled/unsupported, the CPU does
-	 * not save or load debug registers.  Sadly, on CPUs without
-	 * ALLOWED_SEV_FEATURES, KVM can't prevent SNP guests from enabling
-	 * DebugSwap on secondary vCPUs without KVM's knowledge via "AP Create".
-	 * Save all registers if DebugSwap is supported to prevent host state
-	 * from being clobbered by a misbehaving guest.
+	 * not save or load debug registers.  Sadly, KVM can't prevent SNP
+	 * guests from lying about DebugSwap on secondary vCPUs, i.e. the
+	 * SEV_FEATURES provided at "AP Create" isn't guaranteed to match what
+	 * the guest has actually enabled (or not!) in the VMSA.
+	 *
+	 * If DebugSwap is *possible*, save the masks so that they're restored
+	 * if the guest enables DebugSwap.  But for the DRs themselves, do NOT
+	 * rely on the CPU to restore the host values; KVM will restore them as
+	 * needed in common code, via hw_breakpoint_restore().  Note, KVM does
+	 * NOT support virtualizing Breakpoint Extensions, i.e. the mask MSRs
+	 * don't need to be restored per se, KVM just needs to ensure they are
+	 * loaded with the correct values *if* the CPU writes the MSRs.
 	 */
 	if (sev_vcpu_has_debug_swap(svm) ||
 	    (sev_snp_guest(kvm) && cpu_feature_enabled(X86_FEATURE_DEBUG_SWAP))) {
-		hostsa->dr0 = native_get_debugreg(0);
-		hostsa->dr1 = native_get_debugreg(1);
-		hostsa->dr2 = native_get_debugreg(2);
-		hostsa->dr3 = native_get_debugreg(3);
 		hostsa->dr0_addr_mask = amd_get_dr_addr_mask(0);
 		hostsa->dr1_addr_mask = amd_get_dr_addr_mask(1);
 		hostsa->dr2_addr_mask = amd_get_dr_addr_mask(2);
-- 
2.48.1.711.g2feabab25a-goog


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

* [PATCH v2 03/10] KVM: SVM: Refuse to attempt VRMUN if an SEV-ES+ guest has an invalid VMSA
  2025-02-27  1:25 [PATCH v2 00/10] KVM: SVM: Attempt to cleanup SEV_FEATURES Sean Christopherson
  2025-02-27  1:25 ` [PATCH v2 01/10] KVM: SVM: Save host DR masks on CPUs with DebugSwap Sean Christopherson
  2025-02-27  1:25 ` [PATCH v2 02/10] KVM: SVM: Don't rely on DebugSwap to restore host DR0..DR3 Sean Christopherson
@ 2025-02-27  1:25 ` Sean Christopherson
  2025-02-27 16:03   ` Tom Lendacky
  2025-02-27 16:56   ` Gupta, Pankaj
  2025-02-27  1:25 ` [PATCH v2 04/10] KVM: SVM: Don't change target vCPU state on AP Creation VMGEXIT error Sean Christopherson
                   ` (7 subsequent siblings)
  10 siblings, 2 replies; 20+ messages in thread
From: Sean Christopherson @ 2025-02-27  1:25 UTC (permalink / raw)
  To: Sean Christopherson, Paolo Bonzini
  Cc: kvm, linux-kernel, Naveen N Rao, Kim Phillips, Tom Lendacky,
	Alexey Kardashevskiy, Pankaj Gupta

Explicitly reject KVM_RUN with KVM_EXIT_FAIL_ENTRY if userspace "coerces"
KVM into running an SEV-ES+ guest with an invalid VMSA, e.g. by modifying
a vCPU's mp_state to be RUNNABLE after an SNP vCPU has undergone a Destroy
event.  On Destroy or failed Create, KVM marks the vCPU HALTED so that
*KVM* doesn't run the vCPU, but nothing prevents a misbehaving VMM from
manually making the vCPU RUNNABLE via KVM_SET_MP_STATE.

Attempting VMRUN with an invalid VMSA should be harmless, but knowingly
executing VMRUN with bad control state is at best dodgy.

Fixes: e366f92ea99e ("KVM: SEV: Support SEV-SNP AP Creation NAE event")
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/sev.c | 16 +++++++++++++---
 arch/x86/kvm/svm/svm.c | 11 +++++++++--
 arch/x86/kvm/svm/svm.h |  2 +-
 3 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 719cd48330f1..218738a360ba 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -3452,10 +3452,19 @@ void sev_es_unmap_ghcb(struct vcpu_svm *svm)
 	svm->sev_es.ghcb = NULL;
 }
 
-void pre_sev_run(struct vcpu_svm *svm, int cpu)
+int pre_sev_run(struct vcpu_svm *svm, int cpu)
 {
 	struct svm_cpu_data *sd = per_cpu_ptr(&svm_data, cpu);
-	unsigned int asid = sev_get_asid(svm->vcpu.kvm);
+	struct kvm *kvm = svm->vcpu.kvm;
+	unsigned int asid = sev_get_asid(kvm);
+
+	/*
+	 * Reject KVM_RUN if userspace attempts to run the vCPU with an invalid
+	 * VMSA, e.g. if userspace forces the vCPU to be RUNNABLE after an SNP
+	 * AP Destroy event.
+	 */
+	if (sev_es_guest(kvm) && !VALID_PAGE(svm->vmcb->control.vmsa_pa))
+		return -EINVAL;
 
 	/* Assign the asid allocated with this SEV guest */
 	svm->asid = asid;
@@ -3468,11 +3477,12 @@ void pre_sev_run(struct vcpu_svm *svm, int cpu)
 	 */
 	if (sd->sev_vmcbs[asid] == svm->vmcb &&
 	    svm->vcpu.arch.last_vmentry_cpu == cpu)
-		return;
+		return 0;
 
 	sd->sev_vmcbs[asid] = svm->vmcb;
 	svm->vmcb->control.tlb_ctl = TLB_CONTROL_FLUSH_ASID;
 	vmcb_mark_dirty(svm->vmcb, VMCB_ASID);
+	return 0;
 }
 
 #define GHCB_SCRATCH_AREA_LIMIT		(16ULL * PAGE_SIZE)
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index b8aa0f36850f..f72bcf2e590e 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3587,7 +3587,7 @@ static int svm_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
 	return svm_invoke_exit_handler(vcpu, exit_code);
 }
 
-static void pre_svm_run(struct kvm_vcpu *vcpu)
+static int pre_svm_run(struct kvm_vcpu *vcpu)
 {
 	struct svm_cpu_data *sd = per_cpu_ptr(&svm_data, vcpu->cpu);
 	struct vcpu_svm *svm = to_svm(vcpu);
@@ -3609,6 +3609,8 @@ static void pre_svm_run(struct kvm_vcpu *vcpu)
 	/* FIXME: handle wraparound of asid_generation */
 	if (svm->current_vmcb->asid_generation != sd->asid_generation)
 		new_asid(svm, sd);
+
+	return 0;
 }
 
 static void svm_inject_nmi(struct kvm_vcpu *vcpu)
@@ -4231,7 +4233,12 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu,
 	if (force_immediate_exit)
 		smp_send_reschedule(vcpu->cpu);
 
-	pre_svm_run(vcpu);
+	if (pre_svm_run(vcpu)) {
+		vcpu->run->exit_reason = KVM_EXIT_FAIL_ENTRY;
+		vcpu->run->fail_entry.hardware_entry_failure_reason = SVM_EXIT_ERR;
+		vcpu->run->fail_entry.cpu = vcpu->cpu;
+		return EXIT_FASTPATH_EXIT_USERSPACE;
+	}
 
 	sync_lapic_to_cr8(vcpu);
 
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 5b159f017055..e51852977b70 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -713,7 +713,7 @@ void avic_refresh_virtual_apic_mode(struct kvm_vcpu *vcpu);
 
 /* sev.c */
 
-void pre_sev_run(struct vcpu_svm *svm, int cpu);
+int pre_sev_run(struct vcpu_svm *svm, int cpu);
 void sev_init_vmcb(struct vcpu_svm *svm);
 void sev_vcpu_after_set_cpuid(struct vcpu_svm *svm);
 int sev_es_string_io(struct vcpu_svm *svm, int size, unsigned int port, int in);
-- 
2.48.1.711.g2feabab25a-goog


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

* [PATCH v2 04/10] KVM: SVM: Don't change target vCPU state on AP Creation VMGEXIT error
  2025-02-27  1:25 [PATCH v2 00/10] KVM: SVM: Attempt to cleanup SEV_FEATURES Sean Christopherson
                   ` (2 preceding siblings ...)
  2025-02-27  1:25 ` [PATCH v2 03/10] KVM: SVM: Refuse to attempt VRMUN if an SEV-ES+ guest has an invalid VMSA Sean Christopherson
@ 2025-02-27  1:25 ` Sean Christopherson
  2025-02-27 10:25   ` Gupta, Pankaj
  2025-02-27  1:25 ` [PATCH v2 05/10] KVM: SVM: Require AP's "requested" SEV_FEATURES to match KVM's view Sean Christopherson
                   ` (6 subsequent siblings)
  10 siblings, 1 reply; 20+ messages in thread
From: Sean Christopherson @ 2025-02-27  1:25 UTC (permalink / raw)
  To: Sean Christopherson, Paolo Bonzini
  Cc: kvm, linux-kernel, Naveen N Rao, Kim Phillips, Tom Lendacky,
	Alexey Kardashevskiy, Pankaj Gupta

If KVM rejects an AP Creation event, leave the target vCPU state as-is.
Nothing in the GHCB suggests the hypervisor is *allowed* to muck with vCPU
state on failure, let alone required to do so.  Furthermore, kicking only
in the !ON_INIT case leads to divergent behavior, and even the "kick" case
is non-deterministic.

E.g. if an ON_INIT request fails, the guest can successfully retry if the
fixed AP Creation request is made prior to sending INIT.  And if a !ON_INIT
fails, the guest can successfully retry if the fixed AP Creation request is
handled before the target vCPU processes KVM's
KVM_REQ_UPDATE_PROTECTED_GUEST_STATE.

Fixes: e366f92ea99e ("KVM: SEV: Support SEV-SNP AP Creation NAE event")
Cc: stable@vger.kernel.org
Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/sev.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 218738a360ba..9aad0dae3a80 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -3957,16 +3957,12 @@ static int sev_snp_ap_creation(struct vcpu_svm *svm)
 
 	/*
 	 * The target vCPU is valid, so the vCPU will be kicked unless the
-	 * request is for CREATE_ON_INIT. For any errors at this stage, the
-	 * kick will place the vCPU in an non-runnable state.
+	 * request is for CREATE_ON_INIT.
 	 */
 	kick = true;
 
 	mutex_lock(&target_svm->sev_es.snp_vmsa_mutex);
 
-	target_svm->sev_es.snp_vmsa_gpa = INVALID_PAGE;
-	target_svm->sev_es.snp_ap_waiting_for_reset = true;
-
 	/* Interrupt injection mode shouldn't change for AP creation */
 	if (request < SVM_VMGEXIT_AP_DESTROY) {
 		u64 sev_features;
@@ -4012,20 +4008,23 @@ static int sev_snp_ap_creation(struct vcpu_svm *svm)
 		target_svm->sev_es.snp_vmsa_gpa = svm->vmcb->control.exit_info_2;
 		break;
 	case SVM_VMGEXIT_AP_DESTROY:
+		target_svm->sev_es.snp_vmsa_gpa = INVALID_PAGE;
 		break;
 	default:
 		vcpu_unimpl(vcpu, "vmgexit: invalid AP creation request [%#x] from guest\n",
 			    request);
 		ret = -EINVAL;
-		break;
+		goto out;
 	}
 
-out:
+	target_svm->sev_es.snp_ap_waiting_for_reset = true;
+
 	if (kick) {
 		kvm_make_request(KVM_REQ_UPDATE_PROTECTED_GUEST_STATE, target_vcpu);
 		kvm_vcpu_kick(target_vcpu);
 	}
 
+out:
 	mutex_unlock(&target_svm->sev_es.snp_vmsa_mutex);
 
 	return ret;
-- 
2.48.1.711.g2feabab25a-goog


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

* [PATCH v2 05/10] KVM: SVM: Require AP's "requested" SEV_FEATURES to match KVM's view
  2025-02-27  1:25 [PATCH v2 00/10] KVM: SVM: Attempt to cleanup SEV_FEATURES Sean Christopherson
                   ` (3 preceding siblings ...)
  2025-02-27  1:25 ` [PATCH v2 04/10] KVM: SVM: Don't change target vCPU state on AP Creation VMGEXIT error Sean Christopherson
@ 2025-02-27  1:25 ` Sean Christopherson
  2025-02-27  7:12   ` Gupta, Pankaj
  2025-02-27  1:25 ` [PATCH v2 06/10] KVM: SVM: Simplify request+kick logic in SNP AP Creation handling Sean Christopherson
                   ` (5 subsequent siblings)
  10 siblings, 1 reply; 20+ messages in thread
From: Sean Christopherson @ 2025-02-27  1:25 UTC (permalink / raw)
  To: Sean Christopherson, Paolo Bonzini
  Cc: kvm, linux-kernel, Naveen N Rao, Kim Phillips, Tom Lendacky,
	Alexey Kardashevskiy, Pankaj Gupta

When handling an "AP Create" event, return an error if the "requested" SEV
features for the vCPU don't exactly match KVM's view of the VM-scoped
features.  There is no known use case for heterogeneous SEV features across
vCPUs, and while KVM can't actually enforce an exact match since the value
in RAX isn't guaranteed to match what the guest shoved into the VMSA, KVM
can at least avoid knowingly letting the guest run in an unsupported state.

E.g. if a VM is created with DebugSwap disabled, KVM will intercept #DBs
and DRs for all vCPUs, even if an AP is "created" with DebugSwap enabled in
its VMSA.

Note, the GHCB spec only "requires" that "AP use the same interrupt
injection mechanism as the BSP", but given the disaster that is DebugSwap
and SEV_FEATURES in general, it's safe to say that AMD didn't consider all
possible complications with mismatching features between the BSP and APs.

Opportunistically fold the check into the relevant request flavors; the
"request < AP_DESTROY" check is just a bizarre way of implementing the
AP_CREATE_ON_INIT => AP_CREATE fallthrough.

Fixes: e366f92ea99e ("KVM: SEV: Support SEV-SNP AP Creation NAE event")
Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/sev.c | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 9aad0dae3a80..bad5834ec143 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -3932,6 +3932,7 @@ void sev_snp_init_protected_guest_state(struct kvm_vcpu *vcpu)
 
 static int sev_snp_ap_creation(struct vcpu_svm *svm)
 {
+	struct kvm_sev_info *sev = to_kvm_sev_info(svm->vcpu.kvm);
 	struct kvm_vcpu *vcpu = &svm->vcpu;
 	struct kvm_vcpu *target_vcpu;
 	struct vcpu_svm *target_svm;
@@ -3963,26 +3964,18 @@ static int sev_snp_ap_creation(struct vcpu_svm *svm)
 
 	mutex_lock(&target_svm->sev_es.snp_vmsa_mutex);
 
-	/* Interrupt injection mode shouldn't change for AP creation */
-	if (request < SVM_VMGEXIT_AP_DESTROY) {
-		u64 sev_features;
-
-		sev_features = vcpu->arch.regs[VCPU_REGS_RAX];
-		sev_features ^= to_kvm_sev_info(svm->vcpu.kvm)->vmsa_features;
-
-		if (sev_features & SVM_SEV_FEAT_INT_INJ_MODES) {
-			vcpu_unimpl(vcpu, "vmgexit: invalid AP injection mode [%#lx] from guest\n",
-				    vcpu->arch.regs[VCPU_REGS_RAX]);
-			ret = -EINVAL;
-			goto out;
-		}
-	}
-
 	switch (request) {
 	case SVM_VMGEXIT_AP_CREATE_ON_INIT:
 		kick = false;
 		fallthrough;
 	case SVM_VMGEXIT_AP_CREATE:
+		if (vcpu->arch.regs[VCPU_REGS_RAX] != sev->vmsa_features) {
+			vcpu_unimpl(vcpu, "vmgexit: mismatched AP sev_features [%#lx] != [%#llx] from guest\n",
+				    vcpu->arch.regs[VCPU_REGS_RAX], sev->vmsa_features);
+			ret = -EINVAL;
+			goto out;
+		}
+
 		if (!page_address_valid(vcpu, svm->vmcb->control.exit_info_2)) {
 			vcpu_unimpl(vcpu, "vmgexit: invalid AP VMSA address [%#llx] from guest\n",
 				    svm->vmcb->control.exit_info_2);
-- 
2.48.1.711.g2feabab25a-goog


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

* [PATCH v2 06/10] KVM: SVM: Simplify request+kick logic in SNP AP Creation handling
  2025-02-27  1:25 [PATCH v2 00/10] KVM: SVM: Attempt to cleanup SEV_FEATURES Sean Christopherson
                   ` (4 preceding siblings ...)
  2025-02-27  1:25 ` [PATCH v2 05/10] KVM: SVM: Require AP's "requested" SEV_FEATURES to match KVM's view Sean Christopherson
@ 2025-02-27  1:25 ` Sean Christopherson
  2025-02-27  1:25 ` [PATCH v2 07/10] KVM: SVM: Use guard(mutex) to simplify SNP AP Creation error handling Sean Christopherson
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 20+ messages in thread
From: Sean Christopherson @ 2025-02-27  1:25 UTC (permalink / raw)
  To: Sean Christopherson, Paolo Bonzini
  Cc: kvm, linux-kernel, Naveen N Rao, Kim Phillips, Tom Lendacky,
	Alexey Kardashevskiy, Pankaj Gupta

Drop the local "kick" variable and the unnecessary "fallthrough" logic
from sev_snp_ap_creation(), and simply pivot on the request when deciding
whether or not to immediate force a state update on the target vCPU.

No functional change intended.

Reviewed-by: Pankaj Gupta <pankaj.gupta@amd.com>
Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/sev.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index bad5834ec143..ccac840ee7be 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -3938,7 +3938,6 @@ static int sev_snp_ap_creation(struct vcpu_svm *svm)
 	struct vcpu_svm *target_svm;
 	unsigned int request;
 	unsigned int apic_id;
-	bool kick;
 	int ret;
 
 	request = lower_32_bits(svm->vmcb->control.exit_info_1);
@@ -3956,18 +3955,10 @@ static int sev_snp_ap_creation(struct vcpu_svm *svm)
 
 	target_svm = to_svm(target_vcpu);
 
-	/*
-	 * The target vCPU is valid, so the vCPU will be kicked unless the
-	 * request is for CREATE_ON_INIT.
-	 */
-	kick = true;
-
 	mutex_lock(&target_svm->sev_es.snp_vmsa_mutex);
 
 	switch (request) {
 	case SVM_VMGEXIT_AP_CREATE_ON_INIT:
-		kick = false;
-		fallthrough;
 	case SVM_VMGEXIT_AP_CREATE:
 		if (vcpu->arch.regs[VCPU_REGS_RAX] != sev->vmsa_features) {
 			vcpu_unimpl(vcpu, "vmgexit: mismatched AP sev_features [%#lx] != [%#llx] from guest\n",
@@ -4012,7 +4003,11 @@ static int sev_snp_ap_creation(struct vcpu_svm *svm)
 
 	target_svm->sev_es.snp_ap_waiting_for_reset = true;
 
-	if (kick) {
+	/*
+	 * Unless Creation is deferred until INIT, signal the vCPU to update
+	 * its state.
+	 */
+	if (request != SVM_VMGEXIT_AP_CREATE_ON_INIT) {
 		kvm_make_request(KVM_REQ_UPDATE_PROTECTED_GUEST_STATE, target_vcpu);
 		kvm_vcpu_kick(target_vcpu);
 	}
-- 
2.48.1.711.g2feabab25a-goog


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

* [PATCH v2 07/10] KVM: SVM: Use guard(mutex) to simplify SNP AP Creation error handling
  2025-02-27  1:25 [PATCH v2 00/10] KVM: SVM: Attempt to cleanup SEV_FEATURES Sean Christopherson
                   ` (5 preceding siblings ...)
  2025-02-27  1:25 ` [PATCH v2 06/10] KVM: SVM: Simplify request+kick logic in SNP AP Creation handling Sean Christopherson
@ 2025-02-27  1:25 ` Sean Christopherson
  2025-02-27 16:51   ` Gupta, Pankaj
  2025-02-27  1:25 ` [PATCH v2 08/10] KVM: SVM: Mark VMCB dirty before processing incoming snp_vmsa_gpa Sean Christopherson
                   ` (3 subsequent siblings)
  10 siblings, 1 reply; 20+ messages in thread
From: Sean Christopherson @ 2025-02-27  1:25 UTC (permalink / raw)
  To: Sean Christopherson, Paolo Bonzini
  Cc: kvm, linux-kernel, Naveen N Rao, Kim Phillips, Tom Lendacky,
	Alexey Kardashevskiy, Pankaj Gupta

Use guard(mutex) in sev_snp_ap_creation() and modify the error paths to
return directly instead of jumping to a common exit point.

No functional change intended.

Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/sev.c | 22 ++++++----------------
 1 file changed, 6 insertions(+), 16 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index ccac840ee7be..dd9511a2254b 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -3938,7 +3938,6 @@ static int sev_snp_ap_creation(struct vcpu_svm *svm)
 	struct vcpu_svm *target_svm;
 	unsigned int request;
 	unsigned int apic_id;
-	int ret;
 
 	request = lower_32_bits(svm->vmcb->control.exit_info_1);
 	apic_id = upper_32_bits(svm->vmcb->control.exit_info_1);
@@ -3951,11 +3950,9 @@ static int sev_snp_ap_creation(struct vcpu_svm *svm)
 		return -EINVAL;
 	}
 
-	ret = 0;
-
 	target_svm = to_svm(target_vcpu);
 
-	mutex_lock(&target_svm->sev_es.snp_vmsa_mutex);
+	guard(mutex)(&target_svm->sev_es.snp_vmsa_mutex);
 
 	switch (request) {
 	case SVM_VMGEXIT_AP_CREATE_ON_INIT:
@@ -3963,15 +3960,13 @@ static int sev_snp_ap_creation(struct vcpu_svm *svm)
 		if (vcpu->arch.regs[VCPU_REGS_RAX] != sev->vmsa_features) {
 			vcpu_unimpl(vcpu, "vmgexit: mismatched AP sev_features [%#lx] != [%#llx] from guest\n",
 				    vcpu->arch.regs[VCPU_REGS_RAX], sev->vmsa_features);
-			ret = -EINVAL;
-			goto out;
+			return -EINVAL;
 		}
 
 		if (!page_address_valid(vcpu, svm->vmcb->control.exit_info_2)) {
 			vcpu_unimpl(vcpu, "vmgexit: invalid AP VMSA address [%#llx] from guest\n",
 				    svm->vmcb->control.exit_info_2);
-			ret = -EINVAL;
-			goto out;
+			return -EINVAL;
 		}
 
 		/*
@@ -3985,8 +3980,7 @@ static int sev_snp_ap_creation(struct vcpu_svm *svm)
 			vcpu_unimpl(vcpu,
 				    "vmgexit: AP VMSA address [%llx] from guest is unsafe as it is 2M aligned\n",
 				    svm->vmcb->control.exit_info_2);
-			ret = -EINVAL;
-			goto out;
+			return -EINVAL;
 		}
 
 		target_svm->sev_es.snp_vmsa_gpa = svm->vmcb->control.exit_info_2;
@@ -3997,8 +3991,7 @@ static int sev_snp_ap_creation(struct vcpu_svm *svm)
 	default:
 		vcpu_unimpl(vcpu, "vmgexit: invalid AP creation request [%#x] from guest\n",
 			    request);
-		ret = -EINVAL;
-		goto out;
+		return -EINVAL;
 	}
 
 	target_svm->sev_es.snp_ap_waiting_for_reset = true;
@@ -4012,10 +4005,7 @@ static int sev_snp_ap_creation(struct vcpu_svm *svm)
 		kvm_vcpu_kick(target_vcpu);
 	}
 
-out:
-	mutex_unlock(&target_svm->sev_es.snp_vmsa_mutex);
-
-	return ret;
+	return 0;
 }
 
 static int snp_handle_guest_req(struct vcpu_svm *svm, gpa_t req_gpa, gpa_t resp_gpa)
-- 
2.48.1.711.g2feabab25a-goog


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

* [PATCH v2 08/10] KVM: SVM: Mark VMCB dirty before processing incoming snp_vmsa_gpa
  2025-02-27  1:25 [PATCH v2 00/10] KVM: SVM: Attempt to cleanup SEV_FEATURES Sean Christopherson
                   ` (6 preceding siblings ...)
  2025-02-27  1:25 ` [PATCH v2 07/10] KVM: SVM: Use guard(mutex) to simplify SNP AP Creation error handling Sean Christopherson
@ 2025-02-27  1:25 ` Sean Christopherson
  2025-02-27  1:25 ` [PATCH v2 09/10] KVM: SVM: Use guard(mutex) to simplify SNP vCPU state updates Sean Christopherson
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 20+ messages in thread
From: Sean Christopherson @ 2025-02-27  1:25 UTC (permalink / raw)
  To: Sean Christopherson, Paolo Bonzini
  Cc: kvm, linux-kernel, Naveen N Rao, Kim Phillips, Tom Lendacky,
	Alexey Kardashevskiy, Pankaj Gupta

Mark the VMCB dirty, i.e. zero control.clean, prior to handling the new
VMSA.  Nothing in the VALID_PAGE() case touches control.clean, and
isolating the VALID_PAGE() code will allow simplifying the overall logic.

Note, the VMCB probably doesn't need to be marked dirty when the VMSA is
invalid, as KVM will disallow running the vCPU in such a state.  But it
also doesn't hurt anything.

Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/sev.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index dd9511a2254b..c74cc64ceb81 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -3850,6 +3850,12 @@ static int __sev_snp_update_protected_guest_state(struct kvm_vcpu *vcpu)
 	/* Clear use of the VMSA */
 	svm->vmcb->control.vmsa_pa = INVALID_PAGE;
 
+	/*
+	 * When replacing the VMSA during SEV-SNP AP creation,
+	 * mark the VMCB dirty so that full state is always reloaded.
+	 */
+	vmcb_mark_all_dirty(svm->vmcb);
+
 	if (VALID_PAGE(svm->sev_es.snp_vmsa_gpa)) {
 		gfn_t gfn = gpa_to_gfn(svm->sev_es.snp_vmsa_gpa);
 		struct kvm_memory_slot *slot;
@@ -3895,12 +3901,6 @@ static int __sev_snp_update_protected_guest_state(struct kvm_vcpu *vcpu)
 		kvm_release_page_clean(page);
 	}
 
-	/*
-	 * When replacing the VMSA during SEV-SNP AP creation,
-	 * mark the VMCB dirty so that full state is always reloaded.
-	 */
-	vmcb_mark_all_dirty(svm->vmcb);
-
 	return 0;
 }
 
-- 
2.48.1.711.g2feabab25a-goog


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

* [PATCH v2 09/10] KVM: SVM: Use guard(mutex) to simplify SNP vCPU state updates
  2025-02-27  1:25 [PATCH v2 00/10] KVM: SVM: Attempt to cleanup SEV_FEATURES Sean Christopherson
                   ` (7 preceding siblings ...)
  2025-02-27  1:25 ` [PATCH v2 08/10] KVM: SVM: Mark VMCB dirty before processing incoming snp_vmsa_gpa Sean Christopherson
@ 2025-02-27  1:25 ` Sean Christopherson
  2025-02-27  1:25 ` [PATCH v2 10/10] KVM: SVM: Invalidate "next" SNP VMSA GPA even on failure Sean Christopherson
  2025-03-05  1:05 ` [PATCH v2 00/10] KVM: SVM: Attempt to cleanup SEV_FEATURES Sean Christopherson
  10 siblings, 0 replies; 20+ messages in thread
From: Sean Christopherson @ 2025-02-27  1:25 UTC (permalink / raw)
  To: Sean Christopherson, Paolo Bonzini
  Cc: kvm, linux-kernel, Naveen N Rao, Kim Phillips, Tom Lendacky,
	Alexey Kardashevskiy, Pankaj Gupta

Use guard(mutex) in sev_snp_init_protected_guest_state() and pull in its
lock-protected inner helper.  Without an unlock trampoline (and even with
one), there is no real need for an inner helper.  Eliminating the helper
also avoids having to fixup the open coded "lockdep" WARN_ON().

Opportunistically drop the error message if KVM can't obtain the pfn for
the new target VMSA.  The error message provides zero information that
can't be gleaned from the fact that the vCPU is stuck.

Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/sev.c | 122 ++++++++++++++++++-----------------------
 1 file changed, 53 insertions(+), 69 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index c74cc64ceb81..3f85bd1cac37 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -3837,11 +3837,26 @@ static int snp_begin_psc(struct vcpu_svm *svm, struct psc_buffer *psc)
 	BUG();
 }
 
-static int __sev_snp_update_protected_guest_state(struct kvm_vcpu *vcpu)
+/*
+ * Invoked as part of svm_vcpu_reset() processing of an init event.
+ */
+void sev_snp_init_protected_guest_state(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
+	struct kvm_memory_slot *slot;
+	struct page *page;
+	kvm_pfn_t pfn;
+	gfn_t gfn;
 
-	WARN_ON(!mutex_is_locked(&svm->sev_es.snp_vmsa_mutex));
+	if (!sev_snp_guest(vcpu->kvm))
+		return;
+
+	guard(mutex)(&svm->sev_es.snp_vmsa_mutex);
+
+	if (!svm->sev_es.snp_ap_waiting_for_reset)
+		return;
+
+	svm->sev_es.snp_ap_waiting_for_reset = false;
 
 	/* Mark the vCPU as offline and not runnable */
 	vcpu->arch.pv.pv_unhalted = false;
@@ -3856,78 +3871,47 @@ static int __sev_snp_update_protected_guest_state(struct kvm_vcpu *vcpu)
 	 */
 	vmcb_mark_all_dirty(svm->vmcb);
 
-	if (VALID_PAGE(svm->sev_es.snp_vmsa_gpa)) {
-		gfn_t gfn = gpa_to_gfn(svm->sev_es.snp_vmsa_gpa);
-		struct kvm_memory_slot *slot;
-		struct page *page;
-		kvm_pfn_t pfn;
-
-		slot = gfn_to_memslot(vcpu->kvm, gfn);
-		if (!slot)
-			return -EINVAL;
-
-		/*
-		 * The new VMSA will be private memory guest memory, so
-		 * retrieve the PFN from the gmem backend.
-		 */
-		if (kvm_gmem_get_pfn(vcpu->kvm, slot, gfn, &pfn, &page, NULL))
-			return -EINVAL;
-
-		/*
-		 * From this point forward, the VMSA will always be a
-		 * guest-mapped page rather than the initial one allocated
-		 * by KVM in svm->sev_es.vmsa. In theory, svm->sev_es.vmsa
-		 * could be free'd and cleaned up here, but that involves
-		 * cleanups like wbinvd_on_all_cpus() which would ideally
-		 * be handled during teardown rather than guest boot.
-		 * Deferring that also allows the existing logic for SEV-ES
-		 * VMSAs to be re-used with minimal SNP-specific changes.
-		 */
-		svm->sev_es.snp_has_guest_vmsa = true;
-
-		/* Use the new VMSA */
-		svm->vmcb->control.vmsa_pa = pfn_to_hpa(pfn);
-
-		/* Mark the vCPU as runnable */
-		kvm_set_mp_state(vcpu, KVM_MP_STATE_RUNNABLE);
-
-		svm->sev_es.snp_vmsa_gpa = INVALID_PAGE;
-
-		/*
-		 * gmem pages aren't currently migratable, but if this ever
-		 * changes then care should be taken to ensure
-		 * svm->sev_es.vmsa is pinned through some other means.
-		 */
-		kvm_release_page_clean(page);
-	}
-
-	return 0;
-}
-
-/*
- * Invoked as part of svm_vcpu_reset() processing of an init event.
- */
-void sev_snp_init_protected_guest_state(struct kvm_vcpu *vcpu)
-{
-	struct vcpu_svm *svm = to_svm(vcpu);
-	int ret;
-
-	if (!sev_snp_guest(vcpu->kvm))
+	if (!VALID_PAGE(svm->sev_es.snp_vmsa_gpa))
 		return;
 
-	mutex_lock(&svm->sev_es.snp_vmsa_mutex);
+	gfn = gpa_to_gfn(svm->sev_es.snp_vmsa_gpa);
 
-	if (!svm->sev_es.snp_ap_waiting_for_reset)
-		goto unlock;
-
-	svm->sev_es.snp_ap_waiting_for_reset = false;
+	slot = gfn_to_memslot(vcpu->kvm, gfn);
+	if (!slot)
+		return;
 
-	ret = __sev_snp_update_protected_guest_state(vcpu);
-	if (ret)
-		vcpu_unimpl(vcpu, "snp: AP state update on init failed\n");
+	/*
+	 * The new VMSA will be private memory guest memory, so retrieve the
+	 * PFN from the gmem backend.
+	 */
+	if (kvm_gmem_get_pfn(vcpu->kvm, slot, gfn, &pfn, &page, NULL))
+		return;
 
-unlock:
-	mutex_unlock(&svm->sev_es.snp_vmsa_mutex);
+	/*
+	 * From this point forward, the VMSA will always be a guest-mapped page
+	 * rather than the initial one allocated by KVM in svm->sev_es.vmsa. In
+	 * theory, svm->sev_es.vmsa could be free'd and cleaned up here, but
+	 * that involves cleanups like wbinvd_on_all_cpus() which would ideally
+	 * be handled during teardown rather than guest boot.  Deferring that
+	 * also allows the existing logic for SEV-ES VMSAs to be re-used with
+	 * minimal SNP-specific changes.
+	 */
+	svm->sev_es.snp_has_guest_vmsa = true;
+
+	/* Use the new VMSA */
+	svm->vmcb->control.vmsa_pa = pfn_to_hpa(pfn);
+
+	/* Mark the vCPU as runnable */
+	kvm_set_mp_state(vcpu, KVM_MP_STATE_RUNNABLE);
+
+	svm->sev_es.snp_vmsa_gpa = INVALID_PAGE;
+
+	/*
+	 * gmem pages aren't currently migratable, but if this ever changes
+	 * then care should be taken to ensure svm->sev_es.vmsa is pinned
+	 * through some other means.
+	 */
+	kvm_release_page_clean(page);
 }
 
 static int sev_snp_ap_creation(struct vcpu_svm *svm)
-- 
2.48.1.711.g2feabab25a-goog


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

* [PATCH v2 10/10] KVM: SVM: Invalidate "next" SNP VMSA GPA even on failure
  2025-02-27  1:25 [PATCH v2 00/10] KVM: SVM: Attempt to cleanup SEV_FEATURES Sean Christopherson
                   ` (8 preceding siblings ...)
  2025-02-27  1:25 ` [PATCH v2 09/10] KVM: SVM: Use guard(mutex) to simplify SNP vCPU state updates Sean Christopherson
@ 2025-02-27  1:25 ` Sean Christopherson
  2025-03-05  1:05 ` [PATCH v2 00/10] KVM: SVM: Attempt to cleanup SEV_FEATURES Sean Christopherson
  10 siblings, 0 replies; 20+ messages in thread
From: Sean Christopherson @ 2025-02-27  1:25 UTC (permalink / raw)
  To: Sean Christopherson, Paolo Bonzini
  Cc: kvm, linux-kernel, Naveen N Rao, Kim Phillips, Tom Lendacky,
	Alexey Kardashevskiy, Pankaj Gupta

When processing an SNP AP Creation event, invalidate the "next" VMSA GPA
even if acquiring the page/pfn for the new VMSA fails.  In practice, the
next GPA will never be used regardless of whether or not its invalidated,
as the entire flow is guarded by snp_ap_waiting_for_reset, and said guard
and snp_vmsa_gpa are always written as a pair.  But that's really hard to
see in the code.

Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/sev.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 3f85bd1cac37..de153a19fa6c 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -3875,6 +3875,7 @@ void sev_snp_init_protected_guest_state(struct kvm_vcpu *vcpu)
 		return;
 
 	gfn = gpa_to_gfn(svm->sev_es.snp_vmsa_gpa);
+	svm->sev_es.snp_vmsa_gpa = INVALID_PAGE;
 
 	slot = gfn_to_memslot(vcpu->kvm, gfn);
 	if (!slot)
@@ -3904,8 +3905,6 @@ void sev_snp_init_protected_guest_state(struct kvm_vcpu *vcpu)
 	/* Mark the vCPU as runnable */
 	kvm_set_mp_state(vcpu, KVM_MP_STATE_RUNNABLE);
 
-	svm->sev_es.snp_vmsa_gpa = INVALID_PAGE;
-
 	/*
 	 * gmem pages aren't currently migratable, but if this ever changes
 	 * then care should be taken to ensure svm->sev_es.vmsa is pinned
-- 
2.48.1.711.g2feabab25a-goog


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

* Re: [PATCH v2 05/10] KVM: SVM: Require AP's "requested" SEV_FEATURES to match KVM's view
  2025-02-27  1:25 ` [PATCH v2 05/10] KVM: SVM: Require AP's "requested" SEV_FEATURES to match KVM's view Sean Christopherson
@ 2025-02-27  7:12   ` Gupta, Pankaj
  2025-02-27 14:33     ` Sean Christopherson
  0 siblings, 1 reply; 20+ messages in thread
From: Gupta, Pankaj @ 2025-02-27  7:12 UTC (permalink / raw)
  To: Sean Christopherson, Paolo Bonzini
  Cc: kvm, linux-kernel, Naveen N Rao, Kim Phillips, Tom Lendacky,
	Alexey Kardashevskiy

On 2/27/2025 2:25 AM, Sean Christopherson wrote:
> When handling an "AP Create" event, return an error if the "requested" SEV
> features for the vCPU don't exactly match KVM's view of the VM-scoped
> features.  There is no known use case for heterogeneous SEV features across
> vCPUs, and while KVM can't actually enforce an exact match since the value
> in RAX isn't guaranteed to match what the guest shoved into the VMSA, KVM
> can at least avoid knowingly letting the guest run in an unsupported state.
> 
> E.g. if a VM is created with DebugSwap disabled, KVM will intercept #DBs
> and DRs for all vCPUs, even if an AP is "created" with DebugSwap enabled in
> its VMSA.
> 
> Note, the GHCB spec only "requires" that "AP use the same interrupt
> injection mechanism as the BSP", but given the disaster that is DebugSwap
> and SEV_FEATURES in general, it's safe to say that AMD didn't consider all
> possible complications with mismatching features between the BSP and APs.
> 
> Opportunistically fold the check into the relevant request flavors; the
> "request < AP_DESTROY" check is just a bizarre way of implementing the
> AP_CREATE_ON_INIT => AP_CREATE fallthrough.
> 
> Fixes: e366f92ea99e ("KVM: SEV: Support SEV-SNP AP Creation NAE event")
> Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>

Looks good. Even makes code simpler.

A minor query below.

Reviewed-by: Pankaj Gupta <pankaj.gupta@amd.com>

> ---
>   arch/x86/kvm/svm/sev.c | 23 ++++++++---------------
>   1 file changed, 8 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 9aad0dae3a80..bad5834ec143 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -3932,6 +3932,7 @@ void sev_snp_init_protected_guest_state(struct kvm_vcpu *vcpu)
>   
>   static int sev_snp_ap_creation(struct vcpu_svm *svm)
>   {
> +	struct kvm_sev_info *sev = to_kvm_sev_info(svm->vcpu.kvm);
>   	struct kvm_vcpu *vcpu = &svm->vcpu;
>   	struct kvm_vcpu *target_vcpu;
>   	struct vcpu_svm *target_svm;
> @@ -3963,26 +3964,18 @@ static int sev_snp_ap_creation(struct vcpu_svm *svm)
>   
>   	mutex_lock(&target_svm->sev_es.snp_vmsa_mutex);
>   
> -	/* Interrupt injection mode shouldn't change for AP creation */
> -	if (request < SVM_VMGEXIT_AP_DESTROY) {
> -		u64 sev_features;
> -
> -		sev_features = vcpu->arch.regs[VCPU_REGS_RAX];
> -		sev_features ^= to_kvm_sev_info(svm->vcpu.kvm)->vmsa_features;
> -
> -		if (sev_features & SVM_SEV_FEAT_INT_INJ_MODES) {

'SVM_SEV_FEAT_INT_INJ_MODES' would even be required in any future 
use-case, maybe?

Thanks,
Pankaj
> -			vcpu_unimpl(vcpu, "vmgexit: invalid AP injection mode [%#lx] from guest\n",
> -				    vcpu->arch.regs[VCPU_REGS_RAX]);
> -			ret = -EINVAL;
> -			goto out;
> -		}
> -	}
> -
>   	switch (request) {
>   	case SVM_VMGEXIT_AP_CREATE_ON_INIT:
>   		kick = false;
>   		fallthrough;
>   	case SVM_VMGEXIT_AP_CREATE:
> +		if (vcpu->arch.regs[VCPU_REGS_RAX] != sev->vmsa_features) {
> +			vcpu_unimpl(vcpu, "vmgexit: mismatched AP sev_features [%#lx] != [%#llx] from guest\n",
> +				    vcpu->arch.regs[VCPU_REGS_RAX], sev->vmsa_features);
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +
>   		if (!page_address_valid(vcpu, svm->vmcb->control.exit_info_2)) {
>   			vcpu_unimpl(vcpu, "vmgexit: invalid AP VMSA address [%#llx] from guest\n",
>   				    svm->vmcb->control.exit_info_2);


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

* Re: [PATCH v2 04/10] KVM: SVM: Don't change target vCPU state on AP Creation VMGEXIT error
  2025-02-27  1:25 ` [PATCH v2 04/10] KVM: SVM: Don't change target vCPU state on AP Creation VMGEXIT error Sean Christopherson
@ 2025-02-27 10:25   ` Gupta, Pankaj
  0 siblings, 0 replies; 20+ messages in thread
From: Gupta, Pankaj @ 2025-02-27 10:25 UTC (permalink / raw)
  To: Sean Christopherson, Paolo Bonzini
  Cc: kvm, linux-kernel, Naveen N Rao, Kim Phillips, Tom Lendacky,
	Alexey Kardashevskiy

On 2/27/2025 2:25 AM, Sean Christopherson wrote:
> If KVM rejects an AP Creation event, leave the target vCPU state as-is.
> Nothing in the GHCB suggests the hypervisor is *allowed* to muck with vCPU
> state on failure, let alone required to do so.  Furthermore, kicking only
> in the !ON_INIT case leads to divergent behavior, and even the "kick" case
> is non-deterministic.
> 
> E.g. if an ON_INIT request fails, the guest can successfully retry if the
> fixed AP Creation request is made prior to sending INIT.  And if a !ON_INIT
> fails, the guest can successfully retry if the fixed AP Creation request is
> handled before the target vCPU processes KVM's
> KVM_REQ_UPDATE_PROTECTED_GUEST_STATE.
> 
> Fixes: e366f92ea99e ("KVM: SEV: Support SEV-SNP AP Creation NAE event")
> Cc: stable@vger.kernel.org
> Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>

Reviewed-by: Pankaj Gupta <pankaj.gupta@amd.com>
> ---
>   arch/x86/kvm/svm/sev.c | 13 ++++++-------
>   1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 218738a360ba..9aad0dae3a80 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -3957,16 +3957,12 @@ static int sev_snp_ap_creation(struct vcpu_svm *svm)
>   
>   	/*
>   	 * The target vCPU is valid, so the vCPU will be kicked unless the
> -	 * request is for CREATE_ON_INIT. For any errors at this stage, the
> -	 * kick will place the vCPU in an non-runnable state.
> +	 * request is for CREATE_ON_INIT.
>   	 */
>   	kick = true;
>   
>   	mutex_lock(&target_svm->sev_es.snp_vmsa_mutex);
>   
> -	target_svm->sev_es.snp_vmsa_gpa = INVALID_PAGE;
> -	target_svm->sev_es.snp_ap_waiting_for_reset = true;
> -
>   	/* Interrupt injection mode shouldn't change for AP creation */
>   	if (request < SVM_VMGEXIT_AP_DESTROY) {
>   		u64 sev_features;
> @@ -4012,20 +4008,23 @@ static int sev_snp_ap_creation(struct vcpu_svm *svm)
>   		target_svm->sev_es.snp_vmsa_gpa = svm->vmcb->control.exit_info_2;
>   		break;
>   	case SVM_VMGEXIT_AP_DESTROY:
> +		target_svm->sev_es.snp_vmsa_gpa = INVALID_PAGE;
>   		break;
>   	default:
>   		vcpu_unimpl(vcpu, "vmgexit: invalid AP creation request [%#x] from guest\n",
>   			    request);
>   		ret = -EINVAL;
> -		break;
> +		goto out;
>   	}
>   
> -out:
> +	target_svm->sev_es.snp_ap_waiting_for_reset = true;
> +
>   	if (kick) {
>   		kvm_make_request(KVM_REQ_UPDATE_PROTECTED_GUEST_STATE, target_vcpu);
>   		kvm_vcpu_kick(target_vcpu);
>   	}
>   
> +out:
>   	mutex_unlock(&target_svm->sev_es.snp_vmsa_mutex);
>   
>   	return ret;


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

* Re: [PATCH v2 05/10] KVM: SVM: Require AP's "requested" SEV_FEATURES to match KVM's view
  2025-02-27  7:12   ` Gupta, Pankaj
@ 2025-02-27 14:33     ` Sean Christopherson
  2025-02-27 15:18       ` Gupta, Pankaj
  0 siblings, 1 reply; 20+ messages in thread
From: Sean Christopherson @ 2025-02-27 14:33 UTC (permalink / raw)
  To: Pankaj Gupta
  Cc: Paolo Bonzini, kvm, linux-kernel, Naveen N Rao, Kim Phillips,
	Tom Lendacky, Alexey Kardashevskiy

On Thu, Feb 27, 2025, Pankaj Gupta wrote:
> On 2/27/2025 2:25 AM, Sean Christopherson wrote:
> > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> > index 9aad0dae3a80..bad5834ec143 100644
> > --- a/arch/x86/kvm/svm/sev.c
> > +++ b/arch/x86/kvm/svm/sev.c
> > @@ -3932,6 +3932,7 @@ void sev_snp_init_protected_guest_state(struct kvm_vcpu *vcpu)
> >   static int sev_snp_ap_creation(struct vcpu_svm *svm)
> >   {
> > +	struct kvm_sev_info *sev = to_kvm_sev_info(svm->vcpu.kvm);
> >   	struct kvm_vcpu *vcpu = &svm->vcpu;
> >   	struct kvm_vcpu *target_vcpu;
> >   	struct vcpu_svm *target_svm;
> > @@ -3963,26 +3964,18 @@ static int sev_snp_ap_creation(struct vcpu_svm *svm)
> >   	mutex_lock(&target_svm->sev_es.snp_vmsa_mutex);
> > -	/* Interrupt injection mode shouldn't change for AP creation */
> > -	if (request < SVM_VMGEXIT_AP_DESTROY) {
> > -		u64 sev_features;
> > -
> > -		sev_features = vcpu->arch.regs[VCPU_REGS_RAX];
> > -		sev_features ^= to_kvm_sev_info(svm->vcpu.kvm)->vmsa_features;
> > -
> > -		if (sev_features & SVM_SEV_FEAT_INT_INJ_MODES) {
> 
> 'SVM_SEV_FEAT_INT_INJ_MODES' would even be required in any future use-case,
> maybe?

Can you elaborate?  I don't quite follow what you're suggesting.

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

* Re: [PATCH v2 05/10] KVM: SVM: Require AP's "requested" SEV_FEATURES to match KVM's view
  2025-02-27 14:33     ` Sean Christopherson
@ 2025-02-27 15:18       ` Gupta, Pankaj
  2025-02-27 15:42         ` Sean Christopherson
  0 siblings, 1 reply; 20+ messages in thread
From: Gupta, Pankaj @ 2025-02-27 15:18 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Paolo Bonzini, kvm, linux-kernel, Naveen N Rao, Kim Phillips,
	Tom Lendacky, Alexey Kardashevskiy


>>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>>> index 9aad0dae3a80..bad5834ec143 100644
>>> --- a/arch/x86/kvm/svm/sev.c
>>> +++ b/arch/x86/kvm/svm/sev.c
>>> @@ -3932,6 +3932,7 @@ void sev_snp_init_protected_guest_state(struct kvm_vcpu *vcpu)
>>>    static int sev_snp_ap_creation(struct vcpu_svm *svm)
>>>    {
>>> +	struct kvm_sev_info *sev = to_kvm_sev_info(svm->vcpu.kvm);
>>>    	struct kvm_vcpu *vcpu = &svm->vcpu;
>>>    	struct kvm_vcpu *target_vcpu;
>>>    	struct vcpu_svm *target_svm;
>>> @@ -3963,26 +3964,18 @@ static int sev_snp_ap_creation(struct vcpu_svm *svm)
>>>    	mutex_lock(&target_svm->sev_es.snp_vmsa_mutex);
>>> -	/* Interrupt injection mode shouldn't change for AP creation */
>>> -	if (request < SVM_VMGEXIT_AP_DESTROY) {
>>> -		u64 sev_features;
>>> -
>>> -		sev_features = vcpu->arch.regs[VCPU_REGS_RAX];
>>> -		sev_features ^= to_kvm_sev_info(svm->vcpu.kvm)->vmsa_features;
>>> -
>>> -		if (sev_features & SVM_SEV_FEAT_INT_INJ_MODES) {
>>
>> 'SVM_SEV_FEAT_INT_INJ_MODES' would even be required in any future use-case,
>> maybe?
> 
> Can you elaborate?  I don't quite follow what you're suggesting.

SVM_SEV_FEAT_INT_INJ_MODES macro is not used anymore? If there is no 
plan to use it, maybe we can remove that as well?

Or I am missing something.

Thanks,
Pankaj

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

* Re: [PATCH v2 05/10] KVM: SVM: Require AP's "requested" SEV_FEATURES to match KVM's view
  2025-02-27 15:18       ` Gupta, Pankaj
@ 2025-02-27 15:42         ` Sean Christopherson
  0 siblings, 0 replies; 20+ messages in thread
From: Sean Christopherson @ 2025-02-27 15:42 UTC (permalink / raw)
  To: Pankaj Gupta
  Cc: Paolo Bonzini, kvm, linux-kernel, Naveen N Rao, Kim Phillips,
	Tom Lendacky, Alexey Kardashevskiy

On Thu, Feb 27, 2025, Pankaj Gupta wrote:
> 
> > > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> > > > index 9aad0dae3a80..bad5834ec143 100644
> > > > --- a/arch/x86/kvm/svm/sev.c
> > > > +++ b/arch/x86/kvm/svm/sev.c
> > > > @@ -3932,6 +3932,7 @@ void sev_snp_init_protected_guest_state(struct kvm_vcpu *vcpu)
> > > >    static int sev_snp_ap_creation(struct vcpu_svm *svm)
> > > >    {
> > > > +	struct kvm_sev_info *sev = to_kvm_sev_info(svm->vcpu.kvm);
> > > >    	struct kvm_vcpu *vcpu = &svm->vcpu;
> > > >    	struct kvm_vcpu *target_vcpu;
> > > >    	struct vcpu_svm *target_svm;
> > > > @@ -3963,26 +3964,18 @@ static int sev_snp_ap_creation(struct vcpu_svm *svm)
> > > >    	mutex_lock(&target_svm->sev_es.snp_vmsa_mutex);
> > > > -	/* Interrupt injection mode shouldn't change for AP creation */
> > > > -	if (request < SVM_VMGEXIT_AP_DESTROY) {
> > > > -		u64 sev_features;
> > > > -
> > > > -		sev_features = vcpu->arch.regs[VCPU_REGS_RAX];
> > > > -		sev_features ^= to_kvm_sev_info(svm->vcpu.kvm)->vmsa_features;
> > > > -
> > > > -		if (sev_features & SVM_SEV_FEAT_INT_INJ_MODES) {
> > > 
> > > 'SVM_SEV_FEAT_INT_INJ_MODES' would even be required in any future use-case,
> > > maybe?
> > 
> > Can you elaborate?  I don't quite follow what you're suggesting.
> 
> SVM_SEV_FEAT_INT_INJ_MODES macro is not used anymore? If there is no plan to
> use it, maybe we can remove that as well?

Ah, gotcha.  I didn't realize it was a compound macro.  Yeah, unless someone
objects, I'll remove it when applying.

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

* Re: [PATCH v2 03/10] KVM: SVM: Refuse to attempt VRMUN if an SEV-ES+ guest has an invalid VMSA
  2025-02-27  1:25 ` [PATCH v2 03/10] KVM: SVM: Refuse to attempt VRMUN if an SEV-ES+ guest has an invalid VMSA Sean Christopherson
@ 2025-02-27 16:03   ` Tom Lendacky
  2025-02-27 16:56   ` Gupta, Pankaj
  1 sibling, 0 replies; 20+ messages in thread
From: Tom Lendacky @ 2025-02-27 16:03 UTC (permalink / raw)
  To: Sean Christopherson, Paolo Bonzini
  Cc: kvm, linux-kernel, Naveen N Rao, Kim Phillips,
	Alexey Kardashevskiy, Pankaj Gupta

On 2/26/25 19:25, Sean Christopherson wrote:
> Explicitly reject KVM_RUN with KVM_EXIT_FAIL_ENTRY if userspace "coerces"
> KVM into running an SEV-ES+ guest with an invalid VMSA, e.g. by modifying
> a vCPU's mp_state to be RUNNABLE after an SNP vCPU has undergone a Destroy
> event.  On Destroy or failed Create, KVM marks the vCPU HALTED so that
> *KVM* doesn't run the vCPU, but nothing prevents a misbehaving VMM from
> manually making the vCPU RUNNABLE via KVM_SET_MP_STATE.
> 
> Attempting VMRUN with an invalid VMSA should be harmless, but knowingly
> executing VMRUN with bad control state is at best dodgy.
> 
> Fixes: e366f92ea99e ("KVM: SEV: Support SEV-SNP AP Creation NAE event")
> Signed-off-by: Sean Christopherson <seanjc@google.com>

Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>

> ---
>  arch/x86/kvm/svm/sev.c | 16 +++++++++++++---
>  arch/x86/kvm/svm/svm.c | 11 +++++++++--
>  arch/x86/kvm/svm/svm.h |  2 +-
>  3 files changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 719cd48330f1..218738a360ba 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -3452,10 +3452,19 @@ void sev_es_unmap_ghcb(struct vcpu_svm *svm)
>  	svm->sev_es.ghcb = NULL;
>  }
>  
> -void pre_sev_run(struct vcpu_svm *svm, int cpu)
> +int pre_sev_run(struct vcpu_svm *svm, int cpu)
>  {
>  	struct svm_cpu_data *sd = per_cpu_ptr(&svm_data, cpu);
> -	unsigned int asid = sev_get_asid(svm->vcpu.kvm);
> +	struct kvm *kvm = svm->vcpu.kvm;
> +	unsigned int asid = sev_get_asid(kvm);
> +
> +	/*
> +	 * Reject KVM_RUN if userspace attempts to run the vCPU with an invalid
> +	 * VMSA, e.g. if userspace forces the vCPU to be RUNNABLE after an SNP
> +	 * AP Destroy event.
> +	 */
> +	if (sev_es_guest(kvm) && !VALID_PAGE(svm->vmcb->control.vmsa_pa))
> +		return -EINVAL;
>  
>  	/* Assign the asid allocated with this SEV guest */
>  	svm->asid = asid;
> @@ -3468,11 +3477,12 @@ void pre_sev_run(struct vcpu_svm *svm, int cpu)
>  	 */
>  	if (sd->sev_vmcbs[asid] == svm->vmcb &&
>  	    svm->vcpu.arch.last_vmentry_cpu == cpu)
> -		return;
> +		return 0;
>  
>  	sd->sev_vmcbs[asid] = svm->vmcb;
>  	svm->vmcb->control.tlb_ctl = TLB_CONTROL_FLUSH_ASID;
>  	vmcb_mark_dirty(svm->vmcb, VMCB_ASID);
> +	return 0;
>  }
>  
>  #define GHCB_SCRATCH_AREA_LIMIT		(16ULL * PAGE_SIZE)
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index b8aa0f36850f..f72bcf2e590e 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -3587,7 +3587,7 @@ static int svm_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
>  	return svm_invoke_exit_handler(vcpu, exit_code);
>  }
>  
> -static void pre_svm_run(struct kvm_vcpu *vcpu)
> +static int pre_svm_run(struct kvm_vcpu *vcpu)
>  {
>  	struct svm_cpu_data *sd = per_cpu_ptr(&svm_data, vcpu->cpu);
>  	struct vcpu_svm *svm = to_svm(vcpu);
> @@ -3609,6 +3609,8 @@ static void pre_svm_run(struct kvm_vcpu *vcpu)
>  	/* FIXME: handle wraparound of asid_generation */
>  	if (svm->current_vmcb->asid_generation != sd->asid_generation)
>  		new_asid(svm, sd);
> +
> +	return 0;
>  }
>  
>  static void svm_inject_nmi(struct kvm_vcpu *vcpu)
> @@ -4231,7 +4233,12 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu,
>  	if (force_immediate_exit)
>  		smp_send_reschedule(vcpu->cpu);
>  
> -	pre_svm_run(vcpu);
> +	if (pre_svm_run(vcpu)) {
> +		vcpu->run->exit_reason = KVM_EXIT_FAIL_ENTRY;
> +		vcpu->run->fail_entry.hardware_entry_failure_reason = SVM_EXIT_ERR;
> +		vcpu->run->fail_entry.cpu = vcpu->cpu;
> +		return EXIT_FASTPATH_EXIT_USERSPACE;
> +	}
>  
>  	sync_lapic_to_cr8(vcpu);
>  
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index 5b159f017055..e51852977b70 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -713,7 +713,7 @@ void avic_refresh_virtual_apic_mode(struct kvm_vcpu *vcpu);
>  
>  /* sev.c */
>  
> -void pre_sev_run(struct vcpu_svm *svm, int cpu);
> +int pre_sev_run(struct vcpu_svm *svm, int cpu);
>  void sev_init_vmcb(struct vcpu_svm *svm);
>  void sev_vcpu_after_set_cpuid(struct vcpu_svm *svm);
>  int sev_es_string_io(struct vcpu_svm *svm, int size, unsigned int port, int in);

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

* Re: [PATCH v2 07/10] KVM: SVM: Use guard(mutex) to simplify SNP AP Creation error handling
  2025-02-27  1:25 ` [PATCH v2 07/10] KVM: SVM: Use guard(mutex) to simplify SNP AP Creation error handling Sean Christopherson
@ 2025-02-27 16:51   ` Gupta, Pankaj
  0 siblings, 0 replies; 20+ messages in thread
From: Gupta, Pankaj @ 2025-02-27 16:51 UTC (permalink / raw)
  To: Sean Christopherson, Paolo Bonzini
  Cc: kvm, linux-kernel, Naveen N Rao, Kim Phillips, Tom Lendacky,
	Alexey Kardashevskiy

On 2/27/2025 2:25 AM, Sean Christopherson wrote:
> Use guard(mutex) in sev_snp_ap_creation() and modify the error paths to
> return directly instead of jumping to a common exit point.
> 
> No functional change intended.
> 
> Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>

Reviewed-by: Pankaj Gupta <pankaj.gupta@amd.com>

> ---
>   arch/x86/kvm/svm/sev.c | 22 ++++++----------------
>   1 file changed, 6 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index ccac840ee7be..dd9511a2254b 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -3938,7 +3938,6 @@ static int sev_snp_ap_creation(struct vcpu_svm *svm)
>   	struct vcpu_svm *target_svm;
>   	unsigned int request;
>   	unsigned int apic_id;
> -	int ret;
>   
>   	request = lower_32_bits(svm->vmcb->control.exit_info_1);
>   	apic_id = upper_32_bits(svm->vmcb->control.exit_info_1);
> @@ -3951,11 +3950,9 @@ static int sev_snp_ap_creation(struct vcpu_svm *svm)
>   		return -EINVAL;
>   	}
>   
> -	ret = 0;
> -
>   	target_svm = to_svm(target_vcpu);
>   
> -	mutex_lock(&target_svm->sev_es.snp_vmsa_mutex);
> +	guard(mutex)(&target_svm->sev_es.snp_vmsa_mutex);
>   
>   	switch (request) {
>   	case SVM_VMGEXIT_AP_CREATE_ON_INIT:
> @@ -3963,15 +3960,13 @@ static int sev_snp_ap_creation(struct vcpu_svm *svm)
>   		if (vcpu->arch.regs[VCPU_REGS_RAX] != sev->vmsa_features) {
>   			vcpu_unimpl(vcpu, "vmgexit: mismatched AP sev_features [%#lx] != [%#llx] from guest\n",
>   				    vcpu->arch.regs[VCPU_REGS_RAX], sev->vmsa_features);
> -			ret = -EINVAL;
> -			goto out;
> +			return -EINVAL;
>   		}
>   
>   		if (!page_address_valid(vcpu, svm->vmcb->control.exit_info_2)) {
>   			vcpu_unimpl(vcpu, "vmgexit: invalid AP VMSA address [%#llx] from guest\n",
>   				    svm->vmcb->control.exit_info_2);
> -			ret = -EINVAL;
> -			goto out;
> +			return -EINVAL;
>   		}
>   
>   		/*
> @@ -3985,8 +3980,7 @@ static int sev_snp_ap_creation(struct vcpu_svm *svm)
>   			vcpu_unimpl(vcpu,
>   				    "vmgexit: AP VMSA address [%llx] from guest is unsafe as it is 2M aligned\n",
>   				    svm->vmcb->control.exit_info_2);
> -			ret = -EINVAL;
> -			goto out;
> +			return -EINVAL;
>   		}
>   
>   		target_svm->sev_es.snp_vmsa_gpa = svm->vmcb->control.exit_info_2;
> @@ -3997,8 +3991,7 @@ static int sev_snp_ap_creation(struct vcpu_svm *svm)
>   	default:
>   		vcpu_unimpl(vcpu, "vmgexit: invalid AP creation request [%#x] from guest\n",
>   			    request);
> -		ret = -EINVAL;
> -		goto out;
> +		return -EINVAL;
>   	}
>   
>   	target_svm->sev_es.snp_ap_waiting_for_reset = true;
> @@ -4012,10 +4005,7 @@ static int sev_snp_ap_creation(struct vcpu_svm *svm)
>   		kvm_vcpu_kick(target_vcpu);
>   	}
>   
> -out:
> -	mutex_unlock(&target_svm->sev_es.snp_vmsa_mutex);
> -
> -	return ret;
> +	return 0;
>   }
>   
>   static int snp_handle_guest_req(struct vcpu_svm *svm, gpa_t req_gpa, gpa_t resp_gpa)


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

* Re: [PATCH v2 03/10] KVM: SVM: Refuse to attempt VRMUN if an SEV-ES+ guest has an invalid VMSA
  2025-02-27  1:25 ` [PATCH v2 03/10] KVM: SVM: Refuse to attempt VRMUN if an SEV-ES+ guest has an invalid VMSA Sean Christopherson
  2025-02-27 16:03   ` Tom Lendacky
@ 2025-02-27 16:56   ` Gupta, Pankaj
  1 sibling, 0 replies; 20+ messages in thread
From: Gupta, Pankaj @ 2025-02-27 16:56 UTC (permalink / raw)
  To: Sean Christopherson, Paolo Bonzini
  Cc: kvm, linux-kernel, Naveen N Rao, Kim Phillips, Tom Lendacky,
	Alexey Kardashevskiy

On 2/27/2025 2:25 AM, Sean Christopherson wrote:
> Explicitly reject KVM_RUN with KVM_EXIT_FAIL_ENTRY if userspace "coerces"
> KVM into running an SEV-ES+ guest with an invalid VMSA, e.g. by modifying
> a vCPU's mp_state to be RUNNABLE after an SNP vCPU has undergone a Destroy
> event.  On Destroy or failed Create, KVM marks the vCPU HALTED so that
> *KVM* doesn't run the vCPU, but nothing prevents a misbehaving VMM from
> manually making the vCPU RUNNABLE via KVM_SET_MP_STATE.
> 
> Attempting VMRUN with an invalid VMSA should be harmless, but knowingly
> executing VMRUN with bad control state is at best dodgy.
> 
> Fixes: e366f92ea99e ("KVM: SEV: Support SEV-SNP AP Creation NAE event")
> Signed-off-by: Sean Christopherson <seanjc@google.com>

Reviewed-by: Pankaj Gupta <pankaj.gupta@amd.com>

> ---
>   arch/x86/kvm/svm/sev.c | 16 +++++++++++++---
>   arch/x86/kvm/svm/svm.c | 11 +++++++++--
>   arch/x86/kvm/svm/svm.h |  2 +-
>   3 files changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 719cd48330f1..218738a360ba 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -3452,10 +3452,19 @@ void sev_es_unmap_ghcb(struct vcpu_svm *svm)
>   	svm->sev_es.ghcb = NULL;
>   }
>   
> -void pre_sev_run(struct vcpu_svm *svm, int cpu)
> +int pre_sev_run(struct vcpu_svm *svm, int cpu)
>   {
>   	struct svm_cpu_data *sd = per_cpu_ptr(&svm_data, cpu);
> -	unsigned int asid = sev_get_asid(svm->vcpu.kvm);
> +	struct kvm *kvm = svm->vcpu.kvm;
> +	unsigned int asid = sev_get_asid(kvm);
> +
> +	/*
> +	 * Reject KVM_RUN if userspace attempts to run the vCPU with an invalid
> +	 * VMSA, e.g. if userspace forces the vCPU to be RUNNABLE after an SNP
> +	 * AP Destroy event.
> +	 */
> +	if (sev_es_guest(kvm) && !VALID_PAGE(svm->vmcb->control.vmsa_pa))
> +		return -EINVAL;
>   
>   	/* Assign the asid allocated with this SEV guest */
>   	svm->asid = asid;
> @@ -3468,11 +3477,12 @@ void pre_sev_run(struct vcpu_svm *svm, int cpu)
>   	 */
>   	if (sd->sev_vmcbs[asid] == svm->vmcb &&
>   	    svm->vcpu.arch.last_vmentry_cpu == cpu)
> -		return;
> +		return 0;
>   
>   	sd->sev_vmcbs[asid] = svm->vmcb;
>   	svm->vmcb->control.tlb_ctl = TLB_CONTROL_FLUSH_ASID;
>   	vmcb_mark_dirty(svm->vmcb, VMCB_ASID);
> +	return 0;
>   }
>   
>   #define GHCB_SCRATCH_AREA_LIMIT		(16ULL * PAGE_SIZE)
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index b8aa0f36850f..f72bcf2e590e 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -3587,7 +3587,7 @@ static int svm_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
>   	return svm_invoke_exit_handler(vcpu, exit_code);
>   }
>   
> -static void pre_svm_run(struct kvm_vcpu *vcpu)
> +static int pre_svm_run(struct kvm_vcpu *vcpu)
>   {
>   	struct svm_cpu_data *sd = per_cpu_ptr(&svm_data, vcpu->cpu);
>   	struct vcpu_svm *svm = to_svm(vcpu);
> @@ -3609,6 +3609,8 @@ static void pre_svm_run(struct kvm_vcpu *vcpu)
>   	/* FIXME: handle wraparound of asid_generation */
>   	if (svm->current_vmcb->asid_generation != sd->asid_generation)
>   		new_asid(svm, sd);
> +
> +	return 0;
>   }
>   
>   static void svm_inject_nmi(struct kvm_vcpu *vcpu)
> @@ -4231,7 +4233,12 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu,
>   	if (force_immediate_exit)
>   		smp_send_reschedule(vcpu->cpu);
>   
> -	pre_svm_run(vcpu);
> +	if (pre_svm_run(vcpu)) {
> +		vcpu->run->exit_reason = KVM_EXIT_FAIL_ENTRY;
> +		vcpu->run->fail_entry.hardware_entry_failure_reason = SVM_EXIT_ERR;
> +		vcpu->run->fail_entry.cpu = vcpu->cpu;
> +		return EXIT_FASTPATH_EXIT_USERSPACE;
> +	}
>   
>   	sync_lapic_to_cr8(vcpu);
>   
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index 5b159f017055..e51852977b70 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -713,7 +713,7 @@ void avic_refresh_virtual_apic_mode(struct kvm_vcpu *vcpu);
>   
>   /* sev.c */
>   
> -void pre_sev_run(struct vcpu_svm *svm, int cpu);
> +int pre_sev_run(struct vcpu_svm *svm, int cpu);
>   void sev_init_vmcb(struct vcpu_svm *svm);
>   void sev_vcpu_after_set_cpuid(struct vcpu_svm *svm);
>   int sev_es_string_io(struct vcpu_svm *svm, int size, unsigned int port, int in);


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

* Re: [PATCH v2 00/10] KVM: SVM: Attempt to cleanup SEV_FEATURES
  2025-02-27  1:25 [PATCH v2 00/10] KVM: SVM: Attempt to cleanup SEV_FEATURES Sean Christopherson
                   ` (9 preceding siblings ...)
  2025-02-27  1:25 ` [PATCH v2 10/10] KVM: SVM: Invalidate "next" SNP VMSA GPA even on failure Sean Christopherson
@ 2025-03-05  1:05 ` Sean Christopherson
  10 siblings, 0 replies; 20+ messages in thread
From: Sean Christopherson @ 2025-03-05  1:05 UTC (permalink / raw)
  To: Sean Christopherson, Paolo Bonzini
  Cc: kvm, linux-kernel, Naveen N Rao, Kim Phillips, Tom Lendacky,
	Alexey Kardashevskiy, Pankaj Gupta

On Wed, 26 Feb 2025 17:25:31 -0800, Sean Christopherson wrote:
> Try to address the worst of the issues that arise with guest controlled SEV
> features (thanks AP creation)[1].  The most pressing issue is with DebugSwap,
> as a misbehaving guest could clobber host DR masks (which should be relatively
> benign?).
> 
> The other notable issue is that KVM doesn't guard against userspace manually
> making a vCPU RUNNABLE after it has been DESTROYED (or after a failed CREATE).
> This shouldn't be super problematic, as VMRUN is supposed to "only" fail if
> the VMSA page is invalid, but passing a known bad PA to hardware isn't exactly
> desirable.
> 
> [...]

Thanks for the reviews and testing!

Applied:

[01/10] KVM: SVM: Save host DR masks on CPUs with DebugSwap
        https://github.com/kvm-x86/linux/commit/b2653cd3b75f
[02/10] KVM: SVM: Don't rely on DebugSwap to restore host DR0..DR3
        https://github.com/kvm-x86/linux/commit/807cb9ce2ed9

to kvm-x86 fixes, and:

[3/10] KVM: SVM: Refuse to attempt VRMUN if an SEV-ES+ guest has an invalid VMSA
       https://github.com/kvm-x86/linux/commit/72d12715edcd
[4/10] KVM: SVM: Don't change target vCPU state on AP Creation VMGEXIT error
       https://github.com/kvm-x86/linux/commit/d26638bfcdfc
[5/10] KVM: SVM: Require AP's "requested" SEV_FEATURES to match KVM's view
       https://github.com/kvm-x86/linux/commit/745ff82199b1
[6/10] KVM: SVM: Simplify request+kick logic in SNP AP Creation handling
       https://github.com/kvm-x86/linux/commit/c6e129fb2ad2
[7/10] KVM: SVM: Use guard(mutex) to simplify SNP AP Creation error handling
       https://github.com/kvm-x86/linux/commit/46332437e1c5
[8/10] KVM: SVM: Mark VMCB dirty before processing incoming snp_vmsa_gpa
       https://github.com/kvm-x86/linux/commit/e268beee4a25
[9/10] KVM: SVM: Use guard(mutex) to simplify SNP vCPU state updates
       https://github.com/kvm-x86/linux/commit/5279d6f7e43d
[10/10] KVM: SVM: Invalidate "next" SNP VMSA GPA even on failure
       https://github.com/kvm-x86/linux/commit/4e96f010afb2

to kvm-x86 svm.

--
https://github.com/kvm-x86/linux/tree/next

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

end of thread, other threads:[~2025-03-05  1:05 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-02-27  1:25 [PATCH v2 00/10] KVM: SVM: Attempt to cleanup SEV_FEATURES Sean Christopherson
2025-02-27  1:25 ` [PATCH v2 01/10] KVM: SVM: Save host DR masks on CPUs with DebugSwap Sean Christopherson
2025-02-27  1:25 ` [PATCH v2 02/10] KVM: SVM: Don't rely on DebugSwap to restore host DR0..DR3 Sean Christopherson
2025-02-27  1:25 ` [PATCH v2 03/10] KVM: SVM: Refuse to attempt VRMUN if an SEV-ES+ guest has an invalid VMSA Sean Christopherson
2025-02-27 16:03   ` Tom Lendacky
2025-02-27 16:56   ` Gupta, Pankaj
2025-02-27  1:25 ` [PATCH v2 04/10] KVM: SVM: Don't change target vCPU state on AP Creation VMGEXIT error Sean Christopherson
2025-02-27 10:25   ` Gupta, Pankaj
2025-02-27  1:25 ` [PATCH v2 05/10] KVM: SVM: Require AP's "requested" SEV_FEATURES to match KVM's view Sean Christopherson
2025-02-27  7:12   ` Gupta, Pankaj
2025-02-27 14:33     ` Sean Christopherson
2025-02-27 15:18       ` Gupta, Pankaj
2025-02-27 15:42         ` Sean Christopherson
2025-02-27  1:25 ` [PATCH v2 06/10] KVM: SVM: Simplify request+kick logic in SNP AP Creation handling Sean Christopherson
2025-02-27  1:25 ` [PATCH v2 07/10] KVM: SVM: Use guard(mutex) to simplify SNP AP Creation error handling Sean Christopherson
2025-02-27 16:51   ` Gupta, Pankaj
2025-02-27  1:25 ` [PATCH v2 08/10] KVM: SVM: Mark VMCB dirty before processing incoming snp_vmsa_gpa Sean Christopherson
2025-02-27  1:25 ` [PATCH v2 09/10] KVM: SVM: Use guard(mutex) to simplify SNP vCPU state updates Sean Christopherson
2025-02-27  1:25 ` [PATCH v2 10/10] KVM: SVM: Invalidate "next" SNP VMSA GPA even on failure Sean Christopherson
2025-03-05  1:05 ` [PATCH v2 00/10] KVM: SVM: Attempt to cleanup SEV_FEATURES Sean Christopherson

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