linux-kernel - Re: [PATCH v6 04/11] x86: define IA32_FEATUE_CONTROL.SGX

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1511893683.9392.100.camel@intel.com>
Date:   Tue, 28 Nov 2017 10:28:03 -0800
From:   Sean Christopherson <sean.j.christopherson@...el.com>
To:     Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>,
        platform-driver-x86@...r.kernel.org, x86@...nel.org
Cc:     linux-kernel@...r.kernel.org, Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>, Borislav Petkov <bp@...e.de>,
        Janakarajan Natarajan <Janakarajan.Natarajan@....com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
        Kyle Huey <me@...ehuey.com>,
        Vikas Shivappa <vikas.shivappa@...ux.intel.com>,
        Piotr Luc <piotr.luc@...el.com>,
        Grzegorz Andrejczuk <grzegorz.andrejczuk@...el.com>
Subject: Re: [PATCH v6 04/11] x86: define IA32_FEATUE_CONTROL.SGX_LC

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@...ux.intel.com>
> > ---
> >  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 */