| Message ID | 20220720191347.1343986-1-dave.hansen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] x86/sgx: Allow enclaves to use Asynchrounous Exit Notification | expand |
On Wed, Jul 20, 2022, Dave Hansen wrote: > Changes from v1: > * Make sure SGX_ATTR_ASYNC_EXIT_NOTIFY is in the masks that are > used at bare-metal enclave initialization and that enumerates > available attributes to KVM guests. Heh, I was wondering if KVM needed to be updated. > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > index 0c1ba6aa0765..96a73b5b4369 100644 > --- a/arch/x86/kvm/cpuid.c > +++ b/arch/x86/kvm/cpuid.c > @@ -1022,9 +1022,7 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function) > * userspace. ATTRIBUTES.XFRM is not adjusted as userspace is > * expected to derive it from supported XCR0. > */ > - entry->eax &= SGX_ATTR_DEBUG | SGX_ATTR_MODE64BIT | > - SGX_ATTR_PROVISIONKEY | SGX_ATTR_EINITTOKENKEY | > - SGX_ATTR_KSS; > + entry->eax &= SGX_ATTR_PRIV_MASK | SGX_ATTR_UNPRIV_MASK; It may seem like a maintenance burdern, and it is to some extent, but I think it's better for KVM to have to explicitly "enable" each flag. There is no guarantee that a new feature will not require additional KVM enabling, i.e. we want the pain of having to manually update KVM so that we get "feature X isn't virtualized" complaints and not "I upgraded my kernel and my enclaves broke" bug reports. I don't think it's likely that attribute-based features will require additional enabling since there aren't any virtualization controls for the ENCLU side of things (ENCLU is effectively disabled by blocking ENCLS[ECREATE]), but updating KVM isn't particularly difficult so I'd rather be paranoid.
On 7/20/22 12:49, Sean Christopherson wrote: > On Wed, Jul 20, 2022, Dave Hansen wrote: >> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c >> index 0c1ba6aa0765..96a73b5b4369 100644 >> --- a/arch/x86/kvm/cpuid.c >> +++ b/arch/x86/kvm/cpuid.c >> @@ -1022,9 +1022,7 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function) >> * userspace. ATTRIBUTES.XFRM is not adjusted as userspace is >> * expected to derive it from supported XCR0. >> */ >> - entry->eax &= SGX_ATTR_DEBUG | SGX_ATTR_MODE64BIT | >> - SGX_ATTR_PROVISIONKEY | SGX_ATTR_EINITTOKENKEY | >> - SGX_ATTR_KSS; >> + entry->eax &= SGX_ATTR_PRIV_MASK | SGX_ATTR_UNPRIV_MASK; > > It may seem like a maintenance burdern, and it is to some extent, but I think it's > better for KVM to have to explicitly "enable" each flag. There is no guarantee > that a new feature will not require additional KVM enabling, i.e. we want the pain > of having to manually update KVM so that we get "feature X isn't virtualized" > complaints and not "I upgraded my kernel and my enclaves broke" bug reports. > > I don't think it's likely that attribute-based features will require additional > enabling since there aren't any virtualization controls for the ENCLU side of > things (ENCLU is effectively disabled by blocking ENCLS[ECREATE]), but updating > KVM isn't particularly difficult so I'd rather be paranoid. How about something where KVM gets to keep a discrete mask, but where it's at least defined next to the attributes, something like: /* * These attributes will be advertised to KVM guests as being * available. This includes privileged attributes. Only add * to this list when host-side KVM does not require additional * enabling for the attribute. */ #define SGX_ATTR_KVM_MASK (SGX_ATTR_DEBUG | \ SGX_ATTR_MODE64BIT | \ SGX_ATTR_PROVISIONKEY | \ SGX_ATTR_EINITTOKENKEY | \ SGX_ATTR_KSS | \ SGX_ATTR_ASYNC_EXIT_NOTIFY) That at least has a *chance* of someone seeing it who goes to add a new attribute.
On Wed, 2022-07-20 at 12:13 -0700, Dave Hansen wrote: > --- a/arch/x86/kvm/cpuid.c > +++ b/arch/x86/kvm/cpuid.c > @@ -1022,9 +1022,7 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function) > * userspace. ATTRIBUTES.XFRM is not adjusted as userspace is > * expected to derive it from supported XCR0. > */ > - entry->eax &= SGX_ATTR_DEBUG | SGX_ATTR_MODE64BIT | > - SGX_ATTR_PROVISIONKEY | SGX_ATTR_EINITTOKENKEY | > - SGX_ATTR_KSS; > + entry->eax &= SGX_ATTR_PRIV_MASK | SGX_ATTR_UNPRIV_MASK; > entry->ebx &= 0; > break; > /* Intel PT */ > -- > 2.34.1 Did a quick look at the spec. It appears ENCLU[EDECCSSA] should be used together with AEX-notify. So besides advertising the new SGX_ATTR_ASYNC_EXIT_NOTIFY bit to the KVM guest, I think we should also advertise the ENCLU[EDECCSSA] support in guest's CPUID, like below (untested)? diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h index 6466a58b9cff..d2ebb38b31e7 100644 --- a/arch/x86/include/asm/cpufeatures.h +++ b/arch/x86/include/asm/cpufeatures.h @@ -296,6 +296,7 @@ #define X86_FEATURE_PER_THREAD_MBA (11*32+ 7) /* "" Per-thread Memory Bandwidth Allocation */ #define X86_FEATURE_SGX1 (11*32+ 8) /* "" Basic SGX */ #define X86_FEATURE_SGX2 (11*32+ 9) /* "" SGX Enclave Dynamic Memory Management (EDMM) */ +#define X86_FEATURE_SGX_EDECCSSA (11*32+10) /* "" SGX EDECCSSA user leaf function */ /* Intel-defined CPU features, CPUID level 0x00000007:1 (EAX), word 12 */ #define X86_FEATURE_AVX_VNNI (12*32+ 4) /* AVX VNNI instructions */ diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index d47222ab8e6e..121456653417 100644 --- a/arch/x86/kvm/cpuid.c +++ b/arch/x86/kvm/cpuid.c @@ -612,7 +612,7 @@ void kvm_set_cpu_caps(void) ); kvm_cpu_cap_init_scattered(CPUID_12_EAX, - SF(SGX1) | SF(SGX2) + SF(SGX1) | SF(SGX2) | SF(SGX_EDECCSSA) ); kvm_cpu_cap_mask(CPUID_8000_0001_ECX, diff --git a/arch/x86/kvm/reverse_cpuid.h b/arch/x86/kvm/reverse_cpuid.h index a19d473d0184..4e5b8444f161 100644 --- a/arch/x86/kvm/reverse_cpuid.h +++ b/arch/x86/kvm/reverse_cpuid.h @@ -23,6 +23,7 @@ enum kvm_only_cpuid_leafs { /* Intel-defined SGX sub-features, CPUID level 0x12 (EAX). */ #define KVM_X86_FEATURE_SGX1 KVM_X86_FEATURE(CPUID_12_EAX, 0) #define KVM_X86_FEATURE_SGX2 KVM_X86_FEATURE(CPUID_12_EAX, 1) +#define KVM_X86_FEATURE_SGX_EDECCSSA KVM_X86_FEATURE(CPUID_12_EAX, 11) struct cpuid_reg { u32 function; @@ -78,6 +79,8 @@ static __always_inline u32 __feature_translate(int x86_feature) return KVM_X86_FEATURE_SGX1; else if (x86_feature == X86_FEATURE_SGX2) return KVM_X86_FEATURE_SGX2; + else if (x86_feature == X86_FEATURE_SGX_EDECCSSA) + return KVM_X86_FEATURE_SGX_EDECCSSA; return x86_feature; }
On 7/22/22 06:26, Kai Huang wrote: > Did a quick look at the spec. It appears ENCLU[EDECCSSA] should be used > together with AEX-notify. So besides advertising the new > SGX_ATTR_ASYNC_EXIT_NOTIFY bit to the KVM guest, I think we should also > advertise the ENCLU[EDECCSSA] support in guest's CPUID, like below (untested)? Sounds like a great follow-on patch! It doesn't seem truly functionally required from the spec: > EDECCSSA is a new Intel SGX user leaf function > (ENCLU[EDECCSSA]) that can facilitate AEX notification handling... If that's wrong or imprecise, I'd love to hear more about it and also about how the spec will be updated. Oh, and the one-liner patch that I was promised for enabling this is getting a _wee_ bit longer than one line.
On Fri, Jul 22, 2022, Dave Hansen wrote: > On 7/22/22 06:26, Kai Huang wrote: > > Did a quick look at the spec. It appears ENCLU[EDECCSSA] should be used > > together with AEX-notify. So besides advertising the new > > SGX_ATTR_ASYNC_EXIT_NOTIFY bit to the KVM guest, I think we should also > > advertise the ENCLU[EDECCSSA] support in guest's CPUID, like below (untested)? > > Sounds like a great follow-on patch! It doesn't seem truly functionally > required from the spec: > > > EDECCSSA is a new Intel SGX user leaf function > > (ENCLU[EDECCSSA]) that can facilitate AEX notification handling... Yeah, it's enumerated separately. > If that's wrong or imprecise, I'd love to hear more about it and also > about how the spec will be updated. > > Oh, and the one-liner patch that I was promised for enabling this is > getting a _wee_ bit longer than one line. Heh, fool me once...
On Wed, Jul 20, 2022, Dave Hansen wrote: > On 7/20/22 12:49, Sean Christopherson wrote: > > On Wed, Jul 20, 2022, Dave Hansen wrote: > >> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > >> index 0c1ba6aa0765..96a73b5b4369 100644 > >> --- a/arch/x86/kvm/cpuid.c > >> +++ b/arch/x86/kvm/cpuid.c > >> @@ -1022,9 +1022,7 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function) > >> * userspace. ATTRIBUTES.XFRM is not adjusted as userspace is > >> * expected to derive it from supported XCR0. > >> */ > >> - entry->eax &= SGX_ATTR_DEBUG | SGX_ATTR_MODE64BIT | > >> - SGX_ATTR_PROVISIONKEY | SGX_ATTR_EINITTOKENKEY | > >> - SGX_ATTR_KSS; > >> + entry->eax &= SGX_ATTR_PRIV_MASK | SGX_ATTR_UNPRIV_MASK; > > > > It may seem like a maintenance burdern, and it is to some extent, but I think it's > > better for KVM to have to explicitly "enable" each flag. There is no guarantee > > that a new feature will not require additional KVM enabling, i.e. we want the pain > > of having to manually update KVM so that we get "feature X isn't virtualized" > > complaints and not "I upgraded my kernel and my enclaves broke" bug reports. > > > > I don't think it's likely that attribute-based features will require additional > > enabling since there aren't any virtualization controls for the ENCLU side of > > things (ENCLU is effectively disabled by blocking ENCLS[ECREATE]), but updating > > KVM isn't particularly difficult so I'd rather be paranoid. > > How about something where KVM gets to keep a discrete mask, but where > it's at least defined next to the attributes, something like: > > /* > * These attributes will be advertised to KVM guests as being > * available. This includes privileged attributes. Only add > * to this list when host-side KVM does not require additional > * enabling for the attribute. > */ > #define SGX_ATTR_KVM_MASK (SGX_ATTR_DEBUG | \ > SGX_ATTR_MODE64BIT | \ > SGX_ATTR_PROVISIONKEY | \ > SGX_ATTR_EINITTOKENKEY | \ > SGX_ATTR_KSS | \ > SGX_ATTR_ASYNC_EXIT_NOTIFY) > > That at least has a *chance* of someone seeing it who goes to add a new > attribute. Hmm, what if we enforce it in code with a compile-time assert? That will make it even harder to screw things up, and it also avoids a scenario where someone extends SGX_ATTR_KVM_MASK without getting approval from KVM folks. And conversely, KVM won't need to touch SGX files if there's ever a need to tweak KVM behavior. /* * Index 1: SECS.ATTRIBUTES. ATTRIBUTES are restricted a la * feature flags. Advertise all supported flags, including * privileged attributes that require explicit opt-in from * userspace. ATTRIBUTES.XFRM is not adjusted as userspace is * expected to derive it from supported XCR0. */ #define KVM_SGX_ATTR_ALLOWED_MASK (SGX_ATTR_DEBUG | \ SGX_ATTR_MODE64BIT | \ SGX_ATTR_PROVISIONKEY | \ SGX_ATTR_EINITTOKENKEY | \ SGX_ATTR_KSS | \ SGX_ATTR_ASYNC_EXIT_NOTIFY) #define KVM_SGX_ATTR_DENIED_MASK (0) /* * Assert that KVM explicitly allows or denies exposing all * features, i.e. detect attempts to add kernel support without * also updating KVM. */ BUILD_BUG_ON((KVM_SGX_ATTR_ALLOWED_MASK | KVM_SGX_ATTR_DENIED_MASK) != (SGX_ATTR_PRIV_MASK | SGX_ATTR_UNPRIV_MASK)); entry->eax &= KVM_SGX_ATTR_ALLOWED_MASK; entry->ebx &= 0; break;
On 7/22/22 12:00, Sean Christopherson wrote: > /* > * Assert that KVM explicitly allows or denies exposing all > * features, i.e. detect attempts to add kernel support without > * also updating KVM. > */ > BUILD_BUG_ON((KVM_SGX_ATTR_ALLOWED_MASK | KVM_SGX_ATTR_DENIED_MASK) != > (SGX_ATTR_PRIV_MASK | SGX_ATTR_UNPRIV_MASK)); Looks good to me. I'll do that for the next version. Thanks!
On Fri, 2022-07-22 at 08:21 -0700, Dave Hansen wrote: > On 7/22/22 06:26, Kai Huang wrote: > > Did a quick look at the spec. It appears ENCLU[EDECCSSA] should be used > > together with AEX-notify. So besides advertising the new > > SGX_ATTR_ASYNC_EXIT_NOTIFY bit to the KVM guest, I think we should also > > advertise the ENCLU[EDECCSSA] support in guest's CPUID, like below (untested)? > > Sounds like a great follow-on patch! It doesn't seem truly functionally > required from the spec: > > > EDECCSSA is a new Intel SGX user leaf function > > (ENCLU[EDECCSSA]) that can facilitate AEX notification handling... > > If that's wrong or imprecise, I'd love to hear more about it and also > about how the spec will be updated. > They are enumerated separately, but looks in practice the notify handler will use it to switch back to the correct/targeted CSSA to continue to run normally after handling the exit notify. This is my understanding of the "facilitate" mean in the spec. Btw, in real hardware I think the two should come together, meaning no real hardware will only support one. Haitao, could you give us more information?
On Mon, 25 Jul 2022 05:36:17 -0500, Kai Huang <kai.huang@intel.com> wrote: > On Fri, 2022-07-22 at 08:21 -0700, Dave Hansen wrote: >> On 7/22/22 06:26, Kai Huang wrote: >> > Did a quick look at the spec. It appears ENCLU[EDECCSSA] should be >> used >> > together with AEX-notify. So besides advertising the new >> > SGX_ATTR_ASYNC_EXIT_NOTIFY bit to the KVM guest, I think we should >> also >> > advertise the ENCLU[EDECCSSA] support in guest's CPUID, like below >> (untested)? >> >> Sounds like a great follow-on patch! It doesn't seem truly functionally >> required from the spec: >> >> > EDECCSSA is a new Intel SGX user leaf function >> > (ENCLU[EDECCSSA]) that can facilitate AEX notification handling... >> >> If that's wrong or imprecise, I'd love to hear more about it and also >> about how the spec will be updated. >> > > They are enumerated separately, but looks in practice the notify handler > will > use it to switch back to the correct/targeted CSSA to continue to run > normally > after handling the exit notify. This is my understanding of the > "facilitate" > mean in the spec. > > Btw, in real hardware I think the two should come together, meaning no > real > hardware will only support one. > > Haitao, could you give us more information? > You are right. They are enumerated separately and HW should come with both or neither. My understanding it is also possible for enclaves choose not to receive AEX notify but still use EDECCSSA. Thanks Haitao
On Tue, 2022-07-26 at 00:10 -0500, Haitao Huang wrote: > On Mon, 25 Jul 2022 05:36:17 -0500, Kai Huang <kai.huang@intel.com> wrote: > > > On Fri, 2022-07-22 at 08:21 -0700, Dave Hansen wrote: > > > On 7/22/22 06:26, Kai Huang wrote: > > > > Did a quick look at the spec. It appears ENCLU[EDECCSSA] should be > > > used > > > > together with AEX-notify. So besides advertising the new > > > > SGX_ATTR_ASYNC_EXIT_NOTIFY bit to the KVM guest, I think we should > > > also > > > > advertise the ENCLU[EDECCSSA] support in guest's CPUID, like below > > > (untested)? > > > > > > Sounds like a great follow-on patch! It doesn't seem truly functionally > > > required from the spec: > > > > > > > EDECCSSA is a new Intel SGX user leaf function > > > > (ENCLU[EDECCSSA]) that can facilitate AEX notification handling... > > > > > > If that's wrong or imprecise, I'd love to hear more about it and also > > > about how the spec will be updated. > > > > > > > They are enumerated separately, but looks in practice the notify handler > > will > > use it to switch back to the correct/targeted CSSA to continue to run > > normally > > after handling the exit notify. This is my understanding of the > > "facilitate" > > mean in the spec. > > > > Btw, in real hardware I think the two should come together, meaning no > > real > > hardware will only support one. > > > > Haitao, could you give us more information? > > > You are right. They are enumerated separately and HW should come with both > or neither. > My understanding it is also possible for enclaves choose not to receive > AEX notify > but still use EDECCSSA. > What is the use case of using EDECCSSA w/o using AEX notify? If I understand correctly EDECCSSA effectively switches to another thread (using the previous SSA, which is the context of another TCS thread if I understand correctly). Won't this cause problem? It probably makes sense to use only AEX notify w/o using EDECCSSA though, but this should be the case that the enclave detects serious attack and don't want to continue to run normally. In another words, it is enclave's choice, but hardware should always support both AEX notify and EDECCSSA.
On Tue, 26 Jul 2022 05:47:14 -0500, Kai Huang <kai.huang@intel.com> wrote: > On Tue, 2022-07-26 at 00:10 -0500, Haitao Huang wrote: >> On Mon, 25 Jul 2022 05:36:17 -0500, Kai Huang <kai.huang@intel.com> >> wrote: >> >> > On Fri, 2022-07-22 at 08:21 -0700, Dave Hansen wrote: >> > > On 7/22/22 06:26, Kai Huang wrote: >> > > > Did a quick look at the spec. It appears ENCLU[EDECCSSA] should >> be >> > > used >> > > > together with AEX-notify. So besides advertising the new >> > > > SGX_ATTR_ASYNC_EXIT_NOTIFY bit to the KVM guest, I think we should >> > > also >> > > > advertise the ENCLU[EDECCSSA] support in guest's CPUID, like below >> > > (untested)? >> > > >> > > Sounds like a great follow-on patch! It doesn't seem truly >> functionally >> > > required from the spec: >> > > >> > > > EDECCSSA is a new Intel SGX user leaf function >> > > > (ENCLU[EDECCSSA]) that can facilitate AEX notification handling... >> > > >> > > If that's wrong or imprecise, I'd love to hear more about it and >> also >> > > about how the spec will be updated. >> > > >> > >> > They are enumerated separately, but looks in practice the notify >> handler >> > will >> > use it to switch back to the correct/targeted CSSA to continue to run >> > normally >> > after handling the exit notify. This is my understanding of the >> > "facilitate" >> > mean in the spec. >> > >> > Btw, in real hardware I think the two should come together, meaning no >> > real >> > hardware will only support one. >> > >> > Haitao, could you give us more information? >> > >> You are right. They are enumerated separately and HW should come with >> both >> or neither. >> My understanding it is also possible for enclaves choose not to receive >> AEX notify >> but still use EDECCSSA. >> > > What is the use case of using EDECCSSA w/o using AEX notify? > If I understand correctly EDECCSSA effectively switches to another > thread (using > the previous SSA, which is the context of another TCS thread if I > understand > correctly). Won't this cause problem? No. Decrementing CSSA is equivalent to popping stack frames, not switching threads. In some cases such as so-called "first stage" exception handling, one could pop CSSA back to the previous after resetting CPU context and stack frame appropriate to the "second stage" or "real" exception handling routine, then jump to the handler directly. This could improve exception handling performance by saving an EEXIT/ERESUME trip. > > It probably makes sense to use only AEX notify w/o using EDECCSSA > though, but > this should be the case that the enclave detects serious attack and > don't want > to continue to run normally. In another words, it is enclave's choice, > but > hardware should always support both AEX notify and EDECCSSA.
On 7/22/22 06:26, Kai Huang wrote: > Did a quick look at the spec. It appears ENCLU[EDECCSSA] should be used > together with AEX-notify. So besides advertising the new > SGX_ATTR_ASYNC_EXIT_NOTIFY bit to the KVM guest, I think we should also > advertise the ENCLU[EDECCSSA] support in guest's CPUID, like below (untested)? Kai, would you care to send a new version of this with a proper SoB and changelog integrating what you've learned since posting it? It can be merged along with AEX notify itself.
On Tue, 2022-07-26 at 10:28 -0500, Haitao Huang wrote: > On Tue, 26 Jul 2022 05:47:14 -0500, Kai Huang <kai.huang@intel.com> wrote: > > > On Tue, 2022-07-26 at 00:10 -0500, Haitao Huang wrote: > > > On Mon, 25 Jul 2022 05:36:17 -0500, Kai Huang <kai.huang@intel.com> > > > wrote: > > > > > > > On Fri, 2022-07-22 at 08:21 -0700, Dave Hansen wrote: > > > > > On 7/22/22 06:26, Kai Huang wrote: > > > > > > Did a quick look at the spec. It appears ENCLU[EDECCSSA] should > > > be > > > > > used > > > > > > together with AEX-notify. So besides advertising the new > > > > > > SGX_ATTR_ASYNC_EXIT_NOTIFY bit to the KVM guest, I think we should > > > > > also > > > > > > advertise the ENCLU[EDECCSSA] support in guest's CPUID, like below > > > > > (untested)? > > > > > > > > > > Sounds like a great follow-on patch! It doesn't seem truly > > > functionally > > > > > required from the spec: > > > > > > > > > > > EDECCSSA is a new Intel SGX user leaf function > > > > > > (ENCLU[EDECCSSA]) that can facilitate AEX notification handling... > > > > > > > > > > If that's wrong or imprecise, I'd love to hear more about it and > > > also > > > > > about how the spec will be updated. > > > > > > > > > > > > > They are enumerated separately, but looks in practice the notify > > > handler > > > > will > > > > use it to switch back to the correct/targeted CSSA to continue to run > > > > normally > > > > after handling the exit notify. This is my understanding of the > > > > "facilitate" > > > > mean in the spec. > > > > > > > > Btw, in real hardware I think the two should come together, meaning no > > > > real > > > > hardware will only support one. > > > > > > > > Haitao, could you give us more information? > > > > > > > You are right. They are enumerated separately and HW should come with > > > both > > > or neither. > > > My understanding it is also possible for enclaves choose not to receive > > > AEX notify > > > but still use EDECCSSA. > > > > > > > What is the use case of using EDECCSSA w/o using AEX notify? > > If I understand correctly EDECCSSA effectively switches to another > > thread (using > > the previous SSA, which is the context of another TCS thread if I > > understand > > correctly). Won't this cause problem? > > No. Decrementing CSSA is equivalent to popping stack frames, not switching > threads. > In some cases such as so-called "first stage" exception handling, one > could pop CSSA back to the previous after resetting CPU context and stack > frame appropriate to the "second stage" or "real" exception handling > routine, then jump to the handler directly. This could improve exception > handling performance by saving an EEXIT/ERESUME trip. > > Looking at the AEX-notify spec again, EDECCSSA does below: (* At this point, the instruction is guaranteed to complete *) CR_TCS_PA.CSSA := CR_TCS_PA.CSSA - 1; CR_GPR_PA := Physical_Address(DS:TMP_GPR); It doens't reset the RIP to CR_GPA_PA.RIP so looks yes you are right. It only "popping the stack frame" but doesn't switch thread. But the pseudo code of EDECCSSA only updates the CR_TCS_PA and CR_GPR_PA registers (forget about XSAVE not), but doesn't manually updating the actual CPU registers such as GPRs. Are the actual CPU registers updated automatically when CR_xx are updated?
On Tue, 2022-07-26 at 08:36 -0700, Dave Hansen wrote: > On 7/22/22 06:26, Kai Huang wrote: > > Did a quick look at the spec. It appears ENCLU[EDECCSSA] should be used > > together with AEX-notify. So besides advertising the new > > SGX_ATTR_ASYNC_EXIT_NOTIFY bit to the KVM guest, I think we should also > > advertise the ENCLU[EDECCSSA] support in guest's CPUID, like below (untested)? > > Kai, would you care to send a new version of this with a proper SoB and > changelog integrating what you've learned since posting it? It can be > merged along with AEX notify itself. Sure will do today.
On Wed, Jul 20, 2022 at 12:13:47PM -0700, Dave Hansen wrote: > Changes from v1: > * Make sure SGX_ATTR_ASYNC_EXIT_NOTIFY is in the masks that are > used at bare-metal enclave initialization and that enumerates > available attributes to KVM guests. > > -- > > Short Version: > > Allow enclaves to use the new Asynchronous EXit (AEX) > notification mechanism. This mechanism lets enclaves run a > handler after an AEX event. These handlers can run mitigations > for things like SGX-Step[1]. > > AEX Notify will be made available both on upcoming processors and > on some older processors through microcode updates. > > Long Version: > > == SGX Attribute Background == > > The SGX architecture includes a list of SGX "attributes". These > attributes ensure consistency and transparency around specific > enclave features. > > As a simple example, the "DEBUG" attribute allows an enclave to > be debugged, but also destroys virtually all of SGX security. > Using attributes, enclaves can know that they are being debugged. > Attributes also affect enclave attestation so an enclave can, for > instance, be denied access to secrets while it is being debugged. > > The kernel keeps a list of known attributes and will only > initialize enclaves that use a known set of attributes. This > kernel policy eliminates the chance that a new SGX attribute > could cause undesired effects. > > For example, imagine a new attribute was added called > "PROVISIONKEY2" that provided similar functionality to > "PROVISIIONKEY". A kernel policy that allowed indiscriminate use > of unknown attributes and thus PROVISIONKEY2 would undermine the > existing kernel policy which limits use of PROVISIONKEY enclaves. > > == AEX Notify Background == > > "Intel Architecture Instruction Set Extensions and Future > Features - Version 45" is out[2]. There is a new chapter: > > Asynchronous Enclave Exit Notify and the EDECCSSA User Leaf Function. > > Enclaves exit can be either synchronous and consensual (EEXIT for > instance) or asynchronous (on an interrupt or fault). The > asynchronous ones can evidently be exploited to single step > enclaves[1], on top of which other naughty things can be built. > > AEX Notify will be made available both on upcoming processors and > on some older processors through microcode updates. > > == The Problem == > > These attacks are currently entirely opaque to the enclave since > the hardware does the save/restore under the covers. The > Asynchronous Enclave Exit Notify (AEX Notify) mechanism provides > enclaves an ability to detect and mitigate potential exposure to > these kinds of attacks. > > == The Solution == > > Define the new attribute value for AEX Notification. Ensure the > attribute is cleared from the list reserved attributes. Instead > of adding to the open-coded lists of individual attributes, > add named lists of privileged (disallowed by default) and > unprivileged (allowed by default) attributes. Add the AEX notify > attribute as an unprivileged attribute, which will keep the kernel > from rejecting enclaves with it set. > > I just built this and ran it to make sure there were no obvious > regressions since I do not have the hardware (and new microcde) > to test it. > > Testing on bare-metal and in VMs accompanied by Tested-by's > would be much appreciated. (This means you, Intel folks who > actually have systems with the microcode that can do this.) > > 1. https://github.com/jovanbulck/sgx-step > 2. https://cdrdv2.intel.com/v1/dl/getContent/671368?explicitVersion=true > > Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com> > Cc: Jarkko Sakkinen <jarkko@kernel.org> > Cc: Andy Lutomirski <luto@kernel.org> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: Borislav Petkov <bp@alien8.de> > Cc: "H. Peter Anvin" <hpa@zytor.com> > Cc: Sean Christopherson <seanjc@google.com> > Cc: Kai Huang <kai.huang@intel.com> > Cc: Haitao Huang <haitao.huang@linux.intel.com> > Cc: x86@kernel.org > Cc: linux-sgx@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > --- > arch/x86/include/asm/sgx.h | 33 ++++++++++++++++++++++++++------- > arch/x86/kernel/cpu/sgx/ioctl.c | 2 +- > arch/x86/kvm/cpuid.c | 4 +--- > 3 files changed, 28 insertions(+), 11 deletions(-) > > diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h > index 3f9334ef67cd..3004dfe76498 100644 > --- a/arch/x86/include/asm/sgx.h > +++ b/arch/x86/include/asm/sgx.h > @@ -110,17 +110,36 @@ enum sgx_miscselect { > * %SGX_ATTR_EINITTOKENKEY: Allow to use token signing key that is used to > * sign cryptographic tokens that can be passed to > * EINIT as an authorization to run an enclave. > + * %SGX_ATTR_ASYNC_EXIT_NOTIFY: Allow enclaves to be notified after an > + * asynchronous exit has occurred. > */ > enum sgx_attribute { > - SGX_ATTR_INIT = BIT(0), > - SGX_ATTR_DEBUG = BIT(1), > - SGX_ATTR_MODE64BIT = BIT(2), > - SGX_ATTR_PROVISIONKEY = BIT(4), > - SGX_ATTR_EINITTOKENKEY = BIT(5), > - SGX_ATTR_KSS = BIT(7), > + SGX_ATTR_INIT = BIT(0), > + SGX_ATTR_DEBUG = BIT(1), > + SGX_ATTR_MODE64BIT = BIT(2), > + /* BIT(3) is reserved */ > + SGX_ATTR_PROVISIONKEY = BIT(4), > + SGX_ATTR_EINITTOKENKEY = BIT(5), > + /* BIT(6) is for CET */ > + SGX_ATTR_KSS = BIT(7), > + /* BIT(8) is reserved */ > + /* BIT(9) is reserved */ > + SGX_ATTR_ASYNC_EXIT_NOTIFY = BIT(10), > }; > > -#define SGX_ATTR_RESERVED_MASK (BIT_ULL(3) | BIT_ULL(6) | GENMASK_ULL(63, 8)) > +#define SGX_ATTR_RESERVED_MASK (BIT_ULL(3) | \ > + BIT_ULL(6) | \ > + BIT_ULL(8) | \ > + BIT_ULL(9) | \ > + GENMASK_ULL(63, 11)) > + > +#define SGX_ATTR_UNPRIV_MASK (SGX_ATTR_DEBUG | \ > + SGX_ATTR_MODE64BIT | \ > + SGX_ATTR_KSS | \ > + SGX_ATTR_ASYNC_EXIT_NOTIFY) > + > +#define SGX_ATTR_PRIV_MASK (SGX_ATTR_PROVISIONKEY | \ > + SGX_ATTR_EINITTOKENKEY) > > /** > * struct sgx_secs - SGX Enclave Control Structure (SECS) > diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c > index 83df20e3e633..37d523895244 100644 > --- a/arch/x86/kernel/cpu/sgx/ioctl.c > +++ b/arch/x86/kernel/cpu/sgx/ioctl.c > @@ -110,7 +110,7 @@ static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs) > encl->base = secs->base; > encl->size = secs->size; > encl->attributes = secs->attributes; > - encl->attributes_mask = SGX_ATTR_DEBUG | SGX_ATTR_MODE64BIT | SGX_ATTR_KSS; > + encl->attributes_mask = SGX_ATTR_UNPRIV_MASK; > > /* Set only after completion, as encl->lock has not been taken. */ > set_bit(SGX_ENCL_CREATED, &encl->flags); > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > index 0c1ba6aa0765..96a73b5b4369 100644 > --- a/arch/x86/kvm/cpuid.c > +++ b/arch/x86/kvm/cpuid.c > @@ -1022,9 +1022,7 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function) > * userspace. ATTRIBUTES.XFRM is not adjusted as userspace is > * expected to derive it from supported XCR0. > */ > - entry->eax &= SGX_ATTR_DEBUG | SGX_ATTR_MODE64BIT | > - SGX_ATTR_PROVISIONKEY | SGX_ATTR_EINITTOKENKEY | > - SGX_ATTR_KSS; > + entry->eax &= SGX_ATTR_PRIV_MASK | SGX_ATTR_UNPRIV_MASK; > entry->ebx &= 0; > break; > /* Intel PT */ > -- > 2.34.1 > Acked-by: Jarkko Sakkinen <jarkko@kernel.org> BR, Jarkkko
On Tue, 26 Jul 2022 16:21:53 -0500, Kai Huang <kai.huang@intel.com> wrote: > On Tue, 2022-07-26 at 10:28 -0500, Haitao Huang wrote: >> On Tue, 26 Jul 2022 05:47:14 -0500, Kai Huang <kai.huang@intel.com> >> wrote: >> >> > On Tue, 2022-07-26 at 00:10 -0500, Haitao Huang wrote: >> > > On Mon, 25 Jul 2022 05:36:17 -0500, Kai Huang <kai.huang@intel.com> >> > > wrote: >> > > >> > > > On Fri, 2022-07-22 at 08:21 -0700, Dave Hansen wrote: >> > > > > On 7/22/22 06:26, Kai Huang wrote: >> > > > > > Did a quick look at the spec. It appears ENCLU[EDECCSSA] >> should >> > > be >> > > > > used >> > > > > > together with AEX-notify. So besides advertising the new >> > > > > > SGX_ATTR_ASYNC_EXIT_NOTIFY bit to the KVM guest, I think we >> should >> > > > > also >> > > > > > advertise the ENCLU[EDECCSSA] support in guest's CPUID, like >> below >> > > > > (untested)? >> > > > > >> > > > > Sounds like a great follow-on patch! It doesn't seem truly >> > > functionally >> > > > > required from the spec: >> > > > > >> > > > > > EDECCSSA is a new Intel SGX user leaf function >> > > > > > (ENCLU[EDECCSSA]) that can facilitate AEX notification >> handling... >> > > > > >> > > > > If that's wrong or imprecise, I'd love to hear more about it and >> > > also >> > > > > about how the spec will be updated. >> > > > > >> > > > >> > > > They are enumerated separately, but looks in practice the notify >> > > handler >> > > > will >> > > > use it to switch back to the correct/targeted CSSA to continue to >> run >> > > > normally >> > > > after handling the exit notify. This is my understanding of the >> > > > "facilitate" >> > > > mean in the spec. >> > > > >> > > > Btw, in real hardware I think the two should come together, >> meaning no >> > > > real >> > > > hardware will only support one. >> > > > >> > > > Haitao, could you give us more information? >> > > > >> > > You are right. They are enumerated separately and HW should come >> with >> > > both >> > > or neither. >> > > My understanding it is also possible for enclaves choose not to >> receive >> > > AEX notify >> > > but still use EDECCSSA. >> > > >> > >> > What is the use case of using EDECCSSA w/o using AEX notify? >> > If I understand correctly EDECCSSA effectively switches to another >> > thread (using >> > the previous SSA, which is the context of another TCS thread if I >> > understand >> > correctly). Won't this cause problem? >> >> No. Decrementing CSSA is equivalent to popping stack frames, not >> switching >> threads. >> In some cases such as so-called "first stage" exception handling, one >> could pop CSSA back to the previous after resetting CPU context and >> stack >> frame appropriate to the "second stage" or "real" exception handling >> routine, then jump to the handler directly. This could improve exception >> handling performance by saving an EEXIT/ERESUME trip. >> >> > > Looking at the AEX-notify spec again, EDECCSSA does below: > > (* At this point, the instruction is guaranteed to complete *) > CR_TCS_PA.CSSA := CR_TCS_PA.CSSA - 1; > CR_GPR_PA := Physical_Address(DS:TMP_GPR); > > It doens't reset the RIP to CR_GPA_PA.RIP so looks yes you are right. > It only > "popping the stack frame" but doesn't switch thread. > > But the pseudo code of EDECCSSA only updates the CR_TCS_PA and CR_GPR_PA > registers (forget about XSAVE not), but doesn't manually updating the > actual CPU > registers such as GPRs. Are the actual CPU registers updated > automatically when > CR_xx are updated? > No, the enclave code is supposed to do that. Here is are a few more details on the flow I mentioned. On any AEX event, CPU saves states including GPR/XSave into SSA[0]. When AEX-notify is turned off, for enclaves to handle exceptions occurred inside enclave, user space must do EENTER with the same TCS on which the exception occurred. EENTER would give a clean slate of GPR and SSA[1] becomes active for next AEX. It's enclave's responsibility to save GPR/XSave states in SSA[0] to some place (e.g., stack), then EDECCSSA, then jump to the "second stage" handler. (Note now SSA[0] is reactivated and ready if another AEX occurs). The second stage handler then fixes the situation that caused the original AEX, restore CPU context from the saved SSA[0] states, jump back to original place where exception happened.
On Wed, 20 Jul 2022 14:13:47 -0500, Dave Hansen <dave.hansen@linux.intel.com> wrote: > Changes from v1: > * Make sure SGX_ATTR_ASYNC_EXIT_NOTIFY is in the masks that are > used at bare-metal enclave initialization and that enumerates > available attributes to KVM guests. > > -- > > Short Version: > > Allow enclaves to use the new Asynchronous EXit (AEX) > notification mechanism. This mechanism lets enclaves run a > handler after an AEX event. These handlers can run mitigations > for things like SGX-Step[1]. > > AEX Notify will be made available both on upcoming processors and > on some older processors through microcode updates. > > Long Version: > > == SGX Attribute Background == > > The SGX architecture includes a list of SGX "attributes". These > attributes ensure consistency and transparency around specific > enclave features. > > As a simple example, the "DEBUG" attribute allows an enclave to > be debugged, but also destroys virtually all of SGX security. > Using attributes, enclaves can know that they are being debugged. > Attributes also affect enclave attestation so an enclave can, for > instance, be denied access to secrets while it is being debugged. > > The kernel keeps a list of known attributes and will only > initialize enclaves that use a known set of attributes. This > kernel policy eliminates the chance that a new SGX attribute > could cause undesired effects. > > For example, imagine a new attribute was added called > "PROVISIONKEY2" that provided similar functionality to > "PROVISIIONKEY". A kernel policy that allowed indiscriminate use > of unknown attributes and thus PROVISIONKEY2 would undermine the > existing kernel policy which limits use of PROVISIONKEY enclaves. > > == AEX Notify Background == > > "Intel Architecture Instruction Set Extensions and Future > Features - Version 45" is out[2]. There is a new chapter: > > Asynchronous Enclave Exit Notify and the EDECCSSA User Leaf Function. > > Enclaves exit can be either synchronous and consensual (EEXIT for > instance) or asynchronous (on an interrupt or fault). The > asynchronous ones can evidently be exploited to single step > enclaves[1], on top of which other naughty things can be built. > > AEX Notify will be made available both on upcoming processors and > on some older processors through microcode updates. > > == The Problem == > > These attacks are currently entirely opaque to the enclave since > the hardware does the save/restore under the covers. The > Asynchronous Enclave Exit Notify (AEX Notify) mechanism provides > enclaves an ability to detect and mitigate potential exposure to > these kinds of attacks. > > == The Solution == > > Define the new attribute value for AEX Notification. Ensure the > attribute is cleared from the list reserved attributes. Instead > of adding to the open-coded lists of individual attributes, > add named lists of privileged (disallowed by default) and > unprivileged (allowed by default) attributes. Add the AEX notify > attribute as an unprivileged attribute, which will keep the kernel > from rejecting enclaves with it set. > > I just built this and ran it to make sure there were no obvious > regressions since I do not have the hardware (and new microcde) > to test it. > > Testing on bare-metal and in VMs accompanied by Tested-by's > would be much appreciated. (This means you, Intel folks who > actually have systems with the microcode that can do this.) > > 1. https://github.com/jovanbulck/sgx-step > 2. https://cdrdv2.intel.com/v1/dl/getContent/671368?explicitVersion=true > > Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com> > Cc: Jarkko Sakkinen <jarkko@kernel.org> > Cc: Andy Lutomirski <luto@kernel.org> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: Borislav Petkov <bp@alien8.de> > Cc: "H. Peter Anvin" <hpa@zytor.com> > Cc: Sean Christopherson <seanjc@google.com> > Cc: Kai Huang <kai.huang@intel.com> > Cc: Haitao Huang <haitao.huang@linux.intel.com> > Cc: x86@kernel.org > Cc: linux-sgx@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > --- > arch/x86/include/asm/sgx.h | 33 ++++++++++++++++++++++++++------- > arch/x86/kernel/cpu/sgx/ioctl.c | 2 +- > arch/x86/kvm/cpuid.c | 4 +--- > 3 files changed, 28 insertions(+), 11 deletions(-) > > diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h > index 3f9334ef67cd..3004dfe76498 100644 > --- a/arch/x86/include/asm/sgx.h > +++ b/arch/x86/include/asm/sgx.h > @@ -110,17 +110,36 @@ enum sgx_miscselect { > * %SGX_ATTR_EINITTOKENKEY: Allow to use token signing key that is used > to > * sign cryptographic tokens that can be passed to > * EINIT as an authorization to run an enclave. > + * %SGX_ATTR_ASYNC_EXIT_NOTIFY: Allow enclaves to be notified after an > + * asynchronous exit has occurred. > */ > enum sgx_attribute { > - SGX_ATTR_INIT = BIT(0), > - SGX_ATTR_DEBUG = BIT(1), > - SGX_ATTR_MODE64BIT = BIT(2), > - SGX_ATTR_PROVISIONKEY = BIT(4), > - SGX_ATTR_EINITTOKENKEY = BIT(5), > - SGX_ATTR_KSS = BIT(7), > + SGX_ATTR_INIT = BIT(0), > + SGX_ATTR_DEBUG = BIT(1), > + SGX_ATTR_MODE64BIT = BIT(2), > + /* BIT(3) is reserved */ > + SGX_ATTR_PROVISIONKEY = BIT(4), > + SGX_ATTR_EINITTOKENKEY = BIT(5), > + /* BIT(6) is for CET */ > + SGX_ATTR_KSS = BIT(7), > + /* BIT(8) is reserved */ > + /* BIT(9) is reserved */ > + SGX_ATTR_ASYNC_EXIT_NOTIFY = BIT(10), > }; > -#define SGX_ATTR_RESERVED_MASK (BIT_ULL(3) | BIT_ULL(6) | > GENMASK_ULL(63, 8)) > +#define SGX_ATTR_RESERVED_MASK (BIT_ULL(3) | \ > + BIT_ULL(6) | \ > + BIT_ULL(8) | \ > + BIT_ULL(9) | \ > + GENMASK_ULL(63, 11)) > + > +#define SGX_ATTR_UNPRIV_MASK (SGX_ATTR_DEBUG | \ > + SGX_ATTR_MODE64BIT | \ > + SGX_ATTR_KSS | \ > + SGX_ATTR_ASYNC_EXIT_NOTIFY) > + > +#define SGX_ATTR_PRIV_MASK (SGX_ATTR_PROVISIONKEY | \ > + SGX_ATTR_EINITTOKENKEY) > /** > * struct sgx_secs - SGX Enclave Control Structure (SECS) > diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c > b/arch/x86/kernel/cpu/sgx/ioctl.c > index 83df20e3e633..37d523895244 100644 > --- a/arch/x86/kernel/cpu/sgx/ioctl.c > +++ b/arch/x86/kernel/cpu/sgx/ioctl.c > @@ -110,7 +110,7 @@ static int sgx_encl_create(struct sgx_encl *encl, > struct sgx_secs *secs) > encl->base = secs->base; > encl->size = secs->size; > encl->attributes = secs->attributes; > - encl->attributes_mask = SGX_ATTR_DEBUG | SGX_ATTR_MODE64BIT | > SGX_ATTR_KSS; > + encl->attributes_mask = SGX_ATTR_UNPRIV_MASK; > /* Set only after completion, as encl->lock has not been taken. */ > set_bit(SGX_ENCL_CREATED, &encl->flags); > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > index 0c1ba6aa0765..96a73b5b4369 100644 > --- a/arch/x86/kvm/cpuid.c > +++ b/arch/x86/kvm/cpuid.c > @@ -1022,9 +1022,7 @@ static inline int __do_cpuid_func(struct > kvm_cpuid_array *array, u32 function) > * userspace. ATTRIBUTES.XFRM is not adjusted as userspace is > * expected to derive it from supported XCR0. > */ > - entry->eax &= SGX_ATTR_DEBUG | SGX_ATTR_MODE64BIT | > - SGX_ATTR_PROVISIONKEY | SGX_ATTR_EINITTOKENKEY | > - SGX_ATTR_KSS; > + entry->eax &= SGX_ATTR_PRIV_MASK | SGX_ATTR_UNPRIV_MASK; > entry->ebx &= 0; > break; > /* Intel PT */ Tested-by: Haitao Huang <haitao.huang@intel.com> Thanks Haitao
> > Tested-by: Haitao Huang <haitao.huang@intel.com> > > Thanks > Haitao Hi Haitao, Could you also help to test in a VM? You will also need below patch in order to use EDECCSSA in the guest: https://lore.kernel.org/lkml/20220727115442.464380-1-kai.huang@intel.com/ When you create the VM, please use -cpu host.
On Wed, Jul 20, 2022 at 12:13:47PM -0700, Dave Hansen wrote: > Changes from v1: > * Make sure SGX_ATTR_ASYNC_EXIT_NOTIFY is in the masks that are > used at bare-metal enclave initialization and that enumerates > available attributes to KVM guests. > > -- > > Short Version: > > Allow enclaves to use the new Asynchronous EXit (AEX) > notification mechanism. This mechanism lets enclaves run a > handler after an AEX event. These handlers can run mitigations > for things like SGX-Step[1]. > > AEX Notify will be made available both on upcoming processors and > on some older processors through microcode updates. > > Long Version: > > == SGX Attribute Background == > > The SGX architecture includes a list of SGX "attributes". These > attributes ensure consistency and transparency around specific > enclave features. > > As a simple example, the "DEBUG" attribute allows an enclave to > be debugged, but also destroys virtually all of SGX security. > Using attributes, enclaves can know that they are being debugged. > Attributes also affect enclave attestation so an enclave can, for > instance, be denied access to secrets while it is being debugged. > > The kernel keeps a list of known attributes and will only > initialize enclaves that use a known set of attributes. This > kernel policy eliminates the chance that a new SGX attribute > could cause undesired effects. > > For example, imagine a new attribute was added called > "PROVISIONKEY2" that provided similar functionality to > "PROVISIIONKEY". A kernel policy that allowed indiscriminate use > of unknown attributes and thus PROVISIONKEY2 would undermine the > existing kernel policy which limits use of PROVISIONKEY enclaves. > > == AEX Notify Background == > > "Intel Architecture Instruction Set Extensions and Future > Features - Version 45" is out[2]. There is a new chapter: > > Asynchronous Enclave Exit Notify and the EDECCSSA User Leaf Function. > > Enclaves exit can be either synchronous and consensual (EEXIT for > instance) or asynchronous (on an interrupt or fault). The > asynchronous ones can evidently be exploited to single step > enclaves[1], on top of which other naughty things can be built. > > AEX Notify will be made available both on upcoming processors and > on some older processors through microcode updates. > > == The Problem == > > These attacks are currently entirely opaque to the enclave since > the hardware does the save/restore under the covers. The > Asynchronous Enclave Exit Notify (AEX Notify) mechanism provides > enclaves an ability to detect and mitigate potential exposure to > these kinds of attacks. > > == The Solution == > > Define the new attribute value for AEX Notification. Ensure the > attribute is cleared from the list reserved attributes. Instead > of adding to the open-coded lists of individual attributes, > add named lists of privileged (disallowed by default) and > unprivileged (allowed by default) attributes. Add the AEX notify > attribute as an unprivileged attribute, which will keep the kernel > from rejecting enclaves with it set. > > I just built this and ran it to make sure there were no obvious > regressions since I do not have the hardware (and new microcde) > to test it. > > Testing on bare-metal and in VMs accompanied by Tested-by's > would be much appreciated. (This means you, Intel folks who > actually have systems with the microcode that can do this.) > > 1. https://github.com/jovanbulck/sgx-step > 2. https://cdrdv2.intel.com/v1/dl/getContent/671368?explicitVersion=true > > Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com> > Cc: Jarkko Sakkinen <jarkko@kernel.org> > Cc: Andy Lutomirski <luto@kernel.org> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: Borislav Petkov <bp@alien8.de> > Cc: "H. Peter Anvin" <hpa@zytor.com> > Cc: Sean Christopherson <seanjc@google.com> > Cc: Kai Huang <kai.huang@intel.com> > Cc: Haitao Huang <haitao.huang@linux.intel.com> > Cc: x86@kernel.org > Cc: linux-sgx@vger.kernel.org > Cc: linux-kernel@vger.kernel.org I think it would make sense to have a co-maintainer with better access to unreleased ucode patches, if anyone is willing to consider. Perhaps someone from Intel given the constraints. This would help with features such as AEX Notify. I can work out issues with existing hardware and also make sure that the whole stack is usable (as I'm also consumer for SGX). BR, Jarkko
On Tue, 2022-08-02 at 14:21 +1200, Kai Huang wrote: > > > > Tested-by: Haitao Huang <haitao.huang@intel.com> > > > > Thanks > > Haitao > > Hi Haitao, > > Could you also help to test in a VM? > > You will also need below patch in order to use EDECCSSA in the guest: > > https://lore.kernel.org/lkml/20220727115442.464380-1-kai.huang@intel.com/ > > When you create the VM, please use -cpu host. > Hi Haitao, Do you have any update? If it's not easy for you to verify in VM, could you let me know how to set up the testing machine so I can have a try?
On Wed, Aug 10, 2022 at 10:17:31AM +0000, Huang, Kai wrote: > On Tue, 2022-08-02 at 14:21 +1200, Kai Huang wrote: > > > > > > Tested-by: Haitao Huang <haitao.huang@intel.com> > > > > > > Thanks > > > Haitao > > > > Hi Haitao, > > > > Could you also help to test in a VM? > > > > You will also need below patch in order to use EDECCSSA in the guest: > > > > https://lore.kernel.org/lkml/20220727115442.464380-1-kai.huang@intel.com/ > > > > When you create the VM, please use -cpu host. > > > > Hi Haitao, > > Do you have any update? > > If it's not easy for you to verify in VM, could you let me know how to set up > the testing machine so I can have a try? I can give ack for this, given that it is so obvious: Acked-by: Jarkko Sakkinen <jarkko@kernel.org> Would give reviewed-by if there was ucode update available, and I could test this. BR, Jarkko
> On Wed, Aug 10, 2022 at 10:17:31AM +0000, Huang, Kai wrote: > > On Tue, 2022-08-02 at 14:21 +1200, Kai Huang wrote: > > > > > > > > Tested-by: Haitao Huang <haitao.huang@intel.com> > > > > > > > > Thanks > > > > Haitao > > > > > > Hi Haitao, > > > > > > Could you also help to test in a VM? > > > > > > You will also need below patch in order to use EDECCSSA in the guest: > > > > > > https://lore.kernel.org/lkml/20220727115442.464380-1-kai.huang@intel > > > .com/ > > > > > > When you create the VM, please use -cpu host. > > > > > > > Hi Haitao, > > > > Do you have any update? > > > > If it's not easy for you to verify in VM, could you let me know how to > > set up the testing machine so I can have a try? > > I can give ack for this, given that it is so obvious: > > Acked-by: Jarkko Sakkinen <jarkko@kernel.org> > > Would give reviewed-by if there was ucode update available, and I could test > this. > I don't know whether there's ucode update available. Maybe Haitao has more information. I talked to Haitao and I'll try to verify this in a VM. Thanks, -Kai
On Thu, Aug 11, 2022 at 01:57:46AM +0000, Huang, Kai wrote: > > On Wed, Aug 10, 2022 at 10:17:31AM +0000, Huang, Kai wrote: > > > On Tue, 2022-08-02 at 14:21 +1200, Kai Huang wrote: > > > > > > > > > > Tested-by: Haitao Huang <haitao.huang@intel.com> > > > > > > > > > > Thanks > > > > > Haitao > > > > > > > > Hi Haitao, > > > > > > > > Could you also help to test in a VM? > > > > > > > > You will also need below patch in order to use EDECCSSA in the guest: > > > > > > > > https://lore.kernel.org/lkml/20220727115442.464380-1-kai.huang@intel > > > > .com/ > > > > > > > > When you create the VM, please use -cpu host. > > > > > > > > > > Hi Haitao, > > > > > > Do you have any update? > > > > > > If it's not easy for you to verify in VM, could you let me know how to > > > set up the testing machine so I can have a try? > > > > I can give ack for this, given that it is so obvious: > > > > Acked-by: Jarkko Sakkinen <jarkko@kernel.org> > > > > Would give reviewed-by if there was ucode update available, and I could test > > this. > > > > I don't know whether there's ucode update available. Maybe Haitao has more information. > > I talked to Haitao and I'll try to verify this in a VM. > > Thanks, > -Kai There's no release for Icelake existing yet with AEX Notify. BR, Jarkko
On Thu, 2022-08-11 at 01:57 +0000, Huang, Kai wrote: > > On Wed, Aug 10, 2022 at 10:17:31AM +0000, Huang, Kai wrote: > > > On Tue, 2022-08-02 at 14:21 +1200, Kai Huang wrote: > > > > > > > > > > Tested-by: Haitao Huang <haitao.huang@intel.com> > > > > > > > > > > Thanks > > > > > Haitao > > > > > > > > Hi Haitao, > > > > > > > > Could you also help to test in a VM? > > > > > > > > You will also need below patch in order to use EDECCSSA in the guest: > > > > > > > > https://lore.kernel.org/lkml/20220727115442.464380-1-kai.huang@intel > > > > .com/ > > > > > > > > When you create the VM, please use -cpu host. > > > > > > > > > > Hi Haitao, > > > > > > Do you have any update? > > > > > > If it's not easy for you to verify in VM, could you let me know how to > > > set up the testing machine so I can have a try? > > > > I can give ack for this, given that it is so obvious: > > > > Acked-by: Jarkko Sakkinen <jarkko@kernel.org> > > > > Would give reviewed-by if there was ucode update available, and I could test > > this. > > > > I don't know whether there's ucode update available. Maybe Haitao has more > information. > > I talked to Haitao and I'll try to verify this in a VM. > Together with the patch to expose EDECCSSA to guest: https://lore.kernel.org/linux-sgx/20220818023829.1250080-1-kai.huang@intel.com/T/#u I have tested both AEX-notify and EDECCSSA work in KVM guest. So: Tested-by: Kai Huang <kai.huang@intel.com>
diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h index 3f9334ef67cd..3004dfe76498 100644 --- a/arch/x86/include/asm/sgx.h +++ b/arch/x86/include/asm/sgx.h @@ -110,17 +110,36 @@ enum sgx_miscselect { * %SGX_ATTR_EINITTOKENKEY: Allow to use token signing key that is used to * sign cryptographic tokens that can be passed to * EINIT as an authorization to run an enclave. + * %SGX_ATTR_ASYNC_EXIT_NOTIFY: Allow enclaves to be notified after an + * asynchronous exit has occurred. */ enum sgx_attribute { - SGX_ATTR_INIT = BIT(0), - SGX_ATTR_DEBUG = BIT(1), - SGX_ATTR_MODE64BIT = BIT(2), - SGX_ATTR_PROVISIONKEY = BIT(4), - SGX_ATTR_EINITTOKENKEY = BIT(5), - SGX_ATTR_KSS = BIT(7), + SGX_ATTR_INIT = BIT(0), + SGX_ATTR_DEBUG = BIT(1), + SGX_ATTR_MODE64BIT = BIT(2), + /* BIT(3) is reserved */ + SGX_ATTR_PROVISIONKEY = BIT(4), + SGX_ATTR_EINITTOKENKEY = BIT(5), + /* BIT(6) is for CET */ + SGX_ATTR_KSS = BIT(7), + /* BIT(8) is reserved */ + /* BIT(9) is reserved */ + SGX_ATTR_ASYNC_EXIT_NOTIFY = BIT(10), }; -#define SGX_ATTR_RESERVED_MASK (BIT_ULL(3) | BIT_ULL(6) | GENMASK_ULL(63, 8)) +#define SGX_ATTR_RESERVED_MASK (BIT_ULL(3) | \ + BIT_ULL(6) | \ + BIT_ULL(8) | \ + BIT_ULL(9) | \ + GENMASK_ULL(63, 11)) + +#define SGX_ATTR_UNPRIV_MASK (SGX_ATTR_DEBUG | \ + SGX_ATTR_MODE64BIT | \ + SGX_ATTR_KSS | \ + SGX_ATTR_ASYNC_EXIT_NOTIFY) + +#define SGX_ATTR_PRIV_MASK (SGX_ATTR_PROVISIONKEY | \ + SGX_ATTR_EINITTOKENKEY) /** * struct sgx_secs - SGX Enclave Control Structure (SECS) diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c index 83df20e3e633..37d523895244 100644 --- a/arch/x86/kernel/cpu/sgx/ioctl.c +++ b/arch/x86/kernel/cpu/sgx/ioctl.c @@ -110,7 +110,7 @@ static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs) encl->base = secs->base; encl->size = secs->size; encl->attributes = secs->attributes; - encl->attributes_mask = SGX_ATTR_DEBUG | SGX_ATTR_MODE64BIT | SGX_ATTR_KSS; + encl->attributes_mask = SGX_ATTR_UNPRIV_MASK; /* Set only after completion, as encl->lock has not been taken. */ set_bit(SGX_ENCL_CREATED, &encl->flags); diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index 0c1ba6aa0765..96a73b5b4369 100644 --- a/arch/x86/kvm/cpuid.c +++ b/arch/x86/kvm/cpuid.c @@ -1022,9 +1022,7 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function) * userspace. ATTRIBUTES.XFRM is not adjusted as userspace is * expected to derive it from supported XCR0. */ - entry->eax &= SGX_ATTR_DEBUG | SGX_ATTR_MODE64BIT | - SGX_ATTR_PROVISIONKEY | SGX_ATTR_EINITTOKENKEY | - SGX_ATTR_KSS; + entry->eax &= SGX_ATTR_PRIV_MASK | SGX_ATTR_UNPRIV_MASK; entry->ebx &= 0; break; /* Intel PT */
Changes from v1: * Make sure SGX_ATTR_ASYNC_EXIT_NOTIFY is in the masks that are used at bare-metal enclave initialization and that enumerates available attributes to KVM guests. -- Short Version: Allow enclaves to use the new Asynchronous EXit (AEX) notification mechanism. This mechanism lets enclaves run a handler after an AEX event. These handlers can run mitigations for things like SGX-Step[1]. AEX Notify will be made available both on upcoming processors and on some older processors through microcode updates. Long Version: == SGX Attribute Background == The SGX architecture includes a list of SGX "attributes". These attributes ensure consistency and transparency around specific enclave features. As a simple example, the "DEBUG" attribute allows an enclave to be debugged, but also destroys virtually all of SGX security. Using attributes, enclaves can know that they are being debugged. Attributes also affect enclave attestation so an enclave can, for instance, be denied access to secrets while it is being debugged. The kernel keeps a list of known attributes and will only initialize enclaves that use a known set of attributes. This kernel policy eliminates the chance that a new SGX attribute could cause undesired effects. For example, imagine a new attribute was added called "PROVISIONKEY2" that provided similar functionality to "PROVISIIONKEY". A kernel policy that allowed indiscriminate use of unknown attributes and thus PROVISIONKEY2 would undermine the existing kernel policy which limits use of PROVISIONKEY enclaves. == AEX Notify Background == "Intel Architecture Instruction Set Extensions and Future Features - Version 45" is out[2]. There is a new chapter: Asynchronous Enclave Exit Notify and the EDECCSSA User Leaf Function. Enclaves exit can be either synchronous and consensual (EEXIT for instance) or asynchronous (on an interrupt or fault). The asynchronous ones can evidently be exploited to single step enclaves[1], on top of which other naughty things can be built. AEX Notify will be made available both on upcoming processors and on some older processors through microcode updates. == The Problem == These attacks are currently entirely opaque to the enclave since the hardware does the save/restore under the covers. The Asynchronous Enclave Exit Notify (AEX Notify) mechanism provides enclaves an ability to detect and mitigate potential exposure to these kinds of attacks. == The Solution == Define the new attribute value for AEX Notification. Ensure the attribute is cleared from the list reserved attributes. Instead of adding to the open-coded lists of individual attributes, add named lists of privileged (disallowed by default) and unprivileged (allowed by default) attributes. Add the AEX notify attribute as an unprivileged attribute, which will keep the kernel from rejecting enclaves with it set. I just built this and ran it to make sure there were no obvious regressions since I do not have the hardware (and new microcde) to test it. Testing on bare-metal and in VMs accompanied by Tested-by's would be much appreciated. (This means you, Intel folks who actually have systems with the microcode that can do this.) 1. https://github.com/jovanbulck/sgx-step 2. https://cdrdv2.intel.com/v1/dl/getContent/671368?explicitVersion=true Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com> Cc: Jarkko Sakkinen <jarkko@kernel.org> Cc: Andy Lutomirski <luto@kernel.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Ingo Molnar <mingo@redhat.com> Cc: Borislav Petkov <bp@alien8.de> Cc: "H. Peter Anvin" <hpa@zytor.com> Cc: Sean Christopherson <seanjc@google.com> Cc: Kai Huang <kai.huang@intel.com> Cc: Haitao Huang <haitao.huang@linux.intel.com> Cc: x86@kernel.org Cc: linux-sgx@vger.kernel.org Cc: linux-kernel@vger.kernel.org --- arch/x86/include/asm/sgx.h | 33 ++++++++++++++++++++++++++------- arch/x86/kernel/cpu/sgx/ioctl.c | 2 +- arch/x86/kvm/cpuid.c | 4 +--- 3 files changed, 28 insertions(+), 11 deletions(-)