[PATCH for_v37 0/6] x86/vdso: x86/sgx: Rework SGX vDSO API

[PATCH for_v37 0/6] 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
a pass-through param requested by Nathaniel[1], fix a long-standing nit
from Andy[2], and add a flags field to allow for future extensions.

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

RFC->V1:
  - Drop the EXIT_ON_INTERRUPT patch. [Andy]
  - Fix the macro names in the assembly code. [Jarkko]
  - Move the leaf back into the exception sub-struct.  The leaf is fully
    redundant with SGX_SYNCHRONOUS_EXIT.
  - Add selftest support. [Jarkko]

Jarkko, I didn't address you comment about moving the vDSO kernel docs
comments to the .rst file because I have an question on that and didn't
want to hold this up.  But Intel's mail servers appear to be on the fritz,
so it might be a moot point...

Sean Christopherson (6):
  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
  selftests/sgx: Update the SGX selftest to match the reworked vDSO API
  selftests/sgx: Sanity check the return value of the vDSO call
  selftests/sgx: Add a smoke test to ensure the user handler is invoked

 arch/x86/entry/vdso/vsgx_enter_enclave.S | 76 +++++++++++++-------
 arch/x86/include/uapi/asm/sgx.h          | 88 ++++++++++++++++--------
 tools/testing/selftests/sgx/call.S       | 10 +--
 tools/testing/selftests/sgx/main.c       | 61 ++++++++++++----
 tools/testing/selftests/sgx/main.h       |  2 +-
 5 files changed, 160 insertions(+), 77 deletions(-)

-- 
2.28.0

[PATCH for_v37 1/6] 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.

Acked-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Acked-by: Jethro Beekman <jethro@fortanix.com>
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

[PATCH for_v37 2/6] 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, an explicit "exit_reason" to avoid
overloading the return value, and a "flags" field to provide a path for
future extensions.

Moving the params into a struct will also make it less painful to use
dedicated exit reasons values in a future patch.

Cc: Nathaniel McCallum <npmccallum@redhat.com>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/entry/vdso/vsgx_enter_enclave.S | 73 +++++++++++++-------
 arch/x86/include/uapi/asm/sgx.h          | 85 +++++++++++++++---------
 2 files changed, 103 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..4fe3d06dd7878 100644
--- a/arch/x86/entry/vdso/vsgx_enter_enclave.S
+++ b/arch/x86/entry/vdso/vsgx_enter_enclave.S
@@ -7,10 +7,22 @@
 
 #include "extable.h"
 
-#define EX_LEAF		0*8
-#define EX_TRAPNR	0*8+4
-#define EX_ERROR_CODE	0*8+6
-#define EX_ADDRESS	1*8
+/* Offset of 'struct sgx_enclave_run' relative to %rbp. */
+#define SGX_ENCLAVE_RUN_PTR	2*8
+
+/* Offsets into 'struct sgx_enclave_run'. */
+#define SGX_ENCLAVE_RUN_TSC		0*8
+#define SGX_ENCLAVE_RUN_FLAGS		1*8
+#define SGX_ENCLAVE_RUN_EXIT_REASON	1*8 + 4
+#define SGX_ENCLAVE_RUN_USER_HANDLER	2*8
+/* #define SGX_ENCLAVE_RUN_USER_DATA	3*8 */
+#define SGX_ENCLAVE_RUN_EXCEPTION	4*8
+
+/* Offsets into sgx_enter_enclave.exception. */
+#define SGX_EX_LEAF			0*8
+#define SGX_EX_TRAPNR			0*8+4
+#define SGX_EX_ERROR_CODE		0*8+6
+#define SGX_EX_ADDRESS			1*8
 
 .code64
 .section .text, "ax"
@@ -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	SGX_ENCLAVE_RUN_PTR(%rbp), %rcx
+
+	/* No flags are currently defined/supported. */
+	cmpl	$0, SGX_ENCLAVE_RUN_FLAGS(%rcx)
+	jne	.Linvalid_input
 
 	/* Load TCS and AEP */
-	mov	0x10(%rbp), %rbx
+	mov	SGX_ENCLAVE_RUN_TSC(%rcx), %rbx
 	lea	.Lasync_exit_pointer(%rip), %rcx
 
 	/* Single ENCLU serving as both EENTER and AEP (ERESUME) */
@@ -44,13 +62,19 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave)
 	enclu
 
 	/* EEXIT jumps here unless the enclave is doing something fancy. */
-	xor	%eax, %eax
+	mov	SGX_ENCLAVE_RUN_PTR(%rbp), %rbx
+
+	/* Set exit_reason. */
+	movl	$0, SGX_ENCLAVE_RUN_EXIT_REASON(%rbx)
 
 	/* Invoke userspace's exit handler if one was provided. */
 .Lhandle_exit:
-	cmpq	$0, 0x20(%rbp)
+	cmpq	$0, SGX_ENCLAVE_RUN_USER_HANDLER(%rbx)
 	jne	.Linvoke_userspace_handler
 
+	/* Success, in the sense that ENCLU was attempted. */
+	xor	%eax, %eax
+
 .Lout:
 	pop	%rbx
 	leave
@@ -60,28 +84,29 @@ 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	SGX_ENCLAVE_RUN_PTR(%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), SGX_ENCLAVE_RUN_EXIT_REASON(%rbx)
+
+	mov	%eax, (SGX_ENCLAVE_RUN_EXCEPTION + SGX_EX_LEAF)(%rbx)
+	mov	%di,  (SGX_ENCLAVE_RUN_EXCEPTION + SGX_EX_TRAPNR)(%rbx)
+	mov	%si,  (SGX_ENCLAVE_RUN_EXCEPTION + SGX_EX_ERROR_CODE)(%rbx)
+	mov	%rdx, (SGX_ENCLAVE_RUN_EXCEPTION + SGX_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 +118,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	SGX_ENCLAVE_RUN_USER_HANDLER(%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 934de08bc2f4a..608daccc46553 100644
--- a/arch/x86/include/uapi/asm/sgx.h
+++ b/arch/x86/include/uapi/asm/sgx.h
@@ -74,6 +74,28 @@ struct sgx_enclave_provision {
 	__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()
@@ -82,34 +104,42 @@ struct sgx_enclave_provision {
  * @trapnr:	exception trap number, a.k.a. fault vector
  * @error_code:	exception error code
  * @address:	exception address, e.g. CR2 on a #PF
- * @reserved:	reserved for future use
  */
 struct sgx_enclave_exception {
 	__u32 leaf;
 	__u16 trapnr;
 	__u16 error_code;
 	__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_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;
+	__u32 flags;
+	__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 - 32];
+	};
+};
 
 /**
  * __vdso_sgx_enter_enclave() - Enter an SGX enclave
@@ -119,16 +149,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 +188,13 @@ 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

[PATCH for_v37 3/6] 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 4fe3d06dd7878..adbd59d415171 100644
--- a/arch/x86/entry/vdso/vsgx_enter_enclave.S
+++ b/arch/x86/entry/vdso/vsgx_enter_enclave.S
@@ -18,6 +18,9 @@
 /* #define SGX_ENCLAVE_RUN_USER_DATA	3*8 */
 #define SGX_ENCLAVE_RUN_EXCEPTION	4*8
 
+#define SGX_SYNCHRONOUS_EXIT		0
+#define SGX_EXCEPTION_EXIT		1
+
 /* Offsets into sgx_enter_enclave.exception. */
 #define SGX_EX_LEAF			0*8
 #define SGX_EX_TRAPNR			0*8+4
@@ -65,7 +68,7 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave)
 	mov	SGX_ENCLAVE_RUN_PTR(%rbp), %rbx
 
 	/* Set exit_reason. */
-	movl	$0, SGX_ENCLAVE_RUN_EXIT_REASON(%rbx)
+	movl	$SGX_SYNCHRONOUS_EXIT, SGX_ENCLAVE_RUN_EXIT_REASON(%rbx)
 
 	/* Invoke userspace's exit handler if one was provided. */
 .Lhandle_exit:
@@ -92,7 +95,7 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave)
 	mov	SGX_ENCLAVE_RUN_PTR(%rbp), %rbx
 
 	/* Set the exit_reason and exception info. */
-	movl	$(-EFAULT), SGX_ENCLAVE_RUN_EXIT_REASON(%rbx)
+	movl	$SGX_EXCEPTION_EXIT, SGX_ENCLAVE_RUN_EXIT_REASON(%rbx)
 
 	mov	%eax, (SGX_ENCLAVE_RUN_EXCEPTION + SGX_EX_LEAF)(%rbx)
 	mov	%di,  (SGX_ENCLAVE_RUN_EXCEPTION + SGX_EX_TRAPNR)(%rbx)
diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
index 608daccc46553..b1d63f90ad643 100644
--- a/arch/x86/include/uapi/asm/sgx.h
+++ b/arch/x86/include/uapi/asm/sgx.h
@@ -74,6 +74,9 @@ struct sgx_enclave_provision {
 	__u64 attribute_fd;
 };
 
+#define SGX_SYNCHRONOUS_EXIT	0
+#define SGX_EXCEPTION_EXIT	1
+
 struct sgx_enclave_run;
 
 /**
-- 
2.28.0

[PATCH for_v37 4/6] selftests/sgx: Update the SGX selftest to match the reworked vDSO API

[Sean Christopherson]

Update the call to __vdso_sgx_enter_enclave() to use the sgx_enclave_run
struct instead of cramming parameters into the function call itself.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 tools/testing/selftests/sgx/call.S | 10 +++-------
 tools/testing/selftests/sgx/main.c | 11 +++++------
 tools/testing/selftests/sgx/main.h |  2 +-
 3 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/tools/testing/selftests/sgx/call.S b/tools/testing/selftests/sgx/call.S
index 77131e83db424..c4837965dfbb4 100644
--- a/tools/testing/selftests/sgx/call.S
+++ b/tools/testing/selftests/sgx/call.S
@@ -31,15 +31,11 @@ sgx_call_vdso:
 	.cfi_rel_offset		%rbx, 0
 	push	$0
 	.cfi_adjust_cfa_offset	8
-	push	0x48(%rsp)
-	.cfi_adjust_cfa_offset	8
-	push	0x48(%rsp)
-	.cfi_adjust_cfa_offset	8
-	push	0x48(%rsp)
+	push	0x38(%rsp)
 	.cfi_adjust_cfa_offset	8
 	call	*eenter(%rip)
-	add	$0x20, %rsp
-	.cfi_adjust_cfa_offset	-0x20
+	add	$0x10, %rsp
+	.cfi_adjust_cfa_offset	-0x10
 	pop	%rbx
 	.cfi_adjust_cfa_offset	-8
 	pop	%r12
diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
index 8d95569e7a66f..17f36e590fbd2 100644
--- a/tools/testing/selftests/sgx/main.c
+++ b/tools/testing/selftests/sgx/main.c
@@ -125,7 +125,7 @@ static Elf64_Sym *vdso_symtab_get(struct vdso_symtab *symtab, const char *name)
 
 int main(int argc, char *argv[], char *envp[])
 {
-	struct sgx_enclave_exception exception;
+	struct sgx_enclave_run run;
 	struct vdso_symtab symtab;
 	Elf64_Sym *eenter_sym;
 	uint64_t result = 0;
@@ -156,7 +156,8 @@ int main(int argc, char *argv[], char *envp[])
 		}
 	}
 
-	memset(&exception, 0, sizeof(exception));
+	memset(&run, 0, sizeof(run));
+	run.tcs = encl.encl_base;
 
 	addr = vdso_get_base_addr(envp);
 	if (!addr)
@@ -171,8 +172,7 @@ int main(int argc, char *argv[], char *envp[])
 
 	eenter = addr + eenter_sym->st_value;
 
-	sgx_call_vdso((void *)&MAGIC, &result, 0, EENTER, NULL, NULL,
-		      (void *)encl.encl_base, &exception, NULL);
+	sgx_call_vdso((void *)&MAGIC, &result, 0, EENTER, NULL, NULL, &run);
 	if (result != MAGIC) {
 		printf("FAIL: sgx_call_vdso(), expected: 0x%lx, got: 0x%lx\n",
 		       MAGIC, result);
@@ -181,8 +181,7 @@ int main(int argc, char *argv[], char *envp[])
 
 	/* Invoke the vDSO directly. */
 	result = 0;
-	eenter((unsigned long)&MAGIC, (unsigned long)&result, 0, EENTER, 0, 0,
-	       (void *)encl.encl_base, &exception, NULL);
+	eenter((unsigned long)&MAGIC, (unsigned long)&result, 0, EENTER, 0, 0, &run);
 	if (result != MAGIC) {
 		printf("FAIL: eenter(), expected: 0x%lx, got: 0x%lx\n",
 		       MAGIC, result);
diff --git a/tools/testing/selftests/sgx/main.h b/tools/testing/selftests/sgx/main.h
index 999422cc73438..2b4777942500f 100644
--- a/tools/testing/selftests/sgx/main.h
+++ b/tools/testing/selftests/sgx/main.h
@@ -33,6 +33,6 @@ bool encl_measure(struct encl *encl);
 bool encl_build(struct encl *encl);
 
 int sgx_call_vdso(void *rdi, void *rsi, long rdx, u32 leaf, void *r8, void *r9,
-		  void *tcs, struct sgx_enclave_exception *ei, void *cb);
+		  struct sgx_enclave_run *run);
 
 #endif /* MAIN_H */
-- 
2.28.0

[PATCH for_v37 5/6] selftests/sgx: Sanity check the return value of the vDSO call

[Sean Christopherson]

Assert that the vDSO call returns 0 and that the exit reason is always
due to EEXIT.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 tools/testing/selftests/sgx/main.c | 35 +++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
index 17f36e590fbd2..158ea9e2b6d3a 100644
--- a/tools/testing/selftests/sgx/main.c
+++ b/tools/testing/selftests/sgx/main.c
@@ -123,6 +123,24 @@ static Elf64_Sym *vdso_symtab_get(struct vdso_symtab *symtab, const char *name)
 	return NULL;
 }
 
+int check_result(struct sgx_enclave_run *run, int ret, uint64_t result,
+		 const char *test)
+{
+	if (ret) {
+		printf("FAIL: %s() returned: %d\n", test, ret);
+		return ret;
+	} else if (run->exit_reason != SGX_SYNCHRONOUS_EXIT) {
+		printf("FAIL: %s() exit reason, expected: %u, got: %u\n",
+		       test, SGX_SYNCHRONOUS_EXIT, run->exit_reason);
+		return -EIO;
+	} else if (result != MAGIC) {
+		printf("FAIL: %s(), expected: 0x%lx, got: 0x%lx\n",
+		       test, MAGIC, result);
+		return -EIO;
+	}
+	return 0;
+}
+
 int main(int argc, char *argv[], char *envp[])
 {
 	struct sgx_enclave_run run;
@@ -132,6 +150,7 @@ int main(int argc, char *argv[], char *envp[])
 	struct encl encl;
 	unsigned int i;
 	void *addr;
+	int ret;
 
 	if (!encl_load("test_encl.elf", &encl))
 		goto err;
@@ -172,21 +191,17 @@ int main(int argc, char *argv[], char *envp[])
 
 	eenter = addr + eenter_sym->st_value;
 
-	sgx_call_vdso((void *)&MAGIC, &result, 0, EENTER, NULL, NULL, &run);
-	if (result != MAGIC) {
-		printf("FAIL: sgx_call_vdso(), expected: 0x%lx, got: 0x%lx\n",
-		       MAGIC, result);
+	ret = sgx_call_vdso((void *)&MAGIC, &result, 0, EENTER, NULL, NULL, &run);
+	if (check_result(&run, ret, result, "sgx_call_vdso"))
 		goto err;
-	}
+
 
 	/* Invoke the vDSO directly. */
 	result = 0;
-	eenter((unsigned long)&MAGIC, (unsigned long)&result, 0, EENTER, 0, 0, &run);
-	if (result != MAGIC) {
-		printf("FAIL: eenter(), expected: 0x%lx, got: 0x%lx\n",
-		       MAGIC, result);
+	ret = eenter((unsigned long)&MAGIC, (unsigned long)&result, 0, EENTER,
+		     0, 0, &run);
+	if (check_result(&run, ret, result, "eenter"))
 		goto err;
-	}
 
 	printf("SUCCESS\n");
 	encl_delete(&encl);
-- 
2.28.0

[PATCH for_v37 6/6] selftests/sgx: Add a smoke test to ensure the user handler is invoked

[Sean Christopherson]

Add a very simple exit handler and use it as a third variant of the SGX
selftest to verify that the vDSO invokes then user's handler when one is
supplied.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 tools/testing/selftests/sgx/main.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
index 158ea9e2b6d3a..44acb3c720675 100644
--- a/tools/testing/selftests/sgx/main.c
+++ b/tools/testing/selftests/sgx/main.c
@@ -137,10 +137,21 @@ int check_result(struct sgx_enclave_run *run, int ret, uint64_t result,
 		printf("FAIL: %s(), expected: 0x%lx, got: 0x%lx\n",
 		       test, MAGIC, result);
 		return -EIO;
+	} else if (run->user_data) {
+		printf("FAIL: %s() user data, expected: 0x0, got: 0x%llx\n",
+		       test, run->user_data);
+		return -EIO;
 	}
 	return 0;
 }
 
+static int exit_handler(long rdi, long rsi, long rdx, long ursp, long r8, long r9,
+			struct sgx_enclave_run *run)
+{
+	run->user_data = 0;
+	return 0;
+}
+
 int main(int argc, char *argv[], char *envp[])
 {
 	struct sgx_enclave_run run;
@@ -203,6 +214,14 @@ int main(int argc, char *argv[], char *envp[])
 	if (check_result(&run, ret, result, "eenter"))
 		goto err;
 
+	/* And with an exit handler. */
+	run.user_handler = exit_handler;
+	run.user_data = 0xdeadbeef;
+	ret = eenter((unsigned long)&MAGIC, (unsigned long)&result, 0, EENTER,
+		     0, 0, &run);
+	if (check_result(&run, ret, result, "exit_handler"))
+		goto err;
+
 	printf("SUCCESS\n");
 	encl_delete(&encl);
 	exit(0);
-- 
2.28.0

Re: [PATCH for_v37 2/6] x86/vdso: x86/sgx: Rework __vdso_sgx_enter_enclave() API

[Jarkko Sakkinen]

On Fri, Sep 04, 2020 at 03:44:33AM -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, an explicit "exit_reason" to avoid
> overloading the return value, and a "flags" field to provide a path for
> future extensions.
> 
> Moving the params into a struct will also make it less painful to use
> dedicated exit reasons values in a future patch.
> 
> Cc: Nathaniel McCallum <npmccallum@redhat.com>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

I like this a lot. Everything is now so much better tied together.

If I understood the change correctly this solution also addreses my
concerns of eBPF because 'flags' allows to change representation what
handler means (later on, if we ever want).

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

/Jarkko

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

[Jarkko Sakkinen]

On Fri, Sep 04, 2020 at 03:44:34AM -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>

I agree with the change but I think this a the main reason for merging
this change: since we have the IO structure it is not coherent to not have
all IO data there that can be put there. Better to have everything in
one place when possible.

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

/Jarkko

Re: [PATCH for_v37 4/6] selftests/sgx: Update the SGX selftest to match the reworked vDSO API

[Jarkko Sakkinen]

On Fri, Sep 04, 2020 at 03:44:35AM -0700, Sean Christopherson wrote:
> Update the call to __vdso_sgx_enter_enclave() to use the sgx_enclave_run
> struct instead of cramming parameters into the function call itself.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

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

/Jarkko

Re: [PATCH for_v37 5/6] selftests/sgx: Sanity check the return value of the vDSO call

[Jarkko Sakkinen]

On Fri, Sep 04, 2020 at 03:44:36AM -0700, Sean Christopherson wrote:
> Assert that the vDSO call returns 0 and that the exit reason is always
> due to EEXIT.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  tools/testing/selftests/sgx/main.c | 35 +++++++++++++++++++++---------
>  1 file changed, 25 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
> index 17f36e590fbd2..158ea9e2b6d3a 100644
> --- a/tools/testing/selftests/sgx/main.c
> +++ b/tools/testing/selftests/sgx/main.c
> @@ -123,6 +123,24 @@ static Elf64_Sym *vdso_symtab_get(struct vdso_symtab *symtab, const char *name)
>  	return NULL;
>  }
>  
> +int check_result(struct sgx_enclave_run *run, int ret, uint64_t result,
> +		 const char *test)
> +{
> +	if (ret) {
> +		printf("FAIL: %s() returned: %d\n", test, ret);
> +		return ret;
> +	} else if (run->exit_reason != SGX_SYNCHRONOUS_EXIT) {
> +		printf("FAIL: %s() exit reason, expected: %u, got: %u\n",
> +		       test, SGX_SYNCHRONOUS_EXIT, run->exit_reason);
> +		return -EIO;
> +	} else if (result != MAGIC) {
> +		printf("FAIL: %s(), expected: 0x%lx, got: 0x%lx\n",
> +		       test, MAGIC, result);
> +		return -EIO;
> +	}
> +	return 0;
> +}
> +
>  int main(int argc, char *argv[], char *envp[])
>  {
>  	struct sgx_enclave_run run;
> @@ -132,6 +150,7 @@ int main(int argc, char *argv[], char *envp[])
>  	struct encl encl;
>  	unsigned int i;
>  	void *addr;
> +	int ret;
>  
>  	if (!encl_load("test_encl.elf", &encl))
>  		goto err;
> @@ -172,21 +191,17 @@ int main(int argc, char *argv[], char *envp[])
>  
>  	eenter = addr + eenter_sym->st_value;
>  
> -	sgx_call_vdso((void *)&MAGIC, &result, 0, EENTER, NULL, NULL, &run);
> -	if (result != MAGIC) {
> -		printf("FAIL: sgx_call_vdso(), expected: 0x%lx, got: 0x%lx\n",
> -		       MAGIC, result);
> +	ret = sgx_call_vdso((void *)&MAGIC, &result, 0, EENTER, NULL, NULL, &run);
> +	if (check_result(&run, ret, result, "sgx_call_vdso"))
>  		goto err;
> -	}
> +
>  
>  	/* Invoke the vDSO directly. */
>  	result = 0;
> -	eenter((unsigned long)&MAGIC, (unsigned long)&result, 0, EENTER, 0, 0, &run);
> -	if (result != MAGIC) {
> -		printf("FAIL: eenter(), expected: 0x%lx, got: 0x%lx\n",
> -		       MAGIC, result);
> +	ret = eenter((unsigned long)&MAGIC, (unsigned long)&result, 0, EENTER,
> +		     0, 0, &run);
> +	if (check_result(&run, ret, result, "eenter"))
>  		goto err;
> -	}
>  
>  	printf("SUCCESS\n");
>  	encl_delete(&encl);
> -- 
> 2.28.0
> 

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

/Jarkko

Re: [PATCH for_v37 6/6] selftests/sgx: Add a smoke test to ensure the user handler is invoked

[Jarkko Sakkinen]

On Fri, Sep 04, 2020 at 03:44:37AM -0700, Sean Christopherson wrote:
> Add a very simple exit handler and use it as a third variant of the SGX
> selftest to verify that the vDSO invokes then user's handler when one is
> supplied.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
 
Acked-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

I have no concerns of the vDSO code with these changes have been merged.
The way things are structured ('flags' + the fact we have a context
structure 'run' for everything) keeps door open for eBPF (if ever
wanted).

/Jarkko