[PATCH for_v29 0/8] x86/sgx: Make vDSO callable from C

[PATCH for_v29 0/8] x86/sgx: Make vDSO callable from C

[Sean Christopherson]

Nathaniel pointed out that __vdso_sgx_enter_enclave() is tantalizingly
close to being callable from C (with caveats and a cooperative enclave).
The missing pieces are preserving %rbx and taking @leaf as a standard
paramter (hey look, %rcx is available!).

As requested, update the selftest to invoke the vDSO from C, and fix
bugs along the way.  Don't suppose you've changed your mind about waiting
until after upstreaming to add a proper framework?

I also did a lot more testing on my internal code base to verify the
%rsp tweaks play nice with the exit handler, and to prove that an exit
handler can even cleanly handle exceptions in the enclave.

Here's the relevant code from my internal test suite, might want to have
eye bleach ready ;-).  I tested the exception handling by overwriting
@ms in enter_enclave() with a bogus value, e.g. 0xdeadull << 0x48.  The
nested call to the vDSO (in the exit handler) with EMCD_EXCEPTION has the
enclave dump its register.  The host (this runs as cgo in a Golang process)
dumps the exception info.

I also verified the relative %rsp fixup by corrupting %rsp in the enclave,
verifying it exploded as expected (I added extra consumption in the vDSO to
force this) and then adding code to undo the damage in the exit handler and
verifying things got back to normal.

Last thread of thought, IMO taking a context param (as suggested by
Nathaniel) is unnecessary.  As shown below, the runtime effectively has a
context param in the form of "struct sgx_enclave_exception *e", the handler
just needs to be willing to cast (or use container_of()).  There are a
multitude of other ways to pass info as well.

vdso_sgx_enter_enclave_t __enter_enclave;

vDSO::vDSO()
{
    if (__enter_enclave)
        abort();

    uintptr_t vdso = (uintptr_t)getauxval(AT_SYSINFO_EHDR);
    vdso_init_from_sysinfo_ehdr(vdso);

    __enter_enclave = (vdso_sgx_enter_enclave_t)vdso_sym("LINUX_2.6", "__vdso_sgx_enter_enclave");
    if (!__enter_enclave)
        abort();
}

struct sgx_vdso_ctxt {
    sgx_exception_t e;
    long            ret;
    Enclave *       enclave;
    jmp_buf         buf;
};

static int exit_handler(long rdi, long rsi, long rdx, long ursp, long r8, long r9,
                        void *tcs, int ret, struct sgx_enclave_exception *e)
{
    struct sgx_vdso_ctxt *ctxt = (struct sgx_vdso_ctxt *)e;
    int r;

    if (!ret) {
        if (ctxt)
            ctxt->ret = rdi;
        return 0;
    }

    if (!ctxt)
        exit(EFAULT);

    r = __enter_enclave(ECMD_EXCEPTION, (unsigned long)&ctxt->e.regs, 0, EENTER,
                        0, 0, tcs, NULL, exit_handler);
    if (r)
        exit(-r);

    ctxt->enclave->set_exception(ctxt->e);

    longjmp(ctxt->buf, ret);
}

sgx_status_t vDSO::enter_enclave(Enclave &enclave, const long fn, void *ms, tcs_t *tcs)
{
    struct sgx_vdso_ctxt ctxt;
    ctxt.enclave = &enclave;

    int ret = setjmp(ctxt.buf);
    if (ret) {
        if (ret != -EFAULT)
            return SGX_ERROR_INVALID_PARAMETER;

        SE_URTS_ERROR("\nEnclave exception, base = %lx, size = %lx",
                      enclave.get_base(), enclave.get_size());

        return SGX_ERROR_UNHANDLED_EXCEPTION;
    }

    ret = __enter_enclave(fn, (unsigned long)ms, 0, EENTER, 0, 0, tcs,
                          &ctxt.e.fault, exit_handler);
    if (ret == -EINVAL)
        return SGX_ERROR_INVALID_PARAMETER;

    return (sgx_status_t)ctxt.ret;
}

Sean Christopherson (8):
  x86/sgx: vdso: Remove an incorrect statement the enter enclave comment
  x86/sgx: vdso: Make the %rsp fixup on return from handler relative
  x86/sgx: vdso: Make __vdso_sgx_enter_enclave() callable from C code
  x86/sgx: vdso: Define a typedef for __vdso_sgx_enter_enclave
  selftests/x86: sgx: Zero out @result before invoking vDSO sub-test
  selftests/x86: sgx: Pass EENTER to vDSO wrapper instead of hardcoding
  selftests/x86: sgx: Stop clobbering non-volatile registers
  selftests/x86: Add selftest to invoke __vsgx_enter_enclave() from C

 arch/x86/entry/vdso/vsgx_enter_enclave.S      | 72 ++-----------------
 arch/x86/include/uapi/asm/sgx.h               | 61 ++++++++++++++++
 .../selftests/x86/sgx/encl_bootstrap.S        |  6 +-
 tools/testing/selftests/x86/sgx/main.c        | 17 ++++-
 tools/testing/selftests/x86/sgx/sgx_call.S    |  1 -
 tools/testing/selftests/x86/sgx/sgx_call.h    |  2 +-
 6 files changed, 85 insertions(+), 74 deletions(-)

-- 
2.24.1

[PATCH for_v29 1/8] x86/sgx: vdso: Remove an incorrect statement the enter enclave comment

[Sean Christopherson]

Remove the statement about the userspace exit handler being required to
clean up the untrusted stack, the vDSO unconditionally restores %rsp to
its value at the time of EEXIT.

Reported-by: Nathaniel McCallum <npmccallum@redhat.com>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/entry/vdso/vsgx_enter_enclave.S | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.S b/arch/x86/entry/vdso/vsgx_enter_enclave.S
index 94a8e5f99961..22a22e0774d8 100644
--- a/arch/x86/entry/vdso/vsgx_enter_enclave.S
+++ b/arch/x86/entry/vdso/vsgx_enter_enclave.S
@@ -59,10 +59,8 @@
  *   0:		success, return @ret to the caller
  *  <0:		error, return @ret to the caller
  *
- * The userspace exit handler is responsible for unwinding the stack, e.g. to
- * pop @e, u_rsp and @tcs, prior to returning to __vdso_sgx_enter_enclave().
- * The exit handler may also transfer control, e.g. via longjmp() or a C++
- * exception, without returning to __vdso_sgx_enter_enclave().
+ * The exit handler may transfer control, e.g. via longjmp() or C++ exception,
+ * without returning to __vdso_sgx_enter_enclave().
  *
  * Return:
  *  0 on success,
-- 
2.24.1

[PATCH for_v29 2/8] x86/sgx: vdso: Make the %rsp fixup on return from handler relative

[Sean Christopherson]

Modify the %rsp fixup after returning from the exit handler to be
relative instead of absolute to avoid clobbering any %rsp adjustments
made by the exit handler, e.g. if the exit handler modifies the stack
prior to re-entering the enclave.

Reported-by: Nathaniel McCallum <npmccallum@redhat.com>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---

I'm on the fence as to whether or not this is a good idea.  It's not super
painful, but it's not exactly standard/obvious code.  Part of me thinks
its a bug to not let the exit handler manipulate %rsp, the other part of
me thinks it's straight up crazy :-)

 arch/x86/entry/vdso/vsgx_enter_enclave.S | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.S b/arch/x86/entry/vdso/vsgx_enter_enclave.S
index 22a22e0774d8..14f07d5e47ae 100644
--- a/arch/x86/entry/vdso/vsgx_enter_enclave.S
+++ b/arch/x86/entry/vdso/vsgx_enter_enclave.S
@@ -137,8 +137,9 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave)
 	/* Pass the untrusted RSP (at exit) to the callback via %rcx. */
 	mov	%rsp, %rcx
 
-	/* Save the untrusted RSP in %rbx (non-volatile register). */
+	/* Save the untrusted RSP offset in %rbx (non-volatile register). */
 	mov	%rsp, %rbx
+	and	$0xf, %rbx
 
 	/*
 	 * Align stack per x86_64 ABI. Note, %rsp needs to be 16-byte aligned
@@ -159,8 +160,8 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave)
 	mov	0x20(%rbp), %rax
 	call	.Lretpoline
 
-	/* Restore %rsp to its post-exit value. */
-	mov	%rbx, %rsp
+	/* Undo the post-exit %rsp adjustment. */
+	lea	0x20(%rsp,%rbx), %rsp
 
 	/*
 	 * If the return from callback is zero or negative, return immediately,
-- 
2.24.1

[PATCH for_v29 3/8] x86/sgx: vdso: Make __vdso_sgx_enter_enclave() callable from C code

[Sean Christopherson]

Make __vdso_sgx_enter_enclave() callable from C by preserving %rbx
and taking @leaf in %rcx instead of %rax.  Being able to invoke the vDSO
from C reduces the overhead of runtimes that are tightly coupled with
their enclaves, e.g. that can rely on the enclave to save and restore
non-volatile registers, as the runtime doesn't need an assembly wrapper
to preserve non-volatile registers and/or shuffle stack arguments.

Note, both %rcx and %rbx are consumed by EENTER/ERESUME, i.e. consuming
them doesn't violate the primary tenet of __vdso_sgx_enter_enclave()
that "thou shalt not restrict how information is exchanged between an
enclave and its host process".

Suggested-by: Nathaniel McCallum <npmccallum@redhat.com>
Cc: Cedric Xing <cedric.xing@intel.com>
Cc: Jethro Beekman <jethro@fortanix.com>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: linux-sgx@vger.kernel.org
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---

Cedric, please go to town on this, you're much better at the userspace
stack and debugging interactions, e.g. I don't think the CFA directives
need updating, but my knowledge of that stuff is skeeetchy.

Note, if the previous patch to make the %rsp restoration relative is
dropped, then %rbx should be restored relative to %rbp instead of %rsp.

 arch/x86/entry/vdso/vsgx_enter_enclave.S | 33 ++++++++++++++----------
 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.S b/arch/x86/entry/vdso/vsgx_enter_enclave.S
index 14f07d5e47ae..7a0565476a29 100644
--- a/arch/x86/entry/vdso/vsgx_enter_enclave.S
+++ b/arch/x86/entry/vdso/vsgx_enter_enclave.S
@@ -16,25 +16,25 @@
 
 /**
  * __vdso_sgx_enter_enclave() - Enter an SGX enclave
+ * @rdi:	Pass-through value for RDI
+ * @rsi:	Pass-through value for RSI
+ * @rdx:	Pass-through value for 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
  *
- * **Important!**  __vdso_sgx_enter_enclave() is **NOT** compliant with the
- * x86-64 ABI, i.e. cannot be called from standard C code.
- *
- * Input ABI:
- *  @leaf	%eax
- *  @tcs	8(%rsp)
- *  @e 		0x10(%rsp)
- *  @handler	0x18(%rsp)
- *
- * Output ABI:
- *  @ret	%eax
+ * **Important!**  __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.
  *
  * 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
+ * the enclave.  RAX, RBX and RCX are consumed by EENTER and ERESUME and are
  * loaded with @leaf, asynchronous exit pointer, and @tcs respectively.
  *
  * RBP and the stack are used to anchor __vdso_sgx_enter_enclave() to the
@@ -70,9 +70,11 @@
  */
 #ifdef SGX_KERNEL_DOC
 /* C-style function prototype to coerce kernel-doc into parsing the comment. */
-int __vdso_sgx_enter_enclave(int leaf, void *tcs,
+int __vdso_sgx_enter_enclave(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);
+			     sgx_enclave_exit_handler_t handler)
 #endif
 SYM_FUNC_START(__vdso_sgx_enter_enclave)
 	/* Prolog */
@@ -82,7 +84,9 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave)
 	.cfi_rel_offset		%rbp, 0
 	mov	%rsp, %rbp
 	.cfi_def_cfa_register	%rbp
+	push	%rbx
 
+	mov	%ecx, %eax
 .Lenter_enclave:
 	/* EENTER <= leaf <= ERESUME */
 	cmp	$0x2, %eax
@@ -108,6 +112,7 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave)
 	jne	.Linvoke_userspace_handler
 
 .Lout:
+	pop	%rbx
 	leave
 	.cfi_def_cfa		%rsp, 8
 	ret
-- 
2.24.1

[PATCH for_v29 4/8] x86/sgx: vdso: Define a typedef for __vdso_sgx_enter_enclave

[Sean Christopherson]

Define a typedef for the __vdso_sgx_enter_enclave() prototype and move
the entire function comment from the assembly code to the uAPI header,
dropping the kernel doc hack along the way.

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

diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.S b/arch/x86/entry/vdso/vsgx_enter_enclave.S
index 7a0565476a29..46f088927c1d 100644
--- a/arch/x86/entry/vdso/vsgx_enter_enclave.S
+++ b/arch/x86/entry/vdso/vsgx_enter_enclave.S
@@ -14,68 +14,6 @@
 .code64
 .section .text, "ax"
 
-/**
- * __vdso_sgx_enter_enclave() - Enter an SGX enclave
- * @rdi:	Pass-through value for RDI
- * @rsi:	Pass-through value for RSI
- * @rdx:	Pass-through value for 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
- *
- * **Important!**  __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.
- *
- * 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
- * loaded with @leaf, asynchronous exit pointer, and @tcs respectively.
- *
- * RBP and the stack are used to anchor __vdso_sgx_enter_enclave() to the
- * pre-enclave state, e.g. to retrieve @e and @handler after an enclave exit.
- * All other registers are available for use by the enclave and its runtime,
- * e.g. an enclave can push additional data onto the stack (and modify RSP) to
- * pass information to the optional exit handler (see below).
- *
- * Most exceptions reported on ENCLU, including those that occur within the
- * enclave, are fixed up and reported synchronously instead of being delivered
- * via a standard signal. Debug Exceptions (#DB) and Breakpoints (#BP) are
- * never fixed up and are always delivered via standard signals. On synchrously
- * reported exceptions, -EFAULT is returned and details about the exception are
- * recorded in @e, the optional sgx_enclave_exception struct.
-
- * If an exit handler is provided, the handler will be invoked on synchronous
- * exits from the enclave and for all synchronously reported exceptions. In
- * latter case, @e is filled prior to invoking the handler.
- *
- * The exit handler's return value is interpreted as follows:
- *  >0:		continue, restart __vdso_sgx_enter_enclave() with @ret as @leaf
- *   0:		success, return @ret to the caller
- *  <0:		error, return @ret to the caller
- *
- * The exit handler may transfer control, e.g. via longjmp() or C++ exception,
- * without returning to __vdso_sgx_enter_enclave().
- *
- * Return:
- *  0 on success,
- *  -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
- */
-#ifdef SGX_KERNEL_DOC
-/* C-style function prototype to coerce kernel-doc into parsing the comment. */
-int __vdso_sgx_enter_enclave(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)
-#endif
 SYM_FUNC_START(__vdso_sgx_enter_enclave)
 	/* Prolog */
 	.cfi_startproc
diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
index e196cfd44b70..8ca9ef7ea50a 100644
--- a/arch/x86/include/uapi/asm/sgx.h
+++ b/arch/x86/include/uapi/asm/sgx.h
@@ -111,4 +111,65 @@ typedef int (*sgx_enclave_exit_handler_t)(long rdi, long rsi, long rdx,
 					  void *tcs, int ret,
 					  struct sgx_enclave_exception *e);
 
+/**
+ * __vdso_sgx_enter_enclave() - Enter an SGX enclave
+ * @rdi:	Pass-through value for RDI
+ * @rsi:	Pass-through value for RSI
+ * @rdx:	Pass-through value for 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
+ *
+ * **Important!**  __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.
+ *
+ * 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
+ * loaded with @leaf, asynchronous exit pointer, and @tcs respectively.
+ *
+ * RBP and the stack are used to anchor __vdso_sgx_enter_enclave() to the
+ * pre-enclave state, e.g. to retrieve @e and @handler after an enclave exit.
+ * All other registers are available for use by the enclave and its runtime,
+ * e.g. an enclave can push additional data onto the stack (and modify RSP) to
+ * pass information to the optional exit handler (see below).
+ *
+ * Most exceptions reported on ENCLU, including those that occur within the
+ * enclave, are fixed up and reported synchronously instead of being delivered
+ * via a standard signal. Debug Exceptions (#DB) and Breakpoints (#BP) are
+ * never fixed up and are always delivered via standard signals. On synchrously
+ * reported exceptions, -EFAULT is returned and details about the exception are
+ * recorded in @e, the optional sgx_enclave_exception struct.
+
+ * If an exit handler is provided, the handler will be invoked on synchronous
+ * exits from the enclave and for all synchronously reported exceptions. In
+ * latter case, @e is filled prior to invoking the handler.
+ *
+ * The exit handler's return value is interpreted as follows:
+ *  >0:		continue, restart __vdso_sgx_enter_enclave() with @ret as @leaf
+ *   0:		success, return @ret to the caller
+ *  <0:		error, return @ret to the caller
+ *
+ * The exit handler may transfer control, e.g. via longjmp() or C++ exception,
+ * without returning to __vdso_sgx_enter_enclave().
+ *
+ * Return:
+ *  0 on success,
+ *  -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);
+
 #endif /* _UAPI_ASM_X86_SGX_H */
-- 
2.24.1

[PATCH for_v29 5/8] selftests/x86: sgx: Zero out @result before invoking vDSO sub-test

[Sean Christopherson]

Zero out @result before running the vDSO sub-test, otherwise the vDSO
could fail completely and the selftest would be none the wiser, e.g. it
doesn't explicitly check the return value.

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

diff --git a/tools/testing/selftests/x86/sgx/main.c b/tools/testing/selftests/x86/sgx/main.c
index d97cc3cf0093..c9c37d2bbec8 100644
--- a/tools/testing/selftests/x86/sgx/main.c
+++ b/tools/testing/selftests/x86/sgx/main.c
@@ -366,6 +366,7 @@ int main(int argc, char *argv[], char *envp[])
 
 	printf("Input: 0x%lx\n", MAGIC);
 
+	result = 0;
 	sgx_call_vdso((void *)&MAGIC, &result, 0, NULL, NULL, NULL,
 		      (void *)secs.base, &exception, NULL);
 	if (result != MAGIC) {
-- 
2.24.1

[PATCH for_v29 6/8] selftests/x86: sgx: Pass EENTER to vDSO wrapper instead of hardcoding

[Sean Christopherson]

Pass EENTER to the vDSO wrapper via %ecx instead of hardcoding it to
make the wrapper compatible with the new vDSO calling convention.

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

diff --git a/tools/testing/selftests/x86/sgx/main.c b/tools/testing/selftests/x86/sgx/main.c
index c9c37d2bbec8..9b89946a976b 100644
--- a/tools/testing/selftests/x86/sgx/main.c
+++ b/tools/testing/selftests/x86/sgx/main.c
@@ -367,7 +367,7 @@ int main(int argc, char *argv[], char *envp[])
 	printf("Input: 0x%lx\n", MAGIC);
 
 	result = 0;
-	sgx_call_vdso((void *)&MAGIC, &result, 0, NULL, NULL, NULL,
+	sgx_call_vdso((void *)&MAGIC, &result, 0, 2, NULL, NULL,
 		      (void *)secs.base, &exception, NULL);
 	if (result != MAGIC) {
 		fprintf(stderr, "0x%lx != 0x%lx\n", result, MAGIC);
diff --git a/tools/testing/selftests/x86/sgx/sgx_call.S b/tools/testing/selftests/x86/sgx/sgx_call.S
index e71f44f7a995..79b4c80e4a1c 100644
--- a/tools/testing/selftests/x86/sgx/sgx_call.S
+++ b/tools/testing/selftests/x86/sgx/sgx_call.S
@@ -48,7 +48,6 @@ sgx_call_vdso:
 	.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
diff --git a/tools/testing/selftests/x86/sgx/sgx_call.h b/tools/testing/selftests/x86/sgx/sgx_call.h
index a4072c5ecce7..02f1ef292823 100644
--- a/tools/testing/selftests/x86/sgx/sgx_call.h
+++ b/tools/testing/selftests/x86/sgx/sgx_call.h
@@ -8,7 +8,7 @@
 
 void sgx_call_eenter(void *rdi, void *rsi, void *entry);
 
-int sgx_call_vdso(void *rdi, void *rsi, long rdx, void *rcx, void *r8, void *r9,
+int sgx_call_vdso(void *rdi, void *rsi, long rdx, long rcx, void *r8, void *r9,
 		  void *tcs, struct sgx_enclave_exception *ei, void *cb);
 
 #endif /* SGX_CALL_H */
-- 
2.24.1

[PATCH for_v29 7/8] selftests/x86: sgx: Stop clobbering non-volatile registers

[Sean Christopherson]

Stop clearing non-volatile registers in the enclave's trampoline code,
there are no secrets to preserve, and not clobbering the registers makes
the enclave compatible with calling the vDSO from C.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 tools/testing/selftests/x86/sgx/encl_bootstrap.S | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/tools/testing/selftests/x86/sgx/encl_bootstrap.S b/tools/testing/selftests/x86/sgx/encl_bootstrap.S
index 3a1479f1cdcf..173e76fa6da6 100644
--- a/tools/testing/selftests/x86/sgx/encl_bootstrap.S
+++ b/tools/testing/selftests/x86/sgx/encl_bootstrap.S
@@ -40,7 +40,7 @@ encl_entry:
 
 	pop	%rbx # pop the enclave base address
 
-	# Clear GPRs.
+	/* Clear volatile GPRs, except RAX (EEXIT leaf). */
 	xor     %rcx, %rcx
 	xor     %rdx, %rdx
 	xor     %rdi, %rdi
@@ -49,10 +49,6 @@ encl_entry:
 	xor     %r9, %r9
 	xor     %r10, %r10
 	xor     %r11, %r11
-	xor     %r12, %r12
-	xor     %r13, %r13
-	xor     %r14, %r14
-	xor     %r15, %r15
 
 	# Reset status flags.
 	add     %rdx, %rdx # OF = SF = AF = CF = 0; ZF = PF = 1
-- 
2.24.1

[PATCH for_v29 8/8] selftests/x86: Add selftest to invoke __vsgx_enter_enclave() from C

[Sean Christopherson]

Add a selftest to call __vsgx_enter_enclave() from C.

Suggested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 tools/testing/selftests/x86/sgx/main.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/x86/sgx/main.c b/tools/testing/selftests/x86/sgx/main.c
index 9b89946a976b..bf64fa857983 100644
--- a/tools/testing/selftests/x86/sgx/main.c
+++ b/tools/testing/selftests/x86/sgx/main.c
@@ -21,7 +21,7 @@
 #define PAGE_SIZE  4096
 
 static const uint64_t MAGIC = 0x1122334455667788ULL;
-void *eenter;
+vdso_sgx_enter_enclave_t eenter;
 
 struct vdso_symtab {
 	Elf64_Sym *elf_symtab;
@@ -376,5 +376,17 @@ int main(int argc, char *argv[], char *envp[])
 
 	printf("Output: 0x%lx\n", result);
 
+	printf("Input: 0x%lx\n", MAGIC);
+	result = 0;
+
+	/* Invoke the vDSO directly. */
+	eenter((unsigned long)&MAGIC, (unsigned long)&result, 0, 2, 0, 0,
+	       (void *)secs.base, &exception, NULL);
+	if (result != MAGIC) {
+		fprintf(stderr, "0x%lx != 0x%lx\n", result, MAGIC);
+		exit(1);
+	}
+	printf("Output: 0x%lx\n", result);
+
 	exit(0);
 }
-- 
2.24.1

Re: [PATCH for_v29 3/8] x86/sgx: vdso: Make __vdso_sgx_enter_enclave() callable from C code

[Xing, Cedric]

On 3/18/2020 6:11 PM, Sean Christopherson wrote:
> Make __vdso_sgx_enter_enclave() callable from C by preserving %rbx
> and taking @leaf in %rcx instead of %rax.  Being able to invoke the vDSO
> from C reduces the overhead of runtimes that are tightly coupled with
> their enclaves, e.g. that can rely on the enclave to save and restore
> non-volatile registers, as the runtime doesn't need an assembly wrapper
> to preserve non-volatile registers and/or shuffle stack arguments.
> 
> Note, both %rcx and %rbx are consumed by EENTER/ERESUME, i.e. consuming
> them doesn't violate the primary tenet of __vdso_sgx_enter_enclave()
> that "thou shalt not restrict how information is exchanged between an
> enclave and its host process".
> 
> Suggested-by: Nathaniel McCallum <npmccallum@redhat.com>
> Cc: Cedric Xing <cedric.xing@intel.com>
> Cc: Jethro Beekman <jethro@fortanix.com>
> Cc: Andy Lutomirski <luto@amacapital.net>
> Cc: linux-sgx@vger.kernel.org
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
> 
> Cedric, please go to town on this, you're much better at the userspace
> stack and debugging interactions, e.g. I don't think the CFA directives
> need updating, but my knowledge of that stuff is skeeetchy.
> 
> Note, if the previous patch to make the %rsp restoration relative is
> dropped, then %rbx should be restored relative to %rbp instead of %rsp.
> 
>   arch/x86/entry/vdso/vsgx_enter_enclave.S | 33 ++++++++++++++----------
>   1 file changed, 19 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.S b/arch/x86/entry/vdso/vsgx_enter_enclave.S
> index 14f07d5e47ae..7a0565476a29 100644
> --- a/arch/x86/entry/vdso/vsgx_enter_enclave.S
> +++ b/arch/x86/entry/vdso/vsgx_enter_enclave.S
> @@ -16,25 +16,25 @@
>   
>   /**
>    * __vdso_sgx_enter_enclave() - Enter an SGX enclave
> + * @rdi:	Pass-through value for RDI
> + * @rsi:	Pass-through value for RSI
> + * @rdx:	Pass-through value for 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
>    *
> - * **Important!**  __vdso_sgx_enter_enclave() is **NOT** compliant with the
> - * x86-64 ABI, i.e. cannot be called from standard C code.
> - *
> - * Input ABI:
> - *  @leaf	%eax
> - *  @tcs	8(%rsp)
> - *  @e 		0x10(%rsp)
> - *  @handler	0x18(%rsp)
> - *
> - * Output ABI:
> - *  @ret	%eax
> + * **Important!**  __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.
>    *
>    * 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
> + * the enclave.  RAX, RBX and RCX are consumed by EENTER and ERESUME and are
>    * loaded with @leaf, asynchronous exit pointer, and @tcs respectively.
>    *
>    * RBP and the stack are used to anchor __vdso_sgx_enter_enclave() to the
> @@ -70,9 +70,11 @@
>    */
>   #ifdef SGX_KERNEL_DOC
>   /* C-style function prototype to coerce kernel-doc into parsing the comment. */
> -int __vdso_sgx_enter_enclave(int leaf, void *tcs,
> +int __vdso_sgx_enter_enclave(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);
> +			     sgx_enclave_exit_handler_t handler)
>   #endif
>   SYM_FUNC_START(__vdso_sgx_enter_enclave)
>   	/* Prolog */
> @@ -82,7 +84,9 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave)
>   	.cfi_rel_offset		%rbp, 0
>   	mov	%rsp, %rbp
>   	.cfi_def_cfa_register	%rbp
> +	push	%rbx
A CFI directive is needed here:

	.cfi_rel_offset 	%rbx, -8

>   
> +	mov	%ecx, %eax
>   .Lenter_enclave:
>   	/* EENTER <= leaf <= ERESUME */
>   	cmp	$0x2, %eax
> @@ -108,6 +112,7 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave)
>   	jne	.Linvoke_userspace_handler
>   
>   .Lout:
> +	pop	%rbx
>   	leave
>   	.cfi_def_cfa		%rsp, 8
>   	ret
> 

Re: [PATCH for_v29 3/8] x86/sgx: vdso: Make __vdso_sgx_enter_enclave() callable from C code

[Sean Christopherson]

On Thu, Mar 19, 2020 at 01:03:41PM -0700, Xing, Cedric wrote:
> On 3/18/2020 6:11 PM, Sean Christopherson wrote:
> >  #endif
> >  SYM_FUNC_START(__vdso_sgx_enter_enclave)
> >  	/* Prolog */
> >@@ -82,7 +84,9 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave)
> >  	.cfi_rel_offset		%rbp, 0
> >  	mov	%rsp, %rbp
> >  	.cfi_def_cfa_register	%rbp
> >+	push	%rbx
> A CFI directive is needed here:
> 
> 	.cfi_rel_offset 	%rbx, -8

Darn, I suspected as much, but wasn't 100% positive.  Shouldn't have
hedged.  :-)

Is the rule of thumb for adding directives that one is needed any time
there is a new saved value of a register, or if the relative address of
the last saved value changes?  Are CFI directives only used for
non-volatile registers?

> >+	mov	%ecx, %eax
> >  .Lenter_enclave:
> >  	/* EENTER <= leaf <= ERESUME */
> >  	cmp	$0x2, %eax
> >@@ -108,6 +112,7 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave)
> >  	jne	.Linvoke_userspace_handler
> >  .Lout:
> >+	pop	%rbx
> >  	leave
> >  	.cfi_def_cfa		%rsp, 8
> >  	ret
> >

Re: [PATCH for_v29 1/8] x86/sgx: vdso: Remove an incorrect statement the enter enclave comment

[Jarkko Sakkinen]

On Wed, Mar 18, 2020 at 06:11:23PM -0700, Sean Christopherson wrote:
> Remove the statement about the userspace exit handler being required to
> clean up the untrusted stack, the vDSO unconditionally restores %rsp to
> its value at the time of EEXIT.
> 
> Reported-by: Nathaniel McCallum <npmccallum@redhat.com>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

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

[no resend]

/Jarkko

Re: [PATCH for_v29 0/8] x86/sgx: Make vDSO callable from C

[Jarkko Sakkinen]

On Wed, Mar 18, 2020 at 06:11:22PM -0700, Sean Christopherson wrote:
> Nathaniel pointed out that __vdso_sgx_enter_enclave() is tantalizingly
> close to being callable from C (with caveats and a cooperative enclave).
> The missing pieces are preserving %rbx and taking @leaf as a standard
> paramter (hey look, %rcx is available!).
> 
> As requested, update the selftest to invoke the vDSO from C, and fix
> bugs along the way.  Don't suppose you've changed your mind about waiting
> until after upstreaming to add a proper framework?
> 
> I also did a lot more testing on my internal code base to verify the
> %rsp tweaks play nice with the exit handler, and to prove that an exit
> handler can even cleanly handle exceptions in the enclave.
> 
> Here's the relevant code from my internal test suite, might want to have
> eye bleach ready ;-).  I tested the exception handling by overwriting
> @ms in enter_enclave() with a bogus value, e.g. 0xdeadull << 0x48.  The
> nested call to the vDSO (in the exit handler) with EMCD_EXCEPTION has the
> enclave dump its register.  The host (this runs as cgo in a Golang process)
> dumps the exception info.
> 
> I also verified the relative %rsp fixup by corrupting %rsp in the enclave,
> verifying it exploded as expected (I added extra consumption in the vDSO to
> force this) and then adding code to undo the damage in the exit handler and
> verifying things got back to normal.
> 
> Last thread of thought, IMO taking a context param (as suggested by
> Nathaniel) is unnecessary.  As shown below, the runtime effectively has a
> context param in the form of "struct sgx_enclave_exception *e", the handler
> just needs to be willing to cast (or use container_of()).  There are a
> multitude of other ways to pass info as well.
> 
> vdso_sgx_enter_enclave_t __enter_enclave;
> 
> vDSO::vDSO()
> {
>     if (__enter_enclave)
>         abort();
> 
>     uintptr_t vdso = (uintptr_t)getauxval(AT_SYSINFO_EHDR);
>     vdso_init_from_sysinfo_ehdr(vdso);
> 
>     __enter_enclave = (vdso_sgx_enter_enclave_t)vdso_sym("LINUX_2.6", "__vdso_sgx_enter_enclave");
>     if (!__enter_enclave)
>         abort();
> }
> 
> struct sgx_vdso_ctxt {
>     sgx_exception_t e;
>     long            ret;
>     Enclave *       enclave;
>     jmp_buf         buf;
> };
> 
> static int exit_handler(long rdi, long rsi, long rdx, long ursp, long r8, long r9,
>                         void *tcs, int ret, struct sgx_enclave_exception *e)
> {
>     struct sgx_vdso_ctxt *ctxt = (struct sgx_vdso_ctxt *)e;
>     int r;
> 
>     if (!ret) {
>         if (ctxt)
>             ctxt->ret = rdi;
>         return 0;
>     }
> 
>     if (!ctxt)
>         exit(EFAULT);
> 
>     r = __enter_enclave(ECMD_EXCEPTION, (unsigned long)&ctxt->e.regs, 0, EENTER,
>                         0, 0, tcs, NULL, exit_handler);
>     if (r)
>         exit(-r);
> 
>     ctxt->enclave->set_exception(ctxt->e);
> 
>     longjmp(ctxt->buf, ret);
> }
> 
> sgx_status_t vDSO::enter_enclave(Enclave &enclave, const long fn, void *ms, tcs_t *tcs)
> {
>     struct sgx_vdso_ctxt ctxt;
>     ctxt.enclave = &enclave;
> 
>     int ret = setjmp(ctxt.buf);
>     if (ret) {
>         if (ret != -EFAULT)
>             return SGX_ERROR_INVALID_PARAMETER;
> 
>         SE_URTS_ERROR("\nEnclave exception, base = %lx, size = %lx",
>                       enclave.get_base(), enclave.get_size());
> 
>         return SGX_ERROR_UNHANDLED_EXCEPTION;
>     }
> 
>     ret = __enter_enclave(fn, (unsigned long)ms, 0, EENTER, 0, 0, tcs,
>                           &ctxt.e.fault, exit_handler);
>     if (ret == -EINVAL)
>         return SGX_ERROR_INVALID_PARAMETER;
> 
>     return (sgx_status_t)ctxt.ret;
> }
> 
> Sean Christopherson (8):
>   x86/sgx: vdso: Remove an incorrect statement the enter enclave comment
>   x86/sgx: vdso: Make the %rsp fixup on return from handler relative
>   x86/sgx: vdso: Make __vdso_sgx_enter_enclave() callable from C code
>   x86/sgx: vdso: Define a typedef for __vdso_sgx_enter_enclave
>   selftests/x86: sgx: Zero out @result before invoking vDSO sub-test
>   selftests/x86: sgx: Pass EENTER to vDSO wrapper instead of hardcoding
>   selftests/x86: sgx: Stop clobbering non-volatile registers
>   selftests/x86: Add selftest to invoke __vsgx_enter_enclave() from C
> 
>  arch/x86/entry/vdso/vsgx_enter_enclave.S      | 72 ++-----------------
>  arch/x86/include/uapi/asm/sgx.h               | 61 ++++++++++++++++
>  .../selftests/x86/sgx/encl_bootstrap.S        |  6 +-
>  tools/testing/selftests/x86/sgx/main.c        | 17 ++++-
>  tools/testing/selftests/x86/sgx/sgx_call.S    |  1 -
>  tools/testing/selftests/x86/sgx/sgx_call.h    |  2 +-
>  6 files changed, 85 insertions(+), 74 deletions(-)
> 
> -- 
> 2.24.1
> 

Might be a grazy idea but better to go through this anyway.

Given the premise that we need the carry the callback anyway in all
cases, why won't just have the callback.

Why we absolutely need the code path that fills exception info given
that we no matter what need to have a callback route?

Would simplify considerably to have only clear flow.

/Jarkko

Re: [PATCH for_v29 2/8] x86/sgx: vdso: Make the %rsp fixup on return from handler relative

[Jarkko Sakkinen]

On Wed, Mar 18, 2020 at 06:11:24PM -0700, Sean Christopherson wrote:
> Modify the %rsp fixup after returning from the exit handler to be
> relative instead of absolute to avoid clobbering any %rsp adjustments
> made by the exit handler, e.g. if the exit handler modifies the stack
> prior to re-entering the enclave.
> 
> Reported-by: Nathaniel McCallum <npmccallum@redhat.com>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
> 
> I'm on the fence as to whether or not this is a good idea.  It's not super
> painful, but it's not exactly standard/obvious code.  Part of me thinks
> its a bug to not let the exit handler manipulate %rsp, the other part of
> me thinks it's straight up crazy :-)

After some hours of processing this, I think this makes sense.

It makes the interface more robust. This is not printf().

/Jarkko

Re: [PATCH for_v29 2/8] x86/sgx: vdso: Make the %rsp fixup on return from handler relative

[Jarkko Sakkinen]

On Fri, Mar 20, 2020 at 04:39:51AM +0200, Jarkko Sakkinen wrote:
> On Wed, Mar 18, 2020 at 06:11:24PM -0700, Sean Christopherson wrote:
> > Modify the %rsp fixup after returning from the exit handler to be
> > relative instead of absolute to avoid clobbering any %rsp adjustments
> > made by the exit handler, e.g. if the exit handler modifies the stack
> > prior to re-entering the enclave.
> > 
> > Reported-by: Nathaniel McCallum <npmccallum@redhat.com>
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > ---
> > 
> > I'm on the fence as to whether or not this is a good idea.  It's not super
> > painful, but it's not exactly standard/obvious code.  Part of me thinks
> > its a bug to not let the exit handler manipulate %rsp, the other part of
> > me thinks it's straight up crazy :-)
> 
> After some hours of processing this, I think this makes sense.
> 
> It makes the interface more robust. This is not printf().

Has been merged.

/Jarkko

Re: [PATCH for_v29 3/8] x86/sgx: vdso: Make __vdso_sgx_enter_enclave() callable from C code

[Jarkko Sakkinen]

On Thu, Mar 19, 2020 at 01:11:45PM -0700, Sean Christopherson wrote:
> On Thu, Mar 19, 2020 at 01:03:41PM -0700, Xing, Cedric wrote:
> > On 3/18/2020 6:11 PM, Sean Christopherson wrote:
> > >  #endif
> > >  SYM_FUNC_START(__vdso_sgx_enter_enclave)
> > >  	/* Prolog */
> > >@@ -82,7 +84,9 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave)
> > >  	.cfi_rel_offset		%rbp, 0
> > >  	mov	%rsp, %rbp
> > >  	.cfi_def_cfa_register	%rbp
> > >+	push	%rbx
> > A CFI directive is needed here:
> > 
> > 	.cfi_rel_offset 	%rbx, -8
> 
> Darn, I suspected as much, but wasn't 100% positive.  Shouldn't have
> hedged.  :-)
> 
> Is the rule of thumb for adding directives that one is needed any time
> there is a new saved value of a register, or if the relative address of
> the last saved value changes?  Are CFI directives only used for
> non-volatile registers?

AFAIK the convention is just that if you push a register you need to
offset it so that the FDE that is put into .eh_frame by GCC has the
information from which locations of the stack the register values are
copied.

BTW, why use GCC-style ".L-mangled" label names instead of having more
readable ones?

/Jarkko

Re: [PATCH for_v29 0/8] x86/sgx: Make vDSO callable from C

[Sean Christopherson]

On Fri, Mar 20, 2020 at 02:57:24AM +0200, Jarkko Sakkinen wrote:
> On Wed, Mar 18, 2020 at 06:11:22PM -0700, Sean Christopherson wrote:
> > Sean Christopherson (8):
> >   x86/sgx: vdso: Remove an incorrect statement the enter enclave comment
> >   x86/sgx: vdso: Make the %rsp fixup on return from handler relative
> >   x86/sgx: vdso: Make __vdso_sgx_enter_enclave() callable from C code
> >   x86/sgx: vdso: Define a typedef for __vdso_sgx_enter_enclave
> >   selftests/x86: sgx: Zero out @result before invoking vDSO sub-test
> >   selftests/x86: sgx: Pass EENTER to vDSO wrapper instead of hardcoding
> >   selftests/x86: sgx: Stop clobbering non-volatile registers
> >   selftests/x86: Add selftest to invoke __vsgx_enter_enclave() from C
> > 
> >  arch/x86/entry/vdso/vsgx_enter_enclave.S      | 72 ++-----------------
> >  arch/x86/include/uapi/asm/sgx.h               | 61 ++++++++++++++++
> >  .../selftests/x86/sgx/encl_bootstrap.S        |  6 +-
> >  tools/testing/selftests/x86/sgx/main.c        | 17 ++++-
> >  tools/testing/selftests/x86/sgx/sgx_call.S    |  1 -
> >  tools/testing/selftests/x86/sgx/sgx_call.h    |  2 +-
> >  6 files changed, 85 insertions(+), 74 deletions(-)
> > 
> > -- 
> > 2.24.1
> > 
> 
> Might be a grazy idea but better to go through this anyway.
> 
> Given the premise that we need the carry the callback anyway in all
> cases, why won't just have the callback.
> 
> Why we absolutely need the code path that fills exception info given
> that we no matter what need to have a callback route?
> 
> Would simplify considerably to have only clear flow.

Invoking the callback uses a retpoline, which is non-trivial overhead.
For runtimes that need an assembly wrapper for other reasons, and aren't
using the untrusted stack, forcing them to implement a handler would be
painful.

Re: [PATCH for_v29 3/8] x86/sgx: vdso: Make __vdso_sgx_enter_enclave() callable from C code

[Sean Christopherson]

On Fri, Mar 20, 2020 at 05:07:00AM +0200, Jarkko Sakkinen wrote:
> On Thu, Mar 19, 2020 at 01:11:45PM -0700, Sean Christopherson wrote:
> > On Thu, Mar 19, 2020 at 01:03:41PM -0700, Xing, Cedric wrote:
> > > On 3/18/2020 6:11 PM, Sean Christopherson wrote:
> > > >  #endif
> > > >  SYM_FUNC_START(__vdso_sgx_enter_enclave)
> > > >  	/* Prolog */
> > > >@@ -82,7 +84,9 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave)
> > > >  	.cfi_rel_offset		%rbp, 0
> > > >  	mov	%rsp, %rbp
> > > >  	.cfi_def_cfa_register	%rbp
> > > >+	push	%rbx
> > > A CFI directive is needed here:
> > > 
> > > 	.cfi_rel_offset 	%rbx, -8
> > 
> > Darn, I suspected as much, but wasn't 100% positive.  Shouldn't have
> > hedged.  :-)
> > 
> > Is the rule of thumb for adding directives that one is needed any time
> > there is a new saved value of a register, or if the relative address of
> > the last saved value changes?  Are CFI directives only used for
> > non-volatile registers?
> 
> AFAIK the convention is just that if you push a register you need to
> offset it so that the FDE that is put into .eh_frame by GCC has the
> information from which locations of the stack the register values are
> copied.
> 
> BTW, why use GCC-style ".L-mangled" label names instead of having more
> readable ones?

Readable as in dropping the .L part?  E.g.

  enclu_eenter_eresume:

I assumed we'd want to keep the local labels out of the symbols file, but
maybe getting them in there would be a good thing?

Re: [PATCH for_v29 0/8] x86/sgx: Make vDSO callable from C

[Jarkko Sakkinen]

On Fri, Mar 20, 2020 at 04:25:12PM -0700, Sean Christopherson wrote:
> On Fri, Mar 20, 2020 at 02:57:24AM +0200, Jarkko Sakkinen wrote:
> > On Wed, Mar 18, 2020 at 06:11:22PM -0700, Sean Christopherson wrote:
> > > Sean Christopherson (8):
> > >   x86/sgx: vdso: Remove an incorrect statement the enter enclave comment
> > >   x86/sgx: vdso: Make the %rsp fixup on return from handler relative
> > >   x86/sgx: vdso: Make __vdso_sgx_enter_enclave() callable from C code
> > >   x86/sgx: vdso: Define a typedef for __vdso_sgx_enter_enclave
> > >   selftests/x86: sgx: Zero out @result before invoking vDSO sub-test
> > >   selftests/x86: sgx: Pass EENTER to vDSO wrapper instead of hardcoding
> > >   selftests/x86: sgx: Stop clobbering non-volatile registers
> > >   selftests/x86: Add selftest to invoke __vsgx_enter_enclave() from C
> > > 
> > >  arch/x86/entry/vdso/vsgx_enter_enclave.S      | 72 ++-----------------
> > >  arch/x86/include/uapi/asm/sgx.h               | 61 ++++++++++++++++
> > >  .../selftests/x86/sgx/encl_bootstrap.S        |  6 +-
> > >  tools/testing/selftests/x86/sgx/main.c        | 17 ++++-
> > >  tools/testing/selftests/x86/sgx/sgx_call.S    |  1 -
> > >  tools/testing/selftests/x86/sgx/sgx_call.h    |  2 +-
> > >  6 files changed, 85 insertions(+), 74 deletions(-)
> > > 
> > > -- 
> > > 2.24.1
> > > 
> > 
> > Might be a grazy idea but better to go through this anyway.
> > 
> > Given the premise that we need the carry the callback anyway in all
> > cases, why won't just have the callback.
> > 
> > Why we absolutely need the code path that fills exception info given
> > that we no matter what need to have a callback route?
> > 
> > Would simplify considerably to have only clear flow.
> 
> Invoking the callback uses a retpoline, which is non-trivial overhead.
> For runtimes that need an assembly wrapper for other reasons, and aren't
> using the untrusted stack, forcing them to implement a handler would be
> painful.

The non-callback route only exists because we did not know that we have
to do the callback route. It does not make sense to add something for
any other reason than absolute necessity.

Things would simplify in the vDSO implementation considerably. Now it is
overwhelmingly complex for no good reason.

/Jarkko

Re: [PATCH for_v29 3/8] x86/sgx: vdso: Make __vdso_sgx_enter_enclave() callable from C code

[Jarkko Sakkinen]

On Fri, Mar 20, 2020 at 04:26:49PM -0700, Sean Christopherson wrote:
> On Fri, Mar 20, 2020 at 05:07:00AM +0200, Jarkko Sakkinen wrote:
> > On Thu, Mar 19, 2020 at 01:11:45PM -0700, Sean Christopherson wrote:
> > > On Thu, Mar 19, 2020 at 01:03:41PM -0700, Xing, Cedric wrote:
> > > > On 3/18/2020 6:11 PM, Sean Christopherson wrote:
> > > > >  #endif
> > > > >  SYM_FUNC_START(__vdso_sgx_enter_enclave)
> > > > >  	/* Prolog */
> > > > >@@ -82,7 +84,9 @@ SYM_FUNC_START(__vdso_sgx_enter_enclave)
> > > > >  	.cfi_rel_offset		%rbp, 0
> > > > >  	mov	%rsp, %rbp
> > > > >  	.cfi_def_cfa_register	%rbp
> > > > >+	push	%rbx
> > > > A CFI directive is needed here:
> > > > 
> > > > 	.cfi_rel_offset 	%rbx, -8
> > > 
> > > Darn, I suspected as much, but wasn't 100% positive.  Shouldn't have
> > > hedged.  :-)
> > > 
> > > Is the rule of thumb for adding directives that one is needed any time
> > > there is a new saved value of a register, or if the relative address of
> > > the last saved value changes?  Are CFI directives only used for
> > > non-volatile registers?
> > 
> > AFAIK the convention is just that if you push a register you need to
> > offset it so that the FDE that is put into .eh_frame by GCC has the
> > information from which locations of the stack the register values are
> > copied.
> > 
> > BTW, why use GCC-style ".L-mangled" label names instead of having more
> > readable ones?
> 
> Readable as in dropping the .L part?  E.g.
> 
>   enclu_eenter_eresume:
> 
> I assumed we'd want to keep the local labels out of the symbols file, but
> maybe getting them in there would be a good thing?

Right, that was the legit reason. Nope, lets keep them then.

/Jarkko

Re: [PATCH for_v29 0/8] x86/sgx: Make vDSO callable from C

[Sean Christopherson]

On Sat, Mar 21, 2020 at 02:55:28AM +0200, Jarkko Sakkinen wrote:
> On Fri, Mar 20, 2020 at 04:25:12PM -0700, Sean Christopherson wrote:
> > On Fri, Mar 20, 2020 at 02:57:24AM +0200, Jarkko Sakkinen wrote:
> > > On Wed, Mar 18, 2020 at 06:11:22PM -0700, Sean Christopherson wrote:
> > > > Sean Christopherson (8):
> > > >   x86/sgx: vdso: Remove an incorrect statement the enter enclave comment
> > > >   x86/sgx: vdso: Make the %rsp fixup on return from handler relative
> > > >   x86/sgx: vdso: Make __vdso_sgx_enter_enclave() callable from C code
> > > >   x86/sgx: vdso: Define a typedef for __vdso_sgx_enter_enclave
> > > >   selftests/x86: sgx: Zero out @result before invoking vDSO sub-test
> > > >   selftests/x86: sgx: Pass EENTER to vDSO wrapper instead of hardcoding
> > > >   selftests/x86: sgx: Stop clobbering non-volatile registers
> > > >   selftests/x86: Add selftest to invoke __vsgx_enter_enclave() from C
> > > > 
> > > >  arch/x86/entry/vdso/vsgx_enter_enclave.S      | 72 ++-----------------
> > > >  arch/x86/include/uapi/asm/sgx.h               | 61 ++++++++++++++++
> > > >  .../selftests/x86/sgx/encl_bootstrap.S        |  6 +-
> > > >  tools/testing/selftests/x86/sgx/main.c        | 17 ++++-
> > > >  tools/testing/selftests/x86/sgx/sgx_call.S    |  1 -
> > > >  tools/testing/selftests/x86/sgx/sgx_call.h    |  2 +-
> > > >  6 files changed, 85 insertions(+), 74 deletions(-)
> > > > 
> > > > -- 
> > > > 2.24.1
> > > > 
> > > 
> > > Might be a grazy idea but better to go through this anyway.
> > > 
> > > Given the premise that we need the carry the callback anyway in all
> > > cases, why won't just have the callback.
> > > 
> > > Why we absolutely need the code path that fills exception info given
> > > that we no matter what need to have a callback route?
> > > 
> > > Would simplify considerably to have only clear flow.
> > 
> > Invoking the callback uses a retpoline, which is non-trivial overhead.
> > For runtimes that need an assembly wrapper for other reasons, and aren't
> > using the untrusted stack, forcing them to implement a handler would be
> > painful.
> 
> The non-callback route only exists because we did not know that we have
> to do the callback route. It does not make sense to add something for
> any other reason than absolute necessity.

The non-callback route exists because it's simpler for the runtime to use
if it already has an asm wrapper and doesn't do stack shenanigans, e.g. the
runtime doesn't need to play games to get context information when handling
an exit.

And using a callback in conjunction with calling the vDSO from C is by no
means necessary, e.g. if the runtime is happy to die on exceptions, or if
it's doing clever clobbering, function annotation, longjump(), etc... to
avoid consuming corrupted state on an exception.

> Things would simplify in the vDSO implementation considerably. Now it is
> overwhelmingly complex for no good reason.

No it wouldn't, it literally saves zero instructions (unless the vDSO
wanted to crash the caller on a NULL pointer exception). It'd do nothing
more than move this code:

	/* Invoke userspace's exit handler if one was provided. */
.Lhandle_exit:
	cmp	$0, 0x20(%rbp)
	jne	.Linvoke_userspace_handler

up to the beginning of the code as

	/* Return -EINVAL if there's no userspace exit handler. */
	cmp	$0, 0x20(%rbp)
	je	.Linvalid_input

and the exit handler invocation would get inlined.  Unless I'm missing some
clever massaging of code, that's it.

At best the above could be dropped, but that's a whopping two instructions
and a single uop.