[RFC PATCH 0/4] x86/vdso: x86/sgx: Rework SGX vDSO API

[RFC PATCH 0/4] x86/vdso: x86/sgx: Rework SGX vDSO API

[Sean Christopherson]

Rework __vdso_sgx_enter_enclave() to move all input/output params, except
for pass-through GPRs, into a single struct.  With the new struct, add
two new features (requested by Nathaniel and Jethro), and fix a
long-standing nit (from Andy).

 1. Add an opaque param to pass data from the runtime to its handler.
    https://lkml.kernel.org/r/CAOASepOFh-vOrNZEVDFrDSuHs+9GEzzpXUTG-fZMuyjWAkpRWw@mail.gmail.com

 2. Allow the runtime to exit the vDSO on interrupts, e.g. for context
    switching when doing M:N scheduling of enclave threads.
    https://lkml.kernel.org/r/dcebec2e-ea46-48ec-e49b-292b10282373@fortanix.com

 3. Use a dedicated exit reason instead of using -EFAULT for "exception"
    (and effectively -EINTR for interrupts, too).
    https://lkml.kernel.org/r/90D05734-1583-4306-A9A4-18E4A1390F3B@amacapital.net

Patch 1 is a bug fix I found by inspection when reworking the code.

Reworking so much of the code this late in the game is a bit scary, but
the alternative is massive param lists for both the vDSO and the handler,
especially if we add both a flags param and an opaque pointer.  And IMO,
the result is also a tiny bit cleaner than what we have today, even
without adding @flags and @opaque.

typedef int (*vdso_sgx_enter_enclave_t)(unsigned long rdi, unsigned long rsi,
                                        unsigned long rdx, unsigned int leaf,
                                        unsigned long r8,  unsigned long r9,
                                        struct sgx_enclave_run *r);

typedef int (*sgx_enclave_exit_handler_t)(long rdi, long rsi, long rdx,
                                          long ursp, long r8, long r9,
                                          struct sgx_enclave_run *r);

vs.

typedef int (*vdso_sgx_enter_enclave_t)(unsigned long rdi, unsigned long rsi,
                                        unsigned long rdx, unsigned int leaf,
                                        unsigned long r8,  unsigned long r9,
                                        void *tcs,
                                        struct sgx_enclave_exception *e,
                                        sgx_enclave_exit_handler_t handler,
                                        unsigned long flags,
                                        unsigned long opaque);

typedef int (*sgx_enclave_exit_handler_t)(long rdi, long rsi, long rdx,
                                          long ursp, long r8, long r9,
                                          void *tcs, int ret,
                                          struct sgx_enclave_exception *e,
                                          unsigned long opaque);

Sean Christopherson (4):
  x86/vdso: x86/sgx: Explicitly force 8-byte CMP for detecting user
    handler
  x86/vdso: x86/sgx: Rework __vdso_sgx_enter_enclave() API
  x86/vdso: x86/sgx: Introduce dedicated SGX exit reasons for vDSO
  x86/vdso: x86/sgx: Allow the user to exit the vDSO loop on interrupts

 arch/x86/entry/vdso/vsgx_enter_enclave.S | 94 +++++++++++++++++------
 arch/x86/include/uapi/asm/sgx.h          | 96 ++++++++++++++++--------
 2 files changed, 135 insertions(+), 55 deletions(-)

-- 
2.28.0

[RFC PATCH 1/4] x86/vdso: x86/sgx: Explicitly force 8-byte CMP for detecting user handler

[Sean Christopherson]

Use 'cmpq' to force an 8-byte CMP when checking for a user provided exit
handler.  The handler is a pointer, which is guaranteed to be an 8-byte
value since SGX is 64-bit mode only, and gcc defaults to 'cmpl' given a
bare 'cmp', i.e. is only checking the lower 32 bits.  This could cause
a false negative when detecting a user exit handler.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/entry/vdso/vsgx_enter_enclave.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.S b/arch/x86/entry/vdso/vsgx_enter_enclave.S
index be7e467e1efb3..2d88acd408d4e 100644
--- a/arch/x86/entry/vdso/vsgx_enter_enclave.S
+++ b/arch/x86/entry/vdso/vsgx_enter_enclave.S
@@ -48,7 +48,7 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave)
 
 	/* Invoke userspace's exit handler if one was provided. */
 .Lhandle_exit:
-	cmp	$0, 0x20(%rbp)
+	cmpq	$0, 0x20(%rbp)
 	jne	.Linvoke_userspace_handler
 
 .Lout:
-- 
2.28.0

[RFC PATCH 2/4] x86/vdso: x86/sgx: Rework __vdso_sgx_enter_enclave() API

[Sean Christopherson]

Rework __vdso_sgx_enter_enclave() to use a struct to hold the input and
output params.  In the new struct, add an opaque "user_data" that can be
used to pass context across the vDSO, and an explicit "exit_reason" to
avoid overloading the return value.

Moving the params into a struct will also make it less painful to use
dedicated exit reasons, and to support exiting on interrupts in future
patches.

Cc: Nathaniel McCallum <npmccallum@redhat.com>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/entry/vdso/vsgx_enter_enclave.S | 72 ++++++++++++-------
 arch/x86/include/uapi/asm/sgx.h          | 90 ++++++++++++++++--------
 2 files changed, 107 insertions(+), 55 deletions(-)

diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.S b/arch/x86/entry/vdso/vsgx_enter_enclave.S
index 2d88acd408d4e..aaae6d6e28ac3 100644
--- a/arch/x86/entry/vdso/vsgx_enter_enclave.S
+++ b/arch/x86/entry/vdso/vsgx_enter_enclave.S
@@ -7,9 +7,21 @@
 
 #include "extable.h"
 
-#define EX_LEAF		0*8
-#define EX_TRAPNR	0*8+4
-#define EX_ERROR_CODE	0*8+6
+/* Offset of 'struct sgx_enter_enclave' relative to %rbp. */
+#define RUN_OFFSET	2*8
+
+/* Offsets into 'struct sgx_enter_enclave'. */
+#define TCS_OFFEST		0*8
+#define FLAGS_OFFSET		1*8
+#define EXIT_LEAF_OFFSET	2*8
+#define EXIT_REASON_OFFSET	2*8 + 4
+#define USER_HANDLER_OFFSET	3*8
+/* #define USER_DATA_OFFSET	4*8 */
+#define EXCEPTION_OFFSET	5*8
+
+/* Offsets into sgx_enter_enclave.exception. */
+#define EX_TRAPNR	0*8
+#define EX_ERROR_CODE	0*8+2
 #define EX_ADDRESS	1*8
 
 .code64
@@ -30,12 +42,18 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave)
 .Lenter_enclave:
 	/* EENTER <= leaf <= ERESUME */
 	cmp	$EENTER, %eax
-	jb	.Linvalid_leaf
+	jb	.Linvalid_input
 	cmp	$ERESUME, %eax
-	ja	.Linvalid_leaf
+	ja	.Linvalid_input
+
+	mov	RUN_OFFSET(%rbp), %rcx
+
+	/* No flags are currently defined/supported. */
+	cmpq	$0, FLAGS_OFFSET(%rcx)
+	jne	.Linvalid_input
 
 	/* Load TCS and AEP */
-	mov	0x10(%rbp), %rbx
+	mov	TCS_OFFEST(%rcx), %rbx
 	lea	.Lasync_exit_pointer(%rip), %rcx
 
 	/* Single ENCLU serving as both EENTER and AEP (ERESUME) */
@@ -44,13 +62,21 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave)
 	enclu
 
 	/* EEXIT jumps here unless the enclave is doing something fancy. */
-	xor	%eax, %eax
+	mov	RUN_OFFSET(%rbp), %rbx
+
+	/* Set exit_reason. */
+	movl	$0, EXIT_REASON_OFFSET(%rbx)
 
 	/* Invoke userspace's exit handler if one was provided. */
 .Lhandle_exit:
-	cmpq	$0, 0x20(%rbp)
+	mov	%eax, EXIT_LEAF_OFFSET(%rbx)
+
+	cmpq	$0, USER_HANDLER_OFFSET(%rbx)
 	jne	.Linvoke_userspace_handler
 
+	/* Success, in the sense that ENCLU was attempted. */
+	xor	%eax, %eax
+
 .Lout:
 	pop	%rbx
 	leave
@@ -60,28 +86,28 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave)
 	/* The out-of-line code runs with the pre-leave stack frame. */
 	.cfi_def_cfa		%rbp, 16
 
-.Linvalid_leaf:
+.Linvalid_input:
 	mov	$(-EINVAL), %eax
 	jmp	.Lout
 
 .Lhandle_exception:
-	mov	0x18(%rbp), %rcx
-	test    %rcx, %rcx
-	je	.Lskip_exception_info
+	mov	RUN_OFFSET(%rbp), %rbx
 
-	/* Fill optional exception info. */
-	mov	%eax, EX_LEAF(%rcx)
-	mov	%di,  EX_TRAPNR(%rcx)
-	mov	%si,  EX_ERROR_CODE(%rcx)
-	mov	%rdx, EX_ADDRESS(%rcx)
-.Lskip_exception_info:
-	mov	$(-EFAULT), %eax
+	/* Set the exit_reason and exception info. */
+	movl	$(-EFAULT), EXIT_REASON_OFFSET(%rbx)
+
+	mov	%di,  (EXCEPTION_OFFSET + EX_TRAPNR)(%rbx)
+	mov	%si,  (EXCEPTION_OFFSET + EX_ERROR_CODE)(%rbx)
+	mov	%rdx, (EXCEPTION_OFFSET + EX_ADDRESS)(%rbx)
 	jmp	.Lhandle_exit
 
 .Linvoke_userspace_handler:
 	/* Pass the untrusted RSP (at exit) to the callback via %rcx. */
 	mov	%rsp, %rcx
 
+	/* Save @e, %rbx is about to be clobbered. */
+	mov	%rbx, %rax
+
 	/* Save the untrusted RSP offset in %rbx (non-volatile register). */
 	mov	%rsp, %rbx
 	and	$0xf, %rbx
@@ -93,20 +119,18 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave)
 	and	$-0x10, %rsp
 	push	%rax
 
-	/* Push @e, the "return" value and @tcs as params to the callback. */
-	push	0x18(%rbp)
+	/* Push @e as a param to the callback. */
 	push	%rax
-	push	0x10(%rbp)
 
 	/* Clear RFLAGS.DF per x86_64 ABI */
 	cld
 
 	/* Load the callback pointer to %rax and invoke it via retpoline. */
-	mov	0x20(%rbp), %rax
+	mov	USER_HANDLER_OFFSET(%rax), %rax
 	call	.Lretpoline
 
 	/* Undo the post-exit %rsp adjustment. */
-	lea	0x20(%rsp, %rbx), %rsp
+	lea	0x10(%rsp, %rbx), %rsp
 
 	/*
 	 * If the return from callback is zero or negative, return immediately,
diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
index 3760e5d5dc0c7..d3b107aac279d 100644
--- a/arch/x86/include/uapi/asm/sgx.h
+++ b/arch/x86/include/uapi/asm/sgx.h
@@ -74,6 +74,28 @@ struct sgx_enclave_set_attribute {
 	__u64 attribute_fd;
 };
 
+struct sgx_enclave_run;
+
+/**
+ * typedef sgx_enclave_exit_handler_t - Exit handler function accepted by
+ *					__vdso_sgx_enter_enclave()
+ *
+ * @rdi:	RDI at the time of enclave exit
+ * @rsi:	RSI at the time of enclave exit
+ * @rdx:	RDX at the time of enclave exit
+ * @ursp:	RSP at the time of enclave exit (untrusted stack)
+ * @r8:		R8 at the time of enclave exit
+ * @r9:		R9 at the time of enclave exit
+ * @r:		Pointer to struct sgx_enclave_run (as provided by caller)
+ *
+ * Return:
+ *  0 or negative to exit vDSO
+ *  positive to re-enter enclave (must be EENTER or ERESUME leaf)
+ */
+typedef int (*sgx_enclave_exit_handler_t)(long rdi, long rsi, long rdx,
+					  long ursp, long r8, long r9,
+					  struct sgx_enclave_run *r);
+
 /**
  * struct sgx_enclave_exception - structure to report exceptions encountered in
  *				  __vdso_sgx_enter_enclave()
@@ -85,31 +107,43 @@ struct sgx_enclave_set_attribute {
  * @reserved:	reserved for future use
  */
 struct sgx_enclave_exception {
-	__u32 leaf;
 	__u16 trapnr;
 	__u16 error_code;
+	__u32 reserved;
 	__u64 address;
-	__u64 reserved[2];
 };
 
 /**
- * typedef sgx_enclave_exit_handler_t - Exit handler function accepted by
- *					__vdso_sgx_enter_enclave()
+ * struct sgx_enclave_run - Control structure for __vdso_sgx_enter_enclave()
  *
- * @rdi:	RDI at the time of enclave exit
- * @rsi:	RSI at the time of enclave exit
- * @rdx:	RDX at the time of enclave exit
- * @ursp:	RSP at the time of enclave exit (untrusted stack)
- * @r8:		R8 at the time of enclave exit
- * @r9:		R9 at the time of enclave exit
- * @tcs:	Thread Control Structure used to enter enclave
- * @ret:	0 on success (EEXIT), -EFAULT on an exception
- * @e:		Pointer to struct sgx_enclave_exception (as provided by caller)
+ * @tcs:		Thread Control Structure used to enter enclave
+ * @flags:		Control flags
+ * @exit_leaf:		ENCLU leaf from \%eax at time of exit
+ * @exit_reason:	Cause of exit from enclave, e.g. EEXIT vs. exception
+ * @user_handler:	User provided exit handler (optional)
+ * @user_data:		User provided opaque value (optional)
+ * @exception:		Valid on exit due to exception
  */
-typedef int (*sgx_enclave_exit_handler_t)(long rdi, long rsi, long rdx,
-					  long ursp, long r8, long r9,
-					  void *tcs, int ret,
-					  struct sgx_enclave_exception *e);
+struct sgx_enclave_run {
+	__u64 tcs;
+	__u64 flags;
+
+	__u32 exit_leaf;
+	__u32 exit_reason;
+
+	union {
+		sgx_enclave_exit_handler_t user_handler;
+		__u64 __user_handler;
+	};
+	__u64 user_data;
+
+	union {
+		struct sgx_enclave_exception exception;
+
+		/* Pad the entire struct to 256 bytes. */
+		__u8 pad[256 - 40];
+	};
+};
 
 /**
  * __vdso_sgx_enter_enclave() - Enter an SGX enclave
@@ -119,16 +153,14 @@ typedef int (*sgx_enclave_exit_handler_t)(long rdi, long rsi, long rdx,
  * @leaf:	ENCLU leaf, must be EENTER or ERESUME
  * @r8:		Pass-through value for R8
  * @r9:		Pass-through value for R9
- * @tcs:	TCS, must be non-NULL
- * @e:		Optional struct sgx_enclave_exception instance
- * @handler:	Optional enclave exit handler
+ * @r:		struct sgx_enclave_run, must be non-NULL
  *
  * NOTE: __vdso_sgx_enter_enclave() does not ensure full compliance with the
- * x86-64 ABI, e.g. doesn't explicitly clear EFLAGS.DF after EEXIT.  Except for
- * non-volatile general purpose registers, preserving/setting state in
- * accordance with the x86-64 ABI is the responsibility of the enclave and its
- * runtime, i.e. __vdso_sgx_enter_enclave() cannot be called from C code
- * without careful consideration by both the enclave and its runtime.
+ * x86-64 ABI, e.g. doesn't handle XSAVE state.  Except for non-volatile
+ * general purpose registers, EFLAGS.DF, and RSP alignment, preserving/setting
+ * state in accordance with the x86-64 ABI is the responsibility of the enclave
+ * and its runtime, i.e. __vdso_sgx_enter_enclave() cannot be called from C
+ * code without careful consideration by both the enclave and its runtime.
  *
  * All general purpose registers except RAX, RBX and RCX are passed as-is to
  * the enclave.  RAX, RBX and RCX are consumed by EENTER and ERESUME and are
@@ -160,16 +192,12 @@ typedef int (*sgx_enclave_exit_handler_t)(long rdi, long rsi, long rdx,
  * without returning to __vdso_sgx_enter_enclave().
  *
  * Return:
- *  0 on success,
+ *  0 on success (ENCLU reached),
  *  -EINVAL if ENCLU leaf is not allowed,
- *  -EFAULT if an exception occurs on ENCLU or within the enclave
- *  -errno for all other negative values returned by the userspace exit handler
  */
 typedef int (*vdso_sgx_enter_enclave_t)(unsigned long rdi, unsigned long rsi,
 					unsigned long rdx, unsigned int leaf,
 					unsigned long r8,  unsigned long r9,
-					void *tcs,
-					struct sgx_enclave_exception *e,
-					sgx_enclave_exit_handler_t handler);
+					struct sgx_enclave_run *r);
 
 #endif /* _UAPI_ASM_X86_SGX_H */
-- 
2.28.0

[RFC PATCH 3/4] x86/vdso: x86/sgx: Introduce dedicated SGX exit reasons for vDSO

[Sean Christopherson]

Use dedicated exit reasons, e.g. SYNCHRONOUS and EXCEPTION, instead of
'0' and '-EFAULT' respectively.  Using -EFAULT is less than desirable as
it usually means "bad address", which may or may not be true for a fault
in the enclave or on ENCLU.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/entry/vdso/vsgx_enter_enclave.S | 7 +++++--
 arch/x86/include/uapi/asm/sgx.h          | 3 +++
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.S b/arch/x86/entry/vdso/vsgx_enter_enclave.S
index aaae6d6e28ac3..b09e87dbe9334 100644
--- a/arch/x86/entry/vdso/vsgx_enter_enclave.S
+++ b/arch/x86/entry/vdso/vsgx_enter_enclave.S
@@ -19,6 +19,9 @@
 /* #define USER_DATA_OFFSET	4*8 */
 #define EXCEPTION_OFFSET	5*8
 
+#define SGX_SYNCHRONOUS_EXIT	0
+#define SGX_EXCEPTION_EXIT	1
+
 /* Offsets into sgx_enter_enclave.exception. */
 #define EX_TRAPNR	0*8
 #define EX_ERROR_CODE	0*8+2
@@ -65,7 +68,7 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave)
 	mov	RUN_OFFSET(%rbp), %rbx
 
 	/* Set exit_reason. */
-	movl	$0, EXIT_REASON_OFFSET(%rbx)
+	movl	$SGX_SYNCHRONOUS_EXIT, EXIT_REASON_OFFSET(%rbx)
 
 	/* Invoke userspace's exit handler if one was provided. */
 .Lhandle_exit:
@@ -94,7 +97,7 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave)
 	mov	RUN_OFFSET(%rbp), %rbx
 
 	/* Set the exit_reason and exception info. */
-	movl	$(-EFAULT), EXIT_REASON_OFFSET(%rbx)
+	movl	$SGX_EXCEPTION_EXIT, EXIT_REASON_OFFSET(%rbx)
 
 	mov	%di,  (EXCEPTION_OFFSET + EX_TRAPNR)(%rbx)
 	mov	%si,  (EXCEPTION_OFFSET + EX_ERROR_CODE)(%rbx)
diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
index d3b107aac279d..80a8b7a949a23 100644
--- a/arch/x86/include/uapi/asm/sgx.h
+++ b/arch/x86/include/uapi/asm/sgx.h
@@ -74,6 +74,9 @@ struct sgx_enclave_set_attribute {
 	__u64 attribute_fd;
 };
 
+#define SGX_SYNCHRONOUS_EXIT	0
+#define SGX_EXCEPTION_EXIT	1
+
 struct sgx_enclave_run;
 
 /**
-- 
2.28.0

[RFC PATCH 4/4] x86/vdso: x86/sgx: Allow the user to exit the vDSO loop on interrupts

[Sean Christopherson]

Allow userspace to exit the vDSO on interrupts that are acknowledged
while the enclave is active.  This allows the user's runtime to switch
contexts at opportune times without additional overhead, e.g. when using
an M:N threading model (where M user threads run N TCSs, with N > M).

Suggested-by: Jethro Beekman <jethro@fortanix.com>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/entry/vdso/vsgx_enter_enclave.S | 27 ++++++++++++++++++++----
 arch/x86/include/uapi/asm/sgx.h          |  3 +++
 2 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.S b/arch/x86/entry/vdso/vsgx_enter_enclave.S
index b09e87dbe9334..33428c0f94b0d 100644
--- a/arch/x86/entry/vdso/vsgx_enter_enclave.S
+++ b/arch/x86/entry/vdso/vsgx_enter_enclave.S
@@ -21,6 +21,9 @@
 
 #define SGX_SYNCHRONOUS_EXIT	0
 #define SGX_EXCEPTION_EXIT	1
+#define SGX_INTERRUPT_EXIT	2
+
+#define SGX_EXIT_ON_INTERRUPTS	1
 
 /* Offsets into sgx_enter_enclave.exception. */
 #define EX_TRAPNR	0*8
@@ -51,12 +54,17 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave)
 
 	mov	RUN_OFFSET(%rbp), %rcx
 
-	/* No flags are currently defined/supported. */
-	cmpq	$0, FLAGS_OFFSET(%rcx)
-	jne	.Linvalid_input
-
 	/* Load TCS and AEP */
 	mov	TCS_OFFEST(%rcx), %rbx
+
+	/* Use the alternate AEP if the user wants to exit on interrupts. */
+	mov	FLAGS_OFFSET(%rcx), %rcx
+	cmpq	$SGX_EXIT_ON_INTERRUPTS, %rcx
+	je	.Lload_interrupts_aep
+
+	/* All other flags are reserved. */
+	test	%rcx, %rcx
+	jne	.Linvalid_input
 	lea	.Lasync_exit_pointer(%rip), %rcx
 
 	/* Single ENCLU serving as both EENTER and AEP (ERESUME) */
@@ -93,6 +101,17 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave)
 	mov	$(-EINVAL), %eax
 	jmp	.Lout
 
+.Lload_interrupts_aep:
+	lea	.Lhandle_interrupt(%rip), %rcx
+	jmp	.Lenclu_eenter_eresume
+
+.Lhandle_interrupt:
+	mov	RUN_OFFSET(%rbp), %rbx
+
+	/* Set the exit_reason and exception info. */
+	movl	$SGX_INTERRUPT_EXIT, EXIT_REASON_OFFSET(%rbx)
+	jmp	.Lhandle_exit
+
 .Lhandle_exception:
 	mov	RUN_OFFSET(%rbp), %rbx
 
diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
index 80a8b7a949a23..beeabfad6eb81 100644
--- a/arch/x86/include/uapi/asm/sgx.h
+++ b/arch/x86/include/uapi/asm/sgx.h
@@ -76,6 +76,7 @@ struct sgx_enclave_set_attribute {
 
 #define SGX_SYNCHRONOUS_EXIT	0
 #define SGX_EXCEPTION_EXIT	1
+#define SGX_INTERRUPT_EXIT	2
 
 struct sgx_enclave_run;
 
@@ -116,6 +117,8 @@ struct sgx_enclave_exception {
 	__u64 address;
 };
 
+#define SGX_EXIT_ON_INTERRUPTS	(1ULL << 0)
+
 /**
  * struct sgx_enclave_run - Control structure for __vdso_sgx_enter_enclave()
  *
-- 
2.28.0

Re: [RFC PATCH 1/4] x86/vdso: x86/sgx: Explicitly force 8-byte CMP for detecting user handler

[Jarkko Sakkinen]

On Mon, Aug 17, 2020 at 09:24:02PM -0700, Sean Christopherson wrote:
> Use 'cmpq' to force an 8-byte CMP when checking for a user provided exit
> handler.  The handler is a pointer, which is guaranteed to be an 8-byte
> value since SGX is 64-bit mode only, and gcc defaults to 'cmpl' given a
> bare 'cmp', i.e. is only checking the lower 32 bits.  This could cause
> a false negative when detecting a user exit handler.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/entry/vdso/vsgx_enter_enclave.S | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.S b/arch/x86/entry/vdso/vsgx_enter_enclave.S
> index be7e467e1efb3..2d88acd408d4e 100644
> --- a/arch/x86/entry/vdso/vsgx_enter_enclave.S
> +++ b/arch/x86/entry/vdso/vsgx_enter_enclave.S
> @@ -48,7 +48,7 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave)
>  
>  	/* Invoke userspace's exit handler if one was provided. */
>  .Lhandle_exit:
> -	cmp	$0, 0x20(%rbp)
> +	cmpq	$0, 0x20(%rbp)
>  	jne	.Linvoke_userspace_handler
>  
>  .Lout:
> -- 
> 2.28.0
> 

Acked-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

BR,
/Jarkko

Re: [RFC PATCH 2/4] x86/vdso: x86/sgx: Rework __vdso_sgx_enter_enclave() API

[Jarkko Sakkinen]

On Mon, Aug 17, 2020 at 09:24:03PM -0700, Sean Christopherson wrote:
> Rework __vdso_sgx_enter_enclave() to use a struct to hold the input and
> output params.  In the new struct, add an opaque "user_data" that can be
> used to pass context across the vDSO, and an explicit "exit_reason" to
> avoid overloading the return value.
> 
> Moving the params into a struct will also make it less painful to use
> dedicated exit reasons, and to support exiting on interrupts in future
> patches.
> 
> Cc: Nathaniel McCallum <npmccallum@redhat.com>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/entry/vdso/vsgx_enter_enclave.S | 72 ++++++++++++-------
>  arch/x86/include/uapi/asm/sgx.h          | 90 ++++++++++++++++--------
>  2 files changed, 107 insertions(+), 55 deletions(-)
> 
> diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.S b/arch/x86/entry/vdso/vsgx_enter_enclave.S
> index 2d88acd408d4e..aaae6d6e28ac3 100644
> --- a/arch/x86/entry/vdso/vsgx_enter_enclave.S
> +++ b/arch/x86/entry/vdso/vsgx_enter_enclave.S
> @@ -7,9 +7,21 @@
>  
>  #include "extable.h"
>  
> -#define EX_LEAF		0*8
> -#define EX_TRAPNR	0*8+4
> -#define EX_ERROR_CODE	0*8+6
> +/* Offset of 'struct sgx_enter_enclave' relative to %rbp. */
> +#define RUN_OFFSET	2*8

Some better name please.

> +
> +/* Offsets into 'struct sgx_enter_enclave'. */
> +#define TCS_OFFEST		0*8
> +#define FLAGS_OFFSET		1*8
> +#define EXIT_LEAF_OFFSET	2*8
> +#define EXIT_REASON_OFFSET	2*8 + 4
> +#define USER_HANDLER_OFFSET	3*8
> +/* #define USER_DATA_OFFSET	4*8 */
> +#define EXCEPTION_OFFSET	5*8

These non-prefixed constants make the code really stressing to read
given the complexity of it. Please, just write them like
SGX_ENTER_ENCLAVE_TCS and so forth.

/Jarkko

Re: [RFC PATCH 3/4] x86/vdso: x86/sgx: Introduce dedicated SGX exit reasons for vDSO

[Jarkko Sakkinen]

On Mon, Aug 17, 2020 at 09:24:04PM -0700, Sean Christopherson wrote:
> Use dedicated exit reasons, e.g. SYNCHRONOUS and EXCEPTION, instead of
> '0' and '-EFAULT' respectively.  Using -EFAULT is less than desirable as
> it usually means "bad address", which may or may not be true for a fault
> in the enclave or on ENCLU.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/entry/vdso/vsgx_enter_enclave.S | 7 +++++--
>  arch/x86/include/uapi/asm/sgx.h          | 3 +++
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.S b/arch/x86/entry/vdso/vsgx_enter_enclave.S
> index aaae6d6e28ac3..b09e87dbe9334 100644
> --- a/arch/x86/entry/vdso/vsgx_enter_enclave.S
> +++ b/arch/x86/entry/vdso/vsgx_enter_enclave.S
> @@ -19,6 +19,9 @@
>  /* #define USER_DATA_OFFSET	4*8 */
>  #define EXCEPTION_OFFSET	5*8
>  
> +#define SGX_SYNCHRONOUS_EXIT	0
> +#define SGX_EXCEPTION_EXIT	1
> +
>  /* Offsets into sgx_enter_enclave.exception. */
>  #define EX_TRAPNR	0*8
>  #define EX_ERROR_CODE	0*8+2
> @@ -65,7 +68,7 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave)
>  	mov	RUN_OFFSET(%rbp), %rbx
>  
>  	/* Set exit_reason. */
> -	movl	$0, EXIT_REASON_OFFSET(%rbx)
> +	movl	$SGX_SYNCHRONOUS_EXIT, EXIT_REASON_OFFSET(%rbx)
>  
>  	/* Invoke userspace's exit handler if one was provided. */
>  .Lhandle_exit:
> @@ -94,7 +97,7 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave)
>  	mov	RUN_OFFSET(%rbp), %rbx
>  
>  	/* Set the exit_reason and exception info. */
> -	movl	$(-EFAULT), EXIT_REASON_OFFSET(%rbx)
> +	movl	$SGX_EXCEPTION_EXIT, EXIT_REASON_OFFSET(%rbx)
>  
>  	mov	%di,  (EXCEPTION_OFFSET + EX_TRAPNR)(%rbx)
>  	mov	%si,  (EXCEPTION_OFFSET + EX_ERROR_CODE)(%rbx)
> diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
> index d3b107aac279d..80a8b7a949a23 100644
> --- a/arch/x86/include/uapi/asm/sgx.h
> +++ b/arch/x86/include/uapi/asm/sgx.h
> @@ -74,6 +74,9 @@ struct sgx_enclave_set_attribute {
>  	__u64 attribute_fd;
>  };
>  
> +#define SGX_SYNCHRONOUS_EXIT	0
> +#define SGX_EXCEPTION_EXIT	1

enum would be great here.

> +
>  struct sgx_enclave_run;
>  
>  /**
> -- 
> 2.28.0
> 

/Jarkko

Re: [RFC PATCH 4/4] x86/vdso: x86/sgx: Allow the user to exit the vDSO loop on interrupts

[Jarkko Sakkinen]

On Mon, Aug 17, 2020 at 09:24:05PM -0700, Sean Christopherson wrote:
> Allow userspace to exit the vDSO on interrupts that are acknowledged
> while the enclave is active.  This allows the user's runtime to switch
> contexts at opportune times without additional overhead, e.g. when using
> an M:N threading model (where M user threads run N TCSs, with N > M).
> 
> Suggested-by: Jethro Beekman <jethro@fortanix.com>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/entry/vdso/vsgx_enter_enclave.S | 27 ++++++++++++++++++++----
>  arch/x86/include/uapi/asm/sgx.h          |  3 +++
>  2 files changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.S b/arch/x86/entry/vdso/vsgx_enter_enclave.S
> index b09e87dbe9334..33428c0f94b0d 100644
> --- a/arch/x86/entry/vdso/vsgx_enter_enclave.S
> +++ b/arch/x86/entry/vdso/vsgx_enter_enclave.S
> @@ -21,6 +21,9 @@
>  
>  #define SGX_SYNCHRONOUS_EXIT	0
>  #define SGX_EXCEPTION_EXIT	1
> +#define SGX_INTERRUPT_EXIT	2
> +
> +#define SGX_EXIT_ON_INTERRUPTS	1

This naming scheme does not look too great.

I have nothing principal about code changes, *if* this is the approach
where we want to move. I did not look every detail because I'm not sure
what is the consensus yet.

/Jarkko

Re: [RFC PATCH 4/4] x86/vdso: x86/sgx: Allow the user to exit the vDSO loop on interrupts

[Andy Lutomirski]

On Mon, Aug 17, 2020 at 9:24 PM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> Allow userspace to exit the vDSO on interrupts that are acknowledged
> while the enclave is active.  This allows the user's runtime to switch
> contexts at opportune times without additional overhead, e.g. when using
> an M:N threading model (where M user threads run N TCSs, with N > M).

This is IMO rather odd.  We don't support this type of notification on
interrupts for normal user code.  The fact user code can detect
interrupts during enclave execution is IMO an oddity of SGX, and I
have asked Intel to consider replacing the AEX mechanism with
something more transparent to user mode.  If this ever happens, this
mechanism is toast.

Even without architecture changes, building a *reliable* M:N threading
mechanism on top of this will be difficult or impossible, as there is
no particular guarantee that a thread will get timing interrupts at
all or that these interrupts will get lucky and hit enclave code, thus
triggering an AEX.  We certainly don't, and probably never will,
support any corresponding feature for non-enclave code.

So this seems like an odd, and possibly unsupportable, feature to add.

--Andy

Re: [RFC PATCH 4/4] x86/vdso: x86/sgx: Allow the user to exit the vDSO loop on interrupts

[Sean Christopherson]

On Tue, Aug 18, 2020 at 10:15:49AM -0700, Andy Lutomirski wrote:
> On Mon, Aug 17, 2020 at 9:24 PM Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
> >
> > Allow userspace to exit the vDSO on interrupts that are acknowledged
> > while the enclave is active.  This allows the user's runtime to switch
> > contexts at opportune times without additional overhead, e.g. when using
> > an M:N threading model (where M user threads run N TCSs, with N > M).
> 
> This is IMO rather odd.  We don't support this type of notification on
> interrupts for normal user code.  The fact user code can detect
> interrupts during enclave execution is IMO an oddity of SGX, and I
> have asked Intel to consider replacing the AEX mechanism with
> something more transparent to user mode.  If this ever happens, this
> mechanism is toast.
> 
> Even without architecture changes, building a *reliable* M:N threading
> mechanism on top of this will be difficult or impossible, as there is
> no particular guarantee that a thread will get timing interrupts at
> all or that these interrupts will get lucky and hit enclave code, thus
> triggering an AEX.  We certainly don't, and probably never will,
> support any corresponding feature for non-enclave code.
> 
> So this seems like an odd, and possibly unsupportable, feature to add.

I 100% agree that allowing the user to act on interrupts is weird/fragile.
I'll happily kill this off if there's an "official" NAK, but I wanted to
force the issue so that we're not stuck in limbo wondering whether or not
this should be supported.

Re: [RFC PATCH 4/4] x86/vdso: x86/sgx: Allow the user to exit the vDSO loop on interrupts

[Andy Lutomirski]

> On Aug 18, 2020, at 10:31 AM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> 
> On Tue, Aug 18, 2020 at 10:15:49AM -0700, Andy Lutomirski wrote:
>>> On Mon, Aug 17, 2020 at 9:24 PM Sean Christopherson
>>> <sean.j.christopherson@intel.com> wrote:
>>> 
>>> Allow userspace to exit the vDSO on interrupts that are acknowledged
>>> while the enclave is active.  This allows the user's runtime to switch
>>> contexts at opportune times without additional overhead, e.g. when using
>>> an M:N threading model (where M user threads run N TCSs, with N > M).
>> 
>> This is IMO rather odd.  We don't support this type of notification on
>> interrupts for normal user code.  The fact user code can detect
>> interrupts during enclave execution is IMO an oddity of SGX, and I
>> have asked Intel to consider replacing the AEX mechanism with
>> something more transparent to user mode.  If this ever happens, this
>> mechanism is toast.
>> 
>> Even without architecture changes, building a *reliable* M:N threading
>> mechanism on top of this will be difficult or impossible, as there is
>> no particular guarantee that a thread will get timing interrupts at
>> all or that these interrupts will get lucky and hit enclave code, thus
>> triggering an AEX.  We certainly don't, and probably never will,
>> support any corresponding feature for non-enclave code.
>> 
>> So this seems like an odd, and possibly unsupportable, feature to add.
> 
> I 100% agree that allowing the user to act on interrupts is weird/fragile.
> I'll happily kill this off if there's an "official" NAK, but I wanted to
> force the issue so that we're not stuck in limbo wondering whether or not
> this should be supported.

If you would like an official NAK, here you go: NAK!

If users want M:N and SIGALRM isn’t good enough, add something better. Please don’t rely on an architectural wart as an alternative.

Re: [RFC PATCH 4/4] x86/vdso: x86/sgx: Allow the user to exit the vDSO loop on interrupts

[Jethro Beekman]

[-- Attachment #1: Type: text/plain, Size: 2214 bytes --]

On 2020-08-18 19:15, Andy Lutomirski wrote:
> On Mon, Aug 17, 2020 at 9:24 PM Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
>>
>> Allow userspace to exit the vDSO on interrupts that are acknowledged
>> while the enclave is active.  This allows the user's runtime to switch
>> contexts at opportune times without additional overhead, e.g. when using
>> an M:N threading model (where M user threads run N TCSs, with N > M).
> 
> This is IMO rather odd.  We don't support this type of notification on
> interrupts for normal user code.  The fact user code can detect
> interrupts during enclave execution is IMO an oddity of SGX, and I
> have asked Intel to consider replacing the AEX mechanism with
> something more transparent to user mode.  If this ever happens, this
> mechanism is toast.

Let's design the current interface for the current architecture. We can deal with a new architecture if and when Intel provides it.

> Even without architecture changes, building a *reliable* M:N threading
> mechanism on top of this will be difficult or impossible, as there is
> no particular guarantee that a thread will get timing interrupts at> all or that these interrupts will get lucky and hit enclave code, thus
> triggering an AEX.  We certainly don't, and probably never will,
> support any corresponding feature for non-enclave code.

There's no guarantee, but this vDSO exit mechanism is a prerequisite. Both for context switching and aborting an enclave, userspace *must* have a way to trigger exit from enclave mode *and* recover the user stack in a sane manner. Userspace *should* also be able to do this in a way that's compatible with library use, so calling timer_create or pthread_kill to deliver a signal would be ok, but installing a signal handler should be avoided (the whole reason behind this vDSO call).

> So this seems like an odd, and possibly unsupportable, feature to add.

I can implement all this without the vDSO call today, so why not support it? That just means not everyone is going to use the vDSO call, again resulting in potential problems when multiple libraries want to use enclaves.

> --Andy
> 

--
Jethro Beekman | Fortanix


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4054 bytes --]

Re: [RFC PATCH 4/4] x86/vdso: x86/sgx: Allow the user to exit the vDSO loop on interrupts

[Andy Lutomirski]

On Wed, Aug 19, 2020 at 7:21 AM Jethro Beekman <jethro@fortanix.com> wrote:
>
> On 2020-08-18 19:15, Andy Lutomirski wrote:
> > On Mon, Aug 17, 2020 at 9:24 PM Sean Christopherson
> > <sean.j.christopherson@intel.com> wrote:
> >>
> >> Allow userspace to exit the vDSO on interrupts that are acknowledged
> >> while the enclave is active.  This allows the user's runtime to switch
> >> contexts at opportune times without additional overhead, e.g. when using
> >> an M:N threading model (where M user threads run N TCSs, with N > M).
> >
> > This is IMO rather odd.  We don't support this type of notification on
> > interrupts for normal user code.  The fact user code can detect
> > interrupts during enclave execution is IMO an oddity of SGX, and I
> > have asked Intel to consider replacing the AEX mechanism with
> > something more transparent to user mode.  If this ever happens, this
> > mechanism is toast.
>
> Let's design the current interface for the current architecture. We can deal with a new architecture if and when Intel provides it.

No.  If Intel fixes the architecture, the proposed interface will
*stop working*.

>
> > Even without architecture changes, building a *reliable* M:N threading
> > mechanism on top of this will be difficult or impossible, as there is
> > no particular guarantee that a thread will get timing interrupts at> all or that these interrupts will get lucky and hit enclave code, thus
> > triggering an AEX.  We certainly don't, and probably never will,
> > support any corresponding feature for non-enclave code.
>
> There's no guarantee, but this vDSO exit mechanism is a prerequisite. Both for context switching and aborting an enclave, userspace *must* have a way to trigger exit from enclave mode *and* recover the user stack in a sane manner. Userspace *should* also be able to do this in a way that's compatible with library use, so calling timer_create or pthread_kill to deliver a signal would be ok, but installing a signal handler should be avoided (the whole reason behind this vDSO call).

If you want to abort an enclave, abort it the same way you abort any
other user code -- send a signal or something.  If something is wrong
with signal handling in the proposed vDSO interface, then by all
means, let's fix it.  If we need better library signal support, let's
add such a thing.  If you really want to abort an enclave *cleanly*
without any chance of garbage left around, stick it in a subprocess.
We are not playing the game where someone sets a flag, crosses their
fingers, and waits for an interrupt.

>
> > So this seems like an odd, and possibly unsupportable, feature to add.
>
> I can implement all this without the vDSO call today, so why not support it? That just means not everyone is going to use the vDSO call, again resulting in potential problems when multiple libraries want to use enclaves.

No offense, but if you want to write garbage software with entirely
user code, I can't stop you, but the kernel is not going to give this
anything resembling a stamp of approval.  When you inevitably discover
that it has broken for one reason or another and your software stack
stops making forward progress, I don't want anyone playing the "ABI
stability" card and asking the upstream kernel to work around the
breakage.

Now maybe I'm wrong and there's an actual legitimate use case for this
trick, in which case I'd like to be enlightened.  But M:N threading
and enclave aborting don't sound like legitimate use cases.

Re: [RFC PATCH 4/4] x86/vdso: x86/sgx: Allow the user to exit the vDSO loop on interrupts

[Jethro Beekman]

[-- Attachment #1: Type: text/plain, Size: 3038 bytes --]

Tested-by: Jethro Beekman <jethro@fortanix.com>

--
Jethro Beekman | Fortanix

On 2020-08-18 06:24, Sean Christopherson wrote:
> Allow userspace to exit the vDSO on interrupts that are acknowledged
> while the enclave is active.  This allows the user's runtime to switch
> contexts at opportune times without additional overhead, e.g. when using
> an M:N threading model (where M user threads run N TCSs, with N > M).
> 
> Suggested-by: Jethro Beekman <jethro@fortanix.com>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/entry/vdso/vsgx_enter_enclave.S | 27 ++++++++++++++++++++----
>  arch/x86/include/uapi/asm/sgx.h          |  3 +++
>  2 files changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.S b/arch/x86/entry/vdso/vsgx_enter_enclave.S
> index b09e87dbe9334..33428c0f94b0d 100644
> --- a/arch/x86/entry/vdso/vsgx_enter_enclave.S
> +++ b/arch/x86/entry/vdso/vsgx_enter_enclave.S
> @@ -21,6 +21,9 @@
>  
>  #define SGX_SYNCHRONOUS_EXIT	0
>  #define SGX_EXCEPTION_EXIT	1
> +#define SGX_INTERRUPT_EXIT	2
> +
> +#define SGX_EXIT_ON_INTERRUPTS	1
>  
>  /* Offsets into sgx_enter_enclave.exception. */
>  #define EX_TRAPNR	0*8
> @@ -51,12 +54,17 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave)
>  
>  	mov	RUN_OFFSET(%rbp), %rcx
>  
> -	/* No flags are currently defined/supported. */
> -	cmpq	$0, FLAGS_OFFSET(%rcx)
> -	jne	.Linvalid_input
> -
>  	/* Load TCS and AEP */
>  	mov	TCS_OFFEST(%rcx), %rbx
> +
> +	/* Use the alternate AEP if the user wants to exit on interrupts. */
> +	mov	FLAGS_OFFSET(%rcx), %rcx
> +	cmpq	$SGX_EXIT_ON_INTERRUPTS, %rcx
> +	je	.Lload_interrupts_aep
> +
> +	/* All other flags are reserved. */
> +	test	%rcx, %rcx
> +	jne	.Linvalid_input
>  	lea	.Lasync_exit_pointer(%rip), %rcx
>  
>  	/* Single ENCLU serving as both EENTER and AEP (ERESUME) */
> @@ -93,6 +101,17 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave)
>  	mov	$(-EINVAL), %eax
>  	jmp	.Lout
>  
> +.Lload_interrupts_aep:
> +	lea	.Lhandle_interrupt(%rip), %rcx
> +	jmp	.Lenclu_eenter_eresume
> +
> +.Lhandle_interrupt:
> +	mov	RUN_OFFSET(%rbp), %rbx
> +
> +	/* Set the exit_reason and exception info. */
> +	movl	$SGX_INTERRUPT_EXIT, EXIT_REASON_OFFSET(%rbx)
> +	jmp	.Lhandle_exit
> +
>  .Lhandle_exception:
>  	mov	RUN_OFFSET(%rbp), %rbx
>  
> diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
> index 80a8b7a949a23..beeabfad6eb81 100644
> --- a/arch/x86/include/uapi/asm/sgx.h
> +++ b/arch/x86/include/uapi/asm/sgx.h
> @@ -76,6 +76,7 @@ struct sgx_enclave_set_attribute {
>  
>  #define SGX_SYNCHRONOUS_EXIT	0
>  #define SGX_EXCEPTION_EXIT	1
> +#define SGX_INTERRUPT_EXIT	2
>  
>  struct sgx_enclave_run;
>  
> @@ -116,6 +117,8 @@ struct sgx_enclave_exception {
>  	__u64 address;
>  };
>  
> +#define SGX_EXIT_ON_INTERRUPTS	(1ULL << 0)
> +
>  /**
>   * struct sgx_enclave_run - Control structure for __vdso_sgx_enter_enclave()
>   *
> 


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4054 bytes --]

Re: [RFC PATCH 3/4] x86/vdso: x86/sgx: Introduce dedicated SGX exit reasons for vDSO

[Jethro Beekman]

[-- Attachment #1: Type: text/plain, Size: 2261 bytes --]

Acked-by: Jethro Beekman <jethro@fortanix.com>

Jethro Beekman | Fortanix

On 2020-08-18 06:24, Sean Christopherson wrote:
> Use dedicated exit reasons, e.g. SYNCHRONOUS and EXCEPTION, instead of
> '0' and '-EFAULT' respectively.  Using -EFAULT is less than desirable as
> it usually means "bad address", which may or may not be true for a fault
> in the enclave or on ENCLU.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/entry/vdso/vsgx_enter_enclave.S | 7 +++++--
>  arch/x86/include/uapi/asm/sgx.h          | 3 +++
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.S b/arch/x86/entry/vdso/vsgx_enter_enclave.S
> index aaae6d6e28ac3..b09e87dbe9334 100644
> --- a/arch/x86/entry/vdso/vsgx_enter_enclave.S
> +++ b/arch/x86/entry/vdso/vsgx_enter_enclave.S
> @@ -19,6 +19,9 @@
>  /* #define USER_DATA_OFFSET	4*8 */
>  #define EXCEPTION_OFFSET	5*8
>  
> +#define SGX_SYNCHRONOUS_EXIT	0
> +#define SGX_EXCEPTION_EXIT	1
> +
>  /* Offsets into sgx_enter_enclave.exception. */
>  #define EX_TRAPNR	0*8
>  #define EX_ERROR_CODE	0*8+2
> @@ -65,7 +68,7 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave)
>  	mov	RUN_OFFSET(%rbp), %rbx
>  
>  	/* Set exit_reason. */
> -	movl	$0, EXIT_REASON_OFFSET(%rbx)
> +	movl	$SGX_SYNCHRONOUS_EXIT, EXIT_REASON_OFFSET(%rbx)
>  
>  	/* Invoke userspace's exit handler if one was provided. */
>  .Lhandle_exit:
> @@ -94,7 +97,7 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave)
>  	mov	RUN_OFFSET(%rbp), %rbx
>  
>  	/* Set the exit_reason and exception info. */
> -	movl	$(-EFAULT), EXIT_REASON_OFFSET(%rbx)
> +	movl	$SGX_EXCEPTION_EXIT, EXIT_REASON_OFFSET(%rbx)
>  
>  	mov	%di,  (EXCEPTION_OFFSET + EX_TRAPNR)(%rbx)
>  	mov	%si,  (EXCEPTION_OFFSET + EX_ERROR_CODE)(%rbx)
> diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
> index d3b107aac279d..80a8b7a949a23 100644
> --- a/arch/x86/include/uapi/asm/sgx.h
> +++ b/arch/x86/include/uapi/asm/sgx.h
> @@ -74,6 +74,9 @@ struct sgx_enclave_set_attribute {
>  	__u64 attribute_fd;
>  };
>  
> +#define SGX_SYNCHRONOUS_EXIT	0
> +#define SGX_EXCEPTION_EXIT	1
> +
>  struct sgx_enclave_run;
>  
>  /**
> 


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4054 bytes --]

Re: [RFC PATCH 1/4] x86/vdso: x86/sgx: Explicitly force 8-byte CMP for detecting user handler

[Jethro Beekman]

[-- Attachment #1: Type: text/plain, Size: 1179 bytes --]

Acked-by: Jethro Beekman <jethro@fortanix.com>

--
Jethro Beekman | Fortanix

On 2020-08-18 06:24, Sean Christopherson wrote:
> Use 'cmpq' to force an 8-byte CMP when checking for a user provided exit
> handler.  The handler is a pointer, which is guaranteed to be an 8-byte
> value since SGX is 64-bit mode only, and gcc defaults to 'cmpl' given a
> bare 'cmp', i.e. is only checking the lower 32 bits.  This could cause
> a false negative when detecting a user exit handler.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/entry/vdso/vsgx_enter_enclave.S | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.S b/arch/x86/entry/vdso/vsgx_enter_enclave.S
> index be7e467e1efb3..2d88acd408d4e 100644
> --- a/arch/x86/entry/vdso/vsgx_enter_enclave.S
> +++ b/arch/x86/entry/vdso/vsgx_enter_enclave.S
> @@ -48,7 +48,7 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave)
>  
>  	/* Invoke userspace's exit handler if one was provided. */
>  .Lhandle_exit:
> -	cmp	$0, 0x20(%rbp)
> +	cmpq	$0, 0x20(%rbp)
>  	jne	.Linvoke_userspace_handler
>  
>  .Lout:
> 


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4054 bytes --]

Re: [RFC PATCH 4/4] x86/vdso: x86/sgx: Allow the user to exit the vDSO loop on interrupts

[Jethro Beekman]

[-- Attachment #1: Type: text/plain, Size: 5082 bytes --]

On 2020-08-19 17:02, Andy Lutomirski wrote:
> On Wed, Aug 19, 2020 at 7:21 AM Jethro Beekman <jethro@fortanix.com> wrote:
>>
>> On 2020-08-18 19:15, Andy Lutomirski wrote:
>>> On Mon, Aug 17, 2020 at 9:24 PM Sean Christopherson
>>> <sean.j.christopherson@intel.com> wrote:
>>>>
>>>> Allow userspace to exit the vDSO on interrupts that are acknowledged
>>>> while the enclave is active.  This allows the user's runtime to switch
>>>> contexts at opportune times without additional overhead, e.g. when using
>>>> an M:N threading model (where M user threads run N TCSs, with N > M).
>>>
>>> This is IMO rather odd.  We don't support this type of notification on
>>> interrupts for normal user code.  The fact user code can detect
>>> interrupts during enclave execution is IMO an oddity of SGX, and I
>>> have asked Intel to consider replacing the AEX mechanism with
>>> something more transparent to user mode.  If this ever happens, this
>>> mechanism is toast.
>>
>> Let's design the current interface for the current architecture. We can deal with a new architecture if and when Intel provides it.
> 
> No.  If Intel fixes the architecture, the proposed interface will
> *stop working*.
> 
>>
>>> Even without architecture changes, building a *reliable* M:N threading
>>> mechanism on top of this will be difficult or impossible, as there is
>>> no particular guarantee that a thread will get timing interrupts at> all or that these interrupts will get lucky and hit enclave code, thus
>>> triggering an AEX.  We certainly don't, and probably never will,
>>> support any corresponding feature for non-enclave code.
>>
>> There's no guarantee, but this vDSO exit mechanism is a prerequisite. Both for context switching and aborting an enclave, userspace *must* have a way to trigger exit from enclave mode *and* recover the user stack in a sane manner. Userspace *should* also be able to do this in a way that's compatible with library use, so calling timer_create or pthread_kill to deliver a signal would be ok, but installing a signal handler should be avoided (the whole reason behind this vDSO call).
> 
> If you want to abort an enclave, abort it the same way you abort any
> other user code -- send a signal or something.  If something is wrong> with signal handling in the proposed vDSO interface, then by all
> means, let's fix it.  If we need better library signal support, let's
> add such a thing.  If you really want to abort an enclave *cleanly*
> without any chance of garbage left around, stick it in a subprocess.
> We are not playing the game where someone sets a flag, crosses their
> fingers, and waits for an interrupt.

Sending a signal is not sufficient. The current __vdso_sgx_enter_enclave call is not interruptible.

> 
>>
>>> So this seems like an odd, and possibly unsupportable, feature to add.
>>
>> I can implement all this without the vDSO call today, so why not support it? That just means not everyone is going to use the vDSO call, again resulting in potential problems when multiple libraries want to use enclaves.
> 
> No offense, but if you want to write garbage software with entirely
> user code, I can't stop you, but the kernel is not going to give this
> anything resembling a stamp of approval.  When you inevitably discover
> that it has broken for one reason or another and your software stack
> stops making forward progress, I don't want anyone playing the "ABI
> stability" card and asking the upstream kernel to work around the
> breakage.
> 
> Now maybe I'm wrong and there's an actual legitimate use case for this
> trick, in which case I'd like to be enlightened.  But M:N threading
> and enclave aborting don't sound like legitimate use cases.
> 

Why is it ok for this code to return to userspace after 1 second:



void ignore_signal_but_dont_restart(int s) {}

// set SIGALRM handler if none set (FIXME: racy)
struct sigaction sa_old;
struct sigaction sa = {
    .sa_handler = ignore_signal_but_dont_restart,
    .sa_flags = 0,
};
sigaction(SIGALRM, &sa, &sa_old);
if (!(sa_old.sa_handler == SIG_IGN || sa_old.sa_handler == SIG_DFL)
    || (sa_old.sa_flags & SA_SIGINFO)) {
    sa_old.sa_flags &= ~SA_RESTART;
    sigaction(SIGALRM, &sa_old, NULL);
}

alarm(1);

char buf;
read(0, &buf, 1);



But, according to your train of thought, this code must hang indefinitely (assuming the enclave is not calling EEXIT)?



void ignore_signal_but_dont_restart(int s) {}

// set SIGALRM handler if none set (FIXME: racy)
struct sigaction sa_old;
struct sigaction sa = {
    .sa_handler = ignore_signal_but_dont_restart,
    .sa_flags = 0,
};
sigaction(SIGALRM, &sa, &sa_old);
if (!(sa_old.sa_handler == SIG_IGN || sa_old.sa_handler == SIG_DFL)
    || (sa_old.sa_flags & SA_SIGINFO)) {
    sa_old.sa_flags &= ~SA_RESTART;
    sigaction(SIGALRM, &sa_old, NULL);
}

alarm(1);

__vdso_sgx_enter_enclave(0, 0, 0, 2, 0, 0, tcs, NULL, NULL);



Tested on Linux 5.9-rc1 with SGX patches v36.

--
Jethro Beekman | Fortanix


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4054 bytes --]

Re: [RFC PATCH 2/4] x86/vdso: x86/sgx: Rework __vdso_sgx_enter_enclave() API

[Jethro Beekman]

[-- Attachment #1: Type: text/plain, Size: 10587 bytes --]

Tested-by: Jethro Beekman <jethro@fortanix.com>

--
Jethro Beekman | Fortanix

On 2020-08-18 06:24, Sean Christopherson wrote:
> Rework __vdso_sgx_enter_enclave() to use a struct to hold the input and
> output params.  In the new struct, add an opaque "user_data" that can be
> used to pass context across the vDSO, and an explicit "exit_reason" to
> avoid overloading the return value.
> 
> Moving the params into a struct will also make it less painful to use
> dedicated exit reasons, and to support exiting on interrupts in future
> patches.
> 
> Cc: Nathaniel McCallum <npmccallum@redhat.com>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/entry/vdso/vsgx_enter_enclave.S | 72 ++++++++++++-------
>  arch/x86/include/uapi/asm/sgx.h          | 90 ++++++++++++++++--------
>  2 files changed, 107 insertions(+), 55 deletions(-)
> 
> diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.S b/arch/x86/entry/vdso/vsgx_enter_enclave.S
> index 2d88acd408d4e..aaae6d6e28ac3 100644
> --- a/arch/x86/entry/vdso/vsgx_enter_enclave.S
> +++ b/arch/x86/entry/vdso/vsgx_enter_enclave.S
> @@ -7,9 +7,21 @@
>  
>  #include "extable.h"
>  
> -#define EX_LEAF		0*8
> -#define EX_TRAPNR	0*8+4
> -#define EX_ERROR_CODE	0*8+6
> +/* Offset of 'struct sgx_enter_enclave' relative to %rbp. */
> +#define RUN_OFFSET	2*8
> +
> +/* Offsets into 'struct sgx_enter_enclave'. */
> +#define TCS_OFFEST		0*8
> +#define FLAGS_OFFSET		1*8
> +#define EXIT_LEAF_OFFSET	2*8
> +#define EXIT_REASON_OFFSET	2*8 + 4
> +#define USER_HANDLER_OFFSET	3*8
> +/* #define USER_DATA_OFFSET	4*8 */
> +#define EXCEPTION_OFFSET	5*8
> +
> +/* Offsets into sgx_enter_enclave.exception. */
> +#define EX_TRAPNR	0*8
> +#define EX_ERROR_CODE	0*8+2
>  #define EX_ADDRESS	1*8
>  
>  .code64
> @@ -30,12 +42,18 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave)
>  .Lenter_enclave:
>  	/* EENTER <= leaf <= ERESUME */
>  	cmp	$EENTER, %eax
> -	jb	.Linvalid_leaf
> +	jb	.Linvalid_input
>  	cmp	$ERESUME, %eax
> -	ja	.Linvalid_leaf
> +	ja	.Linvalid_input
> +
> +	mov	RUN_OFFSET(%rbp), %rcx
> +
> +	/* No flags are currently defined/supported. */
> +	cmpq	$0, FLAGS_OFFSET(%rcx)
> +	jne	.Linvalid_input
>  
>  	/* Load TCS and AEP */
> -	mov	0x10(%rbp), %rbx
> +	mov	TCS_OFFEST(%rcx), %rbx
>  	lea	.Lasync_exit_pointer(%rip), %rcx
>  
>  	/* Single ENCLU serving as both EENTER and AEP (ERESUME) */
> @@ -44,13 +62,21 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave)
>  	enclu
>  
>  	/* EEXIT jumps here unless the enclave is doing something fancy. */
> -	xor	%eax, %eax
> +	mov	RUN_OFFSET(%rbp), %rbx
> +
> +	/* Set exit_reason. */
> +	movl	$0, EXIT_REASON_OFFSET(%rbx)
>  
>  	/* Invoke userspace's exit handler if one was provided. */
>  .Lhandle_exit:
> -	cmpq	$0, 0x20(%rbp)
> +	mov	%eax, EXIT_LEAF_OFFSET(%rbx)
> +
> +	cmpq	$0, USER_HANDLER_OFFSET(%rbx)
>  	jne	.Linvoke_userspace_handler
>  
> +	/* Success, in the sense that ENCLU was attempted. */
> +	xor	%eax, %eax
> +
>  .Lout:
>  	pop	%rbx
>  	leave
> @@ -60,28 +86,28 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave)
>  	/* The out-of-line code runs with the pre-leave stack frame. */
>  	.cfi_def_cfa		%rbp, 16
>  
> -.Linvalid_leaf:
> +.Linvalid_input:
>  	mov	$(-EINVAL), %eax
>  	jmp	.Lout
>  
>  .Lhandle_exception:
> -	mov	0x18(%rbp), %rcx
> -	test    %rcx, %rcx
> -	je	.Lskip_exception_info
> +	mov	RUN_OFFSET(%rbp), %rbx
>  
> -	/* Fill optional exception info. */
> -	mov	%eax, EX_LEAF(%rcx)
> -	mov	%di,  EX_TRAPNR(%rcx)
> -	mov	%si,  EX_ERROR_CODE(%rcx)
> -	mov	%rdx, EX_ADDRESS(%rcx)
> -.Lskip_exception_info:
> -	mov	$(-EFAULT), %eax
> +	/* Set the exit_reason and exception info. */
> +	movl	$(-EFAULT), EXIT_REASON_OFFSET(%rbx)
> +
> +	mov	%di,  (EXCEPTION_OFFSET + EX_TRAPNR)(%rbx)
> +	mov	%si,  (EXCEPTION_OFFSET + EX_ERROR_CODE)(%rbx)
> +	mov	%rdx, (EXCEPTION_OFFSET + EX_ADDRESS)(%rbx)
>  	jmp	.Lhandle_exit
>  
>  .Linvoke_userspace_handler:
>  	/* Pass the untrusted RSP (at exit) to the callback via %rcx. */
>  	mov	%rsp, %rcx
>  
> +	/* Save @e, %rbx is about to be clobbered. */
> +	mov	%rbx, %rax
> +
>  	/* Save the untrusted RSP offset in %rbx (non-volatile register). */
>  	mov	%rsp, %rbx
>  	and	$0xf, %rbx
> @@ -93,20 +119,18 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave)
>  	and	$-0x10, %rsp
>  	push	%rax
>  
> -	/* Push @e, the "return" value and @tcs as params to the callback. */
> -	push	0x18(%rbp)
> +	/* Push @e as a param to the callback. */
>  	push	%rax
> -	push	0x10(%rbp)
>  
>  	/* Clear RFLAGS.DF per x86_64 ABI */
>  	cld
>  
>  	/* Load the callback pointer to %rax and invoke it via retpoline. */
> -	mov	0x20(%rbp), %rax
> +	mov	USER_HANDLER_OFFSET(%rax), %rax
>  	call	.Lretpoline
>  
>  	/* Undo the post-exit %rsp adjustment. */
> -	lea	0x20(%rsp, %rbx), %rsp
> +	lea	0x10(%rsp, %rbx), %rsp
>  
>  	/*
>  	 * If the return from callback is zero or negative, return immediately,
> diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
> index 3760e5d5dc0c7..d3b107aac279d 100644
> --- a/arch/x86/include/uapi/asm/sgx.h
> +++ b/arch/x86/include/uapi/asm/sgx.h
> @@ -74,6 +74,28 @@ struct sgx_enclave_set_attribute {
>  	__u64 attribute_fd;
>  };
>  
> +struct sgx_enclave_run;
> +
> +/**
> + * typedef sgx_enclave_exit_handler_t - Exit handler function accepted by
> + *					__vdso_sgx_enter_enclave()
> + *
> + * @rdi:	RDI at the time of enclave exit
> + * @rsi:	RSI at the time of enclave exit
> + * @rdx:	RDX at the time of enclave exit
> + * @ursp:	RSP at the time of enclave exit (untrusted stack)
> + * @r8:		R8 at the time of enclave exit
> + * @r9:		R9 at the time of enclave exit
> + * @r:		Pointer to struct sgx_enclave_run (as provided by caller)
> + *
> + * Return:
> + *  0 or negative to exit vDSO
> + *  positive to re-enter enclave (must be EENTER or ERESUME leaf)
> + */
> +typedef int (*sgx_enclave_exit_handler_t)(long rdi, long rsi, long rdx,
> +					  long ursp, long r8, long r9,
> +					  struct sgx_enclave_run *r);
> +
>  /**
>   * struct sgx_enclave_exception - structure to report exceptions encountered in
>   *				  __vdso_sgx_enter_enclave()
> @@ -85,31 +107,43 @@ struct sgx_enclave_set_attribute {
>   * @reserved:	reserved for future use
>   */
>  struct sgx_enclave_exception {
> -	__u32 leaf;
>  	__u16 trapnr;
>  	__u16 error_code;
> +	__u32 reserved;
>  	__u64 address;
> -	__u64 reserved[2];
>  };
>  
>  /**
> - * typedef sgx_enclave_exit_handler_t - Exit handler function accepted by
> - *					__vdso_sgx_enter_enclave()
> + * struct sgx_enclave_run - Control structure for __vdso_sgx_enter_enclave()
>   *
> - * @rdi:	RDI at the time of enclave exit
> - * @rsi:	RSI at the time of enclave exit
> - * @rdx:	RDX at the time of enclave exit
> - * @ursp:	RSP at the time of enclave exit (untrusted stack)
> - * @r8:		R8 at the time of enclave exit
> - * @r9:		R9 at the time of enclave exit
> - * @tcs:	Thread Control Structure used to enter enclave
> - * @ret:	0 on success (EEXIT), -EFAULT on an exception
> - * @e:		Pointer to struct sgx_enclave_exception (as provided by caller)
> + * @tcs:		Thread Control Structure used to enter enclave
> + * @flags:		Control flags
> + * @exit_leaf:		ENCLU leaf from \%eax at time of exit
> + * @exit_reason:	Cause of exit from enclave, e.g. EEXIT vs. exception
> + * @user_handler:	User provided exit handler (optional)
> + * @user_data:		User provided opaque value (optional)
> + * @exception:		Valid on exit due to exception
>   */
> -typedef int (*sgx_enclave_exit_handler_t)(long rdi, long rsi, long rdx,
> -					  long ursp, long r8, long r9,
> -					  void *tcs, int ret,
> -					  struct sgx_enclave_exception *e);
> +struct sgx_enclave_run {
> +	__u64 tcs;
> +	__u64 flags;
> +
> +	__u32 exit_leaf;
> +	__u32 exit_reason;
> +
> +	union {
> +		sgx_enclave_exit_handler_t user_handler;
> +		__u64 __user_handler;
> +	};
> +	__u64 user_data;
> +
> +	union {
> +		struct sgx_enclave_exception exception;
> +
> +		/* Pad the entire struct to 256 bytes. */
> +		__u8 pad[256 - 40];
> +	};
> +};
>  
>  /**
>   * __vdso_sgx_enter_enclave() - Enter an SGX enclave
> @@ -119,16 +153,14 @@ typedef int (*sgx_enclave_exit_handler_t)(long rdi, long rsi, long rdx,
>   * @leaf:	ENCLU leaf, must be EENTER or ERESUME
>   * @r8:		Pass-through value for R8
>   * @r9:		Pass-through value for R9
> - * @tcs:	TCS, must be non-NULL
> - * @e:		Optional struct sgx_enclave_exception instance
> - * @handler:	Optional enclave exit handler
> + * @r:		struct sgx_enclave_run, must be non-NULL
>   *
>   * NOTE: __vdso_sgx_enter_enclave() does not ensure full compliance with the
> - * x86-64 ABI, e.g. doesn't explicitly clear EFLAGS.DF after EEXIT.  Except for
> - * non-volatile general purpose registers, preserving/setting state in
> - * accordance with the x86-64 ABI is the responsibility of the enclave and its
> - * runtime, i.e. __vdso_sgx_enter_enclave() cannot be called from C code
> - * without careful consideration by both the enclave and its runtime.
> + * x86-64 ABI, e.g. doesn't handle XSAVE state.  Except for non-volatile
> + * general purpose registers, EFLAGS.DF, and RSP alignment, preserving/setting
> + * state in accordance with the x86-64 ABI is the responsibility of the enclave
> + * and its runtime, i.e. __vdso_sgx_enter_enclave() cannot be called from C
> + * code without careful consideration by both the enclave and its runtime.
>   *
>   * All general purpose registers except RAX, RBX and RCX are passed as-is to
>   * the enclave.  RAX, RBX and RCX are consumed by EENTER and ERESUME and are
> @@ -160,16 +192,12 @@ typedef int (*sgx_enclave_exit_handler_t)(long rdi, long rsi, long rdx,
>   * without returning to __vdso_sgx_enter_enclave().
>   *
>   * Return:
> - *  0 on success,
> + *  0 on success (ENCLU reached),
>   *  -EINVAL if ENCLU leaf is not allowed,
> - *  -EFAULT if an exception occurs on ENCLU or within the enclave
> - *  -errno for all other negative values returned by the userspace exit handler
>   */
>  typedef int (*vdso_sgx_enter_enclave_t)(unsigned long rdi, unsigned long rsi,
>  					unsigned long rdx, unsigned int leaf,
>  					unsigned long r8,  unsigned long r9,
> -					void *tcs,
> -					struct sgx_enclave_exception *e,
> -					sgx_enclave_exit_handler_t handler);
> +					struct sgx_enclave_run *r);
>  
>  #endif /* _UAPI_ASM_X86_SGX_H */
> 


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4054 bytes --]

Re: [RFC PATCH 4/4] x86/vdso: x86/sgx: Allow the user to exit the vDSO loop on interrupts

[Andy Lutomirski]

On Thu, Aug 20, 2020 at 4:20 AM Jethro Beekman <jethro@fortanix.com> wrote:
>
> On 2020-08-19 17:02, Andy Lutomirski wrote:
> > On Wed, Aug 19, 2020 at 7:21 AM Jethro Beekman <jethro@fortanix.com> wrote:
> >>
> >> On 2020-08-18 19:15, Andy Lutomirski wrote:
> >>> On Mon, Aug 17, 2020 at 9:24 PM Sean Christopherson
> >>> <sean.j.christopherson@intel.com> wrote:
> >>>>
> >>>> Allow userspace to exit the vDSO on interrupts that are acknowledged
> >>>> while the enclave is active.  This allows the user's runtime to switch
> >>>> contexts at opportune times without additional overhead, e.g. when using
> >>>> an M:N threading model (where M user threads run N TCSs, with N > M).
> >>>
> >>> This is IMO rather odd.  We don't support this type of notification on
> >>> interrupts for normal user code.  The fact user code can detect
> >>> interrupts during enclave execution is IMO an oddity of SGX, and I
> >>> have asked Intel to consider replacing the AEX mechanism with
> >>> something more transparent to user mode.  If this ever happens, this
> >>> mechanism is toast.
> >>
> >> Let's design the current interface for the current architecture. We can deal with a new architecture if and when Intel provides it.
> >
> > No.  If Intel fixes the architecture, the proposed interface will
> > *stop working*.
> >
> >>
> >>> Even without architecture changes, building a *reliable* M:N threading
> >>> mechanism on top of this will be difficult or impossible, as there is
> >>> no particular guarantee that a thread will get timing interrupts at> all or that these interrupts will get lucky and hit enclave code, thus
> >>> triggering an AEX.  We certainly don't, and probably never will,
> >>> support any corresponding feature for non-enclave code.
> >>
> >> There's no guarantee, but this vDSO exit mechanism is a prerequisite. Both for context switching and aborting an enclave, userspace *must* have a way to trigger exit from enclave mode *and* recover the user stack in a sane manner. Userspace *should* also be able to do this in a way that's compatible with library use, so calling timer_create or pthread_kill to deliver a signal would be ok, but installing a signal handler should be avoided (the whole reason behind this vDSO call).
> >
> > If you want to abort an enclave, abort it the same way you abort any
> > other user code -- send a signal or something.  If something is wrong> with signal handling in the proposed vDSO interface, then by all
> > means, let's fix it.  If we need better library signal support, let's
> > add such a thing.  If you really want to abort an enclave *cleanly*
> > without any chance of garbage left around, stick it in a subprocess.
> > We are not playing the game where someone sets a flag, crosses their
> > fingers, and waits for an interrupt.
>
> Sending a signal is not sufficient. The current __vdso_sgx_enter_enclave call is not interruptible.
>

Why not?  If we are failing to deliver signals if
__vdso_sgx_enter_enclave() is running, we have a bug and we should fix
it.  We should actually deliver the signal and then either resume the
enclave or return -EINTR or equivalent.  Are we getting this wrong?
Looking at your code, I think we're doing it right but maybe we could
do better.

I suppose we also need a test case for this if we do anything fancy here.

> > Now maybe I'm wrong and there's an actual legitimate use case for this
> > trick, in which case I'd like to be enlightened.  But M:N threading
> > and enclave aborting don't sound like legitimate use cases.
> >
>
> Why is it ok for this code to return to userspace after 1 second:
>
>
>
> void ignore_signal_but_dont_restart(int s) {}
>
> // set SIGALRM handler if none set (FIXME: racy)
> struct sigaction sa_old;
> struct sigaction sa = {
>     .sa_handler = ignore_signal_but_dont_restart,
>     .sa_flags = 0,
> };
> sigaction(SIGALRM, &sa, &sa_old);
> if (!(sa_old.sa_handler == SIG_IGN || sa_old.sa_handler == SIG_DFL)
>     || (sa_old.sa_flags & SA_SIGINFO)) {
>     sa_old.sa_flags &= ~SA_RESTART;
>     sigaction(SIGALRM, &sa_old, NULL);
> }
>
> alarm(1);
>
> char buf;
> read(0, &buf, 1);
>

Seems fine.  That's POSIX.  User code is receiving a signal and is
being affected by the signal.  A signal is a user-visible thing.

>
>
> But, according to your train of thought, this code must hang indefinitely (assuming the enclave is not calling EEXIT)?
>
>
>
> void ignore_signal_but_dont_restart(int s) {}
>
> // set SIGALRM handler if none set (FIXME: racy)
> struct sigaction sa_old;
> struct sigaction sa = {
>     .sa_handler = ignore_signal_but_dont_restart,
>     .sa_flags = 0,
> };
> sigaction(SIGALRM, &sa, &sa_old);
> if (!(sa_old.sa_handler == SIG_IGN || sa_old.sa_handler == SIG_DFL)
>     || (sa_old.sa_flags & SA_SIGINFO)) {
>     sa_old.sa_flags &= ~SA_RESTART;
>     sigaction(SIGALRM, &sa_old, NULL);
> }
>
> alarm(1);
>
> __vdso_sgx_enter_enclave(0, 0, 0, 2, 0, 0, tcs, NULL, NULL);
>

This is a genuine semantic question: is __vdso_sgx_enter_enclave()
like read on a pipe (returns -EINTR on a signal) or is it more like a
restartable syscall or a normal library function that just keeps
running if your signal handler does nothing?  You could siglongjmp()
out, but that's a bit gross.

I wouldn't object to an option to __vdso_sgx_enter_enclave() to make
it return -EINTR if signaled by a non-SA_RESTART signal.  Implementing
it might be distinctly nontrivial, though.

But this isn't what this patch does, and I suspect we've been talking
past each other.  This patch makes __vdso_sgx_enter_enclave() return
if there's an *interrupt*.  If you push a keyboard button, move your
mouse, get a network interrupt on that core, etc, it will return.
This is nonsense.

--Andy

Re: [RFC PATCH 4/4] x86/vdso: x86/sgx: Allow the user to exit the vDSO loop on interrupts

[Jethro Beekman]

[-- Attachment #1: Type: text/plain, Size: 6518 bytes --]

On 2020-08-20 19:44, Andy Lutomirski wrote:
> On Thu, Aug 20, 2020 at 4:20 AM Jethro Beekman <jethro@fortanix.com> wrote:
>>
>> On 2020-08-19 17:02, Andy Lutomirski wrote:
>>> On Wed, Aug 19, 2020 at 7:21 AM Jethro Beekman <jethro@fortanix.com> wrote:
>>>>
>>>> On 2020-08-18 19:15, Andy Lutomirski wrote:
>>>>> On Mon, Aug 17, 2020 at 9:24 PM Sean Christopherson
>>>>> <sean.j.christopherson@intel.com> wrote:
>>>>>>
>>>>>> Allow userspace to exit the vDSO on interrupts that are acknowledged
>>>>>> while the enclave is active.  This allows the user's runtime to switch
>>>>>> contexts at opportune times without additional overhead, e.g. when using
>>>>>> an M:N threading model (where M user threads run N TCSs, with N > M).
>>>>>
>>>>> This is IMO rather odd.  We don't support this type of notification on
>>>>> interrupts for normal user code.  The fact user code can detect
>>>>> interrupts during enclave execution is IMO an oddity of SGX, and I
>>>>> have asked Intel to consider replacing the AEX mechanism with
>>>>> something more transparent to user mode.  If this ever happens, this
>>>>> mechanism is toast.
>>>>
>>>> Let's design the current interface for the current architecture. We can deal with a new architecture if and when Intel provides it.
>>>
>>> No.  If Intel fixes the architecture, the proposed interface will
>>> *stop working*.
>>>
>>>>
>>>>> Even without architecture changes, building a *reliable* M:N threading
>>>>> mechanism on top of this will be difficult or impossible, as there is
>>>>> no particular guarantee that a thread will get timing interrupts at> all or that these interrupts will get lucky and hit enclave code, thus
>>>>> triggering an AEX.  We certainly don't, and probably never will,
>>>>> support any corresponding feature for non-enclave code.
>>>>
>>>> There's no guarantee, but this vDSO exit mechanism is a prerequisite. Both for context switching and aborting an enclave, userspace *must* have a way to trigger exit from enclave mode *and* recover the user stack in a sane manner. Userspace *should* also be able to do this in a way that's compatible with library use, so calling timer_create or pthread_kill to deliver a signal would be ok, but installing a signal handler should be avoided (the whole reason behind this vDSO call).
>>>
>>> If you want to abort an enclave, abort it the same way you abort any
>>> other user code -- send a signal or something.  If something is wrong> with signal handling in the proposed vDSO interface, then by all
>>> means, let's fix it.  If we need better library signal support, let's
>>> add such a thing.  If you really want to abort an enclave *cleanly*
>>> without any chance of garbage left around, stick it in a subprocess.
>>> We are not playing the game where someone sets a flag, crosses their
>>> fingers, and waits for an interrupt.
>>
>> Sending a signal is not sufficient. The current __vdso_sgx_enter_enclave call is not interruptible.
>>
> 
> Why not?  If we are failing to deliver signals if
> __vdso_sgx_enter_enclave() is running, we have a bug and we should fix
> it.  We should actually deliver the signal and then either resume the
> enclave or return -EINTR or equivalent.  Are we getting this wrong?
> Looking at your code, I think we're doing it right but maybe we could
> do better.
> 
> I suppose we also need a test case for this if we do anything fancy here.
> 
>>> Now maybe I'm wrong and there's an actual legitimate use case for this
>>> trick, in which case I'd like to be enlightened.  But M:N threading
>>> and enclave aborting don't sound like legitimate use cases.
>>>
>>
>> Why is it ok for this code to return to userspace after 1 second:
>>
>>
>>
>> void ignore_signal_but_dont_restart(int s) {}
>>
>> // set SIGALRM handler if none set (FIXME: racy)
>> struct sigaction sa_old;
>> struct sigaction sa = {
>>     .sa_handler = ignore_signal_but_dont_restart,
>>     .sa_flags = 0,
>> };
>> sigaction(SIGALRM, &sa, &sa_old);
>> if (!(sa_old.sa_handler == SIG_IGN || sa_old.sa_handler == SIG_DFL)
>>     || (sa_old.sa_flags & SA_SIGINFO)) {
>>     sa_old.sa_flags &= ~SA_RESTART;
>>     sigaction(SIGALRM, &sa_old, NULL);
>> }
>>
>> alarm(1);
>>
>> char buf;
>> read(0, &buf, 1);
>>
> 
> Seems fine.  That's POSIX.  User code is receiving a signal and is
> being affected by the signal.  A signal is a user-visible thing.
> 
>>
>>
>> But, according to your train of thought, this code must hang indefinitely (assuming the enclave is not calling EEXIT)?
>>
>>
>>
>> void ignore_signal_but_dont_restart(int s) {}
>>
>> // set SIGALRM handler if none set (FIXME: racy)
>> struct sigaction sa_old;
>> struct sigaction sa = {
>>     .sa_handler = ignore_signal_but_dont_restart,
>>     .sa_flags = 0,
>> };
>> sigaction(SIGALRM, &sa, &sa_old);
>> if (!(sa_old.sa_handler == SIG_IGN || sa_old.sa_handler == SIG_DFL)
>>     || (sa_old.sa_flags & SA_SIGINFO)) {
>>     sa_old.sa_flags &= ~SA_RESTART;
>>     sigaction(SIGALRM, &sa_old, NULL);
>> }
>>
>> alarm(1);
>>
>> __vdso_sgx_enter_enclave(0, 0, 0, 2, 0, 0, tcs, NULL, NULL);
>>
> 
> This is a genuine semantic question: is __vdso_sgx_enter_enclave()
> like read on a pipe (returns -EINTR on a signal) or is it more like a
> restartable syscall or a normal library function that just keeps
> running if your signal handler does nothing?  You could siglongjmp()
> out, but that's a bit gross.
> 
> I wouldn't object to an option to __vdso_sgx_enter_enclave() to make
> it return -EINTR if signaled by a non-SA_RESTART signal.  Implementing
> it might be distinctly nontrivial, though.
> 
> But this isn't what this patch does, and I suspect we've been talking
> past each other.  This patch makes __vdso_sgx_enter_enclave() return
> if there's an *interrupt*.  If you push a keyboard button, move your
> mouse, get a network interrupt on that core, etc, it will return.
> This is nonsense.

It's not nontrivial to return on signals, this patch does it. This patch *also* returns when there's a HW interrupt, but that's not important. 

It sounds like you're saying you want to subdivide AEXs into “interrupts that lead to user-observable signals” and “other interrupts”, and then hide the second category from the user. I wouldn't object to that, but I don't know how to code this. It seems like a lot of work compared to the obvious solution (this patch).

--
Jethro Beekman | Fortanix


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4054 bytes --]

Re: [RFC PATCH 4/4] x86/vdso: x86/sgx: Allow the user to exit the vDSO loop on interrupts

[Andy Lutomirski]

On Thu, Aug 20, 2020 at 10:53 AM Jethro Beekman <jethro@fortanix.com> wrote:
>
> On 2020-08-20 19:44, Andy Lutomirski wrote:
> > On Thu, Aug 20, 2020 at 4:20 AM Jethro Beekman <jethro@fortanix.com> wrote:
> >>
> >> On 2020-08-19 17:02, Andy Lutomirski wrote:
> >>> On Wed, Aug 19, 2020 at 7:21 AM Jethro Beekman <jethro@fortanix.com> wrote:
> >>>>
> >>>> On 2020-08-18 19:15, Andy Lutomirski wrote:
> >>>>> On Mon, Aug 17, 2020 at 9:24 PM Sean Christopherson
> >>>>> <sean.j.christopherson@intel.com> wrote:
> >>>>>>
> >>>>>> Allow userspace to exit the vDSO on interrupts that are acknowledged
> >>>>>> while the enclave is active.  This allows the user's runtime to switch
> >>>>>> contexts at opportune times without additional overhead, e.g. when using
> >>>>>> an M:N threading model (where M user threads run N TCSs, with N > M).
> >>>>>
> >>>>> This is IMO rather odd.  We don't support this type of notification on
> >>>>> interrupts for normal user code.  The fact user code can detect
> >>>>> interrupts during enclave execution is IMO an oddity of SGX, and I
> >>>>> have asked Intel to consider replacing the AEX mechanism with
> >>>>> something more transparent to user mode.  If this ever happens, this
> >>>>> mechanism is toast.
> >>>>
> >>>> Let's design the current interface for the current architecture. We can deal with a new architecture if and when Intel provides it.
> >>>
> >>> No.  If Intel fixes the architecture, the proposed interface will
> >>> *stop working*.
> >>>
> >>>>
> >>>>> Even without architecture changes, building a *reliable* M:N threading
> >>>>> mechanism on top of this will be difficult or impossible, as there is
> >>>>> no particular guarantee that a thread will get timing interrupts at> all or that these interrupts will get lucky and hit enclave code, thus
> >>>>> triggering an AEX.  We certainly don't, and probably never will,
> >>>>> support any corresponding feature for non-enclave code.
> >>>>
> >>>> There's no guarantee, but this vDSO exit mechanism is a prerequisite. Both for context switching and aborting an enclave, userspace *must* have a way to trigger exit from enclave mode *and* recover the user stack in a sane manner. Userspace *should* also be able to do this in a way that's compatible with library use, so calling timer_create or pthread_kill to deliver a signal would be ok, but installing a signal handler should be avoided (the whole reason behind this vDSO call).
> >>>
> >>> If you want to abort an enclave, abort it the same way you abort any
> >>> other user code -- send a signal or something.  If something is wrong> with signal handling in the proposed vDSO interface, then by all
> >>> means, let's fix it.  If we need better library signal support, let's
> >>> add such a thing.  If you really want to abort an enclave *cleanly*
> >>> without any chance of garbage left around, stick it in a subprocess.
> >>> We are not playing the game where someone sets a flag, crosses their
> >>> fingers, and waits for an interrupt.
> >>
> >> Sending a signal is not sufficient. The current __vdso_sgx_enter_enclave call is not interruptible.
> >>
> >
> > Why not?  If we are failing to deliver signals if
> > __vdso_sgx_enter_enclave() is running, we have a bug and we should fix
> > it.  We should actually deliver the signal and then either resume the
> > enclave or return -EINTR or equivalent.  Are we getting this wrong?
> > Looking at your code, I think we're doing it right but maybe we could
> > do better.
> >
> > I suppose we also need a test case for this if we do anything fancy here.
> >
> >>> Now maybe I'm wrong and there's an actual legitimate use case for this
> >>> trick, in which case I'd like to be enlightened.  But M:N threading
> >>> and enclave aborting don't sound like legitimate use cases.
> >>>
> >>
> >> Why is it ok for this code to return to userspace after 1 second:
> >>
> >>
> >>
> >> void ignore_signal_but_dont_restart(int s) {}
> >>
> >> // set SIGALRM handler if none set (FIXME: racy)
> >> struct sigaction sa_old;
> >> struct sigaction sa = {
> >>     .sa_handler = ignore_signal_but_dont_restart,
> >>     .sa_flags = 0,
> >> };
> >> sigaction(SIGALRM, &sa, &sa_old);
> >> if (!(sa_old.sa_handler == SIG_IGN || sa_old.sa_handler == SIG_DFL)
> >>     || (sa_old.sa_flags & SA_SIGINFO)) {
> >>     sa_old.sa_flags &= ~SA_RESTART;
> >>     sigaction(SIGALRM, &sa_old, NULL);
> >> }
> >>
> >> alarm(1);
> >>
> >> char buf;
> >> read(0, &buf, 1);
> >>
> >
> > Seems fine.  That's POSIX.  User code is receiving a signal and is
> > being affected by the signal.  A signal is a user-visible thing.
> >
> >>
> >>
> >> But, according to your train of thought, this code must hang indefinitely (assuming the enclave is not calling EEXIT)?
> >>
> >>
> >>
> >> void ignore_signal_but_dont_restart(int s) {}
> >>
> >> // set SIGALRM handler if none set (FIXME: racy)
> >> struct sigaction sa_old;
> >> struct sigaction sa = {
> >>     .sa_handler = ignore_signal_but_dont_restart,
> >>     .sa_flags = 0,
> >> };
> >> sigaction(SIGALRM, &sa, &sa_old);
> >> if (!(sa_old.sa_handler == SIG_IGN || sa_old.sa_handler == SIG_DFL)
> >>     || (sa_old.sa_flags & SA_SIGINFO)) {
> >>     sa_old.sa_flags &= ~SA_RESTART;
> >>     sigaction(SIGALRM, &sa_old, NULL);
> >> }
> >>
> >> alarm(1);
> >>
> >> __vdso_sgx_enter_enclave(0, 0, 0, 2, 0, 0, tcs, NULL, NULL);
> >>
> >
> > This is a genuine semantic question: is __vdso_sgx_enter_enclave()
> > like read on a pipe (returns -EINTR on a signal) or is it more like a
> > restartable syscall or a normal library function that just keeps
> > running if your signal handler does nothing?  You could siglongjmp()
> > out, but that's a bit gross.
> >
> > I wouldn't object to an option to __vdso_sgx_enter_enclave() to make
> > it return -EINTR if signaled by a non-SA_RESTART signal.  Implementing
> > it might be distinctly nontrivial, though.
> >
> > But this isn't what this patch does, and I suspect we've been talking
> > past each other.  This patch makes __vdso_sgx_enter_enclave() return
> > if there's an *interrupt*.  If you push a keyboard button, move your
> > mouse, get a network interrupt on that core, etc, it will return.
> > This is nonsense.
>
> It's not nontrivial to return on signals, this patch does it. This patch *also* returns when there's a HW interrupt, but that's not important.

Allow me to quote the changelog of the patch:

This allows the user's runtime to switch
contexts at opportune times without additional overhead, e.g. when using
an M:N threading model (where M user threads run N TCSs, with N > M).

NAK.

>
> It sounds like you're saying you want to subdivide AEXs into “interrupts that lead to user-observable signals” and “other interrupts”, and then hide the second category from the user. I wouldn't object to that, but I don't know how to code this. It seems like a lot of work compared to the obvious solution (this patch).

The obvious solution is NAKked.

Does siglongjmp() really not work for you?

--Andy

Re: [RFC PATCH 2/4] x86/vdso: x86/sgx: Rework __vdso_sgx_enter_enclave() API

[Jethro Beekman]

[-- Attachment #1: Type: text/plain, Size: 2562 bytes --]

On 2020-08-18 06:24, Sean Christopherson wrote:
>  /**
>   * __vdso_sgx_enter_enclave() - Enter an SGX enclave
> @@ -119,16 +153,14 @@ typedef int (*sgx_enclave_exit_handler_t)(long rdi, long rsi, long rdx,
>   * @leaf:	ENCLU leaf, must be EENTER or ERESUME
>   * @r8:		Pass-through value for R8
>   * @r9:		Pass-through value for R9
> - * @tcs:	TCS, must be non-NULL
> - * @e:		Optional struct sgx_enclave_exception instance
> - * @handler:	Optional enclave exit handler
> + * @r:		struct sgx_enclave_run, must be non-NULL
>   *
>   * NOTE: __vdso_sgx_enter_enclave() does not ensure full compliance with the
> - * x86-64 ABI, e.g. doesn't explicitly clear EFLAGS.DF after EEXIT.  Except for
> - * non-volatile general purpose registers, preserving/setting state in
> - * accordance with the x86-64 ABI is the responsibility of the enclave and its
> - * runtime, i.e. __vdso_sgx_enter_enclave() cannot be called from C code
> - * without careful consideration by both the enclave and its runtime.
> + * x86-64 ABI, e.g. doesn't handle XSAVE state.  Except for non-volatile
> + * general purpose registers, EFLAGS.DF, and RSP alignment, preserving/setting
> + * state in accordance with the x86-64 ABI is the responsibility of the enclave
> + * and its runtime, i.e. __vdso_sgx_enter_enclave() cannot be called from C
> + * code without careful consideration by both the enclave and its runtime.
>   *
>   * All general purpose registers except RAX, RBX and RCX are passed as-is to
>   * the enclave.  RAX, RBX and RCX are consumed by EENTER and ERESUME and are
> @@ -160,16 +192,12 @@ typedef int (*sgx_enclave_exit_handler_t)(long rdi, long rsi, long rdx,
>   * without returning to __vdso_sgx_enter_enclave().
>   *
>   * Return:
> - *  0 on success,
> + *  0 on success (ENCLU reached),
>   *  -EINVAL if ENCLU leaf is not allowed,
> - *  -EFAULT if an exception occurs on ENCLU or within the enclave
> - *  -errno for all other negative values returned by the userspace exit handler
>   */
>  typedef int (*vdso_sgx_enter_enclave_t)(unsigned long rdi, unsigned long rsi,
>  					unsigned long rdx, unsigned int leaf,
>  					unsigned long r8,  unsigned long r9,
> -					void *tcs,
> -					struct sgx_enclave_exception *e,
> -					sgx_enclave_exit_handler_t handler);
> +					struct sgx_enclave_run *r);
>  
>  #endif /* _UAPI_ASM_X86_SGX_H */
> 

I think this should return void now, not int? Then, the “return” section of the documentation is also no longer correct.

--
Jethro Beekman | Fortanix



[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4054 bytes --]

Re: [RFC PATCH 4/4] x86/vdso: x86/sgx: Allow the user to exit the vDSO loop on interrupts

[Jethro Beekman]

[-- Attachment #1: Type: text/plain, Size: 2882 bytes --]

On 2020-08-22 23:55, Andy Lutomirski wrote:
> On Thu, Aug 20, 2020 at 10:53 AM Jethro Beekman <jethro@fortanix.com> wrote:
>>
>> On 2020-08-20 19:44, Andy Lutomirski wrote:
>>> This is a genuine semantic question: is __vdso_sgx_enter_enclave()
>>> like read on a pipe (returns -EINTR on a signal) or is it more like a
>>> restartable syscall or a normal library function that just keeps
>>> running if your signal handler does nothing?  You could siglongjmp()
>>> out, but that's a bit gross.
>>>
>>> I wouldn't object to an option to __vdso_sgx_enter_enclave() to make
>>> it return -EINTR if signaled by a non-SA_RESTART signal.  Implementing
>>> it might be distinctly nontrivial, though.
>>>
>>> But this isn't what this patch does, and I suspect we've been talking
>>> past each other.  This patch makes __vdso_sgx_enter_enclave() return
>>> if there's an *interrupt*.  If you push a keyboard button, move your
>>> mouse, get a network interrupt on that core, etc, it will return.
>>> This is nonsense.
>>
>> It's not nontrivial to return on signals, this patch does it. This patch *also* returns when there's a HW interrupt, but that's not important.
> 
> Allow me to quote the changelog of the patch:
> 
> This allows the user's runtime to switch
> contexts at opportune times without additional overhead, e.g. when using
> an M:N threading model (where M user threads run N TCSs, with N > M).
> 
> NAK.
> 
>>
>> It sounds like you're saying you want to subdivide AEXs into “interrupts that lead to user-observable signals” and “other interrupts”, and then hide the second category from the user. I wouldn't object to that, but I don't know how to code this. It seems like a lot of work compared to the obvious solution (this patch).
> 
> The obvious solution is NAKked.
> 
> Does siglongjmp() really not work for you?

Allow me to quote the changelog of "[PATCH v36 18/24] x86/vdso: Add support for exception fixup in vDSO functions":

Putting everything together, userspace enclaves will utilize a library
that must be prepared to handle any and (almost) all exceptions any time
at least one thread may be executing in an enclave.  Leveraging signals
to handle the enclave exceptions is unpleasant, to put it mildly, e.g.
the SGX library must constantly (un)register its signal handler based
on whether or not at least one thread is executing in an enclave, and
filter and forward exceptions that aren't related to its enclaves.  This
becomes particularly nasty when using multiple levels of libraries that
register signal handlers, e.g. running an enclave via cgo inside of the
Go runtime.


If I could use signal handlers I wouldn't use the VDSO call. (Corollary: if I wasn't using the VDSO call, I wouldn't have to think about this because what I want is already supported).

--
Jethro Beekman | Fortanix


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4054 bytes --]

Re: [RFC PATCH 2/4] x86/vdso: x86/sgx: Rework __vdso_sgx_enter_enclave() API

[Jarkko Sakkinen]

On Mon, Aug 24, 2020 at 03:36:11PM +0200, Jethro Beekman wrote:
> On 2020-08-18 06:24, Sean Christopherson wrote:
> >  /**
> >   * __vdso_sgx_enter_enclave() - Enter an SGX enclave
> > @@ -119,16 +153,14 @@ typedef int (*sgx_enclave_exit_handler_t)(long rdi, long rsi, long rdx,
> >   * @leaf:	ENCLU leaf, must be EENTER or ERESUME
> >   * @r8:		Pass-through value for R8
> >   * @r9:		Pass-through value for R9
> > - * @tcs:	TCS, must be non-NULL
> > - * @e:		Optional struct sgx_enclave_exception instance
> > - * @handler:	Optional enclave exit handler
> > + * @r:		struct sgx_enclave_run, must be non-NULL
> >   *
> >   * NOTE: __vdso_sgx_enter_enclave() does not ensure full compliance with the
> > - * x86-64 ABI, e.g. doesn't explicitly clear EFLAGS.DF after EEXIT.  Except for
> > - * non-volatile general purpose registers, preserving/setting state in
> > - * accordance with the x86-64 ABI is the responsibility of the enclave and its
> > - * runtime, i.e. __vdso_sgx_enter_enclave() cannot be called from C code
> > - * without careful consideration by both the enclave and its runtime.
> > + * x86-64 ABI, e.g. doesn't handle XSAVE state.  Except for non-volatile
> > + * general purpose registers, EFLAGS.DF, and RSP alignment, preserving/setting
> > + * state in accordance with the x86-64 ABI is the responsibility of the enclave
> > + * and its runtime, i.e. __vdso_sgx_enter_enclave() cannot be called from C
> > + * code without careful consideration by both the enclave and its runtime.
> >   *
> >   * All general purpose registers except RAX, RBX and RCX are passed as-is to
> >   * the enclave.  RAX, RBX and RCX are consumed by EENTER and ERESUME and are
> > @@ -160,16 +192,12 @@ typedef int (*sgx_enclave_exit_handler_t)(long rdi, long rsi, long rdx,
> >   * without returning to __vdso_sgx_enter_enclave().
> >   *
> >   * Return:
> > - *  0 on success,
> > + *  0 on success (ENCLU reached),
> >   *  -EINVAL if ENCLU leaf is not allowed,
> > - *  -EFAULT if an exception occurs on ENCLU or within the enclave
> > - *  -errno for all other negative values returned by the userspace exit handler
> >   */
> >  typedef int (*vdso_sgx_enter_enclave_t)(unsigned long rdi, unsigned long rsi,
> >  					unsigned long rdx, unsigned int leaf,
> >  					unsigned long r8,  unsigned long r9,
> > -					void *tcs,
> > -					struct sgx_enclave_exception *e,
> > -					sgx_enclave_exit_handler_t handler);
> > +					struct sgx_enclave_run *r);
> >  
> >  #endif /* _UAPI_ASM_X86_SGX_H */
> > 
> 
> I think this should return void now, not int? Then, the “return”
> section of the documentation is also no longer correct.

This documentation should be moved to Documentation/x86/sgx.rst.

It is easier to read from there and then it will be included by kdoc
to the kernel documentation. In here it is not addressed by kdoc and
it is unnecessarily hard to read.

> --
> Jethro Beekman | Fortanix

/Jarkko

Re: [RFC PATCH 2/4] x86/vdso: x86/sgx: Rework __vdso_sgx_enter_enclave() API

[Sean Christopherson]

On Mon, Aug 24, 2020 at 03:36:11PM +0200, Jethro Beekman wrote:
> On 2020-08-18 06:24, Sean Christopherson wrote:
> >  /**
> >   * __vdso_sgx_enter_enclave() - Enter an SGX enclave
> > @@ -119,16 +153,14 @@ typedef int (*sgx_enclave_exit_handler_t)(long rdi, long rsi, long rdx,
> >   * @leaf:	ENCLU leaf, must be EENTER or ERESUME
> >   * @r8:		Pass-through value for R8
> >   * @r9:		Pass-through value for R9
> > - * @tcs:	TCS, must be non-NULL
> > - * @e:		Optional struct sgx_enclave_exception instance
> > - * @handler:	Optional enclave exit handler
> > + * @r:		struct sgx_enclave_run, must be non-NULL
> >   *
> >   * NOTE: __vdso_sgx_enter_enclave() does not ensure full compliance with the
> > - * x86-64 ABI, e.g. doesn't explicitly clear EFLAGS.DF after EEXIT.  Except for
> > - * non-volatile general purpose registers, preserving/setting state in
> > - * accordance with the x86-64 ABI is the responsibility of the enclave and its
> > - * runtime, i.e. __vdso_sgx_enter_enclave() cannot be called from C code
> > - * without careful consideration by both the enclave and its runtime.
> > + * x86-64 ABI, e.g. doesn't handle XSAVE state.  Except for non-volatile
> > + * general purpose registers, EFLAGS.DF, and RSP alignment, preserving/setting
> > + * state in accordance with the x86-64 ABI is the responsibility of the enclave
> > + * and its runtime, i.e. __vdso_sgx_enter_enclave() cannot be called from C
> > + * code without careful consideration by both the enclave and its runtime.
> >   *
> >   * All general purpose registers except RAX, RBX and RCX are passed as-is to
> >   * the enclave.  RAX, RBX and RCX are consumed by EENTER and ERESUME and are
> > @@ -160,16 +192,12 @@ typedef int (*sgx_enclave_exit_handler_t)(long rdi, long rsi, long rdx,
> >   * without returning to __vdso_sgx_enter_enclave().
> >   *
> >   * Return:
> > - *  0 on success,
> > + *  0 on success (ENCLU reached),
> >   *  -EINVAL if ENCLU leaf is not allowed,
> > - *  -EFAULT if an exception occurs on ENCLU or within the enclave
> > - *  -errno for all other negative values returned by the userspace exit handler
> >   */
> >  typedef int (*vdso_sgx_enter_enclave_t)(unsigned long rdi, unsigned long rsi,
> >  					unsigned long rdx, unsigned int leaf,
> >  					unsigned long r8,  unsigned long r9,
> > -					void *tcs,
> > -					struct sgx_enclave_exception *e,
> > -					sgx_enclave_exit_handler_t handler);
> > +					struct sgx_enclave_run *r);
> >  
> >  #endif /* _UAPI_ASM_X86_SGX_H */
> > 
> 
> I think this should return void now, not int? Then, the “return” section of
> the documentation is also no longer correct.

No, it returns -EINVAL if the leaf is bogus (and if unsupported flags are
specified, if a @flags param is ever added).

Re: [RFC PATCH 2/4] x86/vdso: x86/sgx: Rework __vdso_sgx_enter_enclave() API

[Jethro Beekman]

[-- Attachment #1: Type: text/plain, Size: 3180 bytes --]

On 2020-08-25 01:54, Sean Christopherson wrote:
> On Mon, Aug 24, 2020 at 03:36:11PM +0200, Jethro Beekman wrote:
>> On 2020-08-18 06:24, Sean Christopherson wrote:
>>>  /**
>>>   * __vdso_sgx_enter_enclave() - Enter an SGX enclave
>>> @@ -119,16 +153,14 @@ typedef int (*sgx_enclave_exit_handler_t)(long rdi, long rsi, long rdx,
>>>   * @leaf:	ENCLU leaf, must be EENTER or ERESUME
>>>   * @r8:		Pass-through value for R8
>>>   * @r9:		Pass-through value for R9
>>> - * @tcs:	TCS, must be non-NULL
>>> - * @e:		Optional struct sgx_enclave_exception instance
>>> - * @handler:	Optional enclave exit handler
>>> + * @r:		struct sgx_enclave_run, must be non-NULL
>>>   *
>>>   * NOTE: __vdso_sgx_enter_enclave() does not ensure full compliance with the
>>> - * x86-64 ABI, e.g. doesn't explicitly clear EFLAGS.DF after EEXIT.  Except for
>>> - * non-volatile general purpose registers, preserving/setting state in
>>> - * accordance with the x86-64 ABI is the responsibility of the enclave and its
>>> - * runtime, i.e. __vdso_sgx_enter_enclave() cannot be called from C code
>>> - * without careful consideration by both the enclave and its runtime.
>>> + * x86-64 ABI, e.g. doesn't handle XSAVE state.  Except for non-volatile
>>> + * general purpose registers, EFLAGS.DF, and RSP alignment, preserving/setting
>>> + * state in accordance with the x86-64 ABI is the responsibility of the enclave
>>> + * and its runtime, i.e. __vdso_sgx_enter_enclave() cannot be called from C
>>> + * code without careful consideration by both the enclave and its runtime.
>>>   *
>>>   * All general purpose registers except RAX, RBX and RCX are passed as-is to
>>>   * the enclave.  RAX, RBX and RCX are consumed by EENTER and ERESUME and are
>>> @@ -160,16 +192,12 @@ typedef int (*sgx_enclave_exit_handler_t)(long rdi, long rsi, long rdx,
>>>   * without returning to __vdso_sgx_enter_enclave().
>>>   *
>>>   * Return:
>>> - *  0 on success,
>>> + *  0 on success (ENCLU reached),
>>>   *  -EINVAL if ENCLU leaf is not allowed,
>>> - *  -EFAULT if an exception occurs on ENCLU or within the enclave
>>> - *  -errno for all other negative values returned by the userspace exit handler
>>>   */
>>>  typedef int (*vdso_sgx_enter_enclave_t)(unsigned long rdi, unsigned long rsi,
>>>  					unsigned long rdx, unsigned int leaf,
>>>  					unsigned long r8,  unsigned long r9,
>>> -					void *tcs,
>>> -					struct sgx_enclave_exception *e,
>>> -					sgx_enclave_exit_handler_t handler);
>>> +					struct sgx_enclave_run *r);
>>>  
>>>  #endif /* _UAPI_ASM_X86_SGX_H */
>>>
>>
>> I think this should return void now, not int? Then, the “return” section of
>> the documentation is also no longer correct.
> 
> No, it returns -EINVAL if the leaf is bogus (and if unsupported flags are
> specified, if a @flags param is ever added).
> 

Ok, but if I read the code correctly, contrary to the docs, -EFAULT is not returned but passed in struct sgx_enclave_run:

-.Lskip_exception_info:
-	mov	$(-EFAULT), %eax
+	/* Set the exit_reason and exception info. */
+	movl	$(-EFAULT), EXIT_REASON_OFFSET(%rbx)

--
Jethro Beekman | Fortanix


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4054 bytes --]

Re: [RFC PATCH 2/4] x86/vdso: x86/sgx: Rework __vdso_sgx_enter_enclave() API

[Sean Christopherson]

On Tue, Aug 25, 2020 at 09:36:15AM +0200, Jethro Beekman wrote:
> On 2020-08-25 01:54, Sean Christopherson wrote:
> > On Mon, Aug 24, 2020 at 03:36:11PM +0200, Jethro Beekman wrote:
> >> On 2020-08-18 06:24, Sean Christopherson wrote:
> >>>  /**
> >>>   * __vdso_sgx_enter_enclave() - Enter an SGX enclave
> >>> @@ -119,16 +153,14 @@ typedef int (*sgx_enclave_exit_handler_t)(long rdi, long rsi, long rdx,
> >>>   * @leaf:	ENCLU leaf, must be EENTER or ERESUME
> >>>   * @r8:		Pass-through value for R8
> >>>   * @r9:		Pass-through value for R9
> >>> - * @tcs:	TCS, must be non-NULL
> >>> - * @e:		Optional struct sgx_enclave_exception instance
> >>> - * @handler:	Optional enclave exit handler
> >>> + * @r:		struct sgx_enclave_run, must be non-NULL
> >>>   *
> >>>   * NOTE: __vdso_sgx_enter_enclave() does not ensure full compliance with the
> >>> - * x86-64 ABI, e.g. doesn't explicitly clear EFLAGS.DF after EEXIT.  Except for
> >>> - * non-volatile general purpose registers, preserving/setting state in
> >>> - * accordance with the x86-64 ABI is the responsibility of the enclave and its
> >>> - * runtime, i.e. __vdso_sgx_enter_enclave() cannot be called from C code
> >>> - * without careful consideration by both the enclave and its runtime.
> >>> + * x86-64 ABI, e.g. doesn't handle XSAVE state.  Except for non-volatile
> >>> + * general purpose registers, EFLAGS.DF, and RSP alignment, preserving/setting
> >>> + * state in accordance with the x86-64 ABI is the responsibility of the enclave
> >>> + * and its runtime, i.e. __vdso_sgx_enter_enclave() cannot be called from C
> >>> + * code without careful consideration by both the enclave and its runtime.
> >>>   *
> >>>   * All general purpose registers except RAX, RBX and RCX are passed as-is to
> >>>   * the enclave.  RAX, RBX and RCX are consumed by EENTER and ERESUME and are
> >>> @@ -160,16 +192,12 @@ typedef int (*sgx_enclave_exit_handler_t)(long rdi, long rsi, long rdx,
> >>>   * without returning to __vdso_sgx_enter_enclave().
> >>>   *
> >>>   * Return:
> >>> - *  0 on success,
> >>> + *  0 on success (ENCLU reached),
> >>>   *  -EINVAL if ENCLU leaf is not allowed,
> >>> - *  -EFAULT if an exception occurs on ENCLU or within the enclave
> >>> - *  -errno for all other negative values returned by the userspace exit handler
> >>>   */
> >>>  typedef int (*vdso_sgx_enter_enclave_t)(unsigned long rdi, unsigned long rsi,
> >>>  					unsigned long rdx, unsigned int leaf,
> >>>  					unsigned long r8,  unsigned long r9,
> >>> -					void *tcs,
> >>> -					struct sgx_enclave_exception *e,
> >>> -					sgx_enclave_exit_handler_t handler);
> >>> +					struct sgx_enclave_run *r);
> >>>  
> >>>  #endif /* _UAPI_ASM_X86_SGX_H */
> >>>
> >>
> >> I think this should return void now, not int? Then, the “return” section of
> >> the documentation is also no longer correct.
> > 
> > No, it returns -EINVAL if the leaf is bogus (and if unsupported flags are
> > specified, if a @flags param is ever added).
> > 
> 
> Ok, but if I read the code correctly, contrary to the docs, -EFAULT is not
> returned but passed in struct sgx_enclave_run:

The -EFAULT and -errno lines are removed from the "Return:" section of the
above documentation.  Did I miss one?

> 
> -.Lskip_exception_info:
> -	mov	$(-EFAULT), %eax
> +	/* Set the exit_reason and exception info. */
> +	movl	$(-EFAULT), EXIT_REASON_OFFSET(%rbx)
> 
> --
> Jethro Beekman | Fortanix
> 

Re: [RFC PATCH 2/4] x86/vdso: x86/sgx: Rework __vdso_sgx_enter_enclave() API

[Jethro Beekman]

[-- Attachment #1: Type: text/plain, Size: 3511 bytes --]

On 2020-08-25 09:38, Sean Christopherson wrote:
> On Tue, Aug 25, 2020 at 09:36:15AM +0200, Jethro Beekman wrote:
>> On 2020-08-25 01:54, Sean Christopherson wrote:
>>> On Mon, Aug 24, 2020 at 03:36:11PM +0200, Jethro Beekman wrote:
>>>> On 2020-08-18 06:24, Sean Christopherson wrote:
>>>>>  /**
>>>>>   * __vdso_sgx_enter_enclave() - Enter an SGX enclave
>>>>> @@ -119,16 +153,14 @@ typedef int (*sgx_enclave_exit_handler_t)(long rdi, long rsi, long rdx,
>>>>>   * @leaf:	ENCLU leaf, must be EENTER or ERESUME
>>>>>   * @r8:		Pass-through value for R8
>>>>>   * @r9:		Pass-through value for R9
>>>>> - * @tcs:	TCS, must be non-NULL
>>>>> - * @e:		Optional struct sgx_enclave_exception instance
>>>>> - * @handler:	Optional enclave exit handler
>>>>> + * @r:		struct sgx_enclave_run, must be non-NULL
>>>>>   *
>>>>>   * NOTE: __vdso_sgx_enter_enclave() does not ensure full compliance with the
>>>>> - * x86-64 ABI, e.g. doesn't explicitly clear EFLAGS.DF after EEXIT.  Except for
>>>>> - * non-volatile general purpose registers, preserving/setting state in
>>>>> - * accordance with the x86-64 ABI is the responsibility of the enclave and its
>>>>> - * runtime, i.e. __vdso_sgx_enter_enclave() cannot be called from C code
>>>>> - * without careful consideration by both the enclave and its runtime.
>>>>> + * x86-64 ABI, e.g. doesn't handle XSAVE state.  Except for non-volatile
>>>>> + * general purpose registers, EFLAGS.DF, and RSP alignment, preserving/setting
>>>>> + * state in accordance with the x86-64 ABI is the responsibility of the enclave
>>>>> + * and its runtime, i.e. __vdso_sgx_enter_enclave() cannot be called from C
>>>>> + * code without careful consideration by both the enclave and its runtime.
>>>>>   *
>>>>>   * All general purpose registers except RAX, RBX and RCX are passed as-is to
>>>>>   * the enclave.  RAX, RBX and RCX are consumed by EENTER and ERESUME and are
>>>>> @@ -160,16 +192,12 @@ typedef int (*sgx_enclave_exit_handler_t)(long rdi, long rsi, long rdx,
>>>>>   * without returning to __vdso_sgx_enter_enclave().
>>>>>   *
>>>>>   * Return:
>>>>> - *  0 on success,
>>>>> + *  0 on success (ENCLU reached),
>>>>>   *  -EINVAL if ENCLU leaf is not allowed,
>>>>> - *  -EFAULT if an exception occurs on ENCLU or within the enclave
>>>>> - *  -errno for all other negative values returned by the userspace exit handler
>>>>>   */
>>>>>  typedef int (*vdso_sgx_enter_enclave_t)(unsigned long rdi, unsigned long rsi,
>>>>>  					unsigned long rdx, unsigned int leaf,
>>>>>  					unsigned long r8,  unsigned long r9,
>>>>> -					void *tcs,
>>>>> -					struct sgx_enclave_exception *e,
>>>>> -					sgx_enclave_exit_handler_t handler);
>>>>> +					struct sgx_enclave_run *r);
>>>>>  
>>>>>  #endif /* _UAPI_ASM_X86_SGX_H */
>>>>>
>>>>
>>>> I think this should return void now, not int? Then, the “return” section of
>>>> the documentation is also no longer correct.
>>>
>>> No, it returns -EINVAL if the leaf is bogus (and if unsupported flags are
>>> specified, if a @flags param is ever added).
>>>
>>
>> Ok, but if I read the code correctly, contrary to the docs, -EFAULT is not
>> returned but passed in struct sgx_enclave_run:
> 
> The -EFAULT and -errno lines are removed from the "Return:" section of the
> above documentation.  Did I miss one?
> 

No, I'm just bad at reading patches without syntax highlighting. This is fine as is. Sorry for the confusion.

--
Jethro Beekman | Fortanix



[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4054 bytes --]

Re: [RFC PATCH 4/4] x86/vdso: x86/sgx: Allow the user to exit the vDSO loop on interrupts

[Sean Christopherson]

On Mon, Aug 24, 2020 at 03:36:29PM +0200, Jethro Beekman wrote:
> On 2020-08-22 23:55, Andy Lutomirski wrote:
> > On Thu, Aug 20, 2020 at 10:53 AM Jethro Beekman <jethro@fortanix.com> wrote:
> >>
> >> On 2020-08-20 19:44, Andy Lutomirski wrote:
> >>> This is a genuine semantic question: is __vdso_sgx_enter_enclave()
> >>> like read on a pipe (returns -EINTR on a signal) or is it more like a
> >>> restartable syscall or a normal library function that just keeps
> >>> running if your signal handler does nothing?  You could siglongjmp()
> >>> out, but that's a bit gross.
> >>>
> >>> I wouldn't object to an option to __vdso_sgx_enter_enclave() to make
> >>> it return -EINTR if signaled by a non-SA_RESTART signal.  Implementing
> >>> it might be distinctly nontrivial, though.
> >>>
> >>> But this isn't what this patch does, and I suspect we've been talking
> >>> past each other.  This patch makes __vdso_sgx_enter_enclave() return
> >>> if there's an *interrupt*.  If you push a keyboard button, move your
> >>> mouse, get a network interrupt on that core, etc, it will return.
> >>> This is nonsense.
> >>
> >> It's not nontrivial to return on signals, this patch does it. This patch *also* returns when there's a HW interrupt, but that's not important.
> > 
> > Allow me to quote the changelog of the patch:
> > 
> > This allows the user's runtime to switch
> > contexts at opportune times without additional overhead, e.g. when using
> > an M:N threading model (where M user threads run N TCSs, with N > M).
> > 
> > NAK.
> > 
> >>
> >> It sounds like you're saying you want to subdivide AEXs into “interrupts
> >> that lead to user-observable signals” and “other interrupts”, and then
> >> hide the second category from the user. I wouldn't object to that, but I
> >> don't know how to code this. It seems like a lot of work compared to the
> >> obvious solution (this patch).
> > 
> > The obvious solution is NAKked.
> > 
> > Does siglongjmp() really not work for you?
> 
> Allow me to quote the changelog of "[PATCH v36 18/24] x86/vdso: Add support
> for exception fixup in vDSO functions":
> 
> Putting everything together, userspace enclaves will utilize a library
> that must be prepared to handle any and (almost) all exceptions any time
> at least one thread may be executing in an enclave.  Leveraging signals
> to handle the enclave exceptions is unpleasant, to put it mildly, e.g.
> the SGX library must constantly (un)register its signal handler based
> on whether or not at least one thread is executing in an enclave, and
> filter and forward exceptions that aren't related to its enclaves.  This
> becomes particularly nasty when using multiple levels of libraries that
> register signal handlers, e.g. running an enclave via cgo inside of the
> Go runtime.
> 
> 
> If I could use signal handlers I wouldn't use the VDSO call. (Corollary: if I
> wasn't using the VDSO call, I wouldn't have to think about this because what
> I want is already supported).

Jethro, I think what you're requesting is simply not possible.  Or rather,
the SGX vDSO can't provide any additional value relative to what can be
provided by userspace.

What Andy is saying is that having the vDSO take action on interrupts is
undesirable/NAK'd from a kernel perspective, as it's ugly and unreliable.
It happens to mostly work for SIGALRM because there is an interrupt
associated with the expiration of the timer[*].  But even then it will
work if and only if the interrupt arrives while the enclave is running.
If the interrupt arrives just before ENCLU, either on initial entry or
on resume from the callback, SIGALRM will be delivered and the userspace
M:N scheduler that is relying on an exit from the enclave will miss its
"tick".

The vDSO could check for pending signals, but that effectively provides no
value as opposed to checking for signals in the callback (or caller).

The goal of the SGX vDSO is to obviate the need for handling signals that
are due to exceptions that are directly associated with the enclave.  IMO,
the SIGALRM request is completely out of scope as it is not a problem that
is unique to SGX, i.e. wanting to do M:N scheduling in a library without
hooking SIGALRM is a generic request.  The fact that the SGX architcture has
a quirk that allows for a semi-functional hack is not justification for
supporting such a hack in the kernel.

[*] Is it even guaranteed that an interrupt will precede SIGALRM?  Not that
    it matters for this discussion.

Re: [RFC PATCH 4/4] x86/vdso: x86/sgx: Allow the user to exit the vDSO loop on interrupts

[Xing, Cedric]

On 8/26/2020 11:32 AM, Sean Christopherson wrote:
> On Mon, Aug 24, 2020 at 03:36:29PM +0200, Jethro Beekman wrote:
>> On 2020-08-22 23:55, Andy Lutomirski wrote:
>>> On Thu, Aug 20, 2020 at 10:53 AM Jethro Beekman <jethro@fortanix.com> wrote:
>>>>
>>>> On 2020-08-20 19:44, Andy Lutomirski wrote:
>>>>> This is a genuine semantic question: is __vdso_sgx_enter_enclave()
>>>>> like read on a pipe (returns -EINTR on a signal) or is it more like a
>>>>> restartable syscall or a normal library function that just keeps
>>>>> running if your signal handler does nothing?  You could siglongjmp()
>>>>> out, but that's a bit gross.
>>>>>
>>>>> I wouldn't object to an option to __vdso_sgx_enter_enclave() to make
>>>>> it return -EINTR if signaled by a non-SA_RESTART signal.  Implementing
>>>>> it might be distinctly nontrivial, though.
>>>>>
>>>>> But this isn't what this patch does, and I suspect we've been talking
>>>>> past each other.  This patch makes __vdso_sgx_enter_enclave() return
>>>>> if there's an *interrupt*.  If you push a keyboard button, move your
>>>>> mouse, get a network interrupt on that core, etc, it will return.
>>>>> This is nonsense.
>>>>
>>>> It's not nontrivial to return on signals, this patch does it. This patch *also* returns when there's a HW interrupt, but that's not important.
>>>
>>> Allow me to quote the changelog of the patch:
>>>
>>> This allows the user's runtime to switch
>>> contexts at opportune times without additional overhead, e.g. when using
>>> an M:N threading model (where M user threads run N TCSs, with N > M).
>>>
>>> NAK.
>>>
>>>>
>>>> It sounds like you're saying you want to subdivide AEXs into “interrupts
>>>> that lead to user-observable signals” and “other interrupts”, and then
>>>> hide the second category from the user. I wouldn't object to that, but I
>>>> don't know how to code this. It seems like a lot of work compared to the
>>>> obvious solution (this patch).
>>>
>>> The obvious solution is NAKked.
>>>
>>> Does siglongjmp() really not work for you?
>>
>> Allow me to quote the changelog of "[PATCH v36 18/24] x86/vdso: Add support
>> for exception fixup in vDSO functions":
>>
>> Putting everything together, userspace enclaves will utilize a library
>> that must be prepared to handle any and (almost) all exceptions any time
>> at least one thread may be executing in an enclave.  Leveraging signals
>> to handle the enclave exceptions is unpleasant, to put it mildly, e.g.
>> the SGX library must constantly (un)register its signal handler based
>> on whether or not at least one thread is executing in an enclave, and
>> filter and forward exceptions that aren't related to its enclaves.  This
>> becomes particularly nasty when using multiple levels of libraries that
>> register signal handlers, e.g. running an enclave via cgo inside of the
>> Go runtime.
>>
>>
>> If I could use signal handlers I wouldn't use the VDSO call. (Corollary: if I
>> wasn't using the VDSO call, I wouldn't have to think about this because what
>> I want is already supported).
> 
> Jethro, I think what you're requesting is simply not possible.  Or rather,
> the SGX vDSO can't provide any additional value relative to what can be
> provided by userspace.
> 
> What Andy is saying is that having the vDSO take action on interrupts is
> undesirable/NAK'd from a kernel perspective, as it's ugly and unreliable.
> It happens to mostly work for SIGALRM because there is an interrupt
> associated with the expiration of the timer[*].  But even then it will
> work if and only if the interrupt arrives while the enclave is running.
> If the interrupt arrives just before ENCLU, either on initial entry or
> on resume from the callback, SIGALRM will be delivered and the userspace
> M:N scheduler that is relying on an exit from the enclave will miss its
> "tick".
> 
> The vDSO could check for pending signals, but that effectively provides no
> value as opposed to checking for signals in the callback (or caller).
> 
> The goal of the SGX vDSO is to obviate the need for handling signals that
> are due to exceptions that are directly associated with the enclave.  IMO,
> the SIGALRM request is completely out of scope as it is not a problem that
> is unique to SGX, i.e. wanting to do M:N scheduling in a library without
> hooking SIGALRM is a generic request.  The fact that the SGX architcture has
> a quirk that allows for a semi-functional hack is not justification for
> supporting such a hack in the kernel.
> 
> [*] Is it even guaranteed that an interrupt will precede SIGALRM?  Not that
>      it matters for this discussion.
> 

Hi Jethro,

I'm with Andy and Sean that it's unprecedented and undesired to notify 
applications of interrupts from the kernel's perspective.

I think what you really want is to be notified on signals without a 
signal handler, in an effort to avoid conflicts between libraries and 
their user applications. But please keep in mind that regarding signals, 
there are always 2 parts - signaling and handling. Even though you could 
avoid conflicts in the handling, you still may not avoid conflicts in 
the signaling part. For example, say you want SIGALRM every 100ms for 
M:N scheduling in a lib but the user application also uses SIGALRM for 
something else. Even though you can avoid conflict by not registering a 
SIGALRM handler, the user application may use SIGALRM at a different 
frequency. Hence the lib and the application will still interfere with 
each other. Then what's the good of not registering a handler?

Re: [RFC PATCH 2/4] x86/vdso: x86/sgx: Rework __vdso_sgx_enter_enclave() API

[Xing, Cedric]

On 8/17/2020 9:24 PM, Sean Christopherson wrote:
> Rework __vdso_sgx_enter_enclave() to use a struct to hold the input and
> output params.  In the new struct, add an opaque "user_data" that can be
> used to pass context across the vDSO, and an explicit "exit_reason" to
> avoid overloading the return value.
> In order to pass additional parameters to the exit handler, the exinfo 
structure could be embedded in a user-defined structure while the 
handler could pick it up using "container_of" macro. IMO the original 
interface was neat and suffcient, and we are over-engineering it.

> Moving the params into a struct will also make it less painful to use
> dedicated exit reasons, and to support exiting on interrupts in future
> patches.
> 
My apology for not following discussion lately. What did you mean by 
"dedicated exit reasons" and why was it "painful"? According to another 
thread, I guess we wouldn't have "exiting on interrupts".

Re: [RFC PATCH 2/4] x86/vdso: x86/sgx: Rework __vdso_sgx_enter_enclave() API

[Sean Christopherson]

On Wed, Aug 26, 2020 at 12:27:54PM -0700, Xing, Cedric wrote:
> On 8/17/2020 9:24 PM, Sean Christopherson wrote:
> > Rework __vdso_sgx_enter_enclave() to use a struct to hold the input and
> > output params.  In the new struct, add an opaque "user_data" that can be
> > used to pass context across the vDSO, and an explicit "exit_reason" to
> > avoid overloading the return value.
> > In order to pass additional parameters to the exit handler, the exinfo
> structure could be embedded in a user-defined structure while the handler
> could pick it up using "container_of" macro. IMO the original interface was
> neat and suffcient, and we are over-engineering it.

container_of/offsetof shenanigans were my original suggestion as well.
Nathaniel's argument is that using such tricks is less than pleasent in a
Rust environment.

https://lkml.kernel.org/r/CAOASepOFh-vOrNZEVDFrDSuHs+9GEzzpXUTG-fZMuyjWAkpRWw@mail.gmail.com

> > Moving the params into a struct will also make it less painful to use
> > dedicated exit reasons, and to support exiting on interrupts in future
> > patches.
> > 
> My apology for not following discussion lately. What did you mean by
> "dedicated exit reasons" and why was it "painful"? According to another
> thread, I guess we wouldn't have "exiting on interrupts".

Currently the vDSO returns -EFAULT if the enclave encounters an exception,
which is kludgy for two reasons:

 1. EFAULT traditionally means "bad address", whereas an exception could be
    a #UD in the enclave.

 2. Returning -EFAULT provides weird semantics as it implies the vDSO call
    failed, which is not the case.  The vDSO itself was successful, i.e. it
    did the requested EENTER/ERESUME operation, and should really return '0'.

The proposed solution for #1 is to define arbitrary return values to
differentiate between synchronous (EEXIT) exits and asynchronous exits due
to exceptions.  But that doesn't address #2, and gets especially awkward when
dealing with the call back return, which is also overloaded to use positive
return values for ENCLU leafs.

Passing a "run" struct makes it trivially easy to different the return value
of the vDSO function from the enclave exit reason, and to avoid collisions
with the return value from the userspace callback.

Re: [RFC PATCH 2/4] x86/vdso: x86/sgx: Rework __vdso_sgx_enter_enclave() API

[Sean Christopherson]

On Tue, Aug 25, 2020 at 09:41:19AM +0200, Jethro Beekman wrote:
> On 2020-08-25 09:38, Sean Christopherson wrote:
> > On Tue, Aug 25, 2020 at 09:36:15AM +0200, Jethro Beekman wrote:
> >> On 2020-08-25 01:54, Sean Christopherson wrote:
> >>> On Mon, Aug 24, 2020 at 03:36:11PM +0200, Jethro Beekman wrote:
> >>>> On 2020-08-18 06:24, Sean Christopherson wrote:
> >>>>>  /**
> >>>>>   * __vdso_sgx_enter_enclave() - Enter an SGX enclave
> >>>>> @@ -119,16 +153,14 @@ typedef int (*sgx_enclave_exit_handler_t)(long rdi, long rsi, long rdx,
> >>>>>   * @leaf:	ENCLU leaf, must be EENTER or ERESUME
> >>>>>   * @r8:		Pass-through value for R8
> >>>>>   * @r9:		Pass-through value for R9
> >>>>> - * @tcs:	TCS, must be non-NULL
> >>>>> - * @e:		Optional struct sgx_enclave_exception instance
> >>>>> - * @handler:	Optional enclave exit handler
> >>>>> + * @r:		struct sgx_enclave_run, must be non-NULL
> >>>>>   *
> >>>>>   * NOTE: __vdso_sgx_enter_enclave() does not ensure full compliance with the
> >>>>> - * x86-64 ABI, e.g. doesn't explicitly clear EFLAGS.DF after EEXIT.  Except for
> >>>>> - * non-volatile general purpose registers, preserving/setting state in
> >>>>> - * accordance with the x86-64 ABI is the responsibility of the enclave and its
> >>>>> - * runtime, i.e. __vdso_sgx_enter_enclave() cannot be called from C code
> >>>>> - * without careful consideration by both the enclave and its runtime.
> >>>>> + * x86-64 ABI, e.g. doesn't handle XSAVE state.  Except for non-volatile
> >>>>> + * general purpose registers, EFLAGS.DF, and RSP alignment, preserving/setting
> >>>>> + * state in accordance with the x86-64 ABI is the responsibility of the enclave
> >>>>> + * and its runtime, i.e. __vdso_sgx_enter_enclave() cannot be called from C
> >>>>> + * code without careful consideration by both the enclave and its runtime.
> >>>>>   *
> >>>>>   * All general purpose registers except RAX, RBX and RCX are passed as-is to
> >>>>>   * the enclave.  RAX, RBX and RCX are consumed by EENTER and ERESUME and are
> >>>>> @@ -160,16 +192,12 @@ typedef int (*sgx_enclave_exit_handler_t)(long rdi, long rsi, long rdx,
> >>>>>   * without returning to __vdso_sgx_enter_enclave().
> >>>>>   *
> >>>>>   * Return:
> >>>>> - *  0 on success,
> >>>>> + *  0 on success (ENCLU reached),
> >>>>>   *  -EINVAL if ENCLU leaf is not allowed,
> >>>>> - *  -EFAULT if an exception occurs on ENCLU or within the enclave
> >>>>> - *  -errno for all other negative values returned by the userspace exit handler
> >>>>>   */
> >>>>>  typedef int (*vdso_sgx_enter_enclave_t)(unsigned long rdi, unsigned long rsi,
> >>>>>  					unsigned long rdx, unsigned int leaf,
> >>>>>  					unsigned long r8,  unsigned long r9,
> >>>>> -					void *tcs,
> >>>>> -					struct sgx_enclave_exception *e,
> >>>>> -					sgx_enclave_exit_handler_t handler);
> >>>>> +					struct sgx_enclave_run *r);
> >>>>>  
> >>>>>  #endif /* _UAPI_ASM_X86_SGX_H */
> >>>>>
> >>>>
> >>>> I think this should return void now, not int? Then, the “return” section of
> >>>> the documentation is also no longer correct.
> >>>
> >>> No, it returns -EINVAL if the leaf is bogus (and if unsupported flags are
> >>> specified, if a @flags param is ever added).
> >>>
> >>
> >> Ok, but if I read the code correctly, contrary to the docs, -EFAULT is not
> >> returned but passed in struct sgx_enclave_run:
> > 
> > The -EFAULT and -errno lines are removed from the "Return:" section of the
> > above documentation.  Did I miss one?
> > 
> 
> No, I'm just bad at reading patches without syntax highlighting. This is fine
> as is. Sorry for the confusion.

No worries.  Removing the -errno line is actually wrong, the vDSO still returns
-errno if the userspace callback returns a negative value.

Re: [RFC PATCH 2/4] x86/vdso: x86/sgx: Rework __vdso_sgx_enter_enclave() API

[Sean Christopherson]

Andy,

Any thoughts on using a struct versus a plethora of params?  If you don't
have a strong opinion, I'd like to push for the struct option as it fixes
the -EFAULT weirdness, satisfies Nathaniel's request, and gives some
flexibility for the future.  The code impact, both to the vDSO and to the
caller, is largely a lateral move, i.e. it's different but in the end
doesn't require any more or less work.

On Mon, Aug 17, 2020 at 09:24:03PM -0700, Sean Christopherson wrote:
> Rework __vdso_sgx_enter_enclave() to use a struct to hold the input and
> output params.  In the new struct, add an opaque "user_data" that can be
> used to pass context across the vDSO, and an explicit "exit_reason" to
> avoid overloading the return value.
> 
> Moving the params into a struct will also make it less painful to use
> dedicated exit reasons, and to support exiting on interrupts in future
> patches.
> 
> Cc: Nathaniel McCallum <npmccallum@redhat.com>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/entry/vdso/vsgx_enter_enclave.S | 72 ++++++++++++-------
>  arch/x86/include/uapi/asm/sgx.h          | 90 ++++++++++++++++--------
>  2 files changed, 107 insertions(+), 55 deletions(-)
> 
> diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.S b/arch/x86/entry/vdso/vsgx_enter_enclave.S
> index 2d88acd408d4e..aaae6d6e28ac3 100644
> --- a/arch/x86/entry/vdso/vsgx_enter_enclave.S
> +++ b/arch/x86/entry/vdso/vsgx_enter_enclave.S
> @@ -7,9 +7,21 @@
>  
>  #include "extable.h"
>  
> -#define EX_LEAF		0*8
> -#define EX_TRAPNR	0*8+4
> -#define EX_ERROR_CODE	0*8+6
> +/* Offset of 'struct sgx_enter_enclave' relative to %rbp. */
> +#define RUN_OFFSET	2*8
> +
> +/* Offsets into 'struct sgx_enter_enclave'. */
> +#define TCS_OFFEST		0*8
> +#define FLAGS_OFFSET		1*8
> +#define EXIT_LEAF_OFFSET	2*8
> +#define EXIT_REASON_OFFSET	2*8 + 4
> +#define USER_HANDLER_OFFSET	3*8
> +/* #define USER_DATA_OFFSET	4*8 */
> +#define EXCEPTION_OFFSET	5*8
> +
> +/* Offsets into sgx_enter_enclave.exception. */
> +#define EX_TRAPNR	0*8
> +#define EX_ERROR_CODE	0*8+2
>  #define EX_ADDRESS	1*8
>  
>  .code64
> @@ -30,12 +42,18 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave)
>  .Lenter_enclave:
>  	/* EENTER <= leaf <= ERESUME */
>  	cmp	$EENTER, %eax
> -	jb	.Linvalid_leaf
> +	jb	.Linvalid_input
>  	cmp	$ERESUME, %eax
> -	ja	.Linvalid_leaf
> +	ja	.Linvalid_input
> +
> +	mov	RUN_OFFSET(%rbp), %rcx
> +
> +	/* No flags are currently defined/supported. */
> +	cmpq	$0, FLAGS_OFFSET(%rcx)
> +	jne	.Linvalid_input
>  
>  	/* Load TCS and AEP */
> -	mov	0x10(%rbp), %rbx
> +	mov	TCS_OFFEST(%rcx), %rbx
>  	lea	.Lasync_exit_pointer(%rip), %rcx
>  
>  	/* Single ENCLU serving as both EENTER and AEP (ERESUME) */
> @@ -44,13 +62,21 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave)
>  	enclu
>  
>  	/* EEXIT jumps here unless the enclave is doing something fancy. */
> -	xor	%eax, %eax
> +	mov	RUN_OFFSET(%rbp), %rbx
> +
> +	/* Set exit_reason. */
> +	movl	$0, EXIT_REASON_OFFSET(%rbx)
>  
>  	/* Invoke userspace's exit handler if one was provided. */
>  .Lhandle_exit:
> -	cmpq	$0, 0x20(%rbp)
> +	mov	%eax, EXIT_LEAF_OFFSET(%rbx)
> +
> +	cmpq	$0, USER_HANDLER_OFFSET(%rbx)
>  	jne	.Linvoke_userspace_handler
>  
> +	/* Success, in the sense that ENCLU was attempted. */
> +	xor	%eax, %eax
> +
>  .Lout:
>  	pop	%rbx
>  	leave
> @@ -60,28 +86,28 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave)
>  	/* The out-of-line code runs with the pre-leave stack frame. */
>  	.cfi_def_cfa		%rbp, 16
>  
> -.Linvalid_leaf:
> +.Linvalid_input:
>  	mov	$(-EINVAL), %eax
>  	jmp	.Lout
>  
>  .Lhandle_exception:
> -	mov	0x18(%rbp), %rcx
> -	test    %rcx, %rcx
> -	je	.Lskip_exception_info
> +	mov	RUN_OFFSET(%rbp), %rbx
>  
> -	/* Fill optional exception info. */
> -	mov	%eax, EX_LEAF(%rcx)
> -	mov	%di,  EX_TRAPNR(%rcx)
> -	mov	%si,  EX_ERROR_CODE(%rcx)
> -	mov	%rdx, EX_ADDRESS(%rcx)
> -.Lskip_exception_info:
> -	mov	$(-EFAULT), %eax
> +	/* Set the exit_reason and exception info. */
> +	movl	$(-EFAULT), EXIT_REASON_OFFSET(%rbx)
> +
> +	mov	%di,  (EXCEPTION_OFFSET + EX_TRAPNR)(%rbx)
> +	mov	%si,  (EXCEPTION_OFFSET + EX_ERROR_CODE)(%rbx)
> +	mov	%rdx, (EXCEPTION_OFFSET + EX_ADDRESS)(%rbx)
>  	jmp	.Lhandle_exit
>  
>  .Linvoke_userspace_handler:
>  	/* Pass the untrusted RSP (at exit) to the callback via %rcx. */
>  	mov	%rsp, %rcx
>  
> +	/* Save @e, %rbx is about to be clobbered. */
> +	mov	%rbx, %rax
> +
>  	/* Save the untrusted RSP offset in %rbx (non-volatile register). */
>  	mov	%rsp, %rbx
>  	and	$0xf, %rbx
> @@ -93,20 +119,18 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave)
>  	and	$-0x10, %rsp
>  	push	%rax
>  
> -	/* Push @e, the "return" value and @tcs as params to the callback. */
> -	push	0x18(%rbp)
> +	/* Push @e as a param to the callback. */
>  	push	%rax
> -	push	0x10(%rbp)
>  
>  	/* Clear RFLAGS.DF per x86_64 ABI */
>  	cld
>  
>  	/* Load the callback pointer to %rax and invoke it via retpoline. */
> -	mov	0x20(%rbp), %rax
> +	mov	USER_HANDLER_OFFSET(%rax), %rax
>  	call	.Lretpoline
>  
>  	/* Undo the post-exit %rsp adjustment. */
> -	lea	0x20(%rsp, %rbx), %rsp
> +	lea	0x10(%rsp, %rbx), %rsp
>  
>  	/*
>  	 * If the return from callback is zero or negative, return immediately,
> diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
> index 3760e5d5dc0c7..d3b107aac279d 100644
> --- a/arch/x86/include/uapi/asm/sgx.h
> +++ b/arch/x86/include/uapi/asm/sgx.h
> @@ -74,6 +74,28 @@ struct sgx_enclave_set_attribute {
>  	__u64 attribute_fd;
>  };
>  
> +struct sgx_enclave_run;
> +
> +/**
> + * typedef sgx_enclave_exit_handler_t - Exit handler function accepted by
> + *					__vdso_sgx_enter_enclave()
> + *
> + * @rdi:	RDI at the time of enclave exit
> + * @rsi:	RSI at the time of enclave exit
> + * @rdx:	RDX at the time of enclave exit
> + * @ursp:	RSP at the time of enclave exit (untrusted stack)
> + * @r8:		R8 at the time of enclave exit
> + * @r9:		R9 at the time of enclave exit
> + * @r:		Pointer to struct sgx_enclave_run (as provided by caller)
> + *
> + * Return:
> + *  0 or negative to exit vDSO
> + *  positive to re-enter enclave (must be EENTER or ERESUME leaf)
> + */
> +typedef int (*sgx_enclave_exit_handler_t)(long rdi, long rsi, long rdx,
> +					  long ursp, long r8, long r9,
> +					  struct sgx_enclave_run *r);
> +
>  /**
>   * struct sgx_enclave_exception - structure to report exceptions encountered in
>   *				  __vdso_sgx_enter_enclave()
> @@ -85,31 +107,43 @@ struct sgx_enclave_set_attribute {
>   * @reserved:	reserved for future use
>   */
>  struct sgx_enclave_exception {
> -	__u32 leaf;
>  	__u16 trapnr;
>  	__u16 error_code;
> +	__u32 reserved;
>  	__u64 address;
> -	__u64 reserved[2];
>  };
>  
>  /**
> - * typedef sgx_enclave_exit_handler_t - Exit handler function accepted by
> - *					__vdso_sgx_enter_enclave()
> + * struct sgx_enclave_run - Control structure for __vdso_sgx_enter_enclave()
>   *
> - * @rdi:	RDI at the time of enclave exit
> - * @rsi:	RSI at the time of enclave exit
> - * @rdx:	RDX at the time of enclave exit
> - * @ursp:	RSP at the time of enclave exit (untrusted stack)
> - * @r8:		R8 at the time of enclave exit
> - * @r9:		R9 at the time of enclave exit
> - * @tcs:	Thread Control Structure used to enter enclave
> - * @ret:	0 on success (EEXIT), -EFAULT on an exception
> - * @e:		Pointer to struct sgx_enclave_exception (as provided by caller)
> + * @tcs:		Thread Control Structure used to enter enclave
> + * @flags:		Control flags
> + * @exit_leaf:		ENCLU leaf from \%eax at time of exit
> + * @exit_reason:	Cause of exit from enclave, e.g. EEXIT vs. exception
> + * @user_handler:	User provided exit handler (optional)
> + * @user_data:		User provided opaque value (optional)
> + * @exception:		Valid on exit due to exception
>   */
> -typedef int (*sgx_enclave_exit_handler_t)(long rdi, long rsi, long rdx,
> -					  long ursp, long r8, long r9,
> -					  void *tcs, int ret,
> -					  struct sgx_enclave_exception *e);
> +struct sgx_enclave_run {
> +	__u64 tcs;
> +	__u64 flags;
> +
> +	__u32 exit_leaf;
> +	__u32 exit_reason;
> +
> +	union {
> +		sgx_enclave_exit_handler_t user_handler;
> +		__u64 __user_handler;
> +	};
> +	__u64 user_data;
> +
> +	union {
> +		struct sgx_enclave_exception exception;
> +
> +		/* Pad the entire struct to 256 bytes. */
> +		__u8 pad[256 - 40];
> +	};
> +};
>  
>  /**
>   * __vdso_sgx_enter_enclave() - Enter an SGX enclave
> @@ -119,16 +153,14 @@ typedef int (*sgx_enclave_exit_handler_t)(long rdi, long rsi, long rdx,
>   * @leaf:	ENCLU leaf, must be EENTER or ERESUME
>   * @r8:		Pass-through value for R8
>   * @r9:		Pass-through value for R9
> - * @tcs:	TCS, must be non-NULL
> - * @e:		Optional struct sgx_enclave_exception instance
> - * @handler:	Optional enclave exit handler
> + * @r:		struct sgx_enclave_run, must be non-NULL
>   *
>   * NOTE: __vdso_sgx_enter_enclave() does not ensure full compliance with the
> - * x86-64 ABI, e.g. doesn't explicitly clear EFLAGS.DF after EEXIT.  Except for
> - * non-volatile general purpose registers, preserving/setting state in
> - * accordance with the x86-64 ABI is the responsibility of the enclave and its
> - * runtime, i.e. __vdso_sgx_enter_enclave() cannot be called from C code
> - * without careful consideration by both the enclave and its runtime.
> + * x86-64 ABI, e.g. doesn't handle XSAVE state.  Except for non-volatile
> + * general purpose registers, EFLAGS.DF, and RSP alignment, preserving/setting
> + * state in accordance with the x86-64 ABI is the responsibility of the enclave
> + * and its runtime, i.e. __vdso_sgx_enter_enclave() cannot be called from C
> + * code without careful consideration by both the enclave and its runtime.
>   *
>   * All general purpose registers except RAX, RBX and RCX are passed as-is to
>   * the enclave.  RAX, RBX and RCX are consumed by EENTER and ERESUME and are
> @@ -160,16 +192,12 @@ typedef int (*sgx_enclave_exit_handler_t)(long rdi, long rsi, long rdx,
>   * without returning to __vdso_sgx_enter_enclave().
>   *
>   * Return:
> - *  0 on success,
> + *  0 on success (ENCLU reached),
>   *  -EINVAL if ENCLU leaf is not allowed,
> - *  -EFAULT if an exception occurs on ENCLU or within the enclave
> - *  -errno for all other negative values returned by the userspace exit handler
>   */
>  typedef int (*vdso_sgx_enter_enclave_t)(unsigned long rdi, unsigned long rsi,
>  					unsigned long rdx, unsigned int leaf,
>  					unsigned long r8,  unsigned long r9,
> -					void *tcs,
> -					struct sgx_enclave_exception *e,
> -					sgx_enclave_exit_handler_t handler);
> +					struct sgx_enclave_run *r);
>  
>  #endif /* _UAPI_ASM_X86_SGX_H */
> -- 
> 2.28.0
> 

Re: [RFC PATCH 2/4] x86/vdso: x86/sgx: Rework __vdso_sgx_enter_enclave() API

[Andy Lutomirski]

> On Aug 26, 2020, at 1:20 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> 
> Andy,
> 
> Any thoughts on using a struct versus a plethora of params?  If you don't
> have a strong opinion, I'd like to push for the struct option as it fixes
> the -EFAULT weirdness, satisfies Nathaniel's request, and gives some
> flexibility for the future.  The code impact, both to the vDSO and to the
> caller, is largely a lateral move, i.e. it's different but in the end
> doesn't require any more or less work.

I have no preference.

Re: [RFC PATCH 2/4] x86/vdso: x86/sgx: Rework __vdso_sgx_enter_enclave() API

[Xing, Cedric]

On 8/26/2020 1:15 PM, Sean Christopherson wrote:
> On Wed, Aug 26, 2020 at 12:27:54PM -0700, Xing, Cedric wrote:
>> On 8/17/2020 9:24 PM, Sean Christopherson wrote:
>>> Rework __vdso_sgx_enter_enclave() to use a struct to hold the input and
>>> output params.  In the new struct, add an opaque "user_data" that can be
>>> used to pass context across the vDSO, and an explicit "exit_reason" to
>>> avoid overloading the return value.
>>> In order to pass additional parameters to the exit handler, the exinfo
>> structure could be embedded in a user-defined structure while the handler
>> could pick it up using "container_of" macro. IMO the original interface was
>> neat and suffcient, and we are over-engineering it.
> 
> container_of/offsetof shenanigans were my original suggestion as well.
> Nathaniel's argument is that using such tricks is less than pleasent in a
> Rust environment.
> 
> https://lkml.kernel.org/r/CAOASepOFh-vOrNZEVDFrDSuHs+9GEzzpXUTG-fZMuyjWAkpRWw@mail.gmail.com
> 
I don't know much about rust but there seem to be offsetof/container_of 
solutions in rust. Here's one: 
https://gist.github.com/resilar/baefbfbf0d733c69d70970d829938875.

After countless discussions along this upstream journey, I thought we 
had agreed that simplicity was the priority. And for simplicity we were 
even willing to give up compliance to x86_64 ABI. Then how do you 
justify treating rust better than other programming languages? This 
looks a hard sell to me.

>>> Moving the params into a struct will also make it less painful to use
>>> dedicated exit reasons, and to support exiting on interrupts in future
>>> patches.
>>>
>> My apology for not following discussion lately. What did you mean by
>> "dedicated exit reasons" and why was it "painful"? According to another
>> thread, I guess we wouldn't have "exiting on interrupts".
> 
> Currently the vDSO returns -EFAULT if the enclave encounters an exception,
> which is kludgy for two reasons:
> 
>   1. EFAULT traditionally means "bad address", whereas an exception could be
>      a #UD in the enclave.
> 
>   2. Returning -EFAULT provides weird semantics as it implies the vDSO call
>      failed, which is not the case.  The vDSO itself was successful, i.e. it
>      did the requested EENTER/ERESUME operation, and should really return '0'.
> 
EFAULT is descriptive enough for me. And more importantly 
detailed/specific info is always available in exinfo.

IMO, the purpose of return code is for the caller/handler to branch on. 
If 2 cases share the same execution path, then there's no need to 
distinguish them. In the case of SGX, most (if not all) exceptions are 
handled in the same way - nested EENTER, then what's point of 
distinguishing them? Please note that detailed info is always available 
in exinfo to cover corner cases.

Moreover, even the vDSO API itself cannot tell between faults at 
EENTER/ERESUME (the vDSO itself failes) vs. faults inside the enclave 
(vDSO succeeded but the enclave fails). I don't think you can be 
accurate without complicating the code significantly.

> The proposed solution for #1 is to define arbitrary return values to
> differentiate between synchronous (EEXIT) exits and asynchronous exits due
> to exceptions.  But that doesn't address #2, and gets especially awkward when
> dealing with the call back return, which is also overloaded to use positive
> return values for ENCLU leafs.
> 
> Passing a "run" struct makes it trivially easy to different the return value
> of the vDSO function from the enclave exit reason, and to avoid collisions
> with the return value from the userspace callback.
> 
When a callback is configured, the vDSO NEVER returns to caller directly 
- i.e. all return codes must be from the callback but NOT the vDSO. I 
don't see any collisions here.

Re: [RFC PATCH 4/4] x86/vdso: x86/sgx: Allow the user to exit the vDSO loop on interrupts

[Jethro Beekman]

[-- Attachment #1: Type: text/plain, Size: 4826 bytes --]

On 2020-08-26 20:32, Sean Christopherson wrote:
> On Mon, Aug 24, 2020 at 03:36:29PM +0200, Jethro Beekman wrote:
>> On 2020-08-22 23:55, Andy Lutomirski wrote:
>>> On Thu, Aug 20, 2020 at 10:53 AM Jethro Beekman <jethro@fortanix.com> wrote:
>>>>
>>>> On 2020-08-20 19:44, Andy Lutomirski wrote:
>>>>> This is a genuine semantic question: is __vdso_sgx_enter_enclave()
>>>>> like read on a pipe (returns -EINTR on a signal) or is it more like a
>>>>> restartable syscall or a normal library function that just keeps
>>>>> running if your signal handler does nothing?  You could siglongjmp()
>>>>> out, but that's a bit gross.
>>>>>
>>>>> I wouldn't object to an option to __vdso_sgx_enter_enclave() to make
>>>>> it return -EINTR if signaled by a non-SA_RESTART signal.  Implementing
>>>>> it might be distinctly nontrivial, though.
>>>>>
>>>>> But this isn't what this patch does, and I suspect we've been talking
>>>>> past each other.  This patch makes __vdso_sgx_enter_enclave() return
>>>>> if there's an *interrupt*.  If you push a keyboard button, move your
>>>>> mouse, get a network interrupt on that core, etc, it will return.
>>>>> This is nonsense.
>>>>
>>>> It's not nontrivial to return on signals, this patch does it. This patch *also* returns when there's a HW interrupt, but that's not important.
>>>
>>> Allow me to quote the changelog of the patch:
>>>
>>> This allows the user's runtime to switch
>>> contexts at opportune times without additional overhead, e.g. when using
>>> an M:N threading model (where M user threads run N TCSs, with N > M).
>>>
>>> NAK.
>>>
>>>>
>>>> It sounds like you're saying you want to subdivide AEXs into “interrupts
>>>> that lead to user-observable signals” and “other interrupts”, and then
>>>> hide the second category from the user. I wouldn't object to that, but I
>>>> don't know how to code this. It seems like a lot of work compared to the
>>>> obvious solution (this patch).
>>>
>>> The obvious solution is NAKked.
>>>
>>> Does siglongjmp() really not work for you?
>>
>> Allow me to quote the changelog of "[PATCH v36 18/24] x86/vdso: Add support
>> for exception fixup in vDSO functions":
>>
>> Putting everything together, userspace enclaves will utilize a library
>> that must be prepared to handle any and (almost) all exceptions any time
>> at least one thread may be executing in an enclave.  Leveraging signals
>> to handle the enclave exceptions is unpleasant, to put it mildly, e.g.
>> the SGX library must constantly (un)register its signal handler based
>> on whether or not at least one thread is executing in an enclave, and
>> filter and forward exceptions that aren't related to its enclaves.  This
>> becomes particularly nasty when using multiple levels of libraries that
>> register signal handlers, e.g. running an enclave via cgo inside of the
>> Go runtime.
>>
>>
>> If I could use signal handlers I wouldn't use the VDSO call. (Corollary: if I
>> wasn't using the VDSO call, I wouldn't have to think about this because what
>> I want is already supported).
> 
> Jethro, I think what you're requesting is simply not possible.  Or rather,
> the SGX vDSO can't provide any additional value relative to what can be
> provided by userspace.
> 
> What Andy is saying is that having the vDSO take action on interrupts is
> undesirable/NAK'd from a kernel perspective, as it's ugly and unreliable.
> It happens to mostly work for SIGALRM because there is an interrupt
> associated with the expiration of the timer[*].  But even then it will
> work if and only if the interrupt arrives while the enclave is running.
> If the interrupt arrives just before ENCLU, either on initial entry or
> on resume from the callback, SIGALRM will be delivered and the userspace
> M:N scheduler that is relying on an exit from the enclave will miss its
> "tick".
> 
> The vDSO could check for pending signals, but that effectively provides no
> value as opposed to checking for signals in the callback (or caller).
> 
> The goal of the SGX vDSO is to obviate the need for handling signals that
> are due to exceptions that are directly associated with the enclave.  IMO,
> the SIGALRM request is completely out of scope as it is not a problem that
> is unique to SGX, i.e. wanting to do M:N scheduling in a library without
> hooking SIGALRM is a generic request.  The fact that the SGX architcture has
> a quirk that allows for a semi-functional hack is not justification for
> supporting such a hack in the kernel.
> 
> [*] Is it even guaranteed that an interrupt will precede SIGALRM?  Not that
>     it matters for this discussion.
> 

Let's make sure the VDSO call has a flags parameter and we can talk about this after merge.

--
Jethro Beekman | Fortanix


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4054 bytes --]

Re: [RFC PATCH 2/4] x86/vdso: x86/sgx: Rework __vdso_sgx_enter_enclave() API

[Jethro Beekman]

[-- Attachment #1: Type: text/plain, Size: 1260 bytes --]

On 2020-08-26 22:15, Sean Christopherson wrote:
> On Wed, Aug 26, 2020 at 12:27:54PM -0700, Xing, Cedric wrote:
>> On 8/17/2020 9:24 PM, Sean Christopherson wrote:
>>> Rework __vdso_sgx_enter_enclave() to use a struct to hold the input and
>>> output params.  In the new struct, add an opaque "user_data" that can be
>>> used to pass context across the vDSO, and an explicit "exit_reason" to
>>> avoid overloading the return value.
>>> In order to pass additional parameters to the exit handler, the exinfo
>> structure could be embedded in a user-defined structure while the handler
>> could pick it up using "container_of" macro. IMO the original interface was
>> neat and suffcient, and we are over-engineering it.
> 
> container_of/offsetof shenanigans were my original suggestion as well.
> Nathaniel's argument is that using such tricks is less than pleasent in a
> Rust environment.
> 
> https://lkml.kernel.org/r/CAOASepOFh-vOrNZEVDFrDSuHs+9GEzzpXUTG-fZMuyjWAkpRWw@mail.gmail.com

Just for the record I'm a Rust user and I have nothing against a solution that requires computing field offsets. As others have mentioned, assembly doesn't have facilities for this but it hasn't been a problem there.

--
Jethro Beekman | Fortanix


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4054 bytes --]

Re: [RFC PATCH 2/4] x86/vdso: x86/sgx: Rework __vdso_sgx_enter_enclave() API

[Jarkko Sakkinen]

On Wed, Aug 26, 2020 at 01:20:29PM -0700, Sean Christopherson wrote:
> Andy,
> 
> Any thoughts on using a struct versus a plethora of params?  If you don't
> have a strong opinion, I'd like to push for the struct option as it fixes
> the -EFAULT weirdness, satisfies Nathaniel's request, and gives some
> flexibility for the future.  The code impact, both to the vDSO and to the
> caller, is largely a lateral move, i.e. it's different but in the end
> doesn't require any more or less work.

If we ended up ever considering eBPF idea that I throwed a while ago in
the air, most likely the same struct could be use as the context for the
BPF program and larger portion of the existing vDSO code would remain
intact.

I'm not trying gonzo market BPF.

I'm just pointing out that a struct is a great vessel to travel between
kernel and user space (e.g. a BPF program would be executed by the
kernel by using the vDSO provided context struct).

/Jarkko

Re: [RFC PATCH 2/4] x86/vdso: x86/sgx: Rework __vdso_sgx_enter_enclave() API

[Sean Christopherson]

On Wed, Aug 26, 2020 at 04:26:11PM -0700, Xing, Cedric wrote:
> After countless discussions along this upstream journey, I thought we had
> agreed that simplicity was the priority. And for simplicity we were even
> willing to give up compliance to x86_64 ABI. Then how do you justify
> treating rust better than other programming languages? This looks a hard
> sell to me.

If you view using a struct instead of a multiple params as a separate cost,
adding a pass-through param to throw a bone to Rust adds zero complexity.
The flags field has nothing to do with Rust whatsoever.

As for the struct itself, I suppose you could argue it's gratuitous, but
I don't buy that it adds complexity.  The impact on the vDSO is something
like four extra instructions, two of which are a CMP+Jcc to enforce the
flags, and the others are the additional moves need to chase pointers through
the struct.

Unless someone really dislikes structs, there is no added complexity to the
caller, it's just slightly different code, e.g. the selftest update for the
struct variant actually removed code.

> > > > Moving the params into a struct will also make it less painful to use
> > > > dedicated exit reasons, and to support exiting on interrupts in future
> > > > patches.
> > > > 
> > > My apology for not following discussion lately. What did you mean by
> > > "dedicated exit reasons" and why was it "painful"? According to another
> > > thread, I guess we wouldn't have "exiting on interrupts".
> > 
> > Currently the vDSO returns -EFAULT if the enclave encounters an exception,
> > which is kludgy for two reasons:
> > 
> >   1. EFAULT traditionally means "bad address", whereas an exception could be
> >      a #UD in the enclave.
> > 
> >   2. Returning -EFAULT provides weird semantics as it implies the vDSO call
> >      failed, which is not the case.  The vDSO itself was successful, i.e. it
> >      did the requested EENTER/ERESUME operation, and should really return '0'.
> > 
> EFAULT is descriptive enough for me. And more importantly detailed/specific
> info is always available in exinfo.
>
> IMO, the purpose of return code is for the caller/handler to branch on. If 2
> cases share the same execution path, then there's no need to distinguish
> them. In the case of SGX, most (if not all) exceptions are handled in the
> same way - nested EENTER, then what's point of distinguishing them? Please
> note that detailed info is always available in exinfo to cover corner cases.

The primary motiviation is to adhere to the general rules of the kernel,
specifically that -EFAULT isn't intended as a catchall for exceptions. 

> Moreover, even the vDSO API itself cannot tell between faults at
> EENTER/ERESUME (the vDSO itself failes) vs. faults inside the enclave (vDSO
> succeeded but the enclave fails). I don't think you can be accurate without
> complicating the code significantly.

And I didn't even try.  The proposed documentation explicitly states that
"success" is defined as attempting ENCLU.

> > The proposed solution for #1 is to define arbitrary return values to
> > differentiate between synchronous (EEXIT) exits and asynchronous exits due
> > to exceptions.  But that doesn't address #2, and gets especially awkward when
> > dealing with the call back return, which is also overloaded to use positive
> > return values for ENCLU leafs.
> > 
> > Passing a "run" struct makes it trivially easy to different the return value
> > of the vDSO function from the enclave exit reason, and to avoid collisions
> > with the return value from the userspace callback.
> > 
> When a callback is configured, the vDSO NEVER returns to caller directly -
> i.e. all return codes must be from the callback but NOT the vDSO. I don't
> see any collisions here.

Unless the user provides a bad leaf, in which case -EINVAL is returned.

But that wasn't what I was talking about.  If we want to use arbitrary values
instead of hijacking -EFAULT (and possibly -EINTR), then we run into problems
with choosing an arbitrary value that won't collide with the value returned
from the callback to the _vDSO_.  The callback essentially gets dibs on all
negative values and 0 is off limits.  That leaves positive values, but those
are interpreted as the new ENCLU leaf by the vDSO.  That means the vDSO can
"return" a value to the callback that the callback can't safely reflect back
to the vDSO.  Using an exit reason side steps that by virtue of it not being
the return value in either direction.

Re: [RFC PATCH 2/4] x86/vdso: x86/sgx: Rework __vdso_sgx_enter_enclave() API

[Sean Christopherson]

On Mon, Aug 24, 2020 at 10:49:41PM +0300, Jarkko Sakkinen wrote:
> On Mon, Aug 24, 2020 at 03:36:11PM +0200, Jethro Beekman wrote:
> > On 2020-08-18 06:24, Sean Christopherson wrote:
> > >  /**
> > >   * __vdso_sgx_enter_enclave() - Enter an SGX enclave
> > > @@ -119,16 +153,14 @@ typedef int (*sgx_enclave_exit_handler_t)(long rdi, long rsi, long rdx,
> > >   * @leaf:	ENCLU leaf, must be EENTER or ERESUME
> > >   * @r8:		Pass-through value for R8
> > >   * @r9:		Pass-through value for R9
> > > - * @tcs:	TCS, must be non-NULL
> > > - * @e:		Optional struct sgx_enclave_exception instance
> > > - * @handler:	Optional enclave exit handler
> > > + * @r:		struct sgx_enclave_run, must be non-NULL
> > >   *
> > >   * NOTE: __vdso_sgx_enter_enclave() does not ensure full compliance with the
> > > - * x86-64 ABI, e.g. doesn't explicitly clear EFLAGS.DF after EEXIT.  Except for
> > > - * non-volatile general purpose registers, preserving/setting state in
> > > - * accordance with the x86-64 ABI is the responsibility of the enclave and its
> > > - * runtime, i.e. __vdso_sgx_enter_enclave() cannot be called from C code
> > > - * without careful consideration by both the enclave and its runtime.
> > > + * x86-64 ABI, e.g. doesn't handle XSAVE state.  Except for non-volatile
> > > + * general purpose registers, EFLAGS.DF, and RSP alignment, preserving/setting
> > > + * state in accordance with the x86-64 ABI is the responsibility of the enclave
> > > + * and its runtime, i.e. __vdso_sgx_enter_enclave() cannot be called from C
> > > + * code without careful consideration by both the enclave and its runtime.
> > >   *
> > >   * All general purpose registers except RAX, RBX and RCX are passed as-is to
> > >   * the enclave.  RAX, RBX and RCX are consumed by EENTER and ERESUME and are
> > > @@ -160,16 +192,12 @@ typedef int (*sgx_enclave_exit_handler_t)(long rdi, long rsi, long rdx,
> > >   * without returning to __vdso_sgx_enter_enclave().
> > >   *
> > >   * Return:
> > > - *  0 on success,
> > > + *  0 on success (ENCLU reached),
> > >   *  -EINVAL if ENCLU leaf is not allowed,
> > > - *  -EFAULT if an exception occurs on ENCLU or within the enclave
> > > - *  -errno for all other negative values returned by the userspace exit handler
> > >   */
> > >  typedef int (*vdso_sgx_enter_enclave_t)(unsigned long rdi, unsigned long rsi,
> > >  					unsigned long rdx, unsigned int leaf,
> > >  					unsigned long r8,  unsigned long r9,
> > > -					void *tcs,
> > > -					struct sgx_enclave_exception *e,
> > > -					sgx_enclave_exit_handler_t handler);
> > > +					struct sgx_enclave_run *r);
> > >  
> > >  #endif /* _UAPI_ASM_X86_SGX_H */
> > > 
> > 
> > I think this should return void now, not int? Then, the “return”
> > section of the documentation is also no longer correct.
> 
> This documentation should be moved to Documentation/x86/sgx.rst.
> 
> It is easier to read from there and then it will be included by kdoc
> to the kernel documentation. In here it is not addressed by kdoc and
> it is unnecessarily hard to read.

I'm confused, this is all picked up by kernel-doc.  What am I missing?

Re: [RFC PATCH 2/4] x86/vdso: x86/sgx: Rework __vdso_sgx_enter_enclave() API

[Jarkko Sakkinen]

On Fri, Sep 04, 2020 at 03:25:07AM -0700, Sean Christopherson wrote:
> On Mon, Aug 24, 2020 at 10:49:41PM +0300, Jarkko Sakkinen wrote:
> > On Mon, Aug 24, 2020 at 03:36:11PM +0200, Jethro Beekman wrote:
> > > On 2020-08-18 06:24, Sean Christopherson wrote:
> > > >  /**
> > > >   * __vdso_sgx_enter_enclave() - Enter an SGX enclave
> > > > @@ -119,16 +153,14 @@ typedef int (*sgx_enclave_exit_handler_t)(long rdi, long rsi, long rdx,
> > > >   * @leaf:	ENCLU leaf, must be EENTER or ERESUME
> > > >   * @r8:		Pass-through value for R8
> > > >   * @r9:		Pass-through value for R9
> > > > - * @tcs:	TCS, must be non-NULL
> > > > - * @e:		Optional struct sgx_enclave_exception instance
> > > > - * @handler:	Optional enclave exit handler
> > > > + * @r:		struct sgx_enclave_run, must be non-NULL
> > > >   *
> > > >   * NOTE: __vdso_sgx_enter_enclave() does not ensure full compliance with the
> > > > - * x86-64 ABI, e.g. doesn't explicitly clear EFLAGS.DF after EEXIT.  Except for
> > > > - * non-volatile general purpose registers, preserving/setting state in
> > > > - * accordance with the x86-64 ABI is the responsibility of the enclave and its
> > > > - * runtime, i.e. __vdso_sgx_enter_enclave() cannot be called from C code
> > > > - * without careful consideration by both the enclave and its runtime.
> > > > + * x86-64 ABI, e.g. doesn't handle XSAVE state.  Except for non-volatile
> > > > + * general purpose registers, EFLAGS.DF, and RSP alignment, preserving/setting
> > > > + * state in accordance with the x86-64 ABI is the responsibility of the enclave
> > > > + * and its runtime, i.e. __vdso_sgx_enter_enclave() cannot be called from C
> > > > + * code without careful consideration by both the enclave and its runtime.
> > > >   *
> > > >   * All general purpose registers except RAX, RBX and RCX are passed as-is to
> > > >   * the enclave.  RAX, RBX and RCX are consumed by EENTER and ERESUME and are
> > > > @@ -160,16 +192,12 @@ typedef int (*sgx_enclave_exit_handler_t)(long rdi, long rsi, long rdx,
> > > >   * without returning to __vdso_sgx_enter_enclave().
> > > >   *
> > > >   * Return:
> > > > - *  0 on success,
> > > > + *  0 on success (ENCLU reached),
> > > >   *  -EINVAL if ENCLU leaf is not allowed,
> > > > - *  -EFAULT if an exception occurs on ENCLU or within the enclave
> > > > - *  -errno for all other negative values returned by the userspace exit handler
> > > >   */
> > > >  typedef int (*vdso_sgx_enter_enclave_t)(unsigned long rdi, unsigned long rsi,
> > > >  					unsigned long rdx, unsigned int leaf,
> > > >  					unsigned long r8,  unsigned long r9,
> > > > -					void *tcs,
> > > > -					struct sgx_enclave_exception *e,
> > > > -					sgx_enclave_exit_handler_t handler);
> > > > +					struct sgx_enclave_run *r);
> > > >  
> > > >  #endif /* _UAPI_ASM_X86_SGX_H */
> > > > 
> > > 
> > > I think this should return void now, not int? Then, the “return”
> > > section of the documentation is also no longer correct.
> > 
> > This documentation should be moved to Documentation/x86/sgx.rst.
> > 
> > It is easier to read from there and then it will be included by kdoc
> > to the kernel documentation. In here it is not addressed by kdoc and
> > it is unnecessarily hard to read.
> 
> I'm confused, this is all picked up by kernel-doc.  What am I missing?

Right of course, it is in the .h file now. Sorry, it is me being blind.

Was it before in the .S file?

/Jarkko

Re: [RFC PATCH 2/4] x86/vdso: x86/sgx: Rework __vdso_sgx_enter_enclave() API

[Sean Christopherson]

On Fri, Sep 04, 2020 at 04:36:31PM +0300, Jarkko Sakkinen wrote:
> On Fri, Sep 04, 2020 at 03:25:07AM -0700, Sean Christopherson wrote:
> > On Mon, Aug 24, 2020 at 10:49:41PM +0300, Jarkko Sakkinen wrote:
> > > This documentation should be moved to Documentation/x86/sgx.rst.
> > > 
> > > It is easier to read from there and then it will be included by kdoc
> > > to the kernel documentation. In here it is not addressed by kdoc and
> > > it is unnecessarily hard to read.
> > 
> > I'm confused, this is all picked up by kernel-doc.  What am I missing?
> 
> Right of course, it is in the .h file now. Sorry, it is me being blind.

No worries.

> Was it before in the .S file?

Yeah, it was originally in the .S file with an #ifdef hack to trick
kernel-doc into parsing the comment.  It got moved to the .h when the vDSO
was changed to be directly callable from C.