| Message ID | 20190903142655.21943-3-jarkko.sakkinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Intel SGX foundations | expand |
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... :-\
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.
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. :)
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
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
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
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.
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
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.
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?
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.
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.
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.
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
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