linux-kernel - Re: [PATCH v19,RESEND 08/27] x86/cpu/intel: Detect SGX support and update caps appropriately

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]

Date:   Tue, 26 Mar 2019 07:27:19 -0700
From:   Sean Christopherson <sean.j.christopherson@...el.com>
To:     "Huang, Kai" <kai.huang@...el.com>
Cc:     "jarkko.sakkinen@...ux.intel.com" <jarkko.sakkinen@...ux.intel.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-sgx@...r.kernel.org" <linux-sgx@...r.kernel.org>,
        "x86@...nel.org" <x86@...nel.org>,
        "Svahn, Kai" <kai.svahn@...el.com>,
        "nhorman@...hat.com" <nhorman@...hat.com>,
        "josh@...htriplett.org" <josh@...htriplett.org>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "Ayoun, Serge" <serge.ayoun@...el.com>,
        "Huang, Haitao" <haitao.huang@...el.com>,
        "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
        "npmccallum@...hat.com" <npmccallum@...hat.com>,
        "rientjes@...gle.com" <rientjes@...gle.com>,
        "luto@...nel.org" <luto@...nel.org>,
        "Katz-zamir, Shay" <shay.katz-zamir@...el.com>,
        "Hansen, Dave" <dave.hansen@...el.com>,
        "bp@...en8.de" <bp@...en8.de>,
        "andriy.shevchenko@...ux.intel.com" 
        <andriy.shevchenko@...ux.intel.com>
Subject: Re: [PATCH v19,RESEND 08/27] x86/cpu/intel: Detect SGX support and
 update caps appropriately

On Tue, Mar 26, 2019 at 05:17:40AM -0700, Huang, Kai wrote:
> On Wed, 2019-03-20 at 18:21 +0200, Jarkko Sakkinen wrote:
> > From: Sean Christopherson <sean.j.christopherson@...el.com>
> > 
> > Similar to other large Intel features such as VMX and TXT, SGX must be
> > explicitly enabled in IA32_FEATURE_CONTROL MSR to be truly usable.
> > Clear all SGX related capabilities if SGX is not fully enabled in
> > IA32_FEATURE_CONTROL or if the SGX1 instruction set isn't supported
> > (impossible on bare metal, theoretically possible in a VM if the VMM is
> > doing something weird).
> > 
> > Like SGX itself, SGX Launch Control must be explicitly enabled via a
> > flag in IA32_FEATURE_CONTROL. Clear the SGX_LC capability if Launch
> > Control is not fully enabled (or obviously if SGX itself is disabled).
> > 
> > Note that clearing X86_FEATURE_SGX_LC creates a bit of a conundrum
> > regarding the SGXLEPUBKEYHASH MSRs, as it may be desirable to read the
> > MSRs even if they are not writable, e.g. to query the configured key,
> > but clearing the capability leaves no breadcrum for discerning whether
> > or not the MSRs exist.  But, such usage will be rare (KVM is the only
> > known case at this time) and not performance critical, so it's not
> > unreasonable to require the use of rdmsr_safe().  Clearing the cap bit
> > eliminates the need for an additional flag to track whether or not
> > Launch Control is truly enabled, which is what we care about the vast
> > majority of the time.
> 
> [Resend. Somehow my last reply doesn't show up in my mailbox so not sure whether I sent it
> successfully or not. Sorry if you receving duplicated mails.]
> 
> However this is not consistent with HW behavior. If LC feature flag is not present, then MSRs should
> have hash of Intel's key, which is not always the case here, when you expose SGX to KVM. Enclave in
> KVM guest will get unexpected EINIT error when launing Intel enclave, if on HW MSRs are configured
> to 3rd party value but locked to readonly.

Intel doesn't have a singular key.  The internal reset value of the LE
pubkey hash MSRs is micro-architectural, i.e. can change without warning
on any given processor.  All current processors with SGX support may use
the same reset value, but it's not something that customers should/can
rely on.

That being said, this in no way impacts KVM's ability to virtualize SGX,
e.g. KVM can directly do CPUID and {RD,WR}MSR to probe the capabilities
of the platform as needed.

> My opition is we already have enough cases that violates HW behavior in
> SGX virtualization, let's not have one more.

What are the other cases?

> Besides, why do we "need an additional flag to track whether or not
> Launch Control is truly enabled"? Doesn't driver only need to know whether
> MSRs are writable?

Yes, and that's why we're overloading X86_FEATURE_SGX_LC to be set if
and only if SGX_LC is supported *and* enabled, e.g. so that the kernel
can simply check X86_FEATURE_SGX_LC without having to also probe the MSRs.