| Message ID | 20180608171216.26521-8-jarkko.sakkinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> +config INTEL_SGX_CORE > + prompt "Intel SGX core functionality > + depends on X86_64 && CPU_SUP_INTEL > + help > + Intel Software Guard eXtensions (SGX) is a set of CPU instructions > + that allows ring 3 applications to create enclaves; private regions > + of memory that are protected, by hardware, from unauthorized access > + and/or modification. That semicolon needs to be a colon. The second half of that sentence is not a stand-alone statement. > + This option enables kernel recognition of SGX, high-level management > + of the Enclave Page Cache (EPC), tracking and writing of SGX Launch > + Enclave Hash MSRs, and allows for virtualization of SGX via KVM. By > + iteslf, this option does not provide SGX support to userspace. > + > + For details, see Documentation/x86/intel_sgx.rst > + > + If unsure, say N. > + > config EFI > bool "EFI runtime service support" > depends on ACPI > diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h > new file mode 100644 > index 000000000000..fa3e6e0eb8af > --- /dev/null > +++ b/arch/x86/include/asm/sgx.h > @@ -0,0 +1,25 @@ > +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) > +// Copyright(c) 2016-18 Intel Corporation. > +// > +// Authors: > +// > +// Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > +// Suresh Siddha <suresh.b.siddha@intel.com> > +// Sean Christopherson <sean.j.christopherson@intel.com> > + > +#ifndef _ASM_X86_SGX_H > +#define _ASM_X86_SGX_H > + > +#include <linux/types.h> > + > +#define SGX_CPUID 0x12 Hey, I just saw 0x12 as a magic, hard-coded number earlier in these patches. It seems cruel to hard-code it, and then also have a #define that isn't used. > +enum sgx_cpuid { > + SGX_CPUID_CAPABILITIES = 0, > + SGX_CPUID_ATTRIBUTES = 1, > + SGX_CPUID_EPC_BANKS = 2, > +}; These are cpuid *leaves*, right? Please make this clear that these are hardware-defined values and not some kind of software construct. > +bool sgx_enabled __ro_after_init = false; > +EXPORT_SYMBOL(sgx_enabled); > + > +static __init bool sgx_is_enabled(void) > +{ > + unsigned long fc; > + > + if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL) > + return false; Not necessary. CPUID does this part for you. > + if (!boot_cpu_has(X86_FEATURE_SGX)) > + return false; > + > + if (!boot_cpu_has(X86_FEATURE_SGX1)) > + return false; > + > + rdmsrl(MSR_IA32_FEATURE_CONTROL, fc); > + if (!(fc & FEATURE_CONTROL_LOCKED)) > + return false; > + > + if (!(fc & FEATURE_CONTROL_SGX_ENABLE)) > + return false; Comments, please. Why would this happen? What would it mean?
On Mon, Jun 18, 2018 at 8:11 AM Dave Hansen <dave.hansen@intel.com> wrote: > > > +config INTEL_SGX_CORE > > + prompt "Intel SGX core functionality > > + depends on X86_64 && CPU_SUP_INTEL > > + help > > + Intel Software Guard eXtensions (SGX) is a set of CPU instructions > > + that allows ring 3 applications to create enclaves; private regions > > + of memory that are protected, by hardware, from unauthorized access > > + and/or modification. > > That semicolon needs to be a colon. The second half of that sentence is > not a stand-alone statement. > > > + This option enables kernel recognition of SGX, high-level management > > + of the Enclave Page Cache (EPC), tracking and writing of SGX Launch > > + Enclave Hash MSRs, and allows for virtualization of SGX via KVM. By > > + iteslf, this option does not provide SGX support to userspace. > > + > > + For details, see Documentation/x86/intel_sgx.rst > > + > > + If unsure, say N. > > + > > config EFI > > bool "EFI runtime service support" > > depends on ACPI > > diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h > > new file mode 100644 > > index 000000000000..fa3e6e0eb8af > > --- /dev/null > > +++ b/arch/x86/include/asm/sgx.h > > @@ -0,0 +1,25 @@ > > +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) > > +// Copyright(c) 2016-18 Intel Corporation. > > +// > > +// Authors: > > +// > > +// Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > +// Suresh Siddha <suresh.b.siddha@intel.com> > > +// Sean Christopherson <sean.j.christopherson@intel.com> > > + > > +#ifndef _ASM_X86_SGX_H > > +#define _ASM_X86_SGX_H > > + > > +#include <linux/types.h> > > + > > +#define SGX_CPUID 0x12 > > Hey, I just saw 0x12 as a magic, hard-coded number earlier in these > patches. It seems cruel to hard-code it, and then also have a #define > that isn't used. > > > +enum sgx_cpuid { > > + SGX_CPUID_CAPABILITIES = 0, > > + SGX_CPUID_ATTRIBUTES = 1, > > + SGX_CPUID_EPC_BANKS = 2, > > +}; > > These are cpuid *leaves*, right? Please make this clear that these are > hardware-defined values and not some kind of software construct. > > > +bool sgx_enabled __ro_after_init = false; > > +EXPORT_SYMBOL(sgx_enabled); > > + > > +static __init bool sgx_is_enabled(void) > > +{ > > + unsigned long fc; > > + > > + if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL) > > + return false; > > Not necessary. CPUID does this part for you. More to the point, if a non-Intel vendor chooses to support SGX, then the driver should allow it. > > > + if (!boot_cpu_has(X86_FEATURE_SGX)) > > + return false; > > + > > + if (!boot_cpu_has(X86_FEATURE_SGX1)) > > + return false; > > + > > + rdmsrl(MSR_IA32_FEATURE_CONTROL, fc); > > + if (!(fc & FEATURE_CONTROL_LOCKED)) > > + return false; > > + > > + if (!(fc & FEATURE_CONTROL_SGX_ENABLE)) > > + return false; > > Comments, please. Why would this happen? What would it mean? Let's add actual pr_info() statements to document this, like: SGX: disabled by firmware SGX: disabled because CPU does not support flexible launch control SGX: disabled because firmware does not support flexible launch control SGX: disabled because the phase of the moon is wrong SGX: enabled; using SGX1 SGX: enabled, using SGX2 If the CPU doesn't support SGX at all, then I see no reason to print anything. etc. (Is the feature actually called flexible launch control? I may have made that up.)
On Fri, Jun 08, 2018 at 10:36:14AM -0700, Dave Hansen wrote: > > +config INTEL_SGX_CORE > > + prompt "Intel SGX core functionality > > + depends on X86_64 && CPU_SUP_INTEL > > + help > > + Intel Software Guard eXtensions (SGX) is a set of CPU instructions > > + that allows ring 3 applications to create enclaves; private regions > > + of memory that are protected, by hardware, from unauthorized access > > + and/or modification. > > That semicolon needs to be a colon. The second half of that sentence is > not a stand-alone statement. > > > + This option enables kernel recognition of SGX, high-level management > > + of the Enclave Page Cache (EPC), tracking and writing of SGX Launch > > + Enclave Hash MSRs, and allows for virtualization of SGX via KVM. By > > + iteslf, this option does not provide SGX support to userspace. > > + > > + For details, see Documentation/x86/intel_sgx.rst > > + > > + If unsure, say N. > > + > > config EFI > > bool "EFI runtime service support" > > depends on ACPI > > diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h > > new file mode 100644 > > index 000000000000..fa3e6e0eb8af > > --- /dev/null > > +++ b/arch/x86/include/asm/sgx.h > > @@ -0,0 +1,25 @@ > > +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) > > +// Copyright(c) 2016-18 Intel Corporation. > > +// > > +// Authors: > > +// > > +// Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > +// Suresh Siddha <suresh.b.siddha@intel.com> > > +// Sean Christopherson <sean.j.christopherson@intel.com> > > + > > +#ifndef _ASM_X86_SGX_H > > +#define _ASM_X86_SGX_H > > + > > +#include <linux/types.h> > > + > > +#define SGX_CPUID 0x12 > > Hey, I just saw 0x12 as a magic, hard-coded number earlier in these > patches. It seems cruel to hard-code it, and then also have a #define > that isn't used. It i then is a regression in the series. I'll fix it. > > +enum sgx_cpuid { > > + SGX_CPUID_CAPABILITIES = 0, > > + SGX_CPUID_ATTRIBUTES = 1, > > + SGX_CPUID_EPC_BANKS = 2, > > +}; > > These are cpuid *leaves*, right? Please make this clear that these are > hardware-defined values and not some kind of software construct. > > > +bool sgx_enabled __ro_after_init = false; > > +EXPORT_SYMBOL(sgx_enabled); > > + > > +static __init bool sgx_is_enabled(void) > > +{ > > + unsigned long fc; > > + > > + if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL) > > + return false; > > Not necessary. CPUID does this part for you. > > > + if (!boot_cpu_has(X86_FEATURE_SGX)) > > + return false; > > + > > + if (!boot_cpu_has(X86_FEATURE_SGX1)) > > + return false; > > + > > + rdmsrl(MSR_IA32_FEATURE_CONTROL, fc); > > + if (!(fc & FEATURE_CONTROL_LOCKED)) > > + return false; > > + > > + if (!(fc & FEATURE_CONTROL_SGX_ENABLE)) > > + return false; > > Comments, please. Why would this happen? What would it mean? Thanks for the feedback! I'll refine the parts that you pointed out. /Jarkko
On Mon, Jun 11, 2018 at 07:35:21AM -0400, Neil Horman wrote: > On Fri, Jun 08, 2018 at 07:09:42PM +0200, Jarkko Sakkinen wrote: > > From: Sean Christopherson <sean.j.christopherson@intel.com> > > > > Intel(R) SGX is a set of CPU instructions that can be used by applications > > to set aside private regions of code and data. The code outside the enclave > > is disallowed to access the memory inside the enclave by the CPU access > > control. > > > > This commit adds the check for SGX to arch/x86 and a new config option, > > INTEL_SGX_CORE. Exposes a boolean variable 'sgx_enabled' to query whether > > or not the SGX support is available. > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > --- > > arch/x86/Kconfig | 19 ++++++++++++ > > arch/x86/include/asm/sgx.h | 25 ++++++++++++++++ > > arch/x86/include/asm/sgx_pr.h | 20 +++++++++++++ > > arch/x86/kernel/cpu/Makefile | 1 + > > arch/x86/kernel/cpu/intel_sgx.c | 53 +++++++++++++++++++++++++++++++++ > > 5 files changed, 118 insertions(+) > > create mode 100644 arch/x86/include/asm/sgx.h > > create mode 100644 arch/x86/include/asm/sgx_pr.h > > create mode 100644 arch/x86/kernel/cpu/intel_sgx.c > > > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > > index c07f492b871a..42015d5366ef 100644 > > --- a/arch/x86/Kconfig > > +++ b/arch/x86/Kconfig > > @@ -1925,6 +1925,25 @@ config X86_INTEL_MEMORY_PROTECTION_KEYS > > > > If unsure, say y. > > > > +config INTEL_SGX_CORE > > + prompt "Intel SGX core functionality" > > + def_bool n > > + depends on X86_64 && CPU_SUP_INTEL > > + help > > + Intel Software Guard eXtensions (SGX) is a set of CPU instructions > > + that allows ring 3 applications to create enclaves; private regions > > + of memory that are protected, by hardware, from unauthorized access > > + and/or modification. > > + > > + This option enables kernel recognition of SGX, high-level management > > + of the Enclave Page Cache (EPC), tracking and writing of SGX Launch > > + Enclave Hash MSRs, and allows for virtualization of SGX via KVM. By > > + iteslf, this option does not provide SGX support to userspace. > > + > > + For details, see Documentation/x86/intel_sgx.rst > > + > > + If unsure, say N. > > + > > config EFI > > bool "EFI runtime service support" > > depends on ACPI > > diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h > > new file mode 100644 > > index 000000000000..fa3e6e0eb8af > > --- /dev/null > > +++ b/arch/x86/include/asm/sgx.h > > @@ -0,0 +1,25 @@ > > +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) > > +// Copyright(c) 2016-18 Intel Corporation. > > +// > > +// Authors: > > +// > > +// Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > +// Suresh Siddha <suresh.b.siddha@intel.com> > > +// Sean Christopherson <sean.j.christopherson@intel.com> > > + > > +#ifndef _ASM_X86_SGX_H > > +#define _ASM_X86_SGX_H > > + > > +#include <linux/types.h> > > + > > +#define SGX_CPUID 0x12 > > + > Agree with Dave, this can just be remoed and you can use the feature macro from > cpuid.h instead > > Neil Will do, thanks. /Jarkko
On Mon, 2018-06-18 at 14:36 -0700, Andy Lutomirski wrote: > SGX: disabled by firmware > SGX: disabled because CPU does not support flexible launch control > SGX: disabled because firmware does not support flexible launch control > SGX: disabled because the phase of the moon is wrong > SGX: enabled; using SGX1 > SGX: enabled, using SGX2 Makes sense for trouble-shooting (and in that case the check for SGX_ENABLE flag is more appropriate). /Jarkko
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index c07f492b871a..42015d5366ef 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -1925,6 +1925,25 @@ config X86_INTEL_MEMORY_PROTECTION_KEYS If unsure, say y. +config INTEL_SGX_CORE + prompt "Intel SGX core functionality" + def_bool n + depends on X86_64 && CPU_SUP_INTEL + help + Intel Software Guard eXtensions (SGX) is a set of CPU instructions + that allows ring 3 applications to create enclaves; private regions + of memory that are protected, by hardware, from unauthorized access + and/or modification. + + This option enables kernel recognition of SGX, high-level management + of the Enclave Page Cache (EPC), tracking and writing of SGX Launch + Enclave Hash MSRs, and allows for virtualization of SGX via KVM. By + iteslf, this option does not provide SGX support to userspace. + + For details, see Documentation/x86/intel_sgx.rst + + If unsure, say N. + config EFI bool "EFI runtime service support" depends on ACPI diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h new file mode 100644 index 000000000000..fa3e6e0eb8af --- /dev/null +++ b/arch/x86/include/asm/sgx.h @@ -0,0 +1,25 @@ +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) +// Copyright(c) 2016-18 Intel Corporation. +// +// Authors: +// +// Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> +// Suresh Siddha <suresh.b.siddha@intel.com> +// Sean Christopherson <sean.j.christopherson@intel.com> + +#ifndef _ASM_X86_SGX_H +#define _ASM_X86_SGX_H + +#include <linux/types.h> + +#define SGX_CPUID 0x12 + +enum sgx_cpuid { + SGX_CPUID_CAPABILITIES = 0, + SGX_CPUID_ATTRIBUTES = 1, + SGX_CPUID_EPC_BANKS = 2, +}; + +extern bool sgx_enabled; + +#endif /* _ASM_X86_SGX_H */ diff --git a/arch/x86/include/asm/sgx_pr.h b/arch/x86/include/asm/sgx_pr.h new file mode 100644 index 000000000000..876dc44c2ccc --- /dev/null +++ b/arch/x86/include/asm/sgx_pr.h @@ -0,0 +1,20 @@ +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) +// Copyright(c) 2016-17 Intel Corporation. +// +// Authors: +// +// Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> +// Suresh Siddha <suresh.b.siddha@intel.com> +// Serge Ayoun <serge.ayoun@intel.com> +// Shay Katz-zamir <shay.katz-zamir@intel.com> + +#ifndef _ASM_X86_SGX_PR_H +#define _ASM_X86_SGX_PR_H + +#include <linux/printk.h> +#include <linux/ratelimit.h> + +#undef pr_fmt +#define pr_fmt(fmt) "intel_sgx: " fmt + +#endif /* _ASM_X86_SGX_PR_H */ diff --git a/arch/x86/kernel/cpu/Makefile b/arch/x86/kernel/cpu/Makefile index a66229f51b12..9552ff5b4ec3 100644 --- a/arch/x86/kernel/cpu/Makefile +++ b/arch/x86/kernel/cpu/Makefile @@ -36,6 +36,7 @@ obj-$(CONFIG_CPU_SUP_TRANSMETA_32) += transmeta.o obj-$(CONFIG_CPU_SUP_UMC_32) += umc.o obj-$(CONFIG_INTEL_RDT) += intel_rdt.o intel_rdt_rdtgroup.o intel_rdt_monitor.o intel_rdt_ctrlmondata.o +obj-$(CONFIG_INTEL_SGX_CORE) += intel_sgx.o obj-$(CONFIG_X86_MCE) += mcheck/ obj-$(CONFIG_MTRR) += mtrr/ diff --git a/arch/x86/kernel/cpu/intel_sgx.c b/arch/x86/kernel/cpu/intel_sgx.c new file mode 100644 index 000000000000..db6b315334f4 --- /dev/null +++ b/arch/x86/kernel/cpu/intel_sgx.c @@ -0,0 +1,53 @@ +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) +// Copyright(c) 2016-17 Intel Corporation. +// +// Authors: +// +// Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> +// Suresh Siddha <suresh.b.siddha@intel.com> +// Serge Ayoun <serge.ayoun@intel.com> +// Shay Katz-zamir <shay.katz-zamir@intel.com> +// Sean Christopherson <sean.j.christopherson@intel.com> + +#include <asm/sgx.h> +#include <asm/sgx_pr.h> +#include <linux/freezer.h> +#include <linux/highmem.h> +#include <linux/kthread.h> +#include <linux/ratelimit.h> +#include <linux/sched/signal.h> +#include <linux/slab.h> + +bool sgx_enabled __ro_after_init = false; +EXPORT_SYMBOL(sgx_enabled); + +static __init bool sgx_is_enabled(void) +{ + unsigned long fc; + + if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL) + return false; + + if (!boot_cpu_has(X86_FEATURE_SGX)) + return false; + + if (!boot_cpu_has(X86_FEATURE_SGX1)) + return false; + + rdmsrl(MSR_IA32_FEATURE_CONTROL, fc); + if (!(fc & FEATURE_CONTROL_LOCKED)) + return false; + + if (!(fc & FEATURE_CONTROL_SGX_ENABLE)) + return false; + + return true; +} + +static __init int sgx_init(void) +{ + sgx_enabled = sgx_is_enabled(); + return 0; +} + +arch_initcall(sgx_init);