[RFC PATCH v3 0/4] x86: Add exception fixup for SGX ENCLU

[RFC PATCH v3 0/4] x86: Add exception fixup for SGX ENCLU

[Sean Christopherson]

This is effectively v3 of the "x86: Add vDSO exception fixup for SGX"
series, but as you might of noticed, there are no vDSO changes here.

Andy's comment on Spectre/retpoline[1] and Jethro's comment on making
the vDSO as barebones as possible[2] got me wondering if we could let
userspace dynamically set the "return" target of the SGX vDSO function,
taking advantage of the fact that most (all?) SGX processes will have a
single entry point for all enclaves.

The initial idea was to make __vdso_sgx_enter_enclave() a barebones
"function" consisting of ENCLU and UD2.  SGX would provide an IOCTL via
/dev/sgx that could be used to patch the UD2 in the current vDSO image
to a JMP rel32, with rel32 pointing at an address provided by the
user.  Aside from the issues of rel32 in a 64-bit memory space,
patching the vDSO image added a lot of overhead just so that the kernel
could know the address of ENCLU.

At that point I realized it's a hell of a lot easier to simply provide
an IOCTL via /dev/sgx that allows userspace to register a per-process
ENCLU exception handler.  At a high level, the basic idea is the same
as the vDSO approach: provide a hardcoded fixup handler for ENCLU and
attempt to fixup select unhandled exceptions that occurred in user code.

Pros:
  - No vDSO function.
  - Minimal userspace changes.
  - Smaller ABI, i.e. less bikeshedding (fingers crossed).
  - Kernel doesn't prevent userspace from doing stupid things, e.g.
    modifying the process' stack from within the enclave.
  - Kernel can proactively enforce the ENCLU handler as the only
    officialy supported ABI for enclave exception handling.

Cons:
  - Userspace can only register a single ENCLU handler per process.
  - ABI is more esoteric than a standard function call.


This series is based on Jarkko's current master branch, which moves all
of the SGX code into arch/x86.  The full code can be found here:

https://github.com/sean-jc/linux/tree/2538adcece15447b988d93bf677af48073c7d219

I have not actually tested this *exact* commit beyond compiling as I do
not know the health status of Jarkko's code.  To test, I cherry-picked
the patches into an older stable version of the code.  Git was able to
handle the file renaming.

v1: https://lkml.kernel.org/r/20181205232012.28920-1-sean.j.christopherson@intel.com
v2: https://lkml.kernel.org/r/20181206221922.31012-1-sean.j.christopherson@intel.com
v3:
  - Replace the vDSO fixup with SGX specific fixup.
  - Patches 2/4 and 3/4 are essentially identical, the only difference
    being the name of the function that is called.  The changelogs for
    these patches still need a lot of attention.

[1] https://lkml.kernel.org/r/CALCETrVBR+2HjTqX=W4r9GOq69Xg36v4gmCKqK0wUjzAqBJnrw@mail.gmail.com
[2] https://lkml.kernel.org/r/f595c046-682c-0d4a-ce68-44d4634cedf2@fortanix.com

Sean Christopherson (4):
  x86/sgx: Add a per-mm ENCLU exception fixup handler
  x86/fault: Attempt to fixup unhandled #PF on ENCLU before signaling
  x86/traps: Attempt to fixup exceptions in vDSO before signaling
  x86/sgx: Add an SGX IOCTL to register a per-mm ENCLU exception handler

 arch/x86/include/asm/mmu.h             |  4 ++++
 arch/x86/include/asm/sgx.h             | 13 +++++++++++++
 arch/x86/include/uapi/asm/sgx.h        | 23 ++++++++++++++++++-----
 arch/x86/kernel/cpu/sgx/driver/encl.c  |  6 ++++++
 arch/x86/kernel/cpu/sgx/driver/ioctl.c | 20 ++++++++++++++++++++
 arch/x86/kernel/cpu/sgx/main.c         | 18 ++++++++++++++++++
 arch/x86/kernel/traps.c                | 15 +++++++++++++++
 arch/x86/mm/fault.c                    |  7 +++++++
 8 files changed, 101 insertions(+), 5 deletions(-)

-- 
2.19.2

[RFC PATCH v3 1/4] x86/sgx: Add a per-mm ENCLU exception fixup handler

[Sean Christopherson]

Intel Software Guard Extensions (SGX) introduces a new CPL3-only
"enclave" mode that runs as a sort of black box shared object that is
hosted by an untrusted "normal" CPl3 process.

Entering an enclave can only be done through SGX-specific instructions,
EENTER and ERESUME, and is a non-trivial process.  Because of the
complexity of transitioning to/from an enclave, the vast majority of
enclaves are expected to utilize a library to handle the actual
transitions.  This is roughly analogous to how e.g. a libc and dynamic
linker are used by most applications.

Another crucial characteristic of SGX enclaves is that they can generate
exceptions as part of their normal (at least as "normal" as SGX can be)
operation that need to be handled *in* the enclave and/or are unique
to SGX.

And because they are essentially fancy shared objects, a process can
host any number of enclaves, each of which can execute multiple threads
simultaneously.

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

Add per-mm, i.e. per-process, exception fixup on ENCLU to so that the
kernel can redirect unhandled exceptions, i.e. exceptions would otherwise
generate a signal, to a user-provided exception handler.  The exception
handler ABI roughly follows the System V 64-bit ABI for function calls:

  - %rdi: trap number
  - %rsi: error code
  - %rdx: address

Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Josh Triplett <josh@joshtriplett.org>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/include/asm/mmu.h     |  4 ++++
 arch/x86/include/asm/sgx.h     | 13 +++++++++++++
 arch/x86/kernel/cpu/sgx/main.c | 18 ++++++++++++++++++
 3 files changed, 35 insertions(+)

diff --git a/arch/x86/include/asm/mmu.h b/arch/x86/include/asm/mmu.h
index 5ff3e8af2c20..1665c84e5844 100644
--- a/arch/x86/include/asm/mmu.h
+++ b/arch/x86/include/asm/mmu.h
@@ -54,6 +54,10 @@ typedef struct {
 	/* address of the bounds directory */
 	void __user *bd_addr;
 #endif
+#ifdef CONFIG_INTEL_SGX_CORE
+	unsigned long enclu_address;
+	unsigned long enclu_exception_handler;
+#endif
 } mm_context_t;
 
 #define INIT_MM_CONTEXT(mm)						\
diff --git a/arch/x86/include/asm/sgx.h b/arch/x86/include/asm/sgx.h
index d4f61d1c5c2a..bbf808a0ca91 100644
--- a/arch/x86/include/asm/sgx.h
+++ b/arch/x86/include/asm/sgx.h
@@ -309,6 +309,19 @@ static inline int __emodt(struct sgx_secinfo *secinfo, void *addr)
 	return __encls_ret_2(SGX_EMODT, secinfo, addr);
 }
 
+#ifdef CONFIG_INTEL_SGX_CORE
+extern bool fixup_sgx_enclu_exception(struct pt_regs *regs, int trapnr,
+				      unsigned long error_code,
+				      unsigned long fault_addr);
+#else
+static inline bool fixup_sgx_enclu_exception(struct pt_regs *regs, int trapnr,
+					     unsigned long error_code,
+					     unsigned long fault_addr)
+{
+	return false;
+}
+#endif
+
 struct sgx_epc_page *sgx_alloc_page(void *owner, bool reclaim);
 int __sgx_free_page(struct sgx_epc_page *page);
 void sgx_free_page(struct sgx_epc_page *page);
diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index 30fd69f1fc07..1994e003581d 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -363,6 +363,24 @@ void sgx_page_reclaimable(struct sgx_epc_page *page)
 }
 EXPORT_SYMBOL_GPL(sgx_page_reclaimable);
 
+bool fixup_sgx_enclu_exception(struct pt_regs *regs, int trapnr,
+			       unsigned long error_code,
+			       unsigned long fault_addr)
+{
+	if (current->mm->context.enclu_address != regs->ip)
+		return false;
+
+	if (!current->mm->context.enclu_address &&
+	    !current->mm->context.enclu_exception_handler)
+		return false;
+
+	regs->ip = current->mm->context.enclu_exception_handler;
+	regs->di = trapnr;
+	regs->si = error_code;
+	regs->dx = fault_addr;
+	return true;
+}
+
 static __init void sgx_free_epc_section(struct sgx_epc_section *section)
 {
 	struct sgx_epc_page *page;
-- 
2.19.2

[RFC PATCH v3 2/4] x86/fault: Attempt to fixup unhandled #PF on ENCLU before signaling

[Sean Christopherson]

Call fixup_sgx_enclu_exception() in the SIGSEGV and SIGBUS paths of
the page fault handler immediately prior to signaling.  If the fault
is fixed, return cleanly and do not generate a signal.

In the SIGSEGV flow, make sure the error code passed to userspace has
been sanitized.

Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Josh Triplett <josh@joshtriplett.org>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/mm/fault.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index 7e8a7558ca07..d3903965334c 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -28,6 +28,7 @@
 #include <asm/mmu_context.h>		/* vma_pkey()			*/
 #include <asm/efi.h>			/* efi_recover_from_page_fault()*/
 #include <asm/desc.h>			/* store_idt(), ...		*/
+#include <asm/sgx.h>			/* fixup_sgx_enclu_exception()	*/
 
 #define CREATE_TRACE_POINTS
 #include <asm/trace/exceptions.h>
@@ -928,6 +929,9 @@ __bad_area_nosemaphore(struct pt_regs *regs, unsigned long error_code,
 		if (address >= TASK_SIZE_MAX)
 			error_code |= X86_PF_PROT;
 
+		if (fixup_sgx_enclu_exception(regs, X86_TRAP_PF, error_code, address))
+			return;
+
 		if (likely(show_unhandled_signals))
 			show_signal_msg(regs, error_code, address, tsk);
 
@@ -1045,6 +1049,9 @@ do_sigbus(struct pt_regs *regs, unsigned long error_code, unsigned long address,
 	if (is_prefetch(regs, error_code, address))
 		return;
 
+	if (fixup_sgx_enclu_exception(regs, X86_TRAP_PF, error_code, address))
+		return;
+
 	set_signal_archinfo(address, error_code);
 
 #ifdef CONFIG_MEMORY_FAILURE
-- 
2.19.2

[RFC PATCH v3 3/4] x86/traps: Attempt to fixup exceptions in vDSO before signaling

[Sean Christopherson]

Call fixup_sgx_enclu_exception() in all trap flows that generate
signals to userspace immediately prior to generating any such signal.
If the exception is fixed, return cleanly and don't generate a signal.

Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Josh Triplett <josh@joshtriplett.org>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kernel/traps.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 9b7c4ca8f0a7..dd664503803b 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -61,6 +61,7 @@
 #include <asm/mpx.h>
 #include <asm/vm86.h>
 #include <asm/umip.h>
+#include <asm/sgx.h>
 
 #ifdef CONFIG_X86_64
 #include <asm/x86_init.h>
@@ -223,6 +224,10 @@ do_trap_no_signal(struct task_struct *tsk, int trapnr, const char *str,
 	tsk->thread.error_code = error_code;
 	tsk->thread.trap_nr = trapnr;
 
+	if (user_mode(regs) &&
+	    fixup_sgx_enclu_exception(regs, trapnr, error_code, 0))
+		return 0;
+
 	return -1;
 }
 
@@ -563,6 +568,9 @@ do_general_protection(struct pt_regs *regs, long error_code)
 	tsk->thread.error_code = error_code;
 	tsk->thread.trap_nr = X86_TRAP_GP;
 
+	if (fixup_sgx_enclu_exception(regs, X86_TRAP_GP, error_code, 0))
+		return;
+
 	show_signal(tsk, SIGSEGV, "", desc, regs, error_code);
 
 	force_sig(SIGSEGV, tsk);
@@ -791,6 +799,10 @@ dotraplinkage void do_debug(struct pt_regs *regs, long error_code)
 		goto exit;
 	}
 
+	if (user_mode(regs) &&
+	    fixup_sgx_enclu_exception(regs, X86_TRAP_DB, error_code, 0))
+		goto exit;
+
 	if (WARN_ON_ONCE((dr6 & DR_STEP) && !user_mode(regs))) {
 		/*
 		 * Historical junk that used to handle SYSENTER single-stepping.
@@ -854,6 +866,9 @@ static void math_error(struct pt_regs *regs, int error_code, int trapnr)
 	if (!si_code)
 		return;
 
+	if (fixup_sgx_enclu_exception(regs, trapnr, error_code, 0))
+		return;
+
 	force_sig_fault(SIGFPE, si_code,
 			(void __user *)uprobe_get_trap_addr(regs), task);
 }
-- 
2.19.2

[RFC PATCH v3 4/4] x86/sgx: Add an SGX IOCTL to register a per-mm ENCLU exception handler

[Sean Christopherson]

Intel Software Guard Extensions (SGX) SGX introduces a new CPL3-only
enclave mode that runs as a sort of black box shared object that is
hosted by an untrusted normal CPL3 process.

Enclave transitions have semantics that are a lovely blend of SYCALL,
SYSRET and VM-Exit.  In a non-faulting scenario, entering and exiting
an enclave can only be done through SGX-specific instructions, EENTER
and EEXIT respectively.  EENTER+EEXIT is analogous to SYSCALL+SYSRET,
e.g. EENTER/SYSCALL load RCX with the next RIP and EEXIT/SYSRET load
RIP from R{B,C}X.

But in a faulting/interrupting scenario, enclave transitions act more
like VM-Exit and VMRESUME.  Maintaining the black box nature of the
enclave means that hardware must automatically switch CPU context when
an Asynchronous Exiting Event (AEE) occurs, an AEE being any interrupt
or exception (exceptions are AEEs because asynchronous in this context
is relative to the enclave and not CPU execution, e.g. the enclave
doesn't get an opportunity to save/fuzz CPU state).

Like VM-Exits, all AEEs jump to a common location, referred to as the
Asynchronous Exiting Point (AEP).  The AEP is specified at enclave entry
via register passed to EENTER/ERESUME, similar to how the hypervisor
specifies the VM-Exit point (via VMCS.HOST_RIP at VMLAUNCH/VMRESUME).
Resuming the enclave/VM after the exiting event is handled is done via
ERESUME/VMRESUME respectively.  In SGX, AEEs that are handled by the
kernel, e.g. INTR, NMI and most page faults, IRET will journey back to
the AEP which then ERESUMEs th enclave.

Enclaves also behave a bit like VMs in the sense that they can generate
exceptions as part of their normal operation that for all intents and
purposes need to handled in the enclave/VM.  However, unlike VMX, SGX
doesn't allow the host to modify its guest's, a.k.a. enclave's, state,
as doing so would circumvent the enclave's security.  So to handle an
exception, the enclave must first be re-entered through the normal
EENTER flow (SYSCALL/SYSRET behavior), and then resumed via ERESUME
(VMRESUME behavior) after the source of the exception is resolved.

All of the above is just the tip of the iceberg when it comes to running
an enclave.  But, SGX was designed in such a way that the host process
can utilize an enclave agnostic library to build, launch and run an
enclave.  This is roughly analogous to how e.g. normal applications
leverage libc implementations and a standardized dynamic linker so that
the application on business logic instead of the gory details of system
calls, vDSO functions, dynamic linking, etc...

However, offloading the heavy lifting to a library comes with a rather
large caveat.  Because enclaves can generate *and* handle exceptions,
SGX libraries must be prepared to handle nearly any exception whenever
at least one thread is executing in an enclave.  On Linux, this means
the SGX library must register a signal handler in order to intercept
relevant exceptions and forward them to the enclave (or in some cases,
take action on behalf of the enclave).

Unfortunately, Linux's signal mechanism doesn't mesh well with libraries,
e.g. signal handlers are process wide, are difficult to chain, etc...
This becomes particularly nasty when using multiple levels of libraries
that register signal handlers, e.g. running an enclave via cgo inside of
the Go runtime.

Luckily, signals (due to exceptions) can be avoided entirely by taking
advantage of several key properties of SGX/enclaves:

  - Enclaves can only be entered through SGX-specific instructions,
    and all CPL3 SGX instructions share a single umbrella opcode under
    the mnemonic ENCLU.
  - When an event/exception occurs in an enclave, hardware preps the
    post-exit state so that executing ENCLU will automagically ERESUME
    the enclave.  This means that ENCLU[EENTER] and ENCLU[ERESUME] for
    an enclave can be the exact same ENCLU instruction.
  - Exceptions within the enclave appear to the kernel as if they
    occurred on the AEP, i.e. ENCLU[ERESUME].
  - Enclaves are essentially just shared objects with a specialized
    dynamic linker, so it's not unreasonable to require a process to
    use a single loader and entry point, i.e. ENCLU, for all enclaves.

So, to avoid forcing SGX libraries to juggle signal handlers, provide
an IOCTL through /dev/sgx to allow a process to register an exception
handler for a single per-mm, i.e. per-process, ENCLU instruction.  If
an unhandled exception occurs on the ENCLU, i.e. a signal would be
generated, load DI, SI and DX with the trap number, error code and
faulting address respectively in lieu of generating a signal.

Softly enforce the use of the ENCLU handler mechanism by refusing to
create enclaves for a process if it has not registered an ENCLU handler.
In other words, the only ABI supported by the Linux kernel for handling
exceptions on/in enclaves is to register an ENCLU exception handler.
Obviously a process can register a dummy handler, but such behavior is
NOT officially supported.

Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Josh Triplett <josh@joshtriplett.org>
Cc: Haitao Huang <haitao.huang@linux.intel.com>
Cc: Jethro Beekman <jethro@fortanix.com>
Cc: Dr. Greg Wettstein <greg@enjellic.com>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/include/uapi/asm/sgx.h        | 23 ++++++++++++++++++-----
 arch/x86/kernel/cpu/sgx/driver/encl.c  |  6 ++++++
 arch/x86/kernel/cpu/sgx/driver/ioctl.c | 20 ++++++++++++++++++++
 3 files changed, 44 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
index 266b813eefa1..63bd64e9535d 100644
--- a/arch/x86/include/uapi/asm/sgx.h
+++ b/arch/x86/include/uapi/asm/sgx.h
@@ -10,20 +10,33 @@
 
 #define SGX_MAGIC 0xA4
 
+#define SGX_IOC_ENCLU_REGISTER \
+	_IOW(SGX_MAGIC, 0x00, struct sgx_enclu_register)
 #define SGX_IOC_ENCLAVE_CREATE \
-	_IOW(SGX_MAGIC, 0x00, struct sgx_enclave_create)
+	_IOW(SGX_MAGIC, 0x01, struct sgx_enclave_create)
 #define SGX_IOC_ENCLAVE_ADD_PAGE \
-	_IOW(SGX_MAGIC, 0x01, struct sgx_enclave_add_page)
+	_IOW(SGX_MAGIC, 0x02, struct sgx_enclave_add_page)
 #define SGX_IOC_ENCLAVE_INIT \
-	_IOW(SGX_MAGIC, 0x02, struct sgx_enclave_init)
+	_IOW(SGX_MAGIC, 0x03, struct sgx_enclave_init)
 #define SGX_IOC_ENCLAVE_REMOVE_PAGES \
-	_IOW(SGX_MAGIC, 0x03, struct sgx_enclave_remove_pages)
+	_IOW(SGX_MAGIC, 0x04, struct sgx_enclave_remove_pages)
 #define SGX_IOC_ENCLAVE_MODIFY_PAGES \
-	_IOW(SGX_MAGIC, 0x04, struct sgx_enclave_modify_pages)
+	_IOW(SGX_MAGIC, 0x05, struct sgx_enclave_modify_pages)
 
 /* IOCTL return values */
 #define SGX_POWER_LOST_ENCLAVE		0x40000000
 
+/**
+ * struct sgx_enclu_register - parameter structure for the
+ *                             %SGX_IOC_ENCLU_REGISTER ioctl
+ * @enclu:	address of the userspace process' ENCLU instruction
+ * @handler:	address of the userspace process' ENCLU exception handler
+ */
+struct sgx_enclu_register  {
+	__u64	enclu;
+	__u64	handler;
+};
+
 /**
  * struct sgx_enclave_create - parameter structure for the
  *                             %SGX_IOC_ENCLAVE_CREATE ioctl
diff --git a/arch/x86/kernel/cpu/sgx/driver/encl.c b/arch/x86/kernel/cpu/sgx/driver/encl.c
index 61a14cc310f4..ed5df48fba63 100644
--- a/arch/x86/kernel/cpu/sgx/driver/encl.c
+++ b/arch/x86/kernel/cpu/sgx/driver/encl.c
@@ -525,6 +525,12 @@ int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs)
 	}
 
 	down_read(&current->mm->mmap_sem);
+	if (!current->mm->context.enclu_address &&
+	    !current->mm->context.enclu_exception_handler) {
+		up_read(&current->mm->mmap_sem);
+		return -EFAULT;
+	}
+
 	ret = sgx_encl_find(current->mm, secs->base, &vma);
 	if (ret != -ENOENT) {
 		if (!ret)
diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
index 44edfcd9a6ff..66f2aadd8f0a 100644
--- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
@@ -11,6 +11,23 @@
 #include <linux/slab.h>
 #include "driver.h"
 
+static long sgx_ioc_enclu_register(struct file *filep, unsigned int cmd,
+				   unsigned long arg)
+{
+	struct sgx_enclu_register *reg = (struct sgx_enclu_register *)arg;
+
+	if (reg->enclu == reg->handler)
+		return -EINVAL;
+
+	if (down_write_killable(&current->mm->mmap_sem))
+		return -EINTR;
+	current->mm->context.enclu_address = reg->enclu;
+	current->mm->context.enclu_exception_handler = reg->handler;
+	up_write(&current->mm->mmap_sem);
+
+	return 0;
+}
+
 static int sgx_encl_get(unsigned long addr, struct sgx_encl **encl)
 {
 	struct mm_struct *mm = current->mm;
@@ -317,6 +334,9 @@ long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
 	long ret;
 
 	switch (cmd) {
+	case SGX_IOC_ENCLU_REGISTER:
+		handler = sgx_ioc_enclu_register;
+		break;
 	case SGX_IOC_ENCLAVE_CREATE:
 		handler = sgx_ioc_enclave_create;
 		break;
-- 
2.19.2

Re: [RFC PATCH v3 0/4] x86: Add exception fixup for SGX ENCLU

[Josh Triplett]

On Mon, Dec 10, 2018 at 03:21:37PM -0800, Sean Christopherson wrote:
> At that point I realized it's a hell of a lot easier to simply provide
> an IOCTL via /dev/sgx that allows userspace to register a per-process
> ENCLU exception handler.  At a high level, the basic idea is the same
> as the vDSO approach: provide a hardcoded fixup handler for ENCLU and
> attempt to fixup select unhandled exceptions that occurred in user code.

So, on the one hand, this is *absolutely* much cleaner than the VDSO
approach. On the other hand, this is global process state and has some
of the same problems as a signal handler as a result.

Re: [RFC PATCH v3 0/4] x86: Add exception fixup for SGX ENCLU

[Dr. Greg]

On Mon, Dec 10, 2018 at 03:24:50PM -0800, Josh Triplett wrote:

Good morning to everyone, I hope the week is progressing well.

> On Mon, Dec 10, 2018 at 03:21:37PM -0800, Sean Christopherson wrote:
> > At that point I realized it's a hell of a lot easier to simply provide
> > an IOCTL via /dev/sgx that allows userspace to register a per-process
> > ENCLU exception handler.  At a high level, the basic idea is the same
> > as the vDSO approach: provide a hardcoded fixup handler for ENCLU and
> > attempt to fixup select unhandled exceptions that occurred in user code.

> So, on the one hand, this is *absolutely* much cleaner than the VDSO
> approach. On the other hand, this is global process state and has
> some of the same problems as a signal handler as a result.

Sean's architecture is very simple and straight forward and thus has a
lot going for it.

As Sean's approach indicates, by linking the exception handler to
current->mm, SGX is very much a per memory map concept.  The issue is
that there can be multiple enclaves loaded and excecuting in a
processes memory map, the problem is, execution and thus exception
handling, is very much at the per thread level.

Based on the responses to my previous e-mail in this thread, the
primary driver for addressing the exception handler issue is for
shared libraries to implement independent execution of enclaves.  So
the question for Sean becomes where the 'pull' to address this issue
is coming from, is it the Intel SDK/PSW folks?  If so it might be
helpful for them to weigh in on requirements and needs.

To be very frank, there are only 3-4 groups actually working at this
level; the Intel SDK/PSW team, Fortanix, us and probably Baidu.  Given
the complexity of the issues involved with a runtime, the Linux SGX
development community, whether it be application or library developers
are going to be building on top of runtimes maintained by groups such
as these.

If we boil issues down to the very basics, the setup of an exception
handler is going to have to be wrapped into whatever code is being
executed by a thread doing enclave entry for the runtime being used.
It thus may be useful to think about this as being the responsibility
of whatever loaded and initialized the enclave against which
ENCLU[EENTER] is being issued against.

To add an additional wrinkle to this, our runtime is already doing
what amounts to recursive enclave invocation.  We handle remote
attestation quote generation and verification almost completely inside
of enclaves.  This requires that we issue an OCALL in order to exit
the enclave being attested in order to load and execute the platform
certification and quoting enclaves.

So if Linux is going to deal with this correctly, without invoking any
global state, the issue will come down conceptually to registering a
per-thread/per-EENTER exception handler by whatever mechanism the
kernel chooses to provide.  Which means we need to be thinking about
the ramifications of OCALL's.

So whatever we do has to be simple, straight forward and flexible.  If
not the bike shedding will last until something other then SGX gets
invented... :-)

I hope the above reflections are useful.

Best wishes for a productive day.

Dr. Greg

As always,
Dr. G.W. Wettstein, Ph.D.   Enjellic Systems Development, LLC.
4206 N. 19th Ave.           Specializing in information infra-structure
Fargo, ND  58102            development.
PH: 701-281-1686
FAX: 701-281-3949           EMAIL: greg@enjellic.com
------------------------------------------------------------------------------
"If your doing something the same way you have been doing it for ten years,
 the chances are you are doing it wrong."
                                -- Charles Kettering

Re: [RFC PATCH v3 0/4] x86: Add exception fixup for SGX ENCLU

[Sean Christopherson]

On Tue, Dec 11, 2018 at 08:53:39AM -0600, Dr. Greg wrote:
> On Mon, Dec 10, 2018 at 03:24:50PM -0800, Josh Triplett wrote:
> 
> Good morning to everyone, I hope the week is progressing well.
> 
> > On Mon, Dec 10, 2018 at 03:21:37PM -0800, Sean Christopherson wrote:
> > > At that point I realized it's a hell of a lot easier to simply provide
> > > an IOCTL via /dev/sgx that allows userspace to register a per-process
> > > ENCLU exception handler.  At a high level, the basic idea is the same
> > > as the vDSO approach: provide a hardcoded fixup handler for ENCLU and
> > > attempt to fixup select unhandled exceptions that occurred in user code.
> 
> > So, on the one hand, this is *absolutely* much cleaner than the VDSO
> > approach. On the other hand, this is global process state and has
> > some of the same problems as a signal handler as a result.
> 
> Sean's architecture is very simple and straight forward and thus has a
> lot going for it.
> 
> As Sean's approach indicates, by linking the exception handler to
> current->mm, SGX is very much a per memory map concept.  The issue is
> that there can be multiple enclaves loaded and excecuting in a
> processes memory map, the problem is, execution and thus exception
> handling, is very much at the per thread level.

Right, but is there a need to have a per-thread code page?  The handler
isn't per-process any more than the AEP is per-process.

Re: [RFC PATCH v3 0/4] x86: Add exception fixup for SGX ENCLU

[Andy Lutomirski]

> On Dec 10, 2018, at 3:24 PM, Josh Triplett <josh@joshtriplett.org> wrote:
> 
>> On Mon, Dec 10, 2018 at 03:21:37PM -0800, Sean Christopherson wrote:
>> At that point I realized it's a hell of a lot easier to simply provide
>> an IOCTL via /dev/sgx that allows userspace to register a per-process
>> ENCLU exception handler.  At a high level, the basic idea is the same
>> as the vDSO approach: provide a hardcoded fixup handler for ENCLU and
>> attempt to fixup select unhandled exceptions that occurred in user code.
> 
> So, on the one hand, this is *absolutely* much cleaner than the VDSO
> approach. On the other hand, this is global process state and has some
> of the same problems as a signal handler as a result.

I liked the old version better for this reason and for another reason: while this new one looks very very simple, it still has the hidden complexity that the magic values written to registers in the event of an exception are very much Linux specific.

OTOH, the old approach clobbered more regs than needed, but that’s a easy fix.

Re: [RFC PATCH v3 0/4] x86: Add exception fixup for SGX ENCLU

[Sean Christopherson]

On Tue, Dec 11, 2018 at 07:41:27AM -0800, Andy Lutomirski wrote:
> 
> 
> > On Dec 10, 2018, at 3:24 PM, Josh Triplett <josh@joshtriplett.org> wrote:
> > 
> >> On Mon, Dec 10, 2018 at 03:21:37PM -0800, Sean Christopherson wrote:
> >> At that point I realized it's a hell of a lot easier to simply provide
> >> an IOCTL via /dev/sgx that allows userspace to register a per-process
> >> ENCLU exception handler.  At a high level, the basic idea is the same
> >> as the vDSO approach: provide a hardcoded fixup handler for ENCLU and
> >> attempt to fixup select unhandled exceptions that occurred in user code.
> > 
> > So, on the one hand, this is *absolutely* much cleaner than the VDSO
> > approach. On the other hand, this is global process state and has some
> > of the same problems as a signal handler as a result.
> 
> I liked the old version better for this reason 

This isn't fundamentally different than forcing all EENTER calls through
the vDSO, which is also per-process.  Technically this is more flexible
in that regard since userspace gets to choose where their one ENCLU gets
to reside.  Userspace can have per-enclave entry flows so long as the
actual ENLU[EENTER] is common, same as vDSO.

> and for another reason:
> while this new one looks very very simple, it still has the hidden
> complexity that the magic values written to registers in the event of an
> exception are very much Linux specific.

Definitely more magical, but not necessarily more difficult to document.
It'd essentially be an extension of hardware's AEE/AEP behavior.

> OTOH, the old approach clobbered more regs than needed, but that’s a easy fix.

Re: [RFC PATCH v3 0/4] x86: Add exception fixup for SGX ENCLU

[Andy Lutomirski]

> On Dec 11, 2018, at 8:52 AM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
>
>> On Tue, Dec 11, 2018 at 07:41:27AM -0800, Andy Lutomirski wrote:
>>
>>
>>>> On Dec 10, 2018, at 3:24 PM, Josh Triplett <josh@joshtriplett.org> wrote:
>>>>
>>>> On Mon, Dec 10, 2018 at 03:21:37PM -0800, Sean Christopherson wrote:
>>>> At that point I realized it's a hell of a lot easier to simply provide
>>>> an IOCTL via /dev/sgx that allows userspace to register a per-process
>>>> ENCLU exception handler.  At a high level, the basic idea is the same
>>>> as the vDSO approach: provide a hardcoded fixup handler for ENCLU and
>>>> attempt to fixup select unhandled exceptions that occurred in user code.
>>>
>>> So, on the one hand, this is *absolutely* much cleaner than the VDSO
>>> approach. On the other hand, this is global process state and has some
>>> of the same problems as a signal handler as a result.
>>
>> I liked the old version better for this reason
>
> This isn't fundamentally different than forcing all EENTER calls through
> the vDSO, which is also per-process.  Technically this is more flexible
> in that regard since userspace gets to choose where their one ENCLU gets
> to reside.  Userspace can have per-enclave entry flows so long as the
> actual ENLU[EENTER] is common, same as vDSO.

Right. The problem is that user libraries have a remarkably hard time
agreeing on where their one copy of anything lives.

>
>> and for another reason:
>> while this new one looks very very simple, it still has the hidden
>> complexity that the magic values written to registers in the event of an
>> exception are very much Linux specific.
>
> Definitely more magical, but not necessarily more difficult to document.
> It'd essentially be an extension of hardware's AEE/AEP behavior.
>
>> OTOH, the old approach clobbered more regs than needed, but that’s a easy fix.

Re: [RFC PATCH v3 0/4] x86: Add exception fixup for SGX ENCLU

[Sean Christopherson]

On Tue, Dec 11, 2018 at 09:58:19AM -0800, Andy Lutomirski wrote:
> > On Dec 11, 2018, at 8:52 AM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> >
> >> On Tue, Dec 11, 2018 at 07:41:27AM -0800, Andy Lutomirski wrote:
> >>
> >>
> >>>> On Dec 10, 2018, at 3:24 PM, Josh Triplett <josh@joshtriplett.org> wrote:
> >>>>
> >>>> On Mon, Dec 10, 2018 at 03:21:37PM -0800, Sean Christopherson wrote:
> >>>> At that point I realized it's a hell of a lot easier to simply provide
> >>>> an IOCTL via /dev/sgx that allows userspace to register a per-process
> >>>> ENCLU exception handler.  At a high level, the basic idea is the same
> >>>> as the vDSO approach: provide a hardcoded fixup handler for ENCLU and
> >>>> attempt to fixup select unhandled exceptions that occurred in user code.
> >>>
> >>> So, on the one hand, this is *absolutely* much cleaner than the VDSO
> >>> approach. On the other hand, this is global process state and has some
> >>> of the same problems as a signal handler as a result.
> >>
> >> I liked the old version better for this reason
> >
> > This isn't fundamentally different than forcing all EENTER calls through
> > the vDSO, which is also per-process.  Technically this is more flexible
> > in that regard since userspace gets to choose where their one ENCLU gets
> > to reside.  Userspace can have per-enclave entry flows so long as the
> > actual ENLU[EENTER] is common, same as vDSO.
> 
> Right. The problem is that user libraries have a remarkably hard time
> agreeing on where their one copy of anything lives.

Ah, so vDSO is the mean parent that takes away their kids' toys because
thay haven't learned to share :)

Re: [RFC PATCH v3 0/4] x86: Add exception fixup for SGX ENCLU

[Sean Christopherson]

On Tue, Dec 11, 2018 at 09:58:19AM -0800, Andy Lutomirski wrote:
> > On Dec 11, 2018, at 8:52 AM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> >
> >> On Tue, Dec 11, 2018 at 07:41:27AM -0800, Andy Lutomirski wrote:
> >>
> >>
> >>>> On Dec 10, 2018, at 3:24 PM, Josh Triplett <josh@joshtriplett.org> wrote:
> >>>>
> >>>> On Mon, Dec 10, 2018 at 03:21:37PM -0800, Sean Christopherson wrote:
> >>>> At that point I realized it's a hell of a lot easier to simply provide
> >>>> an IOCTL via /dev/sgx that allows userspace to register a per-process
> >>>> ENCLU exception handler.  At a high level, the basic idea is the same
> >>>> as the vDSO approach: provide a hardcoded fixup handler for ENCLU and
> >>>> attempt to fixup select unhandled exceptions that occurred in user code.
> >>>
> >>> So, on the one hand, this is *absolutely* much cleaner than the VDSO
> >>> approach. On the other hand, this is global process state and has some
> >>> of the same problems as a signal handler as a result.
> >>
> >> I liked the old version better for this reason
> >
> > This isn't fundamentally different than forcing all EENTER calls through
> > the vDSO, which is also per-process.  Technically this is more flexible
> > in that regard since userspace gets to choose where their one ENCLU gets
> > to reside.  Userspace can have per-enclave entry flows so long as the
> > actual ENLU[EENTER] is common, same as vDSO.
> 
> Right. The problem is that user libraries have a remarkably hard time
> agreeing on where their one copy of anything lives.

Are you concerned about userspace shooting themselves in the foot, e.g.
unknowingly overwriting their handler?  Requiring unregister->register
to change the handler would mitigate that issue for the most part.  Or
we could even say it's a write-once property.

That obviously doesn't solve the issue of a userspace application
deliberately using two different libraries to run enclaves in a single
process, but I have a hard time envisioning a scenario where someone
would want to use two different *SGX* libraries in a single process.
Don't most of the signal issue arise due to loading multiple libraries
that provide *different* services needing to handle signals?

Re: [RFC PATCH v3 0/4] x86: Add exception fixup for SGX ENCLU

[Andy Lutomirski]

On Tue, Dec 11, 2018 at 2:23 PM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> On Tue, Dec 11, 2018 at 09:58:19AM -0800, Andy Lutomirski wrote:
> > > On Dec 11, 2018, at 8:52 AM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> > >
> > >> On Tue, Dec 11, 2018 at 07:41:27AM -0800, Andy Lutomirski wrote:
> > >>
> > >>
> > >>>> On Dec 10, 2018, at 3:24 PM, Josh Triplett <josh@joshtriplett.org> wrote:
> > >>>>
> > >>>> On Mon, Dec 10, 2018 at 03:21:37PM -0800, Sean Christopherson wrote:
> > >>>> At that point I realized it's a hell of a lot easier to simply provide
> > >>>> an IOCTL via /dev/sgx that allows userspace to register a per-process
> > >>>> ENCLU exception handler.  At a high level, the basic idea is the same
> > >>>> as the vDSO approach: provide a hardcoded fixup handler for ENCLU and
> > >>>> attempt to fixup select unhandled exceptions that occurred in user code.
> > >>>
> > >>> So, on the one hand, this is *absolutely* much cleaner than the VDSO
> > >>> approach. On the other hand, this is global process state and has some
> > >>> of the same problems as a signal handler as a result.
> > >>
> > >> I liked the old version better for this reason
> > >
> > > This isn't fundamentally different than forcing all EENTER calls through
> > > the vDSO, which is also per-process.  Technically this is more flexible
> > > in that regard since userspace gets to choose where their one ENCLU gets
> > > to reside.  Userspace can have per-enclave entry flows so long as the
> > > actual ENLU[EENTER] is common, same as vDSO.
> >
> > Right. The problem is that user libraries have a remarkably hard time
> > agreeing on where their one copy of anything lives.
>
> Are you concerned about userspace shooting themselves in the foot, e.g.
> unknowingly overwriting their handler?  Requiring unregister->register
> to change the handler would mitigate that issue for the most part.  Or
> we could even say it's a write-once property.
>
> That obviously doesn't solve the issue of a userspace application
> deliberately using two different libraries to run enclaves in a single
> process, but I have a hard time envisioning a scenario where someone
> would want to use two different *SGX* libraries in a single process.
> Don't most of the signal issue arise due to loading multiple libraries
> that provide *different* services needing to handle signals?

I can easily imagine two SGX libraries that know nothing about each
other running in the same process.  One or both could be PKCS#11
modules, for example.

I suspect that Linux will eventually want the ability for libraries to
register exception handlers, but that's not going to get designed and
implemented quickly enough for SGX's initial Linux rollout.  A vDSO
helper like in your earlier series should solve most of the problem
without any contention issues.

Re: [RFC PATCH v3 0/4] x86: Add exception fixup for SGX ENCLU

[Sean Christopherson]

On Tue, Dec 11, 2018 at 03:10:52PM -0800, Andy Lutomirski wrote:
> On Tue, Dec 11, 2018 at 2:23 PM Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
> >
> > On Tue, Dec 11, 2018 at 09:58:19AM -0800, Andy Lutomirski wrote:
> > > > On Dec 11, 2018, at 8:52 AM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> > > >
> > > >> On Tue, Dec 11, 2018 at 07:41:27AM -0800, Andy Lutomirski wrote:
> > > >>
> > > >>
> > > >>>> On Dec 10, 2018, at 3:24 PM, Josh Triplett <josh@joshtriplett.org> wrote:
> > > >>>>
> > > >>>> On Mon, Dec 10, 2018 at 03:21:37PM -0800, Sean Christopherson wrote:
> > > >>>> At that point I realized it's a hell of a lot easier to simply provide
> > > >>>> an IOCTL via /dev/sgx that allows userspace to register a per-process
> > > >>>> ENCLU exception handler.  At a high level, the basic idea is the same
> > > >>>> as the vDSO approach: provide a hardcoded fixup handler for ENCLU and
> > > >>>> attempt to fixup select unhandled exceptions that occurred in user code.
> > > >>>
> > > >>> So, on the one hand, this is *absolutely* much cleaner than the VDSO
> > > >>> approach. On the other hand, this is global process state and has some
> > > >>> of the same problems as a signal handler as a result.
> > > >>
> > > >> I liked the old version better for this reason
> > > >
> > > > This isn't fundamentally different than forcing all EENTER calls through
> > > > the vDSO, which is also per-process.  Technically this is more flexible
> > > > in that regard since userspace gets to choose where their one ENCLU gets
> > > > to reside.  Userspace can have per-enclave entry flows so long as the
> > > > actual ENLU[EENTER] is common, same as vDSO.
> > >
> > > Right. The problem is that user libraries have a remarkably hard time
> > > agreeing on where their one copy of anything lives.
> >
> > Are you concerned about userspace shooting themselves in the foot, e.g.
> > unknowingly overwriting their handler?  Requiring unregister->register
> > to change the handler would mitigate that issue for the most part.  Or
> > we could even say it's a write-once property.
> >
> > That obviously doesn't solve the issue of a userspace application
> > deliberately using two different libraries to run enclaves in a single
> > process, but I have a hard time envisioning a scenario where someone
> > would want to use two different *SGX* libraries in a single process.
> > Don't most of the signal issue arise due to loading multiple libraries
> > that provide *different* services needing to handle signals?
> 
> I can easily imagine two SGX libraries that know nothing about each
> other running in the same process.  One or both could be PKCS#11
> modules, for example.

Argh, wasn't thinking about loading other libraries that would also be
using SGX.

Re: [RFC PATCH v3 0/4] x86: Add exception fixup for SGX ENCLU

[Dr. Greg]

On Tue, Dec 11, 2018 at 03:10:52PM -0800, Andy Lutomirski wrote:

Good evening, I hope the day has gone well for everyone.

> > > > On Dec 11, 2018, at 8:52 AM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> > > >
> > > > This isn't fundamentally different than forcing all EENTER
> > > > calls through the vDSO, which is also per-process.
> > > > Technically this is more flexible in that regard since
> > > > userspace gets to choose where their one ENCLU gets to reside.
> > > > Userspace can have per-enclave entry flows so long as the
> > > > actual ENLU[EENTER] is common, same as vDSO.

> > > Right. The problem is that user libraries have a remarkably hard
> > > time agreeing on where their one copy of anything lives.

> > Are you concerned about userspace shooting themselves in the foot,
> > e.g.  unknowingly overwriting their handler?  Requiring
> > unregister->register to change the handler would mitigate that
> > issue for the most part.  Or we could even say it's a write-once
> > property.
> >
> > That obviously doesn't solve the issue of a userspace application
> > deliberately using two different libraries to run enclaves in a
> > single process, but I have a hard time envisioning a scenario
> > where someone would want to use two different *SGX* libraries in a
> > single process.  Don't most of the signal issue arise due to
> > loading multiple libraries that provide *different* services
> > needing to handle signals?

> I can easily imagine two SGX libraries that know nothing about each
> other running in the same process.  One or both could be PKCS#11
> modules, for example.

Very good, I see that Sean agreed with this down thread.  I was
concerned that our discussion was lacking precision and we were
talking past one another.

> I suspect that Linux will eventually want the ability for libraries
> to register exception handlers, but that's not going to get designed
> and implemented quickly enough for SGX's initial Linux rollout.  A
> vDSO helper like in your earlier series should solve most of the
> problem without any contention issues.

Let me see if I can impart some framework for additional clarity as
discussions proceed forward.

I believe it would be helpful if we could agree to refer to a body of
code, possibly in library form, that loads, initializes and executes
an enclave as an 'SGX runtime'.  In this framework, the term 'library'
refers to code that an application links to for domain specific
functionality, ie. libpkcs11, libkrb5, libsasl.  These 'libraries' may
implement enclaves, using 'SGX runtimes' of their choice, to improve
their security guarantees.

In this model it is the 'SGX runtime' that is responsible for
registering SGX exception handlers under their management.

In order for mainline Linux SGX support to be relevant, it must admit
mutually distrusting 'SGX runtimes' in the same process context.  The
SGX exception handler architecture must also support the notion of
'nested enclave' invocation where an enclave may execute an OCALL and
then go on to execute an enclave, possibly based on a different 'SGX
runtime', before returning into its previous enclave.

Hopefully the above will help assist further discussions.

Have a good evening.

Dr. Greg

As always,
Dr. G.W. Wettstein, Ph.D.   Enjellic Systems Development, LLC.
4206 N. 19th Ave.           Specializing in information infra-structure
Fargo, ND  58102            development.
PH: 701-281-1686
FAX: 701-281-3949           EMAIL: greg@enjellic.com
------------------------------------------------------------------------------
"If you think nobody cares if you're alive, try missing a couple of car
 payments."
                                -- Earl Wilson