[RFC PATCH v4 0/3] x86/sgx: Amend vDSO API to allow enclave/host parameter passing on untrusted stack

[RFC PATCH v4 0/3] x86/sgx: Amend vDSO API to allow enclave/host parameter passing on untrusted stack

[Cedric Xing]

This patchset is based upon, and can be applied cleanly on SGX1 patch v20
(https://lkml.org/lkml/2019/4/17/344) by Jarkko Sakkinen.

The current proposed __vdso_sgx_enter_enclave() requires enclaves to preserve
%rsp, which prohibits enclaves from allocating space on the untrusted stack.
However, there are existing enclaves (e.g. those built with current Intel SGX
SDK libraries) relying on the untrusted stack for passing parameters to
untrusted functions (aka. o-calls), which requires allocating space on the
untrusted stack by enclaves. After all, passing data via untrusted stack is
very easy to implement (by enclaves), with essentially no overhead, therefore
is very suitable for exchanging data in small amounts, so could be desirable by
future SGX applications as well.

This patchset introduces a new ABI for __vdso_sgx_enter_enclave() to anchor its
stack frame on %rbp (instead of %rsp), so as to allow enclaves to "push" onto
the untrusted stack by decrementing the untrusted %rsp. And in order to service
o-calls and to preserve the untrusted stack upon exceptions, the new vDSO API
takes one more optional parameter - "callback", which if supplied, will be
invoked on all enclave exits (including normal and asynchronous exits). Ample
details regarding the new ABI have been documented as comments inside the
source code located in arch/x86/entry/vsgx_enter_enclave.S

Please note that there was a lengthy discussion on what is the "best" approach
for passing parameters for trusted/untrusted calls. Unfortunately there's no
single "best" approach that fits all use cases, hence this new ABI has been
designed intentionally to accommodate varieties. Therefore, to those not
interested in using the untrusted stack, whatever worked with the old ABI
proposed by Sean will continue to work with this new ABI.

The SGX selftest has been augmented by two new tests. One exercises the new
callback interface, and serves as a simple example to showcase how to use it;
while the other validates the hand-crafted CFI directives in
__vdso_sgx_enter_enclave() by single-stepping through it and unwinding call
stack at every instruction.

Changelog:
  · This is version 4 of this patch series with the following changes.
    - Removed unrelated cosmetic changes.
    - Rewrote and reformatted comments in
      arch/x86/entry/vdso/vsgx_enter_enclave.S to follow kernel-doc
      conventions. New comments now can be converted to nice looking man pages.
    - Fixed minor issues in the unwinding selftest and now it can run to
      completion successfully with Sean's fix in vDSO fixup code
      (https://patchwork.kernel.org/patch/11040801/). Comments have also been
      added to describe the tests done.
  · v3 - https://patchwork.kernel.org/cover/11039263/
  · v2 - https://patchwork.kernel.org/cover/10914161/
  · v1 - https://patchwork.kernel.org/cover/10911615/

Cedric Xing (3):
  selftests/x86/sgx: Fix Makefile for SGX selftest
  x86/vdso: Modify __vdso_sgx_enter_enclave() to allow parameter passing
    on untrusted stack
  selftests/x86/sgx: Augment SGX selftest to test vDSO API

 arch/x86/entry/vdso/vsgx_enter_enclave.S   | 310 ++++++++++++++-----
 arch/x86/include/uapi/asm/sgx.h            |  14 +-
 tools/testing/selftests/x86/sgx/Makefile   |  49 ++-
 tools/testing/selftests/x86/sgx/main.c     | 344 ++++++++++++++++++---
 tools/testing/selftests/x86/sgx/sgx_call.S |  40 ++-
 5 files changed, 600 insertions(+), 157 deletions(-)

-- 
2.17.1

[RFC PATCH v4 1/3] selftests/x86/sgx: Fix Makefile for SGX selftest

[Cedric Xing]

The original x86/sgx/Makefile didn't work when "x86/sgx" was specified as the
test target, nor did it work with "run_tests" as the make target. Yet another
problem was that it breaks 32-bit only build. This patch fixes those problems,
along with adjustments to compiler/linker options and simplifications to the
build rules.

Signed-off-by: Cedric Xing <cedric.xing@intel.com>
---
 tools/testing/selftests/x86/sgx/Makefile | 45 +++++++++---------------
 1 file changed, 16 insertions(+), 29 deletions(-)

diff --git a/tools/testing/selftests/x86/sgx/Makefile b/tools/testing/selftests/x86/sgx/Makefile
index 1fd6f2708e81..3af15d7c8644 100644
--- a/tools/testing/selftests/x86/sgx/Makefile
+++ b/tools/testing/selftests/x86/sgx/Makefile
@@ -2,47 +2,34 @@ top_srcdir = ../../../../..
 
 include ../../lib.mk
 
-HOST_CFLAGS := -Wall -Werror -g $(INCLUDES) -fPIC
-ENCL_CFLAGS := -Wall -Werror -static -nostdlib -nostartfiles -fPIC \
+ifeq ($(shell $(CC) -dumpmachine | cut --delimiter=- -f1),x86_64)
+all: all_64
+endif
+
+HOST_CFLAGS := -Wall -Werror -g $(INCLUDES)
+ENCL_CFLAGS := -Wall -Werror -static -nostdlib -nostartfiles -fPIE \
 	       -fno-stack-protector -mrdrnd $(INCLUDES)
 
 TEST_CUSTOM_PROGS := $(OUTPUT)/test_sgx
 all_64: $(TEST_CUSTOM_PROGS)
 
-$(TEST_CUSTOM_PROGS): $(OUTPUT)/main.o $(OUTPUT)/sgx_call.o \
-		      $(OUTPUT)/encl_piggy.o
+$(TEST_CUSTOM_PROGS): main.c sgx_call.S $(OUTPUT)/encl_piggy.o
 	$(CC) $(HOST_CFLAGS) -o $@ $^
 
-$(OUTPUT)/main.o: main.c
-	$(CC) $(HOST_CFLAGS) -c $< -o $@
-
-$(OUTPUT)/sgx_call.o: sgx_call.S
-	$(CC) $(HOST_CFLAGS) -c $< -o $@
-
-$(OUTPUT)/encl_piggy.o: $(OUTPUT)/encl.bin $(OUTPUT)/encl.ss
-	$(CC) $(HOST_CFLAGS) -c encl_piggy.S -o $@
+$(OUTPUT)/encl_piggy.o: encl_piggy.S $(OUTPUT)/encl.bin $(OUTPUT)/encl.ss
+	$(CC) $(HOST_CFLAGS) -I$(OUTPUT) -c $< -o $@
 
-$(OUTPUT)/encl.bin: $(OUTPUT)/encl.elf $(OUTPUT)/sgxsign
+$(OUTPUT)/encl.bin: $(OUTPUT)/encl.elf
 	objcopy --remove-section=.got.plt -O binary $< $@
 
-$(OUTPUT)/encl.elf: $(OUTPUT)/encl.o $(OUTPUT)/encl_bootstrap.o
-	$(CC) $(ENCL_CFLAGS) -T encl.lds -o $@ $^
+$(OUTPUT)/encl.elf: encl.lds encl.c encl_bootstrap.S
+	$(CC) $(ENCL_CFLAGS) -T $^ -o $@
 
-$(OUTPUT)/encl.o: encl.c
-	$(CC) $(ENCL_CFLAGS) -c $< -o $@
-
-$(OUTPUT)/encl_bootstrap.o: encl_bootstrap.S
-	$(CC) $(ENCL_CFLAGS) -c $< -o $@
-
-$(OUTPUT)/encl.ss: $(OUTPUT)/encl.bin  $(OUTPUT)/sgxsign
-	$(OUTPUT)/sgxsign signing_key.pem $(OUTPUT)/encl.bin $(OUTPUT)/encl.ss
+$(OUTPUT)/encl.ss: $(OUTPUT)/sgxsign signing_key.pem $(OUTPUT)/encl.bin
+	$^ $@
 
 $(OUTPUT)/sgxsign: sgxsign.c
 	$(CC) -o $@ $< -lcrypto
 
-EXTRA_CLEAN := $(OUTPUT)/sgx-selftest $(OUTPUT)/sgx-selftest.o \
-	       $(OUTPUT)/sgx_call.o $(OUTPUT)/encl.bin $(OUTPUT)/encl.ss \
-	       $(OUTPUT)/encl.elf $(OUTPUT)/encl.o $(OUTPUT)/encl_bootstrap.o \
-	       $(OUTPUT)/sgxsign
-
-.PHONY: clean
+EXTRA_CLEAN := $(TEST_CUSTOM_PROGS) $(addprefix $(OUTPUT)/,	\
+		encl.elf encl.bin encl.ss encl_piggy.o sgxsign)
-- 
2.17.1

[RFC PATCH v4 2/3] x86/vdso: Modify __vdso_sgx_enter_enclave() to allow parameter passing on untrusted stack

[Cedric Xing]

The previous __vdso_sgx_enter_enclave() requires enclaves to preserve %rsp,
which prohibits enclaves from allocating and passing parameters for
untrusted function calls (aka. o-calls) on the untrusted stack.

This patch addresses the problem above by introducing a new ABI that preserves
%rbp instead of %rsp. Then __vdso_sgx_enter_enclave() can anchor its frame
using %rbp so that enclaves are allowed to allocate space on the untrusted
stack by decrementing %rsp. Please note that the stack space allocated in such
way will be part of __vdso_sgx_enter_enclave()'s frame so will be freed after
__vdso_sgx_enter_enclave() returns. Therefore, __vdso_sgx_enter_enclave() has
been revised to take a callback function as an optional parameter, which if
supplied, will be invoked upon enclave exits (both AEX (Asynchronous Enclave
eXit) and normal exits), with the value of %rsp left off by the enclave as a
parameter to the callback.

Here's the summary of API/ABI changes in this patch. More details could be
found in arch/x86/entry/vdso/vsgx_enter_enclave.S.
  * 'struct sgx_enclave_exception' is renamed to 'struct sgx_enclave_exinfo'
    because it is filled upon both AEX (i.e. exceptions) and normal enclave
    exits.
  * __vdso_sgx_enter_enclave() anchors its frame using %rbp (instead of %rsp in
    the previous implementation).
  * __vdso_sgx_enter_enclave() takes one more parameter - a callback function
    to be invoked upon enclave exits. This callback is optional, and if not
    supplied, will cause __vdso_sgx_enter_enclave() to return upon enclave
    exits (same behavior as previous implementation).
  * The callback function is given as a parameter the value of %rsp at enclave
    exit to address data "pushed" by the enclave. A positive value returned by
    the callback will be treated as an ENCLU leaf for re-entering the enclave,
    while a zero or negative value will be passed through as the return
    value of __vdso_sgx_enter_enclave() to its caller. It's also safe to
    leave callback by longjmp() or by throwing a C++ exception.

Signed-off-by: Cedric Xing <cedric.xing@intel.com>
---
 arch/x86/entry/vdso/vsgx_enter_enclave.S | 310 +++++++++++++++++------
 arch/x86/include/uapi/asm/sgx.h          |  14 +-
 2 files changed, 242 insertions(+), 82 deletions(-)

diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.S b/arch/x86/entry/vdso/vsgx_enter_enclave.S
index fe0bf6671d6d..a96542ba6945 100644
--- a/arch/x86/entry/vdso/vsgx_enter_enclave.S
+++ b/arch/x86/entry/vdso/vsgx_enter_enclave.S
@@ -6,96 +6,256 @@
 
 #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
+#define EX_LEAF     0*8
+#define EX_TRAPNR   0*8+4
+#define EX_ERROR_CODE   0*8+6
+#define EX_ADDRESS  1*8
 
 .code64
 .section .text, "ax"
 
 #ifdef SGX_KERNEL_DOC
 /**
- * __vdso_sgx_enter_enclave() - Enter an SGX enclave
+ * typedef sgx_ex_callback - Callback function for __vdso_sgx_enter_enclave()
  *
- * @leaf:	**IN \%eax** - ENCLU leaf, must be EENTER or ERESUME
- * @tcs:	**IN \%rbx** - TCS, must be non-NULL
- * @ex_info:	**IN \%rcx** - Optional 'struct sgx_enclave_exception' pointer
+ * @rdi:    value of %%rdi register at enclave exit
+ * @rsi:    value of %%rsi register at enclave exit
+ * @rdx:    value of %%rdx register at enclave exit
+ * @exinfo: pointer to a sgx_enclave_exinfo structure, which was passed to
+ *      __vdso_sgx_enter_enclave() as input
+ * @r8:     value of %%r8 register at enclave exit
+ * @r9:     value of %%r9 register at enclave exit
+ * @tcs:    TCS used by __vdso_sgx_enter_enclave() to enter the enclave,
+ *      could be used to re-enter the
+ *      enclave
+ * @ursp:   value of %%rsp register at enclave exit
+ *
+ * This is the callback function to be invoked upon enclave exits, including
+ * normal exits (as result of EEXIT), and asynchronous exits (AEX) due to
+ * exceptions occurred at EENTER or within the enclave.
+ *
+ * This callback is expected to follow x86_64 ABI.
  *
  * Return:
- *  **OUT \%eax** -
- *  %0 on a clean entry/exit to/from the enclave, %-EINVAL if ENCLU leaf is
- *  not allowed or if TCS is NULL, %-EFAULT if ENCLU or the enclave faults
- *
- * **Important!**  __vdso_sgx_enter_enclave() is **NOT** compliant with the
- * x86-64 ABI, i.e. cannot be called from standard C code.   As noted above,
- * input parameters must be passed via ``%eax``, ``%rbx`` and ``%rcx``, with
- * the return value passed via ``%eax``.  All registers except ``%rsp`` must
- * be treated as volatile from the caller's perspective, including but not
- * limited to GPRs, EFLAGS.DF, MXCSR, FCW, etc...  Conversely, the enclave
- * being run **must** preserve the untrusted ``%rsp`` and stack.
+ *
+ * EENTER(2) - causes __vdso_sgx_enter_enclave() to issue ENCLU[EENTER] on the
+ * same TCS. All GPRs left off by this callback function will be passed through
+ * back to the enclave, except %%rax, %%rbx and %%rcx, which are clobbered by
+ * ENCLU[EENTER] instruction.
+ *
+ * ERESUME(3) - causes __vdso_sgx_enter_enclave() to issue ENCLU[ERESUME] on
+ * the same TCS.
+ *
+ * 0 (zero) or negative returned values will be returned back to
+ * __vdso_sgx_enter_enclave()'s caller as is.
+ *
+ * All other values will cause -EINVAL to be returned to
+ * __vdso_sgx_enter_enclave()'s caller.
+ *
+ * Note: All general purpose registers (GPRs) left off by the enclave are
+ * passed through to this function, except %%rax, %%rbx and %%rcx, which are
+ * used internally by __vdso_sgx_enter_enclave(). Some of those registers are
+ * accessible as function parameters (i.e. @rdi, @rsi, @rdx, @r8, @r9 and
+ * @ursp), while others can be accessed only from assembly code.
  */
-__vdso_sgx_enter_enclave(u32 leaf, void *tcs,
-			 struct sgx_enclave_exception *ex_info)
-{
-	if (leaf != SGX_EENTER && leaf != SGX_ERESUME)
-		return -EINVAL;
+typedef int sgx_ex_callback(long rdi, long rsi, long rdx,
+                struct sgx_enclave_exinfo *exinfo,
+                long r8, long r9, void *tcs, long ursp);
 
-	if (!tcs)
-		return -EINVAL;
+/**
+ * __vdso_sgx_enter_enclave() - Enter an SGX enclave and capture exceptions
+ *
+ * @leaf:
+ *  passed in %%eax, must be either EENTER(2) or ERESUME(3)
+ * @tcs:
+ *  passed on stack at 8(%%rsp), is the linear address of TCS
+ * @exinfo:
+ *  passed on stack at 0x10(%%rsp), optional, and if non-NULL, shall point
+ *  to an sgx_enclave_exinfo structure to receive information about the
+ *  enclave exit
+ * @callback:
+ *  passed on stack at 0x18(%%rsp), optional, and if non-NULL, points to a
+ *  callback function to be invoked at enclave exits
+ *
+ * __vdso_sgx_enter_enclave() issues either ENCLU[EENTER] or ENCLU[ERESUME] on
+ * @tcs depending on @leaf.
+ *
+ * IMPORTANT! This API is not compliant with x86-64 ABI but adopts a
+ * proprietary calling convention. Please see NOTES section below for details.
+ *
+ * On an enclave exit, @exinfo->leaf will be set to the ENCLU leaf at exit, if
+ * @exinfo is not NULL. That is, @exinfo->leaf may be one of the following:
+ *
+ *   * EEXIT:   Normal exit due to ENCLU[EEXIT] within the enclave. All other
+ *      members will remain intact.
+ *
+ *   * ERESUME: Asynchronous exit due to exceptions within the enclave.
+ *      @exinfo->trapnr, @exinfo->error_code and @exinfo->address are
+ *      set to the trap number, error code and fault address,
+ *      respectively.
+ *
+ *   * EENTER:  Exception occurred when trying to enter the enclave.
+ *      @exinfo->trapnr, @exinfo->error_code and @exinfo->address are
+ *      set to the trap number, error code and fault address,
+ *      accordingly.
+ *
+ * If @callback is NULL, 0 (zero) is returned if the enclave has been entered
+ * and exited normally, or -EFAULT if any exception has occurred, or -EINVAL if
+ * @leaf on input is neither EENTER or ERESUME.
+ *
+ * If @callback is not NULL, it is invoked at enclave exit, and then actions
+ * will be taken depending on its return value - i.e. positive value will be
+ * treated as ENCLU leaf to re-enter the enclave, while 0 (zero) or negative
+ * values will be returned back to the caller as is. Unrecognized leaf values
+ * will cause -EINVAL to be returned.
+ *
+ * Return:
+ *
+ * 0 (zero) is returned on a successful entry and normal exit from the enclave.
+ *
+ * -EINVAL is returned if @leaf is neither EENTER nor ERESUME, or if @callback
+ * is not NULL and returns a positive value that is neither EENTER nor ERESUME
+ * after the enclave exits.
+ *
+ * -EFAULT is returned if an exception has occurred at EENTER or during
+ * execution of the enclave and @callback is NULL, or if @callback is not NULL
+ * and it returns -EFAULT after the enclave exits.
+ *
+ * Other values may be returned as the return value from @callback if it is not
+ * NULL.
+ *
+ * Note: __vdso_sgx_enter_enclave() adopts a proprietary calling convention,
+ * described below:
+ *
+ *    * As noted above, input parameters are passed via %%eax and the stack.
+ *
+ *    * %%rbx and %%rcx must be treated as volatile as they are modified as part
+ *  of enclaves transitions and are used as scratch regs.
+ *
+ *    * %%rdx, %%rdi, %%rsi and %%r8-%%r15 are passed as is and may be freely
+ *  modified by the enclave. Values left in those registers will not be
+ *  altered either, so will be visiable to the callback or the caller (if no
+ *  callback is specified).
+ *
+ *    * %%rsp could be decremented by the enclave to allocate temporary space on
+ *      the untrusted stack. Temporary space allocated this way is retained in
+ *      the context of @callback, and will be freed (i.e. %%rsp will be
+ *      restored) before __vdso_sgx_enter_enclave() returns.
+ */
+int __vdso_sgx_enter_enclave(int leaf, void *tcs,
+                 struct sgx_enclave_exinfo *exinfo,
+                 sgx_ex_callback *callback);
+{
+     while (leaf == EENTER || leaf == ERESUME) {
+    int rc;
+    try {
+        ENCLU[leaf];
+        rc = 0;
+        if (exinfo)
+            exinfo->leaf = EEXIT;
+    } catch (exception) {
+        rc = -EFAULT;
+        if (exinfo)
+            *exinfo = exception;
+    }
 
-	try {
-		ENCLU[leaf];
-	} catch (exception) {
-		if (e)
-			*e = exception;
-		return -EFAULT;
-	}
+    leaf = !callback ? rc: (*callback)(rdi, rsi, rdx, exinfo,
+                       r8, r9, tcs, ursp);
+     }
 
-	return 0;
+     return leaf > 0 ? -EINVAL : leaf;
 }
 #endif
+
 ENTRY(__vdso_sgx_enter_enclave)
-	/* EENTER <= leaf <= ERESUME */
-	cmp	$0x2, %eax
-	jb	bad_input
-
-	cmp	$0x3, %eax
-	ja	bad_input
-
-	/* TCS must be non-NULL */
-	test	%rbx, %rbx
-	je	bad_input
-
-	/* Save @exception_info */
-	push	%rcx
-
-	/* Load AEP for ENCLU */
-	lea	1f(%rip),  %rcx
-1:	enclu
-
-	add	$0x8, %rsp
-	xor	%eax, %eax
-	ret
-
-bad_input:
-	mov     $(-EINVAL), %rax
-	ret
-
-.pushsection .fixup, "ax"
-	/* Re-load @exception_info and fill it (if it's non-NULL) */
-2:	pop	%rcx
-	test    %rcx, %rcx
-	je      3f
-
-	mov	%eax, EX_LEAF(%rcx)
-	mov	%di,  EX_TRAPNR(%rcx)
-	mov	%si,  EX_ERROR_CODE(%rcx)
-	mov	%rdx, EX_ADDRESS(%rcx)
-3:	mov	$(-EFAULT), %rax
-	ret
-.popsection
-
-_ASM_VDSO_EXTABLE_HANDLE(1b, 2b)
+    /* Prolog */
+    .cfi_startproc
+    push    %rbp
+    .cfi_adjust_cfa_offset  8
+    .cfi_rel_offset     %rbp, 0
+    mov %rsp, %rbp
+    .cfi_def_cfa_register   %rbp
+
+1:  /* EENTER <= leaf <= ERESUME */
+    cmp $0x2, %eax
+    jb  6f
+    cmp $0x3, %eax
+    ja  6f
+
+    /* Load TCS and AEP */
+    mov 0x10(%rbp), %rbx
+    lea 2f(%rip), %rcx
+
+    /* Single ENCLU serving as both EENTER and AEP (ERESUME) */
+2:  enclu
+
+    /* EEXIT path */
+    xor %ebx, %ebx
+3:  mov 0x18(%rbp), %rcx
+    jrcxz   4f
+    mov %eax, EX_LEAF(%rcx)
+    jnc 4f
+    mov %di, EX_TRAPNR(%rcx)
+    mov %si, EX_ERROR_CODE(%rcx)
+    mov %rdx, EX_ADDRESS(%rcx)
+
+4:  /* Call *callback if supplied */
+    mov 0x20(%rbp), %rax
+    test    %rax, %rax
+    /*
+     * At this point, %ebx holds the effective return value, which shall be
+     * returned if no callback is specified
+     */
+    cmovz   %rbx, %rax
+    jz  7f
+    /*
+     * Align stack per x86_64 ABI. The original %rsp is saved in %rbx to be
+     * restored after *callback returns.
+     */
+    mov %rsp, %rbx
+    and $-0x10, %rsp
+    /* Clear RFLAGS.DF per x86_64 ABI */
+    cld
+    /* Parameters for *callback */
+    push    %rbx
+    push    0x10(%rbp)
+    /* Call *%rax via retpoline */
+    call    40f
+    /*
+     * Restore %rsp to its original value left off by the enclave from last
+     * exit
+     */
+    mov %rbx, %rsp
+    /*
+     * Positive return value from *callback will be interpreted as an ENCLU
+     * leaf, while a non-positive value will be interpreted as the return
+     * value to be passed back to the caller.
+     */
+    jmp 1b
+40: /* retpoline */
+    call    42f
+41: pause
+    lfence
+    jmp 41b
+42: mov %rax, (%rsp)
+    ret
+
+5:  /* Exception path */
+    mov $-EFAULT, %ebx
+    stc
+    jmp 3b
+
+6:  /* Unsupported ENCLU leaf */
+    cmp $0, %eax
+    jle 7f
+    mov $-EINVAL, %eax
+
+7:  /* Epilog */
+    leave
+    .cfi_def_cfa        %rsp, 8
+    ret
+    .cfi_endproc
+
+_ASM_VDSO_EXTABLE_HANDLE(2b, 5b)
 
 ENDPROC(__vdso_sgx_enter_enclave)
diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
index 9ed690a38c70..50d2b5143e5e 100644
--- a/arch/x86/include/uapi/asm/sgx.h
+++ b/arch/x86/include/uapi/asm/sgx.h
@@ -24,7 +24,7 @@
 
 /**
  * struct sgx_enclave_create - parameter structure for the
- *                             %SGX_IOC_ENCLAVE_CREATE ioctl
+ *			       %SGX_IOC_ENCLAVE_CREATE ioctl
  * @src:	address for the SECS page data
  */
 struct sgx_enclave_create  {
@@ -33,7 +33,7 @@ struct sgx_enclave_create  {
 
 /**
  * struct sgx_enclave_add_page - parameter structure for the
- *                               %SGX_IOC_ENCLAVE_ADD_PAGE ioctl
+ *				 %SGX_IOC_ENCLAVE_ADD_PAGE ioctl
  * @addr:	address within the ELRANGE
  * @src:	address for the page data
  * @secinfo:	address for the SECINFO data
@@ -49,7 +49,7 @@ struct sgx_enclave_add_page {
 
 /**
  * struct sgx_enclave_init - parameter structure for the
- *                           %SGX_IOC_ENCLAVE_INIT ioctl
+ *			     %SGX_IOC_ENCLAVE_INIT ioctl
  * @sigstruct:	address for the SIGSTRUCT data
  */
 struct sgx_enclave_init {
@@ -66,16 +66,16 @@ struct sgx_enclave_set_attribute {
 };
 
 /**
- * struct sgx_enclave_exception - structure to report exceptions encountered in
- *				  __vdso_sgx_enter_enclave()
+ * struct sgx_enclave_exinfo - structure to report exceptions encountered in
+ *			       __vdso_sgx_enter_enclave()
  *
- * @leaf:	ENCLU leaf from \%eax at time of exception
+ * @leaf:	ENCLU leaf from \%eax at time of exception/exit
  * @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 {
+struct sgx_enclave_exinfo {
 	__u32 leaf;
 	__u16 trapnr;
 	__u16 error_code;
-- 
2.17.1

[RFC PATCH v4 3/3] selftests/x86/sgx: Augment SGX selftest to test vDSO API

[Cedric Xing]

This patch augments SGX selftest with two new tests.

The first test exercises the newly added callback interface, by marking the
whole enclave range as PROT_READ, then calling mprotect() upon #PFs to add
necessary PTE permissions per PFEC (#PF Error Code) until the enclave finishes.
This test also serves as an example to demonstrate the callback interface.

The second test single-steps through __vdso_sgx_enter_enclave() to make sure
the call stack can be unwound at every instruction within that vDSO API. Its
purpose is to validate the hand-crafted CFI directives in the assembly.

Besides the new tests, this patch also fixes minor problems in the Makefile,
such as:
  * appended "--build-id=none" to ld command line to suppress the
    ".note.gnu.build-id section discarded" linker warning.
  * removed "--remove-section=.got.plt" from objcopy command line as that
    section would never exist in statically linked (enclave) images.

Signed-off-by: Cedric Xing <cedric.xing@intel.com>
---
 tools/testing/selftests/x86/sgx/Makefile   |   6 +-
 tools/testing/selftests/x86/sgx/main.c     | 344 ++++++++++++++++++---
 tools/testing/selftests/x86/sgx/sgx_call.S |  40 ++-
 3 files changed, 343 insertions(+), 47 deletions(-)

diff --git a/tools/testing/selftests/x86/sgx/Makefile b/tools/testing/selftests/x86/sgx/Makefile
index 3af15d7c8644..31f937e220c4 100644
--- a/tools/testing/selftests/x86/sgx/Makefile
+++ b/tools/testing/selftests/x86/sgx/Makefile
@@ -14,16 +14,16 @@ TEST_CUSTOM_PROGS := $(OUTPUT)/test_sgx
 all_64: $(TEST_CUSTOM_PROGS)
 
 $(TEST_CUSTOM_PROGS): main.c sgx_call.S $(OUTPUT)/encl_piggy.o
-	$(CC) $(HOST_CFLAGS) -o $@ $^
+	$(CC) $(HOST_CFLAGS) -o $@ $^ -lunwind -ldl -Wl,--defsym,__image_base=0 -pie
 
 $(OUTPUT)/encl_piggy.o: encl_piggy.S $(OUTPUT)/encl.bin $(OUTPUT)/encl.ss
 	$(CC) $(HOST_CFLAGS) -I$(OUTPUT) -c $< -o $@
 
 $(OUTPUT)/encl.bin: $(OUTPUT)/encl.elf
-	objcopy --remove-section=.got.plt -O binary $< $@
+	objcopy -O binary $< $@
 
 $(OUTPUT)/encl.elf: encl.lds encl.c encl_bootstrap.S
-	$(CC) $(ENCL_CFLAGS) -T $^ -o $@
+	$(CC) $(ENCL_CFLAGS) -T $^ -o $@ -Wl,--build-id=none
 
 $(OUTPUT)/encl.ss: $(OUTPUT)/sgxsign signing_key.pem $(OUTPUT)/encl.bin
 	$^ $@
diff --git a/tools/testing/selftests/x86/sgx/main.c b/tools/testing/selftests/x86/sgx/main.c
index e2265f841fb0..e47d6c32623f 100644
--- a/tools/testing/selftests/x86/sgx/main.c
+++ b/tools/testing/selftests/x86/sgx/main.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
 // Copyright(c) 2016-18 Intel Corporation.
 
+#define _GNU_SOURCE
 #include <elf.h>
 #include <fcntl.h>
 #include <stdbool.h>
@@ -9,16 +10,31 @@
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
+#include <errno.h>
 #include <sys/ioctl.h>
 #include <sys/mman.h>
 #include <sys/stat.h>
-#include <sys/time.h>
+#include <sys/auxv.h>
+#include <signal.h>
+#include <sys/ucontext.h>
+
+#define UNW_LOCAL_ONLY
+#include <libunwind.h>
+
 #include "encl_piggy.h"
 #include "defines.h"
 #include "../../../../../arch/x86/kernel/cpu/sgx/arch.h"
 #include "../../../../../arch/x86/include/uapi/asm/sgx.h"
 
-static const uint64_t MAGIC = 0x1122334455667788ULL;
+#define _Q(x)	__Q(x)
+#define __Q(x)	#x
+#define ERRLN	"Line " _Q(__LINE__)
+
+#define X86_EFLAGS_TF	(1ul << 8)
+
+extern char __image_base[];
+size_t eenter;
+static size_t vdso_base;
 
 struct vdso_symtab {
 	Elf64_Sym *elf_symtab;
@@ -26,20 +42,11 @@ struct vdso_symtab {
 	Elf64_Word *elf_hashtab;
 };
 
-static void *vdso_get_base_addr(char *envp[])
+static void vdso_init(void)
 {
-	Elf64_auxv_t *auxv;
-	int i;
-
-	for (i = 0; envp[i]; i++);
-	auxv = (Elf64_auxv_t *)&envp[i + 1];
-
-	for (i = 0; auxv[i].a_type != AT_NULL; i++) {
-		if (auxv[i].a_type == AT_SYSINFO_EHDR)
-			return (void *)auxv[i].a_un.a_val;
-	}
-
-	return NULL;
+	vdso_base = getauxval(AT_SYSINFO_EHDR);
+	if (!vdso_base)
+		exit(1);
 }
 
 static Elf64_Dyn *vdso_get_dyntab(void *addr)
@@ -66,8 +73,9 @@ static void *vdso_get_dyn(void *addr, Elf64_Dyn *dyntab, Elf64_Sxword tag)
 	return NULL;
 }
 
-static bool vdso_get_symtab(void *addr, struct vdso_symtab *symtab)
+static bool vdso_get_symtab(struct vdso_symtab *symtab)
 {
+	void *addr = (void *)vdso_base;
 	Elf64_Dyn *dyntab = vdso_get_dyntab(addr);
 
 	symtab->elf_symtab = vdso_get_dyn(addr, dyntab, DT_SYMTAB);
@@ -138,7 +146,7 @@ static bool encl_create(int dev_fd, unsigned long bin_size,
 	base = mmap(NULL, secs->size, PROT_READ | PROT_WRITE | PROT_EXEC,
 		    MAP_SHARED, dev_fd, 0);
 	if (base == MAP_FAILED) {
-		perror("mmap");
+		perror(ERRLN);
 		return false;
 	}
 
@@ -224,35 +232,292 @@ static bool encl_load(struct sgx_secs *secs, unsigned long bin_size)
 	return false;
 }
 
-void sgx_call(void *rdi, void *rsi, void *tcs,
-	      struct sgx_enclave_exception *exception,
-	      void *eenter);
+int sgx_call(void *rdi, void *rsi, long rdx, void *rcx, void *r8, void *r9,
+	     void *tcs, struct sgx_enclave_exinfo *ei, void *cb);
+
+static void show_enclave_exinfo(const struct sgx_enclave_exinfo *exinfop,
+				const char *header)
+{
+	static const char * const enclu_leaves[] = {
+		"EREPORT",
+		"EGETKEY",
+		"EENTER",
+		"ERESUME",
+		"EEXIT"
+	};
+	static const char * const exception_names[] = {
+		"#DE",
+		"#DB",
+		"NMI",
+		"#BP",
+		"#OF",
+		"#BR",
+		"#UD",
+		"#NM",
+		"#DF",
+		"CSO",
+		"#TS",
+		"#NP",
+		"#SS",
+		"#GP",
+		"#PF",
+		"Unknown",
+		"#MF",
+		"#AC",
+		"#MC",
+		"#XM",
+		"#VE",
+		"Unknown",
+		"Unknown",
+		"Unknown",
+		"Unknown",
+		"Unknown",
+		"Unknown",
+		"Unknown",
+		"Unknown",
+		"Unknown",
+		"Unknown",
+		"Unknown"
+	};
+
+	printf("%s: leaf:%s(%d)", header,
+		enclu_leaves[exinfop->leaf], exinfop->leaf);
+	if (exinfop->leaf != 4)
+		printf(" trap:%s(%d) ec:%d addr:0x%llx\n",
+			exception_names[exinfop->trapnr], exinfop->trapnr,
+			exinfop->error_code, exinfop->address);
+	else
+		printf("\n");
+}
+
+static const uint64_t MAGIC = 0x1122334455667788ULL;
+
+/*
+ * test1() tests vDSO API (i.e. __vdso_sgx_enter_enclave) without supplying a
+ * callback function.  It loads a very simple enclave that copies a 64-bit
+ * value from source buffer to the destination. Then it invokes the enclave
+ * twice. At the first time it provides all valid inputs and verifies the
+ * output buffer contains the same value as the source buffer. At the second
+ * time, it provides NULL as the TCS address to exercise the exception flow.
+ */
+static void test1(struct sgx_secs *secs)
+{
+	uint64_t result = 0;
+	struct sgx_enclave_exinfo exinfo;
+
+	printf("[1] Entering the enclave without callback.\n");
+
+	printf("Input: 0x%lx\n Expect: Same as input\n", MAGIC);
+	sgx_call((void *)&MAGIC, &result, 0, NULL, NULL, NULL,
+		 (void *)secs->base, &exinfo, NULL);
+	show_enclave_exinfo(&exinfo, " Exit");
+	if (result != MAGIC) {
+		fprintf(stderr, "0x%lx != 0x%lx\n", result, MAGIC);
+		exit(1);
+	}
+	printf(" Output: 0x%lx\n", result);
+
+	printf("Input: Null TCS\n Expect: #PF at EENTER\n");
+	sgx_call((void *)&MAGIC, &result, 0, NULL, NULL, NULL,
+		 NULL, &exinfo, NULL);
+	show_enclave_exinfo(&exinfo, " Exit");
+	if (exinfo.leaf != 2 /*EENTER*/ || exinfo.trapnr != 14 /*#PF*/)
+		exit(1);
+}
+
+static int test2_callback(long rdi, long rsi, long rdx,
+			  struct sgx_enclave_exinfo *ei, long r8, long r9,
+			  void *tcs, long ursp)
+{
+	show_enclave_exinfo(ei, "  callback");
+
+	switch (ei->leaf) {
+	case 4:
+		return 0;
+	case 3:
+	case 2:
+		switch (ei->trapnr) {
+		case 1:	/*#DB*/
+			break;
+		case 14:/*#PF*/
+			if ((ei->error_code & 1) == 0) {
+				fprintf(stderr, ERRLN
+					": Unexpected #PF error code\n");
+				exit(1);
+			}
+			if (mprotect((void *)(ei->address & -0x1000), 0x1000,
+				     ((ei->error_code & 2) ? PROT_WRITE : 0) |
+				     ((ei->error_code & 0x10) ? PROT_EXEC : 0) |
+				     PROT_READ)) {
+				perror(ERRLN);
+				exit(1);
+			}
+			break;
+		default:
+			fprintf(stderr, ERRLN ": Unexpected exception\n");
+			exit(1);
+		}
+		return ei->leaf == 2 ? -EAGAIN : ei->leaf;
+	}
+	return -EINVAL;
+}
+
+/*
+ * test2() tests the exception/callback mechanism of the vDSO API with a
+ * callback function. Firstly, it supplies all valid inputs along with a
+ * callback function, and verifies that exinfo contains the expected values.
+ * Secondly, it marks the whole enclave virtual range as read-only, and let the
+ * callback fixes the PTE permissions by calling mprotect() along the way. The
+ * callback in this test also serves an example to show how to use the callback
+ * interface.
+ */
+static void test2(struct sgx_secs *secs)
+{
+	uint64_t result = 0;
+	struct sgx_enclave_exinfo exinfo;
+
+	printf("[2] Entering the enclave with callback.\n");
+
+	printf("Input: 0x%lx\n Expect: Same as input\n", MAGIC);
+	sgx_call((void *)&MAGIC, &result, 0, NULL, NULL, NULL,
+		 (void *)secs->base, &exinfo, test2_callback);
+	if (result != MAGIC) {
+		fprintf(stderr, "0x%lx != 0x%lx\n", result, MAGIC);
+		exit(1);
+	}
+	printf(" Output: 0x%lx\n", result);
+
+	printf("Input: Read-only enclave (0x%lx-0x%lx)\n"
+	       " Expect: #PFs to be fixed by callback\n",
+	       secs->base, secs->base + (encl_bin_end - encl_bin) - 1);
+	if (mprotect((void *)secs->base, encl_bin_end - encl_bin, PROT_READ)) {
+		perror(ERRLN);
+		exit(1);
+	}
+	while (sgx_call((void *)&MAGIC, &result, 0, NULL, NULL, NULL,
+			(void *)secs->base, &exinfo, test2_callback) == -EAGAIN)
+		;
+	show_enclave_exinfo(&exinfo, " Exit");
+	if (exinfo.leaf != 4 /*EEXIT*/)
+		exit(1);
+}
+
+static void *test3_caller;
+static struct test3_proc_context {
+	unw_word_t	ip, bx, sp, bp, r12, r13, r14, r15;
+} test3_ctx;
 
-int main(int argc, char *argv[], char *envp[])
+static unw_word_t test3_getcontext(unw_cursor_t *cursor,
+				   struct test3_proc_context *ctxp)
+{
+	unw_get_reg(cursor, UNW_REG_IP, &ctxp->ip);
+	unw_get_reg(cursor, UNW_REG_SP, &ctxp->sp);
+	unw_get_reg(cursor, UNW_X86_64_RBX, &ctxp->bx);
+	unw_get_reg(cursor, UNW_X86_64_RBP, &ctxp->bp);
+	unw_get_reg(cursor, UNW_X86_64_R12, &ctxp->r12);
+	unw_get_reg(cursor, UNW_X86_64_R13, &ctxp->r13);
+	unw_get_reg(cursor, UNW_X86_64_R14, &ctxp->r14);
+	unw_get_reg(cursor, UNW_X86_64_R15, &ctxp->r15);
+	return ctxp->ip;
+}
+
+static void test3_sigtrap(int sig, siginfo_t *info, ucontext_t *ctxp)
+{
+	static int in_vdso_eenter;
+
+	unw_cursor_t cursor;
+	unw_context_t uc;
+	struct test3_proc_context pc;
+
+	if (ctxp->uc_mcontext.gregs[REG_RIP] == eenter) {
+		in_vdso_eenter = 1;
+		printf("  trace started at ip:%llx (vdso:0x%llx)\n",
+			ctxp->uc_mcontext.gregs[REG_RIP],
+			ctxp->uc_mcontext.gregs[REG_RIP] - vdso_base);
+	}
+
+	if (!in_vdso_eenter)
+		return;
+
+	if ((void *)ctxp->uc_mcontext.gregs[REG_RIP] == test3_caller) {
+		in_vdso_eenter = 0;
+		ctxp->uc_mcontext.gregs[REG_EFL] &= ~X86_EFLAGS_TF;
+		printf("  trace ended successfully at ip:%llx (executable:0x%llx)\n",
+			ctxp->uc_mcontext.gregs[REG_RIP],
+			ctxp->uc_mcontext.gregs[REG_RIP] -
+				(size_t)__image_base);
+		return;
+	}
+
+	unw_getcontext(&uc);
+	unw_init_local(&cursor, &uc);
+	while (unw_step(&cursor) > 0 &&
+	       test3_getcontext(&cursor, &pc) != test3_ctx.ip)
+		;
+
+	if (memcmp(&pc, &test3_ctx, sizeof(pc))) {
+		fprintf(stderr, ERRLN ": Error unwinding\n");
+		exit(1);
+	}
+}
+
+__attribute__((noinline))
+static void test3_test_unwind(void (*f)(struct sgx_secs *),
+			      struct sgx_secs *secs)
+{
+	test3_caller = __builtin_return_address(0);
+	__asm__ ("pushfq; orl %0, (%%rsp); popfq" : : "i"(X86_EFLAGS_TF));
+	f(secs);
+}
+
+/*
+ * test3() single-steps through the vDSO API to test out CFI directives inside
+ * the API.
+ */
+static void test3(struct sgx_secs *secs)
+{
+	unw_cursor_t cursor;
+	unw_context_t uc;
+	struct sigaction sa = {
+		.sa_sigaction = (void (*)(int, siginfo_t*, void*))test3_sigtrap,
+		.sa_flags = SA_SIGINFO,
+	};
+
+	unw_getcontext(&uc);
+	unw_init_local(&cursor, &uc);
+	if (unw_step(&cursor) > 0)
+		test3_getcontext(&cursor, &test3_ctx);
+	else {
+		fprintf(stderr, ERRLN ": error initializing unwind context\n");
+		exit(1);
+	}
+
+	if (sigaction(SIGTRAP, &sa, NULL) < 0) {
+		perror(ERRLN);
+		exit(1);
+	}
+
+	test3_test_unwind(test1, secs);
+	test3_test_unwind(test2, secs);
+}
+
+int main(void)
 {
 	unsigned long bin_size = encl_bin_end - encl_bin;
 	unsigned long ss_size = encl_ss_end - encl_ss;
-	struct sgx_enclave_exception exception;
 	Elf64_Sym *eenter_sym;
 	struct vdso_symtab symtab;
 	struct sgx_secs secs;
-	uint64_t result = 0;
-	void *eenter;
-	void *addr;
-
-	memset(&exception, 0, sizeof(exception));
 
-	addr = vdso_get_base_addr(envp);
-	if (!addr)
-		exit(1);
+	vdso_init();
 
-	if (!vdso_get_symtab(addr, &symtab))
+	if (!vdso_get_symtab(&symtab))
 		exit(1);
 
 	eenter_sym = vdso_symtab_get(&symtab, "__vdso_sgx_enter_enclave");
 	if (!eenter_sym)
 		exit(1);
-	eenter = addr + eenter_sym->st_value;
+	eenter = vdso_base + eenter_sym->st_value;
 
 	printf("Binary size %lu (0x%lx), SIGSTRUCT size %lu\n", bin_size,
 	       bin_size, ss_size);
@@ -266,14 +531,11 @@ int main(int argc, char *argv[], char *envp[])
 	if (!encl_load(&secs, bin_size))
 		exit(1);
 
-	printf("Input: 0x%lx\n", MAGIC);
-	sgx_call((void *)&MAGIC, &result, (void *)secs.base, &exception,
-		 eenter);
-	if (result != MAGIC) {
-		fprintf(stderr, "0x%lx != 0x%lx\n", result, MAGIC);
-		exit(1);
-	}
+	printf("--- Functional Tests ---\n");
+	test1(&secs);
+	test2(&secs);
 
-	printf("Output: 0x%lx\n", result);
-	exit(0);
+	printf("--- Unwind Tests ---\n");
+	test3(&secs);
+	return 0;
 }
diff --git a/tools/testing/selftests/x86/sgx/sgx_call.S b/tools/testing/selftests/x86/sgx/sgx_call.S
index 14bd0a044199..ca2c0c947758 100644
--- a/tools/testing/selftests/x86/sgx/sgx_call.S
+++ b/tools/testing/selftests/x86/sgx/sgx_call.S
@@ -7,9 +7,43 @@
 
 	.global sgx_call
 sgx_call:
+	.cfi_startproc
+	push	%r15
+	.cfi_adjust_cfa_offset	8
+	.cfi_rel_offset		%r15, 0
+	push	%r14
+	.cfi_adjust_cfa_offset	8
+	.cfi_rel_offset		%r14, 0
+	push	%r13
+	.cfi_adjust_cfa_offset	8
+	.cfi_rel_offset		%r13, 0
+	push	%r12
+	.cfi_adjust_cfa_offset	8
+	.cfi_rel_offset		%r12, 0
 	push	%rbx
-	mov	$0x02, %rax
-	mov	%rdx, %rbx
-	call	*%r8
+	.cfi_adjust_cfa_offset	8
+	.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)
+	.cfi_adjust_cfa_offset	8
+	mov	$2, %eax
+	call	*eenter(%rip)
+	add	$0x20, %rsp
+	.cfi_adjust_cfa_offset	-0x20
 	pop	%rbx
+	.cfi_adjust_cfa_offset	-8
+	pop	%r12
+	.cfi_adjust_cfa_offset	-8
+	pop	%r13
+	.cfi_adjust_cfa_offset	-8
+	pop	%r14
+	.cfi_adjust_cfa_offset	-8
+	pop	%r15
+	.cfi_adjust_cfa_offset	-8
 	ret
+	.cfi_endproc
-- 
2.17.1

Re: [RFC PATCH v4 2/3] x86/vdso: Modify __vdso_sgx_enter_enclave() to allow parameter passing on untrusted stack

[Jarkko Sakkinen]

On Fri, 2019-07-12 at 23:51 -0700, Cedric Xing wrote:
> The previous __vdso_sgx_enter_enclave() requires enclaves to preserve %rsp,
> which prohibits enclaves from allocating and passing parameters for
> untrusted function calls (aka. o-calls) on the untrusted stack.
> 
> This patch addresses the problem above by introducing a new ABI that preserves
> %rbp instead of %rsp. Then __vdso_sgx_enter_enclave() can anchor its frame
> using %rbp so that enclaves are allowed to allocate space on the untrusted
> stack by decrementing %rsp. Please note that the stack space allocated in such
> way will be part of __vdso_sgx_enter_enclave()'s frame so will be freed after
> __vdso_sgx_enter_enclave() returns. Therefore, __vdso_sgx_enter_enclave() has
> been revised to take a callback function as an optional parameter, which if
> supplied, will be invoked upon enclave exits (both AEX (Asynchronous Enclave
> eXit) and normal exits), with the value of %rsp left off by the enclave as a
> parameter to the callback.
> 
> Here's the summary of API/ABI changes in this patch. More details could be
> found in arch/x86/entry/vdso/vsgx_enter_enclave.S.
>   * 'struct sgx_enclave_exception' is renamed to 'struct sgx_enclave_exinfo'
>     because it is filled upon both AEX (i.e. exceptions) and normal enclave
>     exits.
>   * __vdso_sgx_enter_enclave() anchors its frame using %rbp (instead of %rsp in
>     the previous implementation).
>   * __vdso_sgx_enter_enclave() takes one more parameter - a callback function
>     to be invoked upon enclave exits. This callback is optional, and if not
>     supplied, will cause __vdso_sgx_enter_enclave() to return upon enclave
>     exits (same behavior as previous implementation).
>   * The callback function is given as a parameter the value of %rsp at enclave
>     exit to address data "pushed" by the enclave. A positive value returned by
>     the callback will be treated as an ENCLU leaf for re-entering the enclave,
>     while a zero or negative value will be passed through as the return
>     value of __vdso_sgx_enter_enclave() to its caller. It's also safe to
>     leave callback by longjmp() or by throwing a C++ exception.
> 
> Signed-off-by: Cedric Xing <cedric.xing@intel.com>
> ---
>  arch/x86/entry/vdso/vsgx_enter_enclave.S | 310 +++++++++++++++++------
>  arch/x86/include/uapi/asm/sgx.h          |  14 +-
>  2 files changed, 242 insertions(+), 82 deletions(-)
> 
> diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.S b/arch/x86/entry/vdso/vsgx_enter_enclave.S
> index fe0bf6671d6d..a96542ba6945 100644
> --- a/arch/x86/entry/vdso/vsgx_enter_enclave.S
> +++ b/arch/x86/entry/vdso/vsgx_enter_enclave.S
> @@ -6,96 +6,256 @@
>  
>  #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
> +#define EX_LEAF     0*8
> +#define EX_TRAPNR   0*8+4
> +#define EX_ERROR_CODE   0*8+6
> +#define EX_ADDRESS  1*8

A completely new diff that should not exist in this version.

>  
>  .code64
>  .section .text, "ax"
>  
>  #ifdef SGX_KERNEL_DOC
>  /**
> - * __vdso_sgx_enter_enclave() - Enter an SGX enclave
> + * typedef sgx_ex_callback - Callback function for __vdso_sgx_enter_enclave()
>   *
> - * @leaf:	**IN \%eax** - ENCLU leaf, must be EENTER or ERESUME
> - * @tcs:	**IN \%rbx** - TCS, must be non-NULL
> - * @ex_info:	**IN \%rcx** - Optional 'struct sgx_enclave_exception' pointer
> + * @rdi:    value of %%rdi register at enclave exit
> + * @rsi:    value of %%rsi register at enclave exit
> + * @rdx:    value of %%rdx register at enclave exit
> + * @exinfo: pointer to a sgx_enclave_exinfo structure, which was passed to
> + *      __vdso_sgx_enter_enclave() as input
> + * @r8:     value of %%r8 register at enclave exit
> + * @r9:     value of %%r9 register at enclave exit
> + * @tcs:    TCS used by __vdso_sgx_enter_enclave() to enter the enclave,
> + *      could be used to re-enter the
> + *      enclave
> + * @ursp:   value of %%rsp register at enclave exit
> + *
> + * This is the callback function to be invoked upon enclave exits, including
> + * normal exits (as result of EEXIT), and asynchronous exits (AEX) due to
> + * exceptions occurred at EENTER or within the enclave.
> + *
> + * This callback is expected to follow x86_64 ABI.
>   *
>   * Return:
> - *  **OUT \%eax** -
> - *  %0 on a clean entry/exit to/from the enclave, %-EINVAL if ENCLU leaf is
> - *  not allowed or if TCS is NULL, %-EFAULT if ENCLU or the enclave faults
> - *
> - * **Important!**  __vdso_sgx_enter_enclave() is **NOT** compliant with the
> - * x86-64 ABI, i.e. cannot be called from standard C code.   As noted above,
> - * input parameters must be passed via ``%eax``, ``%rbx`` and ``%rcx``, with
> - * the return value passed via ``%eax``.  All registers except ``%rsp`` must
> - * be treated as volatile from the caller's perspective, including but not
> - * limited to GPRs, EFLAGS.DF, MXCSR, FCW, etc...  Conversely, the enclave
> - * being run **must** preserve the untrusted ``%rsp`` and stack.
> + *
> + * EENTER(2) - causes __vdso_sgx_enter_enclave() to issue ENCLU[EENTER] on the
> + * same TCS. All GPRs left off by this callback function will be passed through
> + * back to the enclave, except %%rax, %%rbx and %%rcx, which are clobbered by
> + * ENCLU[EENTER] instruction.
> + *
> + * ERESUME(3) - causes __vdso_sgx_enter_enclave() to issue ENCLU[ERESUME] on
> + * the same TCS.
> + *
> + * 0 (zero) or negative returned values will be returned back to
> + * __vdso_sgx_enter_enclave()'s caller as is.
> + *
> + * All other values will cause -EINVAL to be returned to
> + * __vdso_sgx_enter_enclave()'s caller.
> + *
> + * Note: All general purpose registers (GPRs) left off by the enclave are
> + * passed through to this function, except %%rax, %%rbx and %%rcx, which are
> + * used internally by __vdso_sgx_enter_enclave(). Some of those registers are
> + * accessible as function parameters (i.e. @rdi, @rsi, @rdx, @r8, @r9 and
> + * @ursp), while others can be accessed only from assembly code.
>   */
> -__vdso_sgx_enter_enclave(u32 leaf, void *tcs,
> -			 struct sgx_enclave_exception *ex_info)
> -{
> -	if (leaf != SGX_EENTER && leaf != SGX_ERESUME)
> -		return -EINVAL;
> +typedef int sgx_ex_callback(long rdi, long rsi, long rdx,
> +                struct sgx_enclave_exinfo *exinfo,
> +                long r8, long r9, void *tcs, long ursp);
>  
> -	if (!tcs)
> -		return -EINVAL;
> +/**
> + * __vdso_sgx_enter_enclave() - Enter an SGX enclave and capture exceptions
> + *
> + * @leaf:
> + *  passed in %%eax, must be either EENTER(2) or ERESUME(3)
> + * @tcs:
> + *  passed on stack at 8(%%rsp), is the linear address of TCS
> + * @exinfo:
> + *  passed on stack at 0x10(%%rsp), optional, and if non-NULL, shall point
> + *  to an sgx_enclave_exinfo structure to receive information about the
> + *  enclave exit
> + * @callback:
> + *  passed on stack at 0x18(%%rsp), optional, and if non-NULL, points to a
> + *  callback function to be invoked at enclave exits
> + *
> + * __vdso_sgx_enter_enclave() issues either ENCLU[EENTER] or ENCLU[ERESUME] on
> + * @tcs depending on @leaf.
> + *
> + * IMPORTANT! This API is not compliant with x86-64 ABI but adopts a
> + * proprietary calling convention. Please see NOTES section below for details.
> + *
> + * On an enclave exit, @exinfo->leaf will be set to the ENCLU leaf at exit, if
> + * @exinfo is not NULL. That is, @exinfo->leaf may be one of the following:
> + *
> + *   * EEXIT:   Normal exit due to ENCLU[EEXIT] within the enclave. All other
> + *      members will remain intact.
> + *
> + *   * ERESUME: Asynchronous exit due to exceptions within the enclave.
> + *      @exinfo->trapnr, @exinfo->error_code and @exinfo->address are
> + *      set to the trap number, error code and fault address,
> + *      respectively.
> + *
> + *   * EENTER:  Exception occurred when trying to enter the enclave.
> + *      @exinfo->trapnr, @exinfo->error_code and @exinfo->address are
> + *      set to the trap number, error code and fault address,
> + *      accordingly.
> + *
> + * If @callback is NULL, 0 (zero) is returned if the enclave has been entered
> + * and exited normally, or -EFAULT if any exception has occurred, or -EINVAL if
> + * @leaf on input is neither EENTER or ERESUME.
> + *
> + * If @callback is not NULL, it is invoked at enclave exit, and then actions
> + * will be taken depending on its return value - i.e. positive value will be
> + * treated as ENCLU leaf to re-enter the enclave, while 0 (zero) or negative
> + * values will be returned back to the caller as is. Unrecognized leaf values
> + * will cause -EINVAL to be returned.
> + *
> + * Return:
> + *
> + * 0 (zero) is returned on a successful entry and normal exit from the enclave.
> + *
> + * -EINVAL is returned if @leaf is neither EENTER nor ERESUME, or if @callback
> + * is not NULL and returns a positive value that is neither EENTER nor ERESUME
> + * after the enclave exits.
> + *
> + * -EFAULT is returned if an exception has occurred at EENTER or during
> + * execution of the enclave and @callback is NULL, or if @callback is not NULL
> + * and it returns -EFAULT after the enclave exits.
> + *
> + * Other values may be returned as the return value from @callback if it is not
> + * NULL.
> + *
> + * Note: __vdso_sgx_enter_enclave() adopts a proprietary calling convention,
> + * described below:
> + *
> + *    * As noted above, input parameters are passed via %%eax and the stack.
> + *
> + *    * %%rbx and %%rcx must be treated as volatile as they are modified as part
> + *  of enclaves transitions and are used as scratch regs.
> + *
> + *    * %%rdx, %%rdi, %%rsi and %%r8-%%r15 are passed as is and may be freely
> + *  modified by the enclave. Values left in those registers will not be
> + *  altered either, so will be visiable to the callback or the caller (if no
> + *  callback is specified).
> + *
> + *    * %%rsp could be decremented by the enclave to allocate temporary space on
> + *      the untrusted stack. Temporary space allocated this way is retained in
> + *      the context of @callback, and will be freed (i.e. %%rsp will be
> + *      restored) before __vdso_sgx_enter_enclave() returns.
> + */
> +int __vdso_sgx_enter_enclave(int leaf, void *tcs,
> +                 struct sgx_enclave_exinfo *exinfo,
> +                 sgx_ex_callback *callback);
> +{
> +     while (leaf == EENTER || leaf == ERESUME) {
> +    int rc;
> +    try {
> +        ENCLU[leaf];
> +        rc = 0;
> +        if (exinfo)
> +            exinfo->leaf = EEXIT;
> +    } catch (exception) {
> +        rc = -EFAULT;
> +        if (exinfo)
> +            *exinfo = exception;
> +    }
>  
> -	try {
> -		ENCLU[leaf];
> -	} catch (exception) {
> -		if (e)
> -			*e = exception;
> -		return -EFAULT;
> -	}
> +    leaf = !callback ? rc: (*callback)(rdi, rsi, rdx, exinfo,
> +                       r8, r9, tcs, ursp);
> +     }
>  
> -	return 0;
> +     return leaf > 0 ? -EINVAL : leaf;
>  }
>  #endif
> +
>  ENTRY(__vdso_sgx_enter_enclave)
> -	/* EENTER <= leaf <= ERESUME */
> -	cmp	$0x2, %eax
> -	jb	bad_input
> -
> -	cmp	$0x3, %eax
> -	ja	bad_input
> -
> -	/* TCS must be non-NULL */
> -	test	%rbx, %rbx
> -	je	bad_input
> -
> -	/* Save @exception_info */
> -	push	%rcx
> -
> -	/* Load AEP for ENCLU */
> -	lea	1f(%rip),  %rcx
> -1:	enclu
> -
> -	add	$0x8, %rsp
> -	xor	%eax, %eax
> -	ret
> -
> -bad_input:
> -	mov     $(-EINVAL), %rax
> -	ret
> -
> -.pushsection .fixup, "ax"
> -	/* Re-load @exception_info and fill it (if it's non-NULL) */
> -2:	pop	%rcx
> -	test    %rcx, %rcx
> -	je      3f
> -
> -	mov	%eax, EX_LEAF(%rcx)
> -	mov	%di,  EX_TRAPNR(%rcx)
> -	mov	%si,  EX_ERROR_CODE(%rcx)
> -	mov	%rdx, EX_ADDRESS(%rcx)
> -3:	mov	$(-EFAULT), %rax
> -	ret
> -.popsection
> -
> -_ASM_VDSO_EXTABLE_HANDLE(1b, 2b)
> +    /* Prolog */
> +    .cfi_startproc
> +    push    %rbp
> +    .cfi_adjust_cfa_offset  8
> +    .cfi_rel_offset     %rbp, 0
> +    mov %rsp, %rbp
> +    .cfi_def_cfa_register   %rbp
> +
> +1:  /* EENTER <= leaf <= ERESUME */
> +    cmp $0x2, %eax
> +    jb  6f
> +    cmp $0x3, %eax
> +    ja  6f
> +
> +    /* Load TCS and AEP */
> +    mov 0x10(%rbp), %rbx
> +    lea 2f(%rip), %rcx
> +
> +    /* Single ENCLU serving as both EENTER and AEP (ERESUME) */
> +2:  enclu
> +
> +    /* EEXIT path */
> +    xor %ebx, %ebx
> +3:  mov 0x18(%rbp), %rcx
> +    jrcxz   4f
> +    mov %eax, EX_LEAF(%rcx)
> +    jnc 4f
> +    mov %di, EX_TRAPNR(%rcx)
> +    mov %si, EX_ERROR_CODE(%rcx)
> +    mov %rdx, EX_ADDRESS(%rcx)
> +
> +4:  /* Call *callback if supplied */
> +    mov 0x20(%rbp), %rax
> +    test    %rax, %rax
> +    /*
> +     * At this point, %ebx holds the effective return value, which shall be
> +     * returned if no callback is specified
> +     */
> +    cmovz   %rbx, %rax
> +    jz  7f
> +    /*
> +     * Align stack per x86_64 ABI. The original %rsp is saved in %rbx to be
> +     * restored after *callback returns.
> +     */
> +    mov %rsp, %rbx
> +    and $-0x10, %rsp
> +    /* Clear RFLAGS.DF per x86_64 ABI */
> +    cld
> +    /* Parameters for *callback */
> +    push    %rbx
> +    push    0x10(%rbp)
> +    /* Call *%rax via retpoline */
> +    call    40f
> +    /*
> +     * Restore %rsp to its original value left off by the enclave from last
> +     * exit
> +     */
> +    mov %rbx, %rsp
> +    /*
> +     * Positive return value from *callback will be interpreted as an ENCLU
> +     * leaf, while a non-positive value will be interpreted as the return
> +     * value to be passed back to the caller.
> +     */
> +    jmp 1b
> +40: /* retpoline */
> +    call    42f
> +41: pause
> +    lfence
> +    jmp 41b
> +42: mov %rax, (%rsp)
> +    ret
> +
> +5:  /* Exception path */
> +    mov $-EFAULT, %ebx
> +    stc
> +    jmp 3b
> +
> +6:  /* Unsupported ENCLU leaf */
> +    cmp $0, %eax
> +    jle 7f
> +    mov $-EINVAL, %eax
> +
> +7:  /* Epilog */
> +    leave
> +    .cfi_def_cfa        %rsp, 8
> +    ret
> +    .cfi_endproc
> +
> +_ASM_VDSO_EXTABLE_HANDLE(2b, 5b)
>  
>  ENDPROC(__vdso_sgx_enter_enclave)
> diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
> index 9ed690a38c70..50d2b5143e5e 100644
> --- a/arch/x86/include/uapi/asm/sgx.h
> +++ b/arch/x86/include/uapi/asm/sgx.h
> @@ -24,7 +24,7 @@
>  
>  /**
>   * struct sgx_enclave_create - parameter structure for the
> - *                             %SGX_IOC_ENCLAVE_CREATE ioctl
> + *			       %SGX_IOC_ENCLAVE_CREATE ioctl

Cruft. Please do not change files if there is:

1. No reason to do it (holds for this).
2. No relation to the patch (also holds for this).

If only (2) holds, create a patch with its own commit message etc.

This is also explained in the kernel process guide. I earlier linked
that.

>   * @src:	address for the SECS page data
>   */
>  struct sgx_enclave_create  {
> @@ -33,7 +33,7 @@ struct sgx_enclave_create  {
>  
>  /**
>   * struct sgx_enclave_add_page - parameter structure for the
> - *                               %SGX_IOC_ENCLAVE_ADD_PAGE ioctl
> + *				 %SGX_IOC_ENCLAVE_ADD_PAGE ioctl

Ditto.

>   * @addr:	address within the ELRANGE
>   * @src:	address for the page data
>   * @secinfo:	address for the SECINFO data
> @@ -49,7 +49,7 @@ struct sgx_enclave_add_page {
>  
>  /**
>   * struct sgx_enclave_init - parameter structure for the
> - *                           %SGX_IOC_ENCLAVE_INIT ioctl
> + *			     %SGX_IOC_ENCLAVE_INIT ioctl

Ditto.

>   * @sigstruct:	address for the SIGSTRUCT data
>   */
>  struct sgx_enclave_init {
> @@ -66,16 +66,16 @@ struct sgx_enclave_set_attribute {
>  };
>  
>  /**
> - * struct sgx_enclave_exception - structure to report exceptions encountered in
> - *				  __vdso_sgx_enter_enclave()
> + * struct sgx_enclave_exinfo - structure to report exceptions encountered in
> + *			       __vdso_sgx_enter_enclave()
>   *
> - * @leaf:	ENCLU leaf from \%eax at time of exception
> + * @leaf:	ENCLU leaf from \%eax at time of exception/exit
>   * @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 {
> +struct sgx_enclave_exinfo {

Ditto.

>  	__u32 leaf;
>  	__u16 trapnr;
>  	__u16 error_code;

I already manually removed those changes already from previous version.

/Jarkko

Re: [RFC PATCH v4 2/3] x86/vdso: Modify __vdso_sgx_enter_enclave() to allow parameter passing on untrusted stack

[Jarkko Sakkinen]

On Sat, 2019-07-13 at 18:04 +0300, Jarkko Sakkinen wrote:
> I already manually removed those changes already from previous version.

I wonder why this was posted anyway given that there was no
improvement. Anyway, I'll use the previous version.

/Jarkko 

Re: [RFC PATCH v4 1/3] selftests/x86/sgx: Fix Makefile for SGX selftest

[Jarkko Sakkinen]

On Fri, 2019-07-12 at 23:51 -0700, Cedric Xing wrote:
> The original x86/sgx/Makefile didn't work when "x86/sgx" was specified as the
> test target, nor did it work with "run_tests" as the make target. Yet another
> problem was that it breaks 32-bit only build. This patch fixes those problems,
> along with adjustments to compiler/linker options and simplifications to the
> build rules.
> 
> Signed-off-by: Cedric Xing <cedric.xing@intel.com>

You must split this in quite a few separate patches:

1. One for each fix.
2. At least one patch for each change in compiler and linker options with a
   commit message clearly expalaining why the change was made.
3. One for each simplification.

We don't support 32-bit build:

config INTEL_SGX
	bool "Intel SGX core functionality"
	depends on X86_64 && CPU_SUP_INTEL
	
/Jarkko

Re: [RFC PATCH v4 1/3] selftests/x86/sgx: Fix Makefile for SGX selftest

[Jarkko Sakkinen]

On Sat, 2019-07-13 at 18:10 +0300, Jarkko Sakkinen wrote:
> On Fri, 2019-07-12 at 23:51 -0700, Cedric Xing wrote:
> > The original x86/sgx/Makefile didn't work when "x86/sgx" was specified as the
> > test target, nor did it work with "run_tests" as the make target. Yet another
> > problem was that it breaks 32-bit only build. This patch fixes those problems,
> > along with adjustments to compiler/linker options and simplifications to the
> > build rules.
> > 
> > Signed-off-by: Cedric Xing <cedric.xing@intel.com>
> 
> You must split this in quite a few separate patches:
> 
> 1. One for each fix.
> 2. At least one patch for each change in compiler and linker options with a
>    commit message clearly expalaining why the change was made.
> 3. One for each simplification.
> 
> We don't support 32-bit build:
> 
> config INTEL_SGX
> 	bool "Intel SGX core functionality"
> 	depends on X86_64 && CPU_SUP_INTEL

This is not to say that changes suck. This just in "unreviewable" state as far
as the kernel processes go...

/Jarkko

Re: [RFC PATCH v4 1/3] selftests/x86/sgx: Fix Makefile for SGX selftest

[Xing, Cedric]

On 7/13/2019 8:15 AM, Jarkko Sakkinen wrote:
> On Sat, 2019-07-13 at 18:10 +0300, Jarkko Sakkinen wrote:
>> On Fri, 2019-07-12 at 23:51 -0700, Cedric Xing wrote:
>>> The original x86/sgx/Makefile didn't work when "x86/sgx" was specified as the
>>> test target, nor did it work with "run_tests" as the make target. Yet another
>>> problem was that it breaks 32-bit only build. This patch fixes those problems,
>>> along with adjustments to compiler/linker options and simplifications to the
>>> build rules.
>>>
>>> Signed-off-by: Cedric Xing <cedric.xing@intel.com>
>>
>> You must split this in quite a few separate patches:
>>
>> 1. One for each fix.
>> 2. At least one patch for each change in compiler and linker options with a
>>     commit message clearly expalaining why the change was made.
>> 3. One for each simplification.
>>
>> We don't support 32-bit build:
>>
>> config INTEL_SGX
>> 	bool "Intel SGX core functionality"
>> 	depends on X86_64 && CPU_SUP_INTEL
> 
> This is not to say that changes suck. This just in "unreviewable" state as far
> as the kernel processes go...

Please note that your patchset hasn't been upstreamed yet. Your Makefile 
is problematic to begin with. Technically it's your job to make it work 
before sending out any patches. You didn't explain what's done for each 
line of Makefile in your commit message either.

Not saying documentation is unimportant, but the purposes for those 
changes are obvious and easy to understand for anyone having reasonable 
knowledge on how Makefile works.

I'm totally fine not fixing the Makefile. You can just leave them out.

> /Jarkko
> 

Re: [RFC PATCH v4 1/3] selftests/x86/sgx: Fix Makefile for SGX selftest

[Jarkko Sakkinen]

On Sat, Jul 13, 2019 at 10:29:12AM -0700, Xing, Cedric wrote:
> Please note that your patchset hasn't been upstreamed yet. Your Makefile is
> problematic to begin with. Technically it's your job to make it work before
> sending out any patches. You didn't explain what's done for each line of
> Makefile in your commit message either.

Yes, it is different case to do the initial version of the whole thing
that suggest fixes to it. The latter needs to have more granularity.
Bug fixes in any type of software development should be isolated to
separate change sets. It is just a sane QA practice.

> Not saying documentation is unimportant, but the purposes for those changes
> are obvious and easy to understand for anyone having reasonable knowledge on
> how Makefile works.
>
> I'm totally fine not fixing the Makefile. You can just leave them out.

/Jarkko