[v22,02/24] x86/cpufeatures: x86/msr: Intel SGX Launch Control hardware bits
diff mbox series

Message ID 20190903142655.21943-3-jarkko.sakkinen@linux.intel.com
State New
Headers show
Series
  • Intel SGX foundations
Related show

Commit Message

Jarkko Sakkinen Sept. 3, 2019, 2:26 p.m. UTC
From: Kai Huang <kai.huang@linux.intel.com>

Add X86_FEATURE_SGX_LC, which informs whether or not the CPU supports SGX
Launch Control.

Add MSR_IA32_SGXLEPUBKEYHASH{0, 1, 2, 3}, which when combined contain a
SHA256 hash of a 3072-bit RSA public key. SGX backed software packages, so
called enclaves, are always signed. All enclaves signed with the public key
are unconditionally allowed to initialize. [1]

Add FEATURE_CONTROL_SGX_LE_WR bit of the feature control MSR, which informs
whether the formentioned MSRs are writable or not. If the bit is off, the
public key MSRs are read-only for the OS.

If the MSRs are read-only, the platform must provide a launch enclave (LE).
LE can create cryptographic tokens for other enclaves that they can pass
together with their signature to the ENCLS(EINIT) opcode, which is used
to initialize enclaves.

Linux is unlikely to support the locked configuration because it takes away
the control of the launch decisions from the kernel.

[1] Intel SDM: 38.1.4 Intel SGX Launch Control Configuration

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
Co-developed-by: Haim Cohen <haim.cohen@intel.com>
Signed-off-by: Haim Cohen <haim.cohen@intel.com>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 arch/x86/include/asm/cpufeatures.h | 1 +
 arch/x86/include/asm/msr-index.h   | 7 +++++++
 2 files changed, 8 insertions(+)

Comments

Borislav Petkov Sept. 24, 2019, 3:52 p.m. UTC | #1
On Tue, Sep 03, 2019 at 05:26:33PM +0300, Jarkko Sakkinen wrote:
> From: Kai Huang <kai.huang@linux.intel.com>
> 
> Add X86_FEATURE_SGX_LC, which informs whether or not the CPU supports SGX
> Launch Control.
> 
> Add MSR_IA32_SGXLEPUBKEYHASH{0, 1, 2, 3}, which when combined contain a
> SHA256 hash of a 3072-bit RSA public key. SGX backed software packages, so
> called enclaves, are always signed. All enclaves signed with the public key
> are unconditionally allowed to initialize. [1]
> 
> Add FEATURE_CONTROL_SGX_LE_WR bit of the feature control MSR, which informs
> whether the formentioned MSRs are writable or not. If the bit is off, the
> public key MSRs are read-only for the OS.
> 
> If the MSRs are read-only, the platform must provide a launch enclave (LE).
> LE can create cryptographic tokens for other enclaves that they can pass
> together with their signature to the ENCLS(EINIT) opcode, which is used
> to initialize enclaves.
> 
> Linux is unlikely to support the locked configuration because it takes away
> the control of the launch decisions from the kernel.

Right, who has control over FEATURE_CONTROL_SGX_LE_WR? Can the
kernel set it and put another hash in there or there will be locked
configurations where setting that bit will trap?

I don't want to leave anything in the hands of the BIOS controlling
whether the platform can set its own key because BIOS is known to f*ck
it up almost every time. And so I'd like for us to be able to fix up
things without depending on the mood of some OEM vendor's BIOS fixing
desire.

> [1] Intel SDM: 38.1.4 Intel SGX Launch Control Configuration
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> Co-developed-by: Haim Cohen <haim.cohen@intel.com>
> Signed-off-by: Haim Cohen <haim.cohen@intel.com>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

This time checkpatch is right:

WARNING: Missing Signed-off-by: line by nominal patch author 'Kai Huang <kai.huang@linux.intel.com>'

And looking at the SOB chain, sounds like people need to make up their
mind about authorship...

> ---
>  arch/x86/include/asm/cpufeatures.h | 1 +
>  arch/x86/include/asm/msr-index.h   | 7 +++++++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> index c5582e766121..ca82226e25ec 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -355,6 +355,7 @@
>  #define X86_FEATURE_CLDEMOTE		(16*32+25) /* CLDEMOTE instruction */
>  #define X86_FEATURE_MOVDIRI		(16*32+27) /* MOVDIRI instruction */
>  #define X86_FEATURE_MOVDIR64B		(16*32+28) /* MOVDIR64B instruction */
> +#define X86_FEATURE_SGX_LC		(16*32+30) /* Software Guard Extensions Launch Control */

Amazing. SGX feature bits are spread around at least three CPUID leafs:

7_EBX, 7_ECX, 12_EAX. Maybe there's a 4th somewhere because hey... :-\
Sean Christopherson Sept. 24, 2019, 8:22 p.m. UTC | #2
On Tue, Sep 24, 2019 at 05:52:32PM +0200, Borislav Petkov wrote:
> On Tue, Sep 03, 2019 at 05:26:33PM +0300, Jarkko Sakkinen wrote:
> > From: Kai Huang <kai.huang@linux.intel.com>
> > 
> > Add X86_FEATURE_SGX_LC, which informs whether or not the CPU supports SGX
> > Launch Control.
> > 
> > Add MSR_IA32_SGXLEPUBKEYHASH{0, 1, 2, 3}, which when combined contain a
> > SHA256 hash of a 3072-bit RSA public key. SGX backed software packages, so
> > called enclaves, are always signed. All enclaves signed with the public key
> > are unconditionally allowed to initialize. [1]
> > 
> > Add FEATURE_CONTROL_SGX_LE_WR bit of the feature control MSR, which informs
> > whether the formentioned MSRs are writable or not. If the bit is off, the
> > public key MSRs are read-only for the OS.
> > 
> > If the MSRs are read-only, the platform must provide a launch enclave (LE).
> > LE can create cryptographic tokens for other enclaves that they can pass
> > together with their signature to the ENCLS(EINIT) opcode, which is used
> > to initialize enclaves.
> > 
> > Linux is unlikely to support the locked configuration because it takes away
> > the control of the launch decisions from the kernel.
> 
> Right, who has control over FEATURE_CONTROL_SGX_LE_WR? Can the
> kernel set it and put another hash in there or there will be locked
> configurations where setting that bit will trap?

Short answer, BIOS controls SGX_LE_WR.

The approach we chose (patch 04, which we were discussing) is to disable
SGX if SGX_LE_WR is not set, i.e. disallow SGX unless the hash MSRs exist
and are fully writable.

WRMSR will #GP if FEATURE_CONTROL is locked (bit 0), e.g. attempting to
set SGX_LE_WR will trap if FEATURE_CONTROL was locked by BIOS.  And
conversely, the various enable bits in FEATURE_CONTROL don't take effect
until FEATURE_CONTROL is locked, e.g. the LE hash MSRs aren't writable if
FEATURE_CONTROL is unlocked, regardless of whether SGX_LE_WR is set.

> I don't want to leave anything in the hands of the BIOS controlling
> whether the platform can set its own key because BIOS is known to f*ck
> it up almost every time. And so I'd like for us to be able to fix up
> things without depending on the mood of some OEM vendor's BIOS fixing
> desire.

Sadly, because FEATURE_CONTROL must be locked to fully enable SGX, the
reality is that any BIOS that supports SGX will lock FEATURE_CONTROL.

That's the status quo today as well since VMX (and SMX/TXT) is also
enabled via FEATURE_CONTROL.  KVM does have logic to enable VMX and lock
FEATURE_CONTROL if the MSR isn't locked, but AIUI that exists only to work
with old BIOSes.

If we want to support setting and locking FEATURE_CONTROL in the extremely
unlikely scenario that BIOS left it unlocked, the proper change would be
to move the existing KVM FEATURE_CONTROL logic into the early-ish boot
flow and try to set all known bits before locking FEATURE_CONTROL.  I
don't have a strong preference either way.  We opted not to try and set
FEATURE_CONTROL as we felt that doing so was more likely to cause breakage
than it was to actually "fix" a broken BIOS.

> > [1] Intel SDM: 38.1.4 Intel SGX Launch Control Configuration

One note on Launch Control that isn't covered in the SDM: the LE hash
MSRs can also be written before SGX is activated.  SGX activation must
occur before FEATURE_CONTROL is locked, meaning BIOS can set the LE
hash MSRs to a non-intel and then lock FEATURE_CONTROL with SGX_LE_WR=0.

There's a blurb on SGX activation in the kernel docs (patch 23).

> > diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> > index c5582e766121..ca82226e25ec 100644
> > --- a/arch/x86/include/asm/cpufeatures.h
> > +++ b/arch/x86/include/asm/cpufeatures.h
> > @@ -355,6 +355,7 @@
> >  #define X86_FEATURE_CLDEMOTE		(16*32+25) /* CLDEMOTE instruction */
> >  #define X86_FEATURE_MOVDIRI		(16*32+27) /* MOVDIRI instruction */
> >  #define X86_FEATURE_MOVDIR64B		(16*32+28) /* MOVDIR64B instruction */
> > +#define X86_FEATURE_SGX_LC		(16*32+30) /* Software Guard Extensions Launch Control */
> 
> Amazing. SGX feature bits are spread around at least three CPUID leafs:
> 
> 7_EBX, 7_ECX, 12_EAX. Maybe there's a 4th somewhere because hey... :-\

Heh, why stop at 4?  12_EBX, 12_1_ECX and 12_1_EDX are effectively feature
leafs as well, although the kernel can ignore them for the most part.
Borislav Petkov Sept. 25, 2019, 8:51 a.m. UTC | #3
On Tue, Sep 24, 2019 at 01:22:10PM -0700, Sean Christopherson wrote:
> The approach we chose (patch 04, which we were discussing) is to disable
> SGX if SGX_LE_WR is not set, i.e. disallow SGX unless the hash MSRs exist
> and are fully writable.

Hmm, so I see

+       if (!(fc & FEATURE_CONTROL_SGX_LE_WR)) {
+               pr_info_once("sgx: The launch control MSRs are not writable\n");
+               goto err_msrs_rdonly;

which clears only X86_FEATURE_SGX_LC but leaves the other three feature
bits set?!

If you'd want to disable SGX then you'd need to jump to the
err_unsupported label and get rid of the err_msrs_rdonly one.

Or am I missing something?

> WRMSR will #GP if FEATURE_CONTROL is locked (bit 0), e.g. attempting to
> set SGX_LE_WR will trap if FEATURE_CONTROL was locked by BIOS.

Right.

> And conversely, the various enable bits in FEATURE_CONTROL don't
> take effect until FEATURE_CONTROL is locked, e.g. the LE hash MSRs
> aren't writable if FEATURE_CONTROL is unlocked, regardless of whether
> SGX_LE_WR is set.

Ok. We want them writable.

> Sadly, because FEATURE_CONTROL must be locked to fully enable SGX, the
> reality is that any BIOS that supports SGX will lock FEATURE_CONTROL.

That's fine. The question is, would OEMs leave the hash MSRs writable?

If, as you say above, we clear SGX feature bit - not only
X86_FEATURE_SGX_LC - when the MSRs are not writable, then we're fine.
Platforms sticking their own hash in there won't be supported but I
guess their aim is not to be supported in Linux anyway.

> That's the status quo today as well since VMX (and SMX/TXT) is also
> enabled via FEATURE_CONTROL.  KVM does have logic to enable VMX and lock
> FEATURE_CONTROL if the MSR isn't locked, but AIUI that exists only to work
> with old BIOSes.
> 
> If we want to support setting and locking FEATURE_CONTROL in the extremely
> unlikely scenario that BIOS left it unlocked, the proper change would be

I wouldn't be too surprised if this happened. BIOS is very inventive.

> One note on Launch Control that isn't covered in the SDM: the LE hash
> MSRs can also be written before SGX is activated.  SGX activation must
> occur before FEATURE_CONTROL is locked, meaning BIOS can set the LE
> hash MSRs to a non-intel and then lock FEATURE_CONTROL with SGX_LE_WR=0.

This is exactly what I'm afraid of. The OEM vendors locking this down.

> Heh, why stop at 4?  12_EBX, 12_1_ECX and 12_1_EDX are effectively feature
> leafs as well, although the kernel can ignore them for the most part.

Yeah, we're mentally prepared for the feature bit space explosion. :)
Jarkko Sakkinen Sept. 25, 2019, 2:09 p.m. UTC | #4
On Tue, Sep 24, 2019 at 05:52:32PM +0200, Borislav Petkov wrote:
> On Tue, Sep 03, 2019 at 05:26:33PM +0300, Jarkko Sakkinen wrote:
> > From: Kai Huang <kai.huang@linux.intel.com>
> > 
> > Add X86_FEATURE_SGX_LC, which informs whether or not the CPU supports SGX
> > Launch Control.
> > 
> > Add MSR_IA32_SGXLEPUBKEYHASH{0, 1, 2, 3}, which when combined contain a
> > SHA256 hash of a 3072-bit RSA public key. SGX backed software packages, so
> > called enclaves, are always signed. All enclaves signed with the public key
> > are unconditionally allowed to initialize. [1]
> > 
> > Add FEATURE_CONTROL_SGX_LE_WR bit of the feature control MSR, which informs
> > whether the formentioned MSRs are writable or not. If the bit is off, the
> > public key MSRs are read-only for the OS.
> > 
> > If the MSRs are read-only, the platform must provide a launch enclave (LE).
> > LE can create cryptographic tokens for other enclaves that they can pass
> > together with their signature to the ENCLS(EINIT) opcode, which is used
> > to initialize enclaves.
> > 
> > Linux is unlikely to support the locked configuration because it takes away
> > the control of the launch decisions from the kernel.
> 
> Right, who has control over FEATURE_CONTROL_SGX_LE_WR? Can the
> kernel set it and put another hash in there or there will be locked
> configurations where setting that bit will trap?
> 
> I don't want to leave anything in the hands of the BIOS controlling
> whether the platform can set its own key because BIOS is known to f*ck
> it up almost every time. And so I'd like for us to be able to fix up
> things without depending on the mood of some OEM vendor's BIOS fixing
> desire.

The BIOS has control over the feature control bit because, as we know,
the feature control register is usually locked down before handover to
the OS.

The driver will support only the case where the bit is set i.e. that
it can freely write to the MSRs MSR_IA32_SGXLEPUBKEYHASH{0, 1, 2, 3}.
It will refuse to initialize otherwise.

> > [1] Intel SDM: 38.1.4 Intel SGX Launch Control Configuration
> > 
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > Co-developed-by: Haim Cohen <haim.cohen@intel.com>
> > Signed-off-by: Haim Cohen <haim.cohen@intel.com>
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> 
> This time checkpatch is right:
> 
> WARNING: Missing Signed-off-by: line by nominal patch author 'Kai Huang <kai.huang@linux.intel.com>'
> 
> And looking at the SOB chain, sounds like people need to make up their
> mind about authorship...

I'll make myself the sole author for this one as 98% of the effort in
this patch is really the commit message, which I rewrote for v22, and 2%
are the code changes (mechanical, peek at SDM).  This patch was squashed
from three patches, all like one line changes, and Kai was author of one
of them.

The next version will thus have only my SOB and author information will
be changed. I doubt anyone will complain if I do that.

/Jarkko
Jarkko Sakkinen Sept. 25, 2019, 2:10 p.m. UTC | #5
On Wed, Sep 25, 2019 at 05:09:03PM +0300, Jarkko Sakkinen wrote:
> > > [1] Intel SDM: 38.1.4 Intel SGX Launch Control Configuration
> > > 
> > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > > Co-developed-by: Haim Cohen <haim.cohen@intel.com>
> > > Signed-off-by: Haim Cohen <haim.cohen@intel.com>
> > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > 
> > This time checkpatch is right:
> > 
> > WARNING: Missing Signed-off-by: line by nominal patch author 'Kai Huang <kai.huang@linux.intel.com>'
> > 
> > And looking at the SOB chain, sounds like people need to make up their
> > mind about authorship...
> 
> I'll make myself the sole author for this one as 98% of the effort in
> this patch is really the commit message, which I rewrote for v22, and 2%
> are the code changes (mechanical, peek at SDM).  This patch was squashed
> from three patches, all like one line changes, and Kai was author of one
> of them.
> 
> The next version will thus have only my SOB and author information will
> be changed. I doubt anyone will complain if I do that.

I'll take the same action also for "x86/cpufeatures: x86/msr: Add Intel
SGX hardware bits"

/Jarkko
Jarkko Sakkinen Sept. 25, 2019, 2:38 p.m. UTC | #6
On Wed, Sep 25, 2019 at 05:10:58PM +0300, Jarkko Sakkinen wrote:
> On Wed, Sep 25, 2019 at 05:09:03PM +0300, Jarkko Sakkinen wrote:
> > > > [1] Intel SDM: 38.1.4 Intel SGX Launch Control Configuration
> > > > 
> > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > > > Co-developed-by: Haim Cohen <haim.cohen@intel.com>
> > > > Signed-off-by: Haim Cohen <haim.cohen@intel.com>
> > > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > 
> > > This time checkpatch is right:
> > > 
> > > WARNING: Missing Signed-off-by: line by nominal patch author 'Kai Huang <kai.huang@linux.intel.com>'
> > > 
> > > And looking at the SOB chain, sounds like people need to make up their
> > > mind about authorship...
> > 
> > I'll make myself the sole author for this one as 98% of the effort in
> > this patch is really the commit message, which I rewrote for v22, and 2%
> > are the code changes (mechanical, peek at SDM).  This patch was squashed
> > from three patches, all like one line changes, and Kai was author of one
> > of them.
> > 
> > The next version will thus have only my SOB and author information will
> > be changed. I doubt anyone will complain if I do that.
> 
> I'll take the same action also for "x86/cpufeatures: x86/msr: Add Intel
> SGX hardware bits"

I put to both:

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Co-developed-by: Sean Christopherson <sean.j.christopherson@intel.com>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

And changed author to me.

/Jarkko
Borislav Petkov Sept. 25, 2019, 3:19 p.m. UTC | #7
On Wed, Sep 25, 2019 at 05:09:03PM +0300, Jarkko Sakkinen wrote:
> The driver will support only the case where the bit is set i.e. that
> it can freely write to the MSRs MSR_IA32_SGXLEPUBKEYHASH{0, 1, 2, 3}.
> It will refuse to initialize otherwise.

See this:

https://lkml.kernel.org/r/20190925085156.GA3891@zn.tnic

AFAICT, when FEATURE_CONTROL_SGX_LE_WR is not set, you're not clearing
all SGX feature bits. But you should, methinks.

> The next version will thus have only my SOB and author information will
> be changed. I doubt anyone will complain if I do that.

Ok.
Sean Christopherson Sept. 25, 2019, 4:49 p.m. UTC | #8
On Wed, Sep 25, 2019 at 05:19:49PM +0200, Borislav Petkov wrote:
> On Wed, Sep 25, 2019 at 05:09:03PM +0300, Jarkko Sakkinen wrote:
> > The driver will support only the case where the bit is set i.e. that
> > it can freely write to the MSRs MSR_IA32_SGXLEPUBKEYHASH{0, 1, 2, 3}.
> > It will refuse to initialize otherwise.
> 
> See this:
> 
> https://lkml.kernel.org/r/20190925085156.GA3891@zn.tnic
> 
> AFAICT, when FEATURE_CONTROL_SGX_LE_WR is not set, you're not clearing
> all SGX feature bits. But you should, methinks.

Correct, only X86_FEATURE_SGX_LC is cleared.  The idea is to have SGX_LC
reflect whether or not flexible launch control is fully enabled, no more
no less.

Functionally, this doesn't impact support for native enclaves as the
driver will refuse to load if SGX_LC=0.

Looking forward, this *will* affect KVM.  As proposed, KVM would expose
SGX to a guest regardless of SGX_LC support.

https://lkml.kernel.org/r/20190727055214.9282-17-sean.j.christopherson@intel.com
Sean Christopherson Sept. 25, 2019, 5:18 p.m. UTC | #9
On Wed, Sep 25, 2019 at 10:51:56AM +0200, Borislav Petkov wrote:
> On Tue, Sep 24, 2019 at 01:22:10PM -0700, Sean Christopherson wrote:
> > Sadly, because FEATURE_CONTROL must be locked to fully enable SGX, the
> > reality is that any BIOS that supports SGX will lock FEATURE_CONTROL.
> 
> That's fine. The question is, would OEMs leave the hash MSRs writable?

Realistically, there will likely be a non-trivial number of systems with
SGX_LE_WR=0 but SGX enabled.

> If, as you say above, we clear SGX feature bit - not only
> X86_FEATURE_SGX_LC - when the MSRs are not writable, then we're fine.
> Platforms sticking their own hash in there won't be supported but I
> guess their aim is not to be supported in Linux anyway.
> 
> > That's the status quo today as well since VMX (and SMX/TXT) is also
> > enabled via FEATURE_CONTROL.  KVM does have logic to enable VMX and lock
> > FEATURE_CONTROL if the MSR isn't locked, but AIUI that exists only to work
> > with old BIOSes.
> > 
> > If we want to support setting and locking FEATURE_CONTROL in the extremely
> > unlikely scenario that BIOS left it unlocked, the proper change would be
> 
> I wouldn't be too surprised if this happened. BIOS is very inventive.

Given the number of steps BIOS needs to take to enable SGX, that'd be one
"inventive" BIOS. :-)

Anyways, adding logic to opportunistically set FEATURE_CONTROL during boot
should be trivial.  I'll prep a patch and send it separately from the SGX
series, moving the existing KVM code would be a good change irrespective
of SGX.

> > One note on Launch Control that isn't covered in the SDM: the LE hash
> > MSRs can also be written before SGX is activated.  SGX activation must
> > occur before FEATURE_CONTROL is locked, meaning BIOS can set the LE
> > hash MSRs to a non-intel and then lock FEATURE_CONTROL with SGX_LE_WR=0.
> 
> This is exactly what I'm afraid of. The OEM vendors locking this down.

It's inevitable that some systems will lock down the LE hash MSRs, either
intentionally or due to lack of support for SGX_LE_WR.  The latter is
probably going to be more common than OEMs intentionally locking the MSRs,
because some Intel reference BIOSes simply don't support SGX_LE_WR, e.g. I
have a Coffee Lake SDP that has hardware support for SGX_LC, but the BIOS
doesn't provide any way to set SGX_LE_WR or leave FEATURE_CONTROL unlocked.
Borislav Petkov Sept. 25, 2019, 5:28 p.m. UTC | #10
On Wed, Sep 25, 2019 at 09:49:32AM -0700, Sean Christopherson wrote:
> Correct, only X86_FEATURE_SGX_LC is cleared.  The idea is to have SGX_LC
> reflect whether or not flexible launch control is fully enabled, no more
> no less.

So we do not disable SGX when the MSRs are read-only - we disable only
launch control.

> Functionally, this doesn't impact support for native enclaves as the
> driver will refuse to load if SGX_LC=0.

So why aren't we clearing all feature bits then? What's the purpose for
leaving them set if we're not going to support hardcoded OEM vendor hash
in the MSRs anyway?

> Looking forward, this *will* affect KVM.  As proposed, KVM would expose
> SGX to a guest regardless of SGX_LC support.
> 
> https://lkml.kernel.org/r/20190727055214.9282-17-sean.j.christopherson@intel.com

... which would do what exactly? Guests can execute SGX only
when signed by the Intel key, when LC is disabled?
Sean Christopherson Sept. 25, 2019, 6:18 p.m. UTC | #11
On Wed, Sep 25, 2019 at 07:28:15PM +0200, Borislav Petkov wrote:
> On Wed, Sep 25, 2019 at 09:49:32AM -0700, Sean Christopherson wrote:
> > Correct, only X86_FEATURE_SGX_LC is cleared.  The idea is to have SGX_LC
> > reflect whether or not flexible launch control is fully enabled, no more
> > no less.
> 
> So we do not disable SGX when the MSRs are read-only - we disable only
> launch control.
> 
> > Functionally, this doesn't impact support for native enclaves as the
> > driver will refuse to load if SGX_LC=0.
> 
> So why aren't we clearing all feature bits then? What's the purpose for
> leaving them set if we're not going to support hardcoded OEM vendor hash
> in the MSRs anyway?

To allow KVM to expose SGX to guests even if the MSRs are locked down.

> > Looking forward, this *will* affect KVM.  As proposed, KVM would expose
> > SGX to a guest regardless of SGX_LC support.
> > 
> > https://lkml.kernel.org/r/20190727055214.9282-17-sean.j.christopherson@intel.com
> 
> ... which would do what exactly? Guests can execute SGX only
> when signed by the Intel key, when LC is disabled?

Guest can only run launch enclaves that are signed by whatever key matches
the LE hash MSRs.  That could be an Intel key, e.g. if BIOS neglected to
set FEATURE_CONTROL.SGX_LE_WR=1, or some third party key if BIOS
deliberately rewrote the hash MSRs and cleared SGX_LE_WR.

A mainline Linux kernel in the guest would not allow running enclaves due
to the MSRs being locked, i.e. doing an end-around on the host kernel to
run enclaves on a locked system would require a custom Linux kernel or a
different OS entirely.
Borislav Petkov Sept. 25, 2019, 6:31 p.m. UTC | #12
On Wed, Sep 25, 2019 at 10:18:24AM -0700, Sean Christopherson wrote:
> Realistically, there will likely be a non-trivial number of systems with
> SGX_LE_WR=0 but SGX enabled.

Well no. We won't support those. I remember very vividly at Tech Days a
couple of years ago where we said we won't support locked down systems.

> Given the number of steps BIOS needs to take to enable SGX, that'd be one
> "inventive" BIOS. :-)

Oh, you have no idea the amount of BIOS shit I've experienced.

> It's inevitable that some systems will lock down the LE hash MSRs, either
> intentionally or due to lack of support for SGX_LE_WR.  The latter is
> probably going to be more common than OEMs intentionally locking the MSRs,
> because some Intel reference BIOSes simply don't support SGX_LE_WR, e.g. I
> have a Coffee Lake SDP that has hardware support for SGX_LC, but the BIOS
> doesn't provide any way to set SGX_LE_WR or leave FEATURE_CONTROL unlocked.

We won't support those too. Nothing changes since a couple of years ago.
We won't support locked down systems and unfinished BIOS systems.

... reading your other mail about KVM...

I guess KVM could be an exception here if people wanna run different
OSes in the guest. IMHO.

For that, though, we should still clear all SGX feature bits in the
host, I'd say, and let the kvm module rediscover everything itself
through CPUID directly and not using *cpu_has*

Why, you ask? Because otherwise users will start asking why do they have
"sgx" in /proc/cpuinfo but they can't run their own enclaves.

But maybe someone has a better idea.

In any case, I think it would be bad idea to show only a subset of
features in /proc/cpuinfo of a locked-down system and have to explain it
to users why they can't do own enclaves.

But again, someone might have a better idea.

Thx.
Sean Christopherson Sept. 25, 2019, 7:08 p.m. UTC | #13
On Wed, Sep 25, 2019 at 08:31:36PM +0200, Borislav Petkov wrote:
> On Wed, Sep 25, 2019 at 10:18:24AM -0700, Sean Christopherson wrote:
> > Realistically, there will likely be a non-trivial number of systems with
> > SGX_LE_WR=0 but SGX enabled.
> 
> Well no. We won't support those. I remember very vividly at Tech Days a
> couple of years ago where we said we won't support locked down systems.

Yep, that's our intent as well.

> > It's inevitable that some systems will lock down the LE hash MSRs, either
> > intentionally or due to lack of support for SGX_LE_WR.  The latter is
> > probably going to be more common than OEMs intentionally locking the MSRs,
> > because some Intel reference BIOSes simply don't support SGX_LE_WR, e.g. I
> > have a Coffee Lake SDP that has hardware support for SGX_LC, but the BIOS
> > doesn't provide any way to set SGX_LE_WR or leave FEATURE_CONTROL unlocked.
> 
> We won't support those too. Nothing changes since a couple of years ago.
> We won't support locked down systems and unfinished BIOS systems.

Yep.

> ... reading your other mail about KVM...
> 
> I guess KVM could be an exception here if people wanna run different
> OSes in the guest. IMHO.
>
> For that, though, we should still clear all SGX feature bits in the
> host, I'd say, and let the kvm module rediscover everything itself
> through CPUID directly and not using *cpu_has*
> 
> Why, you ask? Because otherwise users will start asking why do they have
> "sgx" in /proc/cpuinfo but they can't run their own enclaves.

That makes sense.  I was thinking it'd be helpful to leave the bits set,
e.g. for users to differentiate between "I don't have SGX" and "I can't
use SGX because SGX_LC is disabled".  But I'm probably being slightly
optomistic...

> But maybe someone has a better idea.
> 
> In any case, I think it would be bad idea to show only a subset of
> features in /proc/cpuinfo of a locked-down system and have to explain it
> to users why they can't do own enclaves.
> 
> But again, someone might have a better idea.

I'm 99% certain this won't even require a change to the proposed KVM
patches, as KVM mostly pulls SGX support directly from CPUID.  The only
thing it checks via cpu_has() is SGX_LC to query whether or not the MSRs
are fully writable.

Keeping the SGX feature bits set was more about reflecting hardware
capabilities than it was a functional requirement.
Jarkko Sakkinen Sept. 27, 2019, 4:11 p.m. UTC | #14
On Wed, Sep 25, 2019 at 10:18:24AM -0700, Sean Christopherson wrote:
> > I wouldn't be too surprised if this happened. BIOS is very inventive.
> 
> Given the number of steps BIOS needs to take to enable SGX, that'd be one
> "inventive" BIOS. :-)
> 
> Anyways, adding logic to opportunistically set FEATURE_CONTROL during boot
> should be trivial.  I'll prep a patch and send it separately from the SGX
> series, moving the existing KVM code would be a good change irrespective
> of SGX.

Also, based on Borislav's remarks, the commit message should be more
clear about launch control (separately describe the driver and KVM
use). I can rework that.

/Jarkko

Patch
diff mbox series

diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index c5582e766121..ca82226e25ec 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -355,6 +355,7 @@ 
 #define X86_FEATURE_CLDEMOTE		(16*32+25) /* CLDEMOTE instruction */
 #define X86_FEATURE_MOVDIRI		(16*32+27) /* MOVDIRI instruction */
 #define X86_FEATURE_MOVDIR64B		(16*32+28) /* MOVDIR64B instruction */
+#define X86_FEATURE_SGX_LC		(16*32+30) /* Software Guard Extensions Launch Control */
 
 /* AMD-defined CPU features, CPUID level 0x80000007 (EBX), word 17 */
 #define X86_FEATURE_OVERFLOW_RECOV	(17*32+ 0) /* MCA overflow recovery support */
diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index c006ba8187aa..24da5800b1c6 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -542,6 +542,7 @@ 
 #define FEATURE_CONTROL_LOCKED				(1<<0)
 #define FEATURE_CONTROL_VMXON_ENABLED_INSIDE_SMX	(1<<1)
 #define FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX	(1<<2)
+#define FEATURE_CONTROL_SGX_LE_WR			(1<<17)
 #define FEATURE_CONTROL_SGX_ENABLE			(1<<18)
 #define FEATURE_CONTROL_LMCE				(1<<20)
 
@@ -555,6 +556,12 @@ 
 #define MSR_IA32_UCODE_WRITE		0x00000079
 #define MSR_IA32_UCODE_REV		0x0000008b
 
+/* Intel SGX Launch Enclave Public Key Hash MSRs */
+#define MSR_IA32_SGXLEPUBKEYHASH0	0x0000008C
+#define MSR_IA32_SGXLEPUBKEYHASH1	0x0000008D
+#define MSR_IA32_SGXLEPUBKEYHASH2	0x0000008E
+#define MSR_IA32_SGXLEPUBKEYHASH3	0x0000008F
+
 #define MSR_IA32_SMM_MONITOR_CTL	0x0000009b
 #define MSR_IA32_SMBASE			0x0000009e