[PATCH for_v25 0/3] x86/sgx: Fix feature control rebase

[PATCH for_v25 0/3] x86/sgx: Fix feature control rebase

[Sean Christopherson]

Two patches to fix the Feature Control MSR bit definitions, which didn't
get correctly rebased.

Patch 03 reworks SGX handling of Feature Control to use the new feat_ctl.c
code, which was the entire point of that series.  Patch 03 is a drop-in
replacement for 4249f9b240b7 ("x86/cpu/intel: Detect SGX supprt").

*** DISCLAIMER ***
These patches are untested against your master, as your master doesn't boot
on my system.  The fully tested version is available at 
https://github.com/sean-jc/linux.git, branch sgx/for_jarkko_v25_rebased.
I'll dig into the boot issue next week.

Sean Christopherson (3):
  x86/msr: Fixup "Add Intel SGX hardware bits"
  x86/msr: Fixup "Intel SGX Launch Control hardware bits"
  x86/cpu: Configure SGX support when initializing feature control MSR

 arch/x86/include/asm/msr-index.h |  9 ++------
 arch/x86/kernel/cpu/feat_ctl.c   | 29 +++++++++++++++++++++++-
 arch/x86/kernel/cpu/intel.c      | 39 --------------------------------
 3 files changed, 30 insertions(+), 47 deletions(-)

-- 
2.24.1

[PATCH for_v25 1/3] x86/msr: Fixup "Add Intel SGX hardware bits"

[Sean Christopherson]

Fix the Feature Control MSR bit definition for SGX that got borked
during the rebase to the latest upstream.

Fixes: 8813e054c085 ("x86/cpufeatures: x86/msr: Add Intel SGX hardware bits")
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/include/asm/msr-index.h | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 75f41095f9b9..3ddc6336aaa2 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -564,6 +564,7 @@
 #define FEAT_CTL_LOCKED				BIT(0)
 #define FEAT_CTL_VMX_ENABLED_INSIDE_SMX		BIT(1)
 #define FEAT_CTL_VMX_ENABLED_OUTSIDE_SMX	BIT(2)
+#define FEAT_CTL_SGX_ENABLED			BIT(18)
 #define FEAT_CTL_LMCE_ENABLED			BIT(20)
 
 #define MSR_IA32_TSC_ADJUST             0x0000003b
@@ -573,13 +574,6 @@
 
 #define MSR_IA32_XSS			0x00000da0
 
-#define FEATURE_CONTROL_LOCKED				(1<<0)
-#define FEATURE_CONTROL_VMXON_ENABLED_INSIDE_SMX	(1<<1)
-#define FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX	(1<<2)
-#define FEATURE_CONTROL_SGX_LE_WR			(1<<17)
-#define FEATURE_CONTROL_SGX_ENABLE			(1<<18)
-#define FEATURE_CONTROL_LMCE				(1<<20)
-
 #define MSR_IA32_APICBASE		0x0000001b
 #define MSR_IA32_APICBASE_BSP		(1<<8)
 #define MSR_IA32_APICBASE_ENABLE	(1<<11)
-- 
2.24.1

[PATCH for_v25 2/3] x86/msr: Fixup "Intel SGX Launch Control hardware bits"

[Sean Christopherson]

Fix the Feature Control MSR bit definition for SGX LC that got borked
during the rebase to the latest upstream.

Note, the name diverges from what was previously used in the SGX series.
The decision made by/with Boris was to follow the SDM names.

Fixes: 24670c2036be ("x86/cpufeatures: x86/msr: Intel SGX Launch Control hardware bits")
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/include/asm/msr-index.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 3ddc6336aaa2..a0776c262820 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -564,6 +564,7 @@
 #define FEAT_CTL_LOCKED				BIT(0)
 #define FEAT_CTL_VMX_ENABLED_INSIDE_SMX		BIT(1)
 #define FEAT_CTL_VMX_ENABLED_OUTSIDE_SMX	BIT(2)
+#define FEAT_CTL_SGX_LC_ENABLED			BIT(17)
 #define FEAT_CTL_SGX_ENABLED			BIT(18)
 #define FEAT_CTL_LMCE_ENABLED			BIT(20)
 
-- 
2.24.1

[PATCH for_v25 3/3] x86/cpu: Configure SGX support when initializing feature control MSR

[Sean Christopherson]

Configure SGX as part of feature control MSR initialization and update
the associated X86_FEATURE flags accordingly.  Because the kernel will
require the LE hash MSRs to be writable when running native enclaves,
disable X86_FEATURE_SGX (and all derivatives) if SGX Launch Control is
not (or cannot) be fully enabled via feature control MSR.

Note, unlike VMX, clear the X86_FEATURE_SGX* flags for all CPUs if any
CPU lacks SGX support as the kernel expects SGX to be available on all
CPUs.  X86_FEATURE_VMX is intentionally cleared only for the current CPU
so that KVM can provide additional information if KVM fails to load,
e.g. print which CPU doesn't support VMX.  KVM/VMX requires additional
per-CPU enabling, e.g. to set CR4.VMXE and do VMXON, and so already has
the necessary infrastructure to do per-CPU checks.  SGX on the other
hand doesn't require additional enabling, so clearing the feature flags
on all CPUs means the SGX subsystem doesn't need to manually do support
checks on a per-CPU basis.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---

This reverts commit 4249f9b240b7, i.e. is a drop-in replacement.
Fixes: 4249f9b240b7 ("x86/cpu/intel: Detect SGX supprt")

 arch/x86/kernel/cpu/feat_ctl.c | 29 ++++++++++++++++++++++++-
 arch/x86/kernel/cpu/intel.c    | 39 ----------------------------------
 2 files changed, 28 insertions(+), 40 deletions(-)

diff --git a/arch/x86/kernel/cpu/feat_ctl.c b/arch/x86/kernel/cpu/feat_ctl.c
index 0268185bef94..b16b71a6da74 100644
--- a/arch/x86/kernel/cpu/feat_ctl.c
+++ b/arch/x86/kernel/cpu/feat_ctl.c
@@ -92,6 +92,14 @@ static void init_vmx_capabilities(struct cpuinfo_x86 *c)
 }
 #endif /* CONFIG_X86_VMX_FEATURE_NAMES */
 
+static void clear_sgx_caps(void)
+{
+	setup_clear_cpu_cap(X86_FEATURE_SGX);
+	setup_clear_cpu_cap(X86_FEATURE_SGX_LC);
+	setup_clear_cpu_cap(X86_FEATURE_SGX1);
+	setup_clear_cpu_cap(X86_FEATURE_SGX2);
+}
+
 void init_ia32_feat_ctl(struct cpuinfo_x86 *c)
 {
 	bool tboot = tboot_enabled();
@@ -99,6 +107,7 @@ void init_ia32_feat_ctl(struct cpuinfo_x86 *c)
 
 	if (rdmsrl_safe(MSR_IA32_FEAT_CTL, &msr)) {
 		clear_cpu_cap(c, X86_FEATURE_VMX);
+		clear_sgx_caps();
 		return;
 	}
 
@@ -123,13 +132,21 @@ void init_ia32_feat_ctl(struct cpuinfo_x86 *c)
 			msr |= FEAT_CTL_VMX_ENABLED_INSIDE_SMX;
 	}
 
+	/*
+	 * Enable SGX if and only if the kernel supports SGX and Launch Control
+	 * is supported, i.e. disable SGX if the LE hash MSRs can't be written.
+	 */
+	if (cpu_has(c, X86_FEATURE_SGX) && cpu_has(c, X86_FEATURE_SGX_LC) &&
+	    IS_ENABLED(CONFIG_INTEL_SGX))
+		msr |= FEAT_CTL_SGX_ENABLED | FEAT_CTL_SGX_LC_ENABLED;
+
 	wrmsrl(MSR_IA32_FEAT_CTL, msr);
 
 update_caps:
 	set_cpu_cap(c, X86_FEATURE_MSR_IA32_FEAT_CTL);
 
 	if (!cpu_has(c, X86_FEATURE_VMX))
-		return;
+		goto update_sgx;
 
 	if ( (tboot && !(msr & FEAT_CTL_VMX_ENABLED_INSIDE_SMX)) ||
 	    (!tboot && !(msr & FEAT_CTL_VMX_ENABLED_OUTSIDE_SMX))) {
@@ -142,4 +159,14 @@ void init_ia32_feat_ctl(struct cpuinfo_x86 *c)
 		init_vmx_capabilities(c);
 #endif
 	}
+
+update_sgx:
+	if (!cpu_has(c, X86_FEATURE_SGX) || !cpu_has(c, X86_FEATURE_SGX_LC)) {
+		clear_sgx_caps();
+	} else if (!(msr & FEAT_CTL_SGX_ENABLED) ||
+		   !(msr & FEAT_CTL_SGX_LC_ENABLED)) {
+		if (IS_ENABLED(CONFIG_INTEL_SGX))
+			pr_err_once("SGX disabled by BIOS\n");
+		clear_sgx_caps();
+	}
 }
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index cab0940784a7..be82cd5841c3 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -542,40 +542,6 @@ static void detect_tme(struct cpuinfo_x86 *c)
 	c->x86_phys_bits -= keyid_bits;
 }
 
-static void __maybe_unused detect_sgx(struct cpuinfo_x86 *c)
-{
-	unsigned long long fc;
-
-	rdmsrl(MSR_IA32_FEAT_CTL, fc);
-	if (!(fc & FEATURE_CONTROL_LOCKED)) {
-		pr_err_once("sgx: The feature control MSR is not locked\n");
-		goto err_unsupported;
-	}
-
-	if (!(fc & FEATURE_CONTROL_SGX_ENABLE)) {
-		pr_err_once("sgx: SGX is not enabled in IA32_FEATURE_CONTROL MSR\n");
-		goto err_unsupported;
-	}
-
-	if (!cpu_has(c, X86_FEATURE_SGX1)) {
-		pr_err_once("sgx: SGX1 instruction set is not supported\n");
-		goto err_unsupported;
-	}
-
-	if (!(fc & FEATURE_CONTROL_SGX_LE_WR)) {
-		pr_info_once("sgx: The launch control MSRs are not writable\n");
-		goto err_unsupported;
-	}
-
-	return;
-
-err_unsupported:
-	setup_clear_cpu_cap(X86_FEATURE_SGX);
-	setup_clear_cpu_cap(X86_FEATURE_SGX1);
-	setup_clear_cpu_cap(X86_FEATURE_SGX2);
-	setup_clear_cpu_cap(X86_FEATURE_SGX_LC);
-}
-
 static void init_cpuid_fault(struct cpuinfo_x86 *c)
 {
 	u64 msr;
@@ -712,11 +678,6 @@ static void init_intel(struct cpuinfo_x86 *c)
 	if (cpu_has(c, X86_FEATURE_TME))
 		detect_tme(c);
 
-#ifdef CONFIG_INTEL_SGX
-	if (cpu_has(c, X86_FEATURE_SGX))
-		detect_sgx(c);
-#endif
-
 	init_intel_misc_features(c);
 
 	if (tsx_ctrl_state == TSX_CTRL_ENABLE)
-- 
2.24.1

Re: [PATCH for_v25 0/3] x86/sgx: Fix feature control rebase

[Jarkko Sakkinen]

On Sat, Feb 01, 2020 at 09:49:37AM -0800, Sean Christopherson wrote:
> Two patches to fix the Feature Control MSR bit definitions, which didn't
> get correctly rebased.
> 
> Patch 03 reworks SGX handling of Feature Control to use the new feat_ctl.c
> code, which was the entire point of that series.  Patch 03 is a drop-in
> replacement for 4249f9b240b7 ("x86/cpu/intel: Detect SGX supprt").
> 
> *** DISCLAIMER ***
> These patches are untested against your master, as your master doesn't boot
> on my system.  The fully tested version is available at 
> https://github.com/sean-jc/linux.git, branch sgx/for_jarkko_v25_rebased.
> I'll dig into the boot issue next week.

Possibly it was related  that I was temporarily on top of x86/tip?  It
is now again on top of Linus' tree. I created v25-rc2 tag that has
these updates:

tag v25-rc2
Tagger: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Date:   Sat Feb 1 21:27:24 2020 +0200

x86/sgx: v25-rc2

* Fix a double-free issue when SGX_IOC_ENCLAVE_ADD_PAGES
  fails on executing ENCLS[EADD]. The rollback path executed
  radix_tree_delete() on the same address twice when this happened.
* Return -EINTR instead of -ERESTARTSYS in SGX_IOC_ENCLAVE_ADD_PAGES when
  a signal is pending.
* As requested by Borislav, move the CPUID 0x12 features to their own word
  in cpufeatures.
* Sean fixed a bug from sgx_reclaimer_write() where sgx_encl_put_backing()
  was called with an uninitialized pointer when sgx_encl_get_backing()
  fails.
* Migrated /dev/sgx/* to misc. This is future-proof as struct miscdevice
  has 'groups' for setting up sysfs attributes for the device.
* Use device_initcall instead of subsys_initcall so that misc_class is
  initialized before SGX is initialized.
* Return -EACCES in SGX_IOC_ENCLAVE_INIT when caller tries to select
  enclave attributes that we the kernel does not allow it to set instead
  of -EINVAL.
* Unless SGX public key MSRs are writable always deny the feature from
  Linux. Previously this was only denied from driver. How VMs should be
  supported is not really part of initial patch set, which makes this
  an obvious choice.
* Rewrote the documentation. Lessened the fine-grained micro architecture
  details as they can be looked up from Intel SDM in order to make the
  core ideas more approachable.
-----BEGIN PGP SIGNATURE-----

iJYEABYIAD4WIQRE6pSOnaBC00OEHEIaerohdGur0gUCXjXRMiAcamFya2tvLnNh
a2tpbmVuQGxpbnV4LmludGVsLmNvbQAKCRAaerohdGur0ihBAQC3VE2u6zyPYFLN
hCBFEF3LKqpNk26DjkO9M5tRZfUhSgEA/sF6AKHJRDqYUePW6N6Rtc3GOZY9DmbD
R+qOg0XcbAs=
=OtqV
-----END PGP SIGNATURE-----

If you fix any regression, use solely this tag as the baseline for
fixes. I worry about possible merge conflicts with the master.

/Jarkko

Re: [PATCH for_v25 0/3] x86/sgx: Fix feature control rebase

[Jarkko Sakkinen]

On Sat, Feb 01, 2020 at 09:35:59PM +0200, Jarkko Sakkinen wrote:
> On Sat, Feb 01, 2020 at 09:49:37AM -0800, Sean Christopherson wrote:
> > Two patches to fix the Feature Control MSR bit definitions, which didn't
> > get correctly rebased.
> > 
> > Patch 03 reworks SGX handling of Feature Control to use the new feat_ctl.c
> > code, which was the entire point of that series.  Patch 03 is a drop-in
> > replacement for 4249f9b240b7 ("x86/cpu/intel: Detect SGX supprt").
> > 
> > *** DISCLAIMER ***
> > These patches are untested against your master, as your master doesn't boot
> > on my system.  The fully tested version is available at 
> > https://github.com/sean-jc/linux.git, branch sgx/for_jarkko_v25_rebased.
> > I'll dig into the boot issue next week.
> 
> Possibly it was related  that I was temporarily on top of x86/tip?  It
> is now again on top of Linus' tree. I created v25-rc2 tag that has
> these updates:
> 
> tag v25-rc2
> Tagger: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> Date:   Sat Feb 1 21:27:24 2020 +0200
> 
> x86/sgx: v25-rc2
> 
> * Fix a double-free issue when SGX_IOC_ENCLAVE_ADD_PAGES
>   fails on executing ENCLS[EADD]. The rollback path executed
>   radix_tree_delete() on the same address twice when this happened.
> * Return -EINTR instead of -ERESTARTSYS in SGX_IOC_ENCLAVE_ADD_PAGES when
>   a signal is pending.
> * As requested by Borislav, move the CPUID 0x12 features to their own word
>   in cpufeatures.
> * Sean fixed a bug from sgx_reclaimer_write() where sgx_encl_put_backing()
>   was called with an uninitialized pointer when sgx_encl_get_backing()
>   fails.
> * Migrated /dev/sgx/* to misc. This is future-proof as struct miscdevice
>   has 'groups' for setting up sysfs attributes for the device.
> * Use device_initcall instead of subsys_initcall so that misc_class is
>   initialized before SGX is initialized.
> * Return -EACCES in SGX_IOC_ENCLAVE_INIT when caller tries to select
>   enclave attributes that we the kernel does not allow it to set instead
>   of -EINVAL.
> * Unless SGX public key MSRs are writable always deny the feature from
>   Linux. Previously this was only denied from driver. How VMs should be
>   supported is not really part of initial patch set, which makes this
>   an obvious choice.
> * Rewrote the documentation. Lessened the fine-grained micro architecture
>   details as they can be looked up from Intel SDM in order to make the
>   core ideas more approachable.
> -----BEGIN PGP SIGNATURE-----
> 
> iJYEABYIAD4WIQRE6pSOnaBC00OEHEIaerohdGur0gUCXjXRMiAcamFya2tvLnNh
> a2tpbmVuQGxpbnV4LmludGVsLmNvbQAKCRAaerohdGur0ihBAQC3VE2u6zyPYFLN
> hCBFEF3LKqpNk26DjkO9M5tRZfUhSgEA/sF6AKHJRDqYUePW6N6Rtc3GOZY9DmbD
> R+qOg0XcbAs=
> =OtqV
> -----END PGP SIGNATURE-----
> 
> If you fix any regression, use solely this tag as the baseline for
> fixes. I worry about possible merge conflicts with the master.
> 
> /Jarkko

As far as I'm concerned master has now everything for v25, so I removed
the tag. Just test against master.

/Jarkko

Re: [PATCH for_v25 0/3] x86/sgx: Fix feature control rebase

[Sean Christopherson]

On Sun, Feb 02, 2020 at 12:18:04AM +0200, Jarkko Sakkinen wrote:
> On Sat, Feb 01, 2020 at 09:35:59PM +0200, Jarkko Sakkinen wrote:
> > If you fix any regression, use solely this tag as the baseline for
> > fixes. I worry about possible merge conflicts with the master.
> > 
> > /Jarkko
> 
> As far as I'm concerned master has now everything for v25, so I removed
> the tag. Just test against master.

Sent a fix for the boot issue.

Can you also rebase to Linus' latest tree?  Or any tree that containts
commit 8df5bb4a03b0 ("char: hpet: Fix out-of-bounds read bug").  Spent the
morning bisecting another boot crash due to the hpet bug...

Re: [PATCH for_v25 0/3] x86/sgx: Fix feature control rebase

[Jarkko Sakkinen]

On Mon, Feb 03, 2020 at 12:04:41PM -0800, Sean Christopherson wrote:
> On Sun, Feb 02, 2020 at 12:18:04AM +0200, Jarkko Sakkinen wrote:
> > On Sat, Feb 01, 2020 at 09:35:59PM +0200, Jarkko Sakkinen wrote:
> > > If you fix any regression, use solely this tag as the baseline for
> > > fixes. I worry about possible merge conflicts with the master.
> > > 
> > > /Jarkko
> > 
> > As far as I'm concerned master has now everything for v25, so I removed
> > the tag. Just test against master.
> 
> Sent a fix for the boot issue.
> 
> Can you also rebase to Linus' latest tree?  Or any tree that containts
> commit 8df5bb4a03b0 ("char: hpet: Fix out-of-bounds read bug").  Spent the
> morning bisecting another boot crash due to the hpet bug...

OK, I'll send the patch set today.

/Jarkko