[v24,07/24] x86/cpu/intel: Detect SGX supprt
diff mbox series

Message ID 20191129231326.18076-8-jarkko.sakkinen@linux.intel.com
State New
Headers show
Series
  • Intel SGX foundations
Related show

Commit Message

Jarkko Sakkinen Nov. 29, 2019, 11:13 p.m. UTC
From: Sean Christopherson <sean.j.christopherson@intel.com>

When the CPU supports SGX, check that the BIOS has enabled SGX and SGX1
opcodes are available. Otherwise, all the SGX related capabilities.

In addition, clear X86_FEATURE_SGX_LC also in the case when the launch
enclave are read-only. This way the feature bit reflects the level that
Linux supports the launch control.

The check is done for every CPU, not just BSP, in order to verify that
MSR_IA32_FEATURE_CONTROL is correctly configured on all CPUs. The other
parts of the kernel, like the enclave driver, expect the same
configuration from all CPUs.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
Co-developed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 arch/x86/kernel/cpu/intel.c | 41 +++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

Comments

Borislav Petkov Dec. 17, 2019, 3:17 p.m. UTC | #1
On Sat, Nov 30, 2019 at 01:13:09AM +0200, Jarkko Sakkinen wrote:

Typo in the subject:

Subject: Re: [PATCH v24 07/24] x86/cpu/intel: Detect SGX supprt
							 ^^^^^^
> From: Sean Christopherson <sean.j.christopherson@intel.com>
> 
> When the CPU supports SGX, check that the BIOS has enabled SGX and SGX1
> opcodes are available. Otherwise, all the SGX related capabilities.
> 
> In addition, clear X86_FEATURE_SGX_LC also in the case when the launch
> enclave are read-only. This way the feature bit reflects the level that
> Linux supports the launch control.
> 
> The check is done for every CPU, not just BSP, in order to verify that
> MSR_IA32_FEATURE_CONTROL is correctly configured on all CPUs. The other
> parts of the kernel, like the enclave driver, expect the same
> configuration from all CPUs.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> Co-developed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
>  arch/x86/kernel/cpu/intel.c | 41 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)

...

> @@ -761,6 +797,11 @@ 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

You can remove the ifdeffery here and put the ifdef around the function
body and drop the __maybe_unused tag:

diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index ef41431b3f70..2f3414eff99d 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -624,8 +624,9 @@ static void detect_tme(struct cpuinfo_x86 *c)
 	c->x86_phys_bits -= keyid_bits;
 }
 
-static void __maybe_unused detect_sgx(struct cpuinfo_x86 *c)
+static void detect_sgx(struct cpuinfo_x86 *c)
 {
+#ifdef CONFIG_INTEL_SGX
 	unsigned long long fc;
 
 	rdmsrl(MSR_IA32_FEATURE_CONTROL, fc);
@@ -658,6 +659,7 @@ static void __maybe_unused detect_sgx(struct cpuinfo_x86 *c)
 
 err_msrs_rdonly:
 	setup_clear_cpu_cap(X86_FEATURE_SGX_LC);
+#endif
 }
 
 static void init_cpuid_fault(struct cpuinfo_x86 *c)
@@ -797,10 +799,8 @@ 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);
Jarkko Sakkinen Dec. 19, 2019, 12:42 a.m. UTC | #2
On Tue, 2019-12-17 at 16:17 +0100, Borislav Petkov wrote:
> On Sat, Nov 30, 2019 at 01:13:09AM +0200, Jarkko Sakkinen wrote:
> 
> Typo in the subject:
> 
> Subject: Re: [PATCH v24 07/24] x86/cpu/intel: Detect SGX supprt
> 							 ^^^^^^
> > From: Sean Christopherson <sean.j.christopherson@intel.com>
> > 
> > When the CPU supports SGX, check that the BIOS has enabled SGX and SGX1
> > opcodes are available. Otherwise, all the SGX related capabilities.
> > 
> > In addition, clear X86_FEATURE_SGX_LC also in the case when the launch
> > enclave are read-only. This way the feature bit reflects the level that
> > Linux supports the launch control.
> > 
> > The check is done for every CPU, not just BSP, in order to verify that
> > MSR_IA32_FEATURE_CONTROL is correctly configured on all CPUs. The other
> > parts of the kernel, like the enclave driver, expect the same
> > configuration from all CPUs.
> > 
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > Co-developed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > ---
> >  arch/x86/kernel/cpu/intel.c | 41 +++++++++++++++++++++++++++++++++++++
> >  1 file changed, 41 insertions(+)
> 
> ...
> 
> > @@ -761,6 +797,11 @@ 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
> 
> You can remove the ifdeffery here and put the ifdef around the function
> body and drop the __maybe_unused tag:

OK, so I remember getting feedback on some unrelated to SGX stuff
that I should use __maybe_unused in the past. That is why I used
it here instead of ifdeffery.

It is used in bunch of places in the kernel. I'm a bit confused
when using it is a right thing and when it should be avoided.

/Jarkko
Borislav Petkov Dec. 19, 2019, 6:15 a.m. UTC | #3
On Thu, Dec 19, 2019 at 02:42:20AM +0200, Jarkko Sakkinen wrote:
> It is used in bunch of places in the kernel. I'm a bit confused
> when using it is a right thing and when it should be avoided.

Yeah, we try to avoid ifdeffery as much as possible. Even if it means
pushing it into the called function and hiding it there. :-)

Thx.
Borislav Petkov Dec. 23, 2019, 9:46 a.m. UTC | #4
On Sat, Nov 30, 2019 at 01:13:09AM +0200, Jarkko Sakkinen wrote:
> From: Sean Christopherson <sean.j.christopherson@intel.com>
> 
> When the CPU supports SGX, check that the BIOS has enabled SGX and SGX1
> opcodes are available. Otherwise, all the SGX related capabilities.
> 
> In addition, clear X86_FEATURE_SGX_LC also in the case when the launch
> enclave are read-only. This way the feature bit reflects the level that
> Linux supports the launch control.
> 
> The check is done for every CPU, not just BSP, in order to verify that
> MSR_IA32_FEATURE_CONTROL is correctly configured on all CPUs. The other
> parts of the kernel, like the enclave driver, expect the same
> configuration from all CPUs.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> Co-developed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
>  arch/x86/kernel/cpu/intel.c | 41 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> index c2fdc00df163..89a71367716c 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -624,6 +624,42 @@ 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_FEATURE_CONTROL, 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_msrs_rdonly;
> +	}

One more thing - and we talked about this already - when the hash
MSRs are not writable, the kernel needs to disable all SGX support by
default. Basically, no SGX support is present.

If the user wants to run KVM guests with SGX enclaves, then she should
probably boot with a special kvm param or so. Details on how can exactly
control that can be discussed later - just making sure you guys are not
forgetting this use angle.

Thx.
Jarkko Sakkinen Dec. 27, 2019, 6 a.m. UTC | #5
On Mon, 2019-12-23 at 10:46 +0100, Borislav Petkov wrote:
> On Sat, Nov 30, 2019 at 01:13:09AM +0200, Jarkko Sakkinen wrote:
> > From: Sean Christopherson <sean.j.christopherson@intel.com>
> > 
> > When the CPU supports SGX, check that the BIOS has enabled SGX and SGX1
> > opcodes are available. Otherwise, all the SGX related capabilities.
> > 
> > In addition, clear X86_FEATURE_SGX_LC also in the case when the launch
> > enclave are read-only. This way the feature bit reflects the level that
> > Linux supports the launch control.
> > 
> > The check is done for every CPU, not just BSP, in order to verify that
> > MSR_IA32_FEATURE_CONTROL is correctly configured on all CPUs. The other
> > parts of the kernel, like the enclave driver, expect the same
> > configuration from all CPUs.
> > 
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > Co-developed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > ---
> >  arch/x86/kernel/cpu/intel.c | 41 +++++++++++++++++++++++++++++++++++++
> >  1 file changed, 41 insertions(+)
> > 
> > diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> > index c2fdc00df163..89a71367716c 100644
> > --- a/arch/x86/kernel/cpu/intel.c
> > +++ b/arch/x86/kernel/cpu/intel.c
> > @@ -624,6 +624,42 @@ 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_FEATURE_CONTROL, 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_msrs_rdonly;
> > +	}
> 
> One more thing - and we talked about this already - when the hash
> MSRs are not writable, the kernel needs to disable all SGX support by
> default. Basically, no SGX support is present.
> 
> If the user wants to run KVM guests with SGX enclaves, then she should
> probably boot with a special kvm param or so. Details on how can exactly
> control that can be discussed later - just making sure you guys are not
> forgetting this use angle.
> 
> Thx.

Sure.

Overally, I guess the right marching order is to hold on with the 
next version of SGX patch set up until Sean's other patch set is
ready to be merged.

/Jarkko
Sean Christopherson Jan. 14, 2020, 6:36 p.m. UTC | #6
On Mon, Dec 23, 2019 at 10:46:14AM +0100, Borislav Petkov wrote:
> On Sat, Nov 30, 2019 at 01:13:09AM +0200, Jarkko Sakkinen wrote:
> > From: Sean Christopherson <sean.j.christopherson@intel.com>
> > 
> > When the CPU supports SGX, check that the BIOS has enabled SGX and SGX1
> > opcodes are available. Otherwise, all the SGX related capabilities.
> > 
> > In addition, clear X86_FEATURE_SGX_LC also in the case when the launch
> > enclave are read-only. This way the feature bit reflects the level that
> > Linux supports the launch control.
> > 
> > The check is done for every CPU, not just BSP, in order to verify that
> > MSR_IA32_FEATURE_CONTROL is correctly configured on all CPUs. The other
> > parts of the kernel, like the enclave driver, expect the same
> > configuration from all CPUs.
> > 
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > Co-developed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > ---
> >  arch/x86/kernel/cpu/intel.c | 41 +++++++++++++++++++++++++++++++++++++
> >  1 file changed, 41 insertions(+)
> > 
> > diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> > index c2fdc00df163..89a71367716c 100644
> > --- a/arch/x86/kernel/cpu/intel.c
> > +++ b/arch/x86/kernel/cpu/intel.c
> > @@ -624,6 +624,42 @@ 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_FEATURE_CONTROL, 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_msrs_rdonly;
> > +	}
> 
> One more thing - and we talked about this already - when the hash
> MSRs are not writable, the kernel needs to disable all SGX support by
> default. Basically, no SGX support is present.

Yep.
 
> If the user wants to run KVM guests with SGX enclaves, then she should
> probably boot with a special kvm param or so. Details on how can exactly
> control that can be discussed later - just making sure you guys are not
> forgetting this use angle.

Ya, planning on having a 'sgx' KVM module param.  It could take a string
if we want to allow the admin to enable SGX virtualization if and only if
SGX LC is fully enabled, e.g. sgx='off|on|lc'.

Patch
diff mbox series

diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index c2fdc00df163..89a71367716c 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -624,6 +624,42 @@  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_FEATURE_CONTROL, 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_msrs_rdonly;
+	}
+
+	return;
+
+err_unsupported:
+	setup_clear_cpu_cap(X86_FEATURE_SGX);
+	setup_clear_cpu_cap(X86_FEATURE_SGX1);
+	setup_clear_cpu_cap(X86_FEATURE_SGX2);
+
+err_msrs_rdonly:
+	setup_clear_cpu_cap(X86_FEATURE_SGX_LC);
+}
+
 static void init_cpuid_fault(struct cpuinfo_x86 *c)
 {
 	u64 msr;
@@ -761,6 +797,11 @@  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);
 }