[PATCH] [v2] x86/sgx: Allow enclaves to use Asynchrounous Exit Notification

[PATCH] [v2] x86/sgx: Allow enclaves to use Asynchrounous Exit Notification

[Dave Hansen]

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

Re: [PATCH] [v2] x86/sgx: Allow enclaves to use Asynchrounous Exit Notification

[Sean Christopherson]

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.

Re: [PATCH] [v2] x86/sgx: Allow enclaves to use Asynchrounous Exit Notification

[Dave Hansen]

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.

Re: [PATCH] [v2] x86/sgx: Allow enclaves to use Asynchrounous Exit Notification

[Kai Huang]

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;
 }

Re: [PATCH] [v2] x86/sgx: Allow enclaves to use Asynchrounous Exit Notification

[Dave Hansen]

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.

Re: [PATCH] [v2] x86/sgx: Allow enclaves to use Asynchrounous Exit Notification

[Sean Christopherson]

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... 

Re: [PATCH] [v2] x86/sgx: Allow enclaves to use Asynchrounous Exit Notification

[Sean Christopherson]

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;

Re: [PATCH] [v2] x86/sgx: Allow enclaves to use Asynchrounous Exit Notification

[Dave Hansen]

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!

Re: [PATCH] [v2] x86/sgx: Allow enclaves to use Asynchrounous Exit Notification

[Kai Huang]

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?

-- 
Thanks,
-Kai

Re: [PATCH] [v2] x86/sgx: Allow enclaves to use Asynchrounous Exit Notification

[Haitao Huang]

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

Re: [PATCH] [v2] x86/sgx: Allow enclaves to use Asynchrounous Exit Notification

[Kai Huang]

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.

-- 
Thanks,
-Kai

Re: [PATCH] [v2] x86/sgx: Allow enclaves to use Asynchrounous Exit Notification

[Haitao Huang]

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.

Re: [PATCH] [v2] x86/sgx: Allow enclaves to use Asynchrounous Exit Notification

[Dave Hansen]

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.

Re: [PATCH] [v2] x86/sgx: Allow enclaves to use Asynchrounous Exit Notification

[Kai Huang]

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?

-- 
Thanks,
-Kai

Re: [PATCH] [v2] x86/sgx: Allow enclaves to use Asynchrounous Exit Notification

[Kai Huang]

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.

-- 
Thanks,
-Kai

Re: [PATCH] [v2] x86/sgx: Allow enclaves to use Asynchrounous Exit Notification

[Jarkko Sakkinen]

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

Re: [PATCH] [v2] x86/sgx: Allow enclaves to use Asynchrounous Exit Notification

[Haitao Huang]

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.

Re: [PATCH] [v2] x86/sgx: Allow enclaves to use Asynchrounous Exit Notification

[Haitao Huang]

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

Re: [PATCH] [v2] x86/sgx: Allow enclaves to use Asynchrounous Exit Notification

[Kai Huang]

> 
> 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.

-- 
Thanks,
-Kai

Re: [PATCH] [v2] x86/sgx: Allow enclaves to use Asynchrounous Exit Notification

[Jarkko Sakkinen]

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

Re: [PATCH] [v2] x86/sgx: Allow enclaves to use Asynchrounous Exit Notification

[Huang, Kai]

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?

-- 
Thanks,
-Kai

Re: [PATCH] [v2] x86/sgx: Allow enclaves to use Asynchrounous Exit Notification

[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.

BR, Jarkko

RE: [PATCH] [v2] x86/sgx: Allow enclaves to use Asynchrounous Exit Notification

[Huang, Kai]

> 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

Re: [PATCH] [v2] x86/sgx: Allow enclaves to use Asynchrounous Exit Notification

[jarkko]

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

Re: [PATCH] [v2] x86/sgx: Allow enclaves to use Asynchrounous Exit Notification

[Huang, Kai]

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>

-- 
Thanks,
-Kai