Skip to contentSkip to conversation

KVM: x86: SGX vs. XCR0 cleanups

PATCH Applied Sean Christopherson 2 participants 6 messages May 3, 2023 – May 19, 2023

Subsystems (3)
x86 KVM KVM x86
lore
Share & tools
lore

Sean Christopherson

author maintainer
#0 lore raw

v2 cover KVM: x86: SGX vs. XCR0 cleanups

Stop adjusting the guest's CPUID info for the allowed XFRM (a.k.a. XCR0) for SGX enclaves. Past me didn't understand the roles and responsibilities between userspace and KVM with respect to CPUID leafs, i.e. I thought I was being helpful by having KVM adjust the entries.

This is clearly an ABI change, but QEMU does the right thing and AFAIK no other VMMs support SGX (yet), so I'm hopeful/confident that we can excise the ugly before userspace starts depending on the bad behavior.

v2:
- Collect reviews/testing. [Kai]
- Require FP+SSE to always be set in XFRM, and exempt them from the XFRM vs. XCR0 check. [Kai]

v1: https://lore.kernel.org/all/[email protected]

Sean Christopherson (3):
KVM: VMX: Don't rely _only_ on CPUID to enforce XCR0 restrictions for
ECREATE
KVM: x86: Don't adjust guest's CPUID.0x12.1 (allowed SGX enclave XFRM)
KVM: x86: Open code supported XCR0 calculation in
kvm_vcpu_after_set_cpuid()

 arch/x86/kvm/cpuid.c   | 43 ++++++++++--------------------------------
 arch/x86/kvm/vmx/sgx.c | 11 +++++++++--
 2 files changed, 19 insertions(+), 35 deletions(-)

base-commit: 5c291b93e5d665380dbecc6944973583f9565ee5

Signature · 1 line
2.40.1.495.gc816e09b53d-goog
author maintainer
#1 lore raw

PATCH v2 1/3 KVM: VMX: Don't rely _only_ on CPUID to enforce XCR0 restrictions for ECREATE

Explicitly check the vCPU's supported XCR0 when determining whether or not the XFRM for ECREATE is valid. Checking CPUID works because KVM updates guest CPUID.0x12.1 to restrict the leaf to a subset of the guest's allowed XCR0, but that is rather subtle and KVM should not modify guest CPUID except for modeling true runtime behavior (allowed XFRM is most definitely not "runtime" behavior).

Reviewed-by Kai Huang <[email protected]>
Tested-by Kai Huang <[email protected]>
Signed-off-by Sean Christopherson <[email protected]>
---
 arch/x86/kvm/vmx/sgx.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
arch/x86/kvm/vmx/sgx.c +9 -2
index 0574030b071f..2261b684a7d4 100644
@@ -170,12 +170,19 @@ static int __handle_encls_ecreate(struct kvm_vcpu *vcpu,
 		return 1;
 	}
 
-	/* Enforce CPUID restrictions on MISCSELECT, ATTRIBUTES and XFRM. */
+	/*
+	 * Enforce CPUID restrictions on MISCSELECT, ATTRIBUTES and XFRM.  Note
+	 * that the allowed XFRM (XFeature Request Mask) isn't strictly bound
+	 * by the supported XCR0.  FP+SSE *must* be set in XFRM, even if XSAVE
+	 * is unsupported, i.e. even if XCR0 itself is completely unsupported.
+	 */
 	if ((u32)miscselect & ~sgx_12_0->ebx ||
 	    (u32)attributes & ~sgx_12_1->eax ||
 	    (u32)(attributes >> 32) & ~sgx_12_1->ebx ||
 	    (u32)xfrm & ~sgx_12_1->ecx ||
-	    (u32)(xfrm >> 32) & ~sgx_12_1->edx) {
+	    (u32)(xfrm >> 32) & ~sgx_12_1->edx ||
+	    xfrm & ~(vcpu->arch.guest_supported_xcr0 | XFEATURE_MASK_FPSSE) ||
+	    (xfrm & XFEATURE_MASK_FPSSE) != XFEATURE_MASK_FPSSE) {
 		kvm_inject_gp(vcpu, 0);
 		return 1;
 	}
Signature · 1 line
2.40.1.495.gc816e09b53d-goog
author maintainer
#2 lore raw

PATCH v2 2/3 KVM: x86: Don't adjust guest's CPUID.0x12.1 (allowed SGX enclave XFRM)

In reply to Sean Christopherson

Drop KVM's manipulation of guest's CPUID.0x12.1 ECX and EDX, i.e. the allowed XFRM of SGX enclaves, now that KVM explicitly checks the guest's allowed XCR0 when emulating ECREATE.

Note, this could theoretically break a setup where userspace advertises a "bad" XFRM and relies on KVM to provide a sane CPUID model, but QEMU is the only known user of KVM SGX, and QEMU explicitly sets the SGX CPUID XFRM subleaf based on the guest's XCR0.

Reviewed-by Kai Huang <[email protected]>
Tested-by Kai Huang <[email protected]>
Signed-off-by Sean Christopherson <[email protected]>
---
 arch/x86/kvm/cpuid.c | 16 ----------------
 1 file changed, 16 deletions(-)
arch/x86/kvm/cpuid.c +0 -16
index 123bf8b97a4b..0c9660a07b23 100644
@@ -253,7 +253,6 @@ static void __kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu, struct kvm_cpuid_e
 				       int nent)
 {
 	struct kvm_cpuid_entry2 *best;
-	u64 guest_supported_xcr0 = cpuid_get_supported_xcr0(entries, nent);
 
 	best = cpuid_entry2_find(entries, nent, 1, KVM_CPUID_INDEX_NOT_SIGNIFICANT);
 	if (best) {

@@ -292,21 +291,6 @@ static void __kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu, struct kvm_cpuid_e
 					   vcpu->arch.ia32_misc_enable_msr &
 					   MSR_IA32_MISC_ENABLE_MWAIT);
 	}
-
-	/*
-	 * Bits 127:0 of the allowed SECS.ATTRIBUTES (CPUID.0x12.0x1) enumerate
-	 * the supported XSAVE Feature Request Mask (XFRM), i.e. the enclave's
-	 * requested XCR0 value.  The enclave's XFRM must be a subset of XCRO
-	 * at the time of EENTER, thus adjust the allowed XFRM by the guest's
-	 * supported XCR0.  Similar to XCR0 handling, FP and SSE are forced to
-	 * '1' even on CPUs that don't support XSAVE.
-	 */
-	best = cpuid_entry2_find(entries, nent, 0x12, 0x1);
-	if (best) {
-		best->ecx &= guest_supported_xcr0 & 0xffffffff;
-		best->edx &= guest_supported_xcr0 >> 32;
-		best->ecx |= XFEATURE_MASK_FPSSE;
-	}
 }
 
 void kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu)
Signature · 1 line
2.40.1.495.gc816e09b53d-goog
author maintainer
#3 lore raw

PATCH v2 3/3 KVM: x86: Open code supported XCR0 calculation in kvm_vcpu_after_set_cpuid()

In reply to Sean Christopherson

Drop cpuid_get_supported_xcr0() now that its bastardized usage in __kvm_update_cpuid_runtime() is gone, and open code the logic in its sole caller, kvm_vcpu_after_set_cpuid().

No functional change intended.

Reviewed-by Kai Huang <[email protected]>
Tested-by Kai Huang <[email protected]>
Signed-off-by Sean Christopherson <[email protected]>
---
 arch/x86/kvm/cpuid.c | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)
arch/x86/kvm/cpuid.c +10 -17
index 0c9660a07b23..491c88e196c1 100644
@@ -234,21 +234,6 @@ void kvm_update_pv_runtime(struct kvm_vcpu *vcpu)
 		vcpu->arch.pv_cpuid.features = best->eax;
 }
 
-/*
- * Calculate guest's supported XCR0 taking into account guest CPUID data and
- * KVM's supported XCR0 (comprised of host's XCR0 and KVM_SUPPORTED_XCR0).
- */
-static u64 cpuid_get_supported_xcr0(struct kvm_cpuid_entry2 *entries, int nent)
-{
-	struct kvm_cpuid_entry2 *best;
-
-	best = cpuid_entry2_find(entries, nent, 0xd, 0);
-	if (!best)
-		return 0;
-
-	return (best->eax | ((u64)best->edx << 32)) & kvm_caps.supported_xcr0;
-}
-
 static void __kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *entries,
 				       int nent)
 {

@@ -323,8 +308,16 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
 		kvm_apic_set_version(vcpu);
 	}
 
-	vcpu->arch.guest_supported_xcr0 =
-		cpuid_get_supported_xcr0(vcpu->arch.cpuid_entries, vcpu->arch.cpuid_nent);
+	/*
+	 * Calculate guest's supported XCR0 taking into account guest CPUID data and
+	 * KVM's supported XCR0 (comprised of host's XCR0 and KVM_SUPPORTED_XCR0).
+	 */
+	best = kvm_find_cpuid_entry_index(vcpu, 0xd, 0);
+	if (!best)
+		vcpu->arch.guest_supported_xcr0 = 0;
+	else
+		vcpu->arch.guest_supported_xcr0 = (best->eax | ((u64)best->edx << 32)) &
+						  kvm_caps.supported_xcr0;
 
 	/*
 	 * FP+SSE can always be saved/restored via KVM_{G,S}ET_XSAVE, even if
Signature · 1 line
2.40.1.495.gc816e09b53d-goog
maintainer Applied
#4 lore raw
In reply to Sean Christopherson
Quoted 23 lines · Sean Christopherson

Stop adjusting the guest's CPUID info for the allowed XFRM (a.k.a. XCR0) for SGX enclaves. Past me didn't understand the roles and responsibilities between userspace and KVM with respect to CPUID leafs, i.e. I thought I was being helpful by having KVM adjust the entries.

This is clearly an ABI change, but QEMU does the right thing and AFAIK no other VMMs support SGX (yet), so I'm hopeful/confident that we can excise the ugly before userspace starts depending on the bad behavior.

v2:
- Collect reviews/testing. [Kai]
- Require FP+SSE to always be set in XFRM, and exempt them from the XFRM vs. XCR0 check. [Kai]

v1: https://lore.kernel.org/all/[email protected]

Sean Christopherson (3):
KVM: VMX: Don't rely _only_ on CPUID to enforce XCR0 restrictions for
ECREATE
KVM: x86: Don't adjust guest's CPUID.0x12.1 (allowed SGX enclave XFRM)
KVM: x86: Open code supported XCR0 calculation in
kvm_vcpu_after_set_cpuid()

Quoted 6 lines
  arch/x86/kvm/cpuid.c   | 43 ++++++++++--------------------------------
  arch/x86/kvm/vmx/sgx.c | 11 +++++++++--
  2 files changed, 19 insertions(+), 35 deletions(-)

base-commit: 5c291b93e5d665380dbecc6944973583f9565ee5

Queued, thanks. But why patch 3? Small functions are nice and remove the need to remember what is in EDX:EAX of CPUID[0xD,0].

Paolo

author maintainer
#5 lore raw
Quoted 23 lines · Paolo Bonzini
Quoted 22 lines · Sean Christopherson

Stop adjusting the guest's CPUID info for the allowed XFRM (a.k.a. XCR0) for SGX enclaves. Past me didn't understand the roles and responsibilities between userspace and KVM with respect to CPUID leafs, i.e. I thought I was being helpful by having KVM adjust the entries.

This is clearly an ABI change, but QEMU does the right thing and AFAIK no other VMMs support SGX (yet), so I'm hopeful/confident that we can excise the ugly before userspace starts depending on the bad behavior. v2:
- Collect reviews/testing. [Kai]
- Require FP+SSE to always be set in XFRM, and exempt them from the XFRM vs. XCR0 check. [Kai]

v1: https://lore.kernel.org/all/[email protected]

Sean Christopherson (3):
KVM: VMX: Don't rely _only_ on CPUID to enforce XCR0 restrictions for
ECREATE
KVM: x86: Don't adjust guest's CPUID.0x12.1 (allowed SGX enclave XFRM)
KVM: x86: Open code supported XCR0 calculation in
kvm_vcpu_after_set_cpuid()

Quoted 8 lines
Quoted 6 lines
  arch/x86/kvm/cpuid.c   | 43 ++++++++++--------------------------------
  arch/x86/kvm/vmx/sgx.c | 11 +++++++++--
  2 files changed, 19 insertions(+), 35 deletions(-)

base-commit: 5c291b93e5d665380dbecc6944973583f9565ee5

Queued, thanks. But why patch 3?

I want to guard against future misuse of calculating the support XCR0 before the CPUID update is complete. I suppose I could have done this:

static u64 guest_cpuid_supported_xcr0(struct kvm_vcpu *vcpu)

  {
	struct kvm_cpuid_entry2 *best;

	best = kvm_find_cpuid_entry_index(vcpu, 0xd, 0);
	if (!best)
		return 0;

	return (best->eax | ((u64)best->edx << 32)) & kvm_caps.supported_xcr0;

}

but I don't really see the point since there should only ever be one caller, e.g. unlike cpuid_query_maxphyaddr() which needs a non-zero default.

Quoted 2 lines

Small functions are nice and remove the need to remember what is in EDX:EAX of CPUID[0xD,0].

Hmm, yes and no. Specifically, I dislike single-use helpers that bury unintuitive details in the helper, e.g. in this case, filtering the raw iguest CPUID with KVM's kvm_caps.supported_xcr0. Communicating that in the name of the function so that they're are no surprises is often more difficult than just open coding things.