| Message ID | 20180827185507.17087-7-jarkko.sakkinen@linux.intel.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | Intel SGX1 support | expand |
> +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 is a bit comma-crazy. Also, considering some of our recent CVE fun, I'd probably not claim hardware protection. :) Maybe: Intel Software Guard eXtensions (SGX) CPU feature that allows ring 3 applications to create enclaves: private regions of memory that are architecturally protected 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. itself > diff --git a/arch/x86/include/asm/sgx_pr.h b/arch/x86/include/asm/sgx_pr.h > new file mode 100644 > index 000000000000..c68578127620 > --- /dev/null > +++ b/arch/x86/include/asm/sgx_pr.h > @@ -0,0 +1,13 @@ > +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) > +// Copyright(c) 2016-17 Intel Corporation. > + > +#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) "sgx: " fmt > + > +#endif /* _ASM_X86_SGX_PR_H */ I don't think this belongs in a generic header. Generally, we do the pr_fmt stuff in .c files, not in headers. If someone includes this header directly or indirectly, they'll get a big surprise. If you *must* have this in a .h file, put it in arch/x86/kernel/cpu/intel_sgx.h or something and #include "intel_sgx.h" in all the .c files where you want this. > +static __init int sgx_init(void) > +{ > + unsigned long fc; > + > + 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)) { > + pr_info("IA32_FEATURE_CONTROL MSR is not locked\n"); > + return false; > + } This is a rather crummy error message. Doesn't this keep sgx from initializing? Would something like this be more informative? pr_info("failed init: IA32_FEATURE_CONTROL MSR not locked\n"); > + if (!(fc & FEATURE_CONTROL_SGX_ENABLE)) { > + pr_info("disabled by the firmware\n"); > + return false; > + } > + > + if (!(fc & FEATURE_CONTROL_SGX_LE_WR)) > + pr_info("IA32_SGXLEPUBKEYHASHn MSRs are not writable\n"); How about something that might help an end user? Perhaps: pr_warn("launch configuration not available\n"); > + sgx_enabled = true; > + sgx_lc_enabled = !!(fc & FEATURE_CONTROL_SGX_LE_WR); > + return 0; > +} > + > +arch_initcall(sgx_init); >
On Mon, Aug 27, 2018 at 12:53:59PM -0700, Dave Hansen wrote: > > +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 is a bit comma-crazy. Also, considering some of our recent CVE > fun, I'd probably not claim hardware protection. :) Agreed :) > Maybe: > > Intel Software Guard eXtensions (SGX) CPU feature that allows > ring 3 applications to create enclaves: private regions > of memory that are architecturally protected from unauthorized > access and/or modification. Yeah, looks way more better structured. > > + 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. > > itself > > > > diff --git a/arch/x86/include/asm/sgx_pr.h b/arch/x86/include/asm/sgx_pr.h > > new file mode 100644 > > index 000000000000..c68578127620 > > --- /dev/null > > +++ b/arch/x86/include/asm/sgx_pr.h > > @@ -0,0 +1,13 @@ > > +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) > > +// Copyright(c) 2016-17 Intel Corporation. > > + > > +#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) "sgx: " fmt > > + > > +#endif /* _ASM_X86_SGX_PR_H */ > > I don't think this belongs in a generic header. Generally, we do the > pr_fmt stuff in .c files, not in headers. If someone includes this > header directly or indirectly, they'll get a big surprise. > > If you *must* have this in a .h file, put it in > arch/x86/kernel/cpu/intel_sgx.h or something and #include "intel_sgx.h" > in all the .c files where you want this. I think for intel_sgx.c (the core part) we could just manually add the "sgx:" prefix because there are only few log messages. I would move the definition to drivers/platform/x86/intel_sgx/sgx.h because the prefix makes sense for all .c files there AFAIK. > > +static __init int sgx_init(void) > > +{ > > + unsigned long fc; > > + > > + 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)) { > > + pr_info("IA32_FEATURE_CONTROL MSR is not locked\n"); > > + return false; > > + } > > This is a rather crummy error message. Doesn't this keep sgx from > initializing? Would something like this be more informative? > > pr_info("failed init: IA32_FEATURE_CONTROL MSR not locked\n"); What about: pr_err(FW_BUG "IA32_FEATURE_CONTROL MSR not locked\n"); > > + if (!(fc & FEATURE_CONTROL_SGX_ENABLE)) { > > + pr_info("disabled by the firmware\n"); > > + return false; > > + } > > + > > + if (!(fc & FEATURE_CONTROL_SGX_LE_WR)) > > + pr_info("IA32_SGXLEPUBKEYHASHn MSRs are not writable\n"); > > How about something that might help an end user? Perhaps: > > pr_warn("launch configuration not available\n"); I think this message is a false flag here in the first place as KVM does not require writable MSRs. It really should be moved to the driver. > > + sgx_enabled = true; > > + sgx_lc_enabled = !!(fc & FEATURE_CONTROL_SGX_LE_WR); > > + return 0; > > +} > > + > > +arch_initcall(sgx_init); > > > > /Jarkko
On Mon, Aug 27, 2018 at 9:57 PM Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> 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. > > Add a check for SGX to arch/x86 and a new config option, INTEL_SGX_CORE. > Expose a boolean variable 'sgx_enabled' to query whether or not the SGX > support is available. > + prompt "Intel SGX core functionality" > + def_bool n Default 'default' is 'n'. Perhaps changing prompt to bool will make it one line less. > +#include <asm/sgx.h> > +#include <asm/sgx_pr.h> Don't we put linux/* followed by asm/* ? > +#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>
On Mon, Sep 03, 2018 at 05:26:43PM +0300, Andy Shevchenko wrote: > On Mon, Aug 27, 2018 at 9:57 PM Jarkko Sakkinen > <jarkko.sakkinen@linux.intel.com> 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. > > > > Add a check for SGX to arch/x86 and a new config option, INTEL_SGX_CORE. > > Expose a boolean variable 'sgx_enabled' to query whether or not the SGX > > support is available. > > > + prompt "Intel SGX core functionality" > > + def_bool n > > Default 'default' is 'n'. Perhaps changing prompt to bool will make it > one line less. Sure! > > +#include <asm/sgx.h> > > +#include <asm/sgx_pr.h> > > Don't we put linux/* followed by asm/* ? Yes. > > > +#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> > > -- > With Best Regards, > Andy Shevchenko /Jarkko
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index b0312f8947ce..3c7571422a07 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -1911,6 +1911,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..2130e639ab49 --- /dev/null +++ b/arch/x86/include/asm/sgx.h @@ -0,0 +1,12 @@ +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) +// Copyright(c) 2016-18 Intel Corporation. + +#ifndef _ASM_X86_SGX_H +#define _ASM_X86_SGX_H + +#include <linux/types.h> + +extern bool sgx_enabled; +extern bool sgx_lc_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..c68578127620 --- /dev/null +++ b/arch/x86/include/asm/sgx_pr.h @@ -0,0 +1,13 @@ +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) +// Copyright(c) 2016-17 Intel Corporation. + +#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) "sgx: " fmt + +#endif /* _ASM_X86_SGX_PR_H */ diff --git a/arch/x86/kernel/cpu/Makefile b/arch/x86/kernel/cpu/Makefile index 347137e80bf5..71876f2b35fc 100644 --- a/arch/x86/kernel/cpu/Makefile +++ b/arch/x86/kernel/cpu/Makefile @@ -38,6 +38,7 @@ obj-$(CONFIG_CPU_SUP_UMC_32) += umc.o obj-$(CONFIG_INTEL_RDT) += intel_rdt.o intel_rdt_rdtgroup.o intel_rdt_monitor.o obj-$(CONFIG_INTEL_RDT) += intel_rdt_ctrlmondata.o intel_rdt_pseudo_lock.o CFLAGS_intel_rdt_pseudo_lock.o = -I$(src) +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..17b46bec9c54 --- /dev/null +++ b/arch/x86/kernel/cpu/intel_sgx.c @@ -0,0 +1,47 @@ +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) +// Copyright(c) 2016-17 Intel Corporation. + +#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; +EXPORT_SYMBOL_GPL(sgx_enabled); +bool sgx_lc_enabled __ro_after_init; +EXPORT_SYMBOL_GPL(sgx_lc_enabled); + +static __init int sgx_init(void) +{ + unsigned long fc; + + 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)) { + pr_info("IA32_FEATURE_CONTROL MSR is not locked\n"); + return false; + } + + if (!(fc & FEATURE_CONTROL_SGX_ENABLE)) { + pr_info("disabled by the firmware\n"); + return false; + } + + if (!(fc & FEATURE_CONTROL_SGX_LE_WR)) + pr_info("IA32_SGXLEPUBKEYHASHn MSRs are not writable\n"); + + sgx_enabled = true; + sgx_lc_enabled = !!(fc & FEATURE_CONTROL_SGX_LE_WR); + return 0; +} + +arch_initcall(sgx_init);