Re: [PATCH v6 04/11] x86: define IA32_FEATUE_CONTROL.SGX_LC

[Sean Christopherson]

On Tue, 2017-11-28 at 09:16 -0800, Sean Christopherson wrote:
> On Sat, 2017-11-25 at 21:29 +0200, Jarkko Sakkinen wrote:
> > 
> > When IA32_FEATURE_CONTROL.SGX_LC identifies that the root key for
> > enclave signatures can be configured either by the OS or pre-boot
> > firmware.
> > 
> > If this the case, IA32_SGXLEPUBKEYHASHn MSRs (0 < n < 4) can be used
> > to
> > set the SHA256 of the root key. IA32_FEATURE_CONTROL bit 17 controls
> > whether the MSRs are writable by the OS. The pre-boot firmware can
> > decided whether toÂÂset this bit before setting
> > IA32_FEATURE_CONTROL.LOCK.
> The commit message (feature control bit) doesn't match the patch (CPUID
> bit).

Also, assuming this message is destined for the commit that adds SGX_LC
to feature control, I think it should first and foremost describe the
hardware behavior. ÂThe firmware vs OS interaction and use cases are
valuable to document but IMO should come after the hardware description.

And though it's not documented in the SDM, I think it's worthwhile to
describe the SGX activation sequence and its relationship with the SGX
MSRs, e.g. the LE hash MSRs are writable prior to SGX activation.
Without that information, it's unclear as to how the LE hash MSRs could
be different than Intel's reset value.

So, maybe something like this?

  After SGX is activated[1] the IA32_SGXLEPUBKEYHASHn MSRs are writable
  if and only if SGX_LC is set in the IA32_FEATURE_CONTROL MSR and the
  IA32_FEATURE_CONTROL MSR is locked, otherwise they are read-only.

  For example, firmware can allow the OS to change the launch enclave
  root key by setting IA32_FEATURE_CONTROL.SGX_LC, and thus give the
  OS complete control over the enclaves it runs. ÂAlternatively,
  firmware can clear IA32_FEATURE_CONTROL.SGX_LC to lock down the root
  key and restrict the OS to running enclaves signed with the root key
  or whitelisted/trusted by a launch enclave (which must be signed with
  the root key).

  [1] SGX related bits in IA32_FEATURE_CONTROL cannot be set until SGX
    is activated, e.g. by firmware. ÂSGX activation is triggered by
    setting bit 0 in MSR 0x7a. ÂUntil SGX is activated, the LE hash
    MSRs are writable, e.g. to allow firmware to lock down the LE
    root key with a non-Intel value.

> > 
> > 
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@xxxxxxxxxxxxxxx>
> > ---
> > Âarch/x86/include/asm/cpufeatures.h | 1 +
> > Â1 file changed, 1 insertion(+)
> > 
> > diff --git a/arch/x86/include/asm/cpufeatures.h
> > b/arch/x86/include/asm/cpufeatures.h
> > index 31a7d1c0f204..43130f3c18a1 100644
> > --- a/arch/x86/include/asm/cpufeatures.h
> > +++ b/arch/x86/include/asm/cpufeatures.h
> > @@ -298,6 +298,7 @@
> > Â#define X86_FEATURE_AVX512_VPOPCNTDQ (16*32+14) /* POPCNT for vectors
> > of DW/QW */
> > Â#define X86_FEATURE_LA57	(16*32+16) /* 5-level page tables */
> > Â#define X86_FEATURE_RDPID	(16*32+22) /* RDPID instruction */
> > +#define X86_FEATURE_SGX_LC	(16*32+30) /* supports SGX launch
> > configuration */
> > Â
> > Â/* AMD-defined CPU features, CPUID level 0x80000007 (ebx), word 17 */
> > Â#define X86_FEATURE_OVERFLOW_RECOV (17*32+0) /* MCA overflow recovery
> > support */