| Message ID | 20190903142655.21943-5-jarkko.sakkinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Intel SGX foundations | expand |
On Tue, Sep 03, 2019 at 05:26:35PM +0300, 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. > > 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 | 39 +++++++++++++++++++++++++++++++++++++ > 1 file changed, 39 insertions(+) > > diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c > index 8d6d92ebeb54..777ea63b4f85 100644 > --- a/arch/x86/kernel/cpu/intel.c > +++ b/arch/x86/kernel/cpu/intel.c > @@ -623,6 +623,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; > @@ -760,6 +796,9 @@ static void init_intel(struct cpuinfo_x86 *c) > if (cpu_has(c, X86_FEATURE_TME)) > detect_tme(c); > > + if (IS_ENABLED(CONFIG_INTEL_SGX) && cpu_has(c, X86_FEATURE_SGX)) > + detect_sgx(c); Looks to me like this should run only once on the BSP instead of on every CPU. The pr_*_once things above are a good sign for that, I'd say. If so, define your own ->c_bsp_init function and run that from there instead.
On Tue, Sep 24, 2019 at 06:13:01PM +0200, Borislav Petkov wrote: > On Tue, Sep 03, 2019 at 05:26:35PM +0300, Jarkko Sakkinen wrote: > > +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"); This should be pr_err_once. > > + 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; > > @@ -760,6 +796,9 @@ static void init_intel(struct cpuinfo_x86 *c) > > if (cpu_has(c, X86_FEATURE_TME)) > > detect_tme(c); > > > > + if (IS_ENABLED(CONFIG_INTEL_SGX) && cpu_has(c, X86_FEATURE_SGX)) > > + detect_sgx(c); > > Looks to me like this should run only once on the BSP instead of on > every CPU. The pr_*_once things above are a good sign for that, I'd say. > > If so, define your own ->c_bsp_init function and run that from there > instead. The intent of running on every CPU is to verify MSR_IA32_FEATURE_CONTROL is correctly configured on all CPUs. It's extremely unlikely that firmware would misconfigure or fail to write the MSR on only APs, but if that does happen we'll spam dmesg and possibly panic or hang the kernel. The severity of the fallout is why we're being paranoid. KVM is similarly paranoid about VMX enabling since it'll BUG() on an unexpected fault due to a misconfigured FEATURE_CONTROL.
On Tue, Sep 24, 2019 at 10:43:11AM -0700, Sean Christopherson wrote: > The intent of running on every CPU is to verify MSR_IA32_FEATURE_CONTROL > is correctly configured on all CPUs. It's extremely unlikely that > firmware would misconfigure or fail to write the MSR on only APs, but if > that does happen we'll spam dmesg and possibly panic or hang the kernel. > > The severity of the fallout is why we're being paranoid. KVM is similarly > paranoid about VMX enabling since it'll BUG() on an unexpected fault due > to a misconfigured FEATURE_CONTROL. None of that is in the commit message or written anywhere AFAICT. And my crystal ball doesn't show it either so please write down properly why this is needed. Better over the function as a comment I'd say. Thx.
On Tue, Sep 24, 2019 at 08:21:19PM +0200, Borislav Petkov wrote: > On Tue, Sep 24, 2019 at 10:43:11AM -0700, Sean Christopherson wrote: > > The intent of running on every CPU is to verify MSR_IA32_FEATURE_CONTROL > > is correctly configured on all CPUs. It's extremely unlikely that > > firmware would misconfigure or fail to write the MSR on only APs, but if > > that does happen we'll spam dmesg and possibly panic or hang the kernel. > > > > The severity of the fallout is why we're being paranoid. KVM is similarly > > paranoid about VMX enabling since it'll BUG() on an unexpected fault due > > to a misconfigured FEATURE_CONTROL. > > None of that is in the commit message or written anywhere AFAICT. And my > crystal ball doesn't show it either so please write down properly why > this is needed. Better over the function as a comment I'd say. Added a remark: 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. I think here is not necessary to go into KVM implementation details to make a case for this one. This is just a sane contract/expectation for anything using SGX and thus it is better to validate it before anything gets to use it. /Jarkko
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c index 8d6d92ebeb54..777ea63b4f85 100644 --- a/arch/x86/kernel/cpu/intel.c +++ b/arch/x86/kernel/cpu/intel.c @@ -623,6 +623,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; @@ -760,6 +796,9 @@ static void init_intel(struct cpuinfo_x86 *c) if (cpu_has(c, X86_FEATURE_TME)) detect_tme(c); + if (IS_ENABLED(CONFIG_INTEL_SGX) && cpu_has(c, X86_FEATURE_SGX)) + detect_sgx(c); + init_intel_misc_features(c); }