lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240510015822.503071-3-michael.roth@amd.com>
Date: Thu, 9 May 2024 20:58:22 -0500
From: Michael Roth <michael.roth@....com>
To: <kvm@...r.kernel.org>
CC: <linux-coco@...ts.linux.dev>, <linux-mm@...ck.org>,
	<linux-crypto@...r.kernel.org>, <x86@...nel.org>,
	<linux-kernel@...r.kernel.org>, <tglx@...utronix.de>, <mingo@...hat.com>,
	<jroedel@...e.de>, <thomas.lendacky@....com>, <hpa@...or.com>,
	<ardb@...nel.org>, <pbonzini@...hat.com>, <seanjc@...gle.com>,
	<vkuznets@...hat.com>, <jmattson@...gle.com>, <luto@...nel.org>,
	<dave.hansen@...ux.intel.com>, <slp@...hat.com>, <pgonda@...gle.com>,
	<peterz@...radead.org>, <srinivas.pandruvada@...ux.intel.com>,
	<rientjes@...gle.com>, <dovmurik@...ux.ibm.com>, <tobin@....com>,
	<bp@...en8.de>, <vbabka@...e.cz>, <kirill@...temov.name>,
	<ak@...ux.intel.com>, <tony.luck@...el.com>,
	<sathyanarayanan.kuppuswamy@...ux.intel.com>, <alpergun@...gle.com>,
	<jarkko@...nel.org>, <ashish.kalra@....com>, <nikunj.dadhania@....com>,
	<pankaj.gupta@....com>, <liam.merwick@...cle.com>, <papaluri@....com>
Subject: [PATCH v15 23/23] KVM: SEV: Fix PSC handling for SMASH/UNSMASH and partial update ops

There are a few edge-cases that the current processing for GHCB PSC
requests doesn't handle properly:

 - KVM properly ignores SMASH/UNSMASH ops when they are embedded in a
   PSC request buffer which contains other PSC operations, but
   inadvertantly forwards them to userspace as private->shared PSC
   requests if they appear at the end of the buffer. Make sure these are
   ignored instead, just like cases where they are not at the end of the
   request buffer.

 - Current code handles non-zero 'cur_page' fields when they are at the
   beginning of a new GPA range, but it is not handling properly when
   iterating through subsequent entries which are otherwise part of a
   contiguous range. Fix up the handling so that these entries are not
   combined into a larger contiguous range that include unintended GPA
   ranges and are instead processed later as the start of a new
   contiguous range.

 - The page size variable used to track 2M entries in KVM for inflight PSCs
   might be artifically set to a different value, which can lead to
   unexpected values in the entry's final 'cur_page' update. Use the
   entry's 'pagesize' field instead to determine what the value of
   'cur_page' should be upon completion of processing.

While here, also add a small helper for clearing in-flight PSCs
variables and fix up comments for better readability.

Fixes: 266205d810d2 ("KVM: SEV: Add support to handle Page State Change VMGEXIT")
Signed-off-by: Michael Roth <michael.roth@....com>
---
 arch/x86/kvm/svm/sev.c | 73 +++++++++++++++++++++++++++---------------
 1 file changed, 47 insertions(+), 26 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 35f0bd91f92e..ab23329e2bd0 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -3555,43 +3555,50 @@ struct psc_buffer {
 
 static int snp_begin_psc(struct vcpu_svm *svm, struct psc_buffer *psc);
 
-static int snp_complete_psc(struct kvm_vcpu *vcpu)
+static void snp_reset_inflight_psc(struct vcpu_svm *svm)
+{
+	svm->sev_es.psc_idx = 0;
+	svm->sev_es.psc_inflight = 0;
+	svm->sev_es.psc_2m = false;
+}
+
+static void __snp_complete_psc(struct vcpu_svm *svm)
 {
-	struct vcpu_svm *svm = to_svm(vcpu);
 	struct psc_buffer *psc = svm->sev_es.ghcb_sa;
 	struct psc_entry *entries = psc->entries;
 	struct psc_hdr *hdr = &psc->hdr;
-	__u64 psc_ret;
 	__u16 idx;
 
-	if (vcpu->run->hypercall.ret) {
-		psc_ret = VMGEXIT_PSC_ERROR_GENERIC;
-		goto out_resume;
-	}
-
 	/*
 	 * Everything in-flight has been processed successfully. Update the
-	 * corresponding entries in the guest's PSC buffer.
+	 * corresponding entries in the guest's PSC buffer and zero out the
+	 * count of in-flight PSC entries.
 	 */
 	for (idx = svm->sev_es.psc_idx; svm->sev_es.psc_inflight;
 	     svm->sev_es.psc_inflight--, idx++) {
 		struct psc_entry *entry = &entries[idx];
 
-		entry->cur_page = svm->sev_es.psc_2m ? 512 : 1;
+		entry->cur_page = entry->pagesize ? 512 : 1;
 	}
 
 	hdr->cur_entry = idx;
+}
 
-	/* Handle the next range (if any). */
-	return snp_begin_psc(svm, psc);
+static int snp_complete_psc(struct kvm_vcpu *vcpu)
+{
+	struct vcpu_svm *svm = to_svm(vcpu);
+	struct psc_buffer *psc = svm->sev_es.ghcb_sa;
 
-out_resume:
-	svm->sev_es.psc_idx = 0;
-	svm->sev_es.psc_inflight = 0;
-	svm->sev_es.psc_2m = false;
-	ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, psc_ret);
+	if (vcpu->run->hypercall.ret) {
+		snp_reset_inflight_psc(svm);
+		ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, VMGEXIT_PSC_ERROR_GENERIC);
+		return 1; /* resume guest */
+	}
 
-	return 1; /* resume guest */
+	__snp_complete_psc(svm);
+
+	/* Handle the next range (if any). */
+	return snp_begin_psc(svm, psc);
 }
 
 static int snp_begin_psc(struct vcpu_svm *svm, struct psc_buffer *psc)
@@ -3634,6 +3641,7 @@ static int snp_begin_psc(struct vcpu_svm *svm, struct psc_buffer *psc)
 		goto out_resume;
 	}
 
+next_range:
 	/* Find the start of the next range which needs processing. */
 	for (idx = idx_start; idx <= idx_end; idx++, hdr->cur_entry++) {
 		__u16 cur_page;
@@ -3642,11 +3650,6 @@ static int snp_begin_psc(struct vcpu_svm *svm, struct psc_buffer *psc)
 
 		entry_start = entries[idx];
 
-		/* Only private/shared conversions are currently supported. */
-		if (entry_start.operation != VMGEXIT_PSC_OP_PRIVATE &&
-		    entry_start.operation != VMGEXIT_PSC_OP_SHARED)
-			continue;
-
 		gfn = entry_start.gfn;
 		cur_page = entry_start.cur_page;
 		huge = entry_start.pagesize;
@@ -3687,6 +3690,7 @@ static int snp_begin_psc(struct vcpu_svm *svm, struct psc_buffer *psc)
 
 		if (entry.operation != entry_start.operation ||
 		    entry.gfn != entry_start.gfn + npages ||
+		    entry.cur_page != 0 ||
 		    !!entry.pagesize != svm->sev_es.psc_2m)
 			break;
 
@@ -3694,6 +3698,25 @@ static int snp_begin_psc(struct vcpu_svm *svm, struct psc_buffer *psc)
 		npages += entry_start.pagesize ? 512 : 1;
 	}
 
+	/*
+	 * Only shared/private PSC operations are currently supported, so if the
+	 * entire range consists of unsupported operations (e.g. SMASH/UNSMASH),
+	 * then consider the entire range completed and avoid exiting to
+	 * userspace. In theory snp_complete_psc() can always be called directly
+	 * at this point to complete the current range and start the next one,
+	 * but that could lead to unexpected levels of recursion, so only do
+	 * that if there are no more entries to process and the entire request
+	 * has been completed.
+	 */
+	if (entry_start.operation != VMGEXIT_PSC_OP_PRIVATE &&
+	    entry_start.operation != VMGEXIT_PSC_OP_SHARED) {
+		if (idx > idx_end)
+			return snp_complete_psc(vcpu);
+
+		__snp_complete_psc(svm);
+		goto next_range;
+	}
+
 	vcpu->run->exit_reason = KVM_EXIT_HYPERCALL;
 	vcpu->run->hypercall.nr = KVM_HC_MAP_GPA_RANGE;
 	vcpu->run->hypercall.args[0] = gpa;
@@ -3709,9 +3732,7 @@ static int snp_begin_psc(struct vcpu_svm *svm, struct psc_buffer *psc)
 	return 0; /* forward request to userspace */
 
 out_resume:
-	svm->sev_es.psc_idx = 0;
-	svm->sev_es.psc_inflight = 0;
-	svm->sev_es.psc_2m = false;
+	snp_reset_inflight_psc(svm);
 	ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, psc_ret);
 
 	return 1; /* resume guest */
-- 
2.25.1


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ