[PATCH v2 0/3] KVM: x86: SGX vs. XCR0 cleanups

[PATCH v2 0/3] KVM: x86: SGX vs. XCR0 cleanups

[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/20230405005911.423699-1-seanjc@google.com

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
-- 
2.40.1.495.gc816e09b53d-goog

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

[Sean Christopherson]

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 <kai.huang@intel.com>
Tested-by: Kai Huang <kai.huang@intel.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/vmx/sgx.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/vmx/sgx.c b/arch/x86/kvm/vmx/sgx.c
index 0574030b071f..2261b684a7d4 100644
--- a/arch/x86/kvm/vmx/sgx.c
+++ b/arch/x86/kvm/vmx/sgx.c
@@ -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;
 	}
-- 
2.40.1.495.gc816e09b53d-goog

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

[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 <kai.huang@intel.com>
Tested-by: Kai Huang <kai.huang@intel.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/cpuid.c | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 123bf8b97a4b..0c9660a07b23 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -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)
-- 
2.40.1.495.gc816e09b53d-goog

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

[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 <kai.huang@intel.com>
Tested-by: Kai Huang <kai.huang@intel.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/cpuid.c | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 0c9660a07b23..491c88e196c1 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -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
-- 
2.40.1.495.gc816e09b53d-goog

Re: [PATCH v2 0/3] KVM: x86: SGX vs. XCR0 cleanups

[Paolo Bonzini]

On 5/3/23 18:08, Sean Christopherson wrote:
> 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/20230405005911.423699-1-seanjc@google.com
> 
> 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

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

Re: [PATCH v2 0/3] KVM: x86: SGX vs. XCR0 cleanups

[Sean Christopherson]

On Fri, May 19, 2023, Paolo Bonzini wrote:
> On 5/3/23 18:08, Sean Christopherson wrote:
> > 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/20230405005911.423699-1-seanjc@google.com
> > 
> > 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
> 
> 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.

> 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.