Re: [PATCH v22 04/24] x86/cpu/intel: Detect SGX supprt

[Sean Christopherson]

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.