[PATCH v2 0/4] selftests/sgx: Harden test enclave

[PATCH v2 0/4] selftests/sgx: Harden test enclave

[Jo Van Bulck]

Hi,

This patch series fixes several issues in the SGX example test enclave:

1. Adhere to enclave programming best practices by sanitizing untrusted
   user inputs (ABI registers and API pointer arguments).
2. Ensure correct behavior with compiler optimizations (gcc -O{1,2,3,s}).

Motivation
==========

While I understand that the bare-metal Intel SGX selftest enclave is
certainly not intended as a full-featured independent production runtime,
it has been noted on this mailing list before that "people are likely to
copy this code for their own enclaves" and that it provides a "great
starting point if you want to do things from scratch" [1]. Indeed, at least
one real-world SGX project (Alibaba Inclavare Containers) appears to build
directly on the Linux selftest enclave as a skeleton [2]. Thus, proper and
complete example code is vital for security-sensitive functionality,
like the selftest example enclave.

The purpose of this patch series is, hence, to make the test enclave adhere
to required enclave programming defensive measures by, to the extent
possible and practical, sanitizing attacker-controlled inputs through
minimal checks. Note that this is in line with the existing check in the
test enclave to guard against buffer overflow of the encl_op_array through
the op.type input.

Proposed changes
================

This patch series adds the minimally required sanitization checks, as well
as makes the test enclave compatible with gcc compiler optimizations. The
added functionality is separated in this patch series as follows:

1. Minimal changes in the enclave entry assembly stub as per the x86-64
   ABI expected by the C compiler. Particularly, sanitize the DF and AC
   bits in RFLAGS, which have been dangerously abused in prior SGX attack
   research [3,4]. Also add a test case to validate the sanitization.

   Note that, compiling the existing, unmodified test enclave on my machine
   (gcc v11.3.0) with -Os optimizations yields assembly code that uses the
   x86 REP string instructions for memcpy/memset. Hence, such a compiled
   test enclave would be directly vulnerable to severe memory-corruption
   attacks by trivially inverting RFLAGS.DF before enclave entry (similar
   to CVE-2019-14565 -- Intel SA-00293 [3]).

   Finally note that the proposed patch does currently _not_ sanitize the
   extended CPU state using XSAVE/XRSTOR, as has been shown in prior attack
   research to be necessary for SGX enclaves using floating-point
   instructions [5]. I found that prior versions of the selftest enclave
   _did_ partially cleanse extended CPU state, but that his functionality
   has been removed, as it was argued that "the test enclave doesn't touch
   state managed by XSAVE, let alone put secrets into said state" [6].
   However, I found that compiling the unmodified test enclave with gcc
   -O{2,3} optimization levels may still use the XMM registers to store
   some intermediate results (i.e., clobber them as allowed per the x86-64
   ABI). Hence, for now, add the -mno-sse compilation option to prevent
   this behavior and add a note to explicitly document the assumption that
   extended state should remain untouched in the selftest enclave.

   This may also be an argument to consider re-adding the XRSTOR
   functionality?

2. Make the selftest enclave aware of its protected ELRANGE: add a linker
   symbol __enclave_base at the start of the enclave binary and reserve
   space for __enclave_size to be filled in by the untrusted loader when
   determining the size of the final enclave image (depending on allocated
   heap etc.). The final value for __enclave_size is filled in before
   actual enclave loading and will be measured as part of MRENCLAVE,
   allowing to trust the size within the enclave validation logic. This
   approach is similar to how this is done in real-world enclave runtimes
   (e.g., Intel SGX-SDK).

3. Add minimal validation logic in the enclave C code to ensure that
   incoming pointer struct arguments properly point outside the enclave
   before dereference, preventing confused-deputy attacks. Use a C macro to
   copy struct arguments fully inside the enclave to avoid time-of-check to
   time-of-use issues for nested pointers. Note that the test enclave
   deliberately allows arbitrary reads/writes in enclave memory through the
   get_from_addr/put_to_addr operations for explicit testing purposes.
   Hence, add an explicit note for this case and only allow remaining
   unchecked pointer dereferences in these functions.

4. Ensure correct behavior under gcc compiler optimizations. Declare
   encl_op_array static to ensure RIP-relative addressing is used to access
   the function-pointer table and rebase the loaded function-pointer
   entries at runtime before jumping. Declare the secinfo structure as
   volatile to ensure the compiler passes an aligned address to ENCLU.

   To ensure future compatibility, it may also be worthwhile to rewrite the
   test framework to exhaustively execute all tests for test_encl.elf
   compiled with all possible gcc optimizations -O{0,1,2,3,s}?

Best,
Jo

[1] https://patchwork.kernel.org/comment/23202425/
[2] https://github.com/inclavare-containers/inclavare-containers/tree/master/rune/libenclave/internal/runtime/pal/skeleton
[3] https://jovanbulck.github.io/files/ccs19-tale.pdf
[4] https://jovanbulck.github.io/files/systex22-abi.pdf
[5] https://jovanbulck.github.io/files/acsac20-fpu.pdf
[6] https://patchwork.kernel.org/comment/23216515/

Changelog
---------

v2
  - Add explanation for added test cases (Jarkko)
  - Rename encl_size_pt and reverse xmas tree ordering (Jarkko)
  - Use static inline functions instead of C macros (Jarkko)

Jo Van Bulck (4):
  selftests/sgx: Harden test enclave ABI
  selftests/sgx: Store base address and size in test enclave
  selftests/sgx: Harden test enclave API
  selftests/sgx: Fix compiler optimizations in test enclave

 tools/testing/selftests/sgx/Makefile          |   2 +-
 tools/testing/selftests/sgx/load.c            |   3 +-
 tools/testing/selftests/sgx/main.c            |  62 ++++++
 tools/testing/selftests/sgx/test_encl.c       | 198 +++++++++++++-----
 tools/testing/selftests/sgx/test_encl.lds     |   1 +
 .../selftests/sgx/test_encl_bootstrap.S       |  29 +++
 6 files changed, 246 insertions(+), 49 deletions(-)

-- 
2.34.1

[PATCH 1/4] selftests/sgx: Harden test enclave ABI

[Jo Van Bulck]

The System V x86-64 ABI used by the C compiler defines certain low-level
CPU configuration registers to be set to expected values upon function
entry. However, SGX enclaves cannot expect the untrusted caller to respect
these ABI conventions. Therefore, adhere to SGX runtime best practices by
sanitizing RFLAGS.DF=0 before transitioning to C code. Additionally
sanitize RFLAGS.AC=0 to protect against known #AC-fault side channels for
unaligned memory accesses.

Note that the test enclave does currently not use any floating-point
instructions (-mno-sse). Hence, keep the code simple by _not_ using XRSTOR
to cleanse extended x87/SSE state.

Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be>
---
 tools/testing/selftests/sgx/Makefile          |  2 +-
 tools/testing/selftests/sgx/main.c            | 25 +++++++++++++++++++
 .../selftests/sgx/test_encl_bootstrap.S       | 12 +++++++++
 3 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/sgx/Makefile b/tools/testing/selftests/sgx/Makefile
index 50aab6b57..c2a13bc6e 100644
--- a/tools/testing/selftests/sgx/Makefile
+++ b/tools/testing/selftests/sgx/Makefile
@@ -14,7 +14,7 @@ endif
 INCLUDES := -I$(top_srcdir)/tools/include
 HOST_CFLAGS := -Wall -Werror -g $(INCLUDES) -fPIC -z noexecstack
 ENCL_CFLAGS := -Wall -Werror -static -nostdlib -nostartfiles -fPIC \
-	       -fno-stack-protector -mrdrnd $(INCLUDES)
+	       -fno-stack-protector -mrdrnd -mno-sse $(INCLUDES)
 
 TEST_CUSTOM_PROGS := $(OUTPUT)/test_sgx
 TEST_FILES := $(OUTPUT)/test_encl.elf
diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
index 9820b3809..9a671dbc6 100644
--- a/tools/testing/selftests/sgx/main.c
+++ b/tools/testing/selftests/sgx/main.c
@@ -307,6 +307,31 @@ TEST_F(enclave, unclobbered_vdso)
 	EXPECT_EQ(self->run.user_data, 0);
 }
 
+/*
+ * Sanity check that the test enclave properly sanitizes untrusted
+ * CPU configuration registers.
+ */
+TEST_F(enclave, poison_args)
+{
+	struct encl_op_header nop_op;
+	uint64_t flags = -1;
+
+	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata));
+
+	memset(&self->run, 0, sizeof(self->run));
+	self->run.tcs = self->encl.encl_base;
+
+	/* attempt ABI register poisoning */
+	nop_op.type = ENCL_OP_NOP;
+	asm("std\n\t");
+	EXPECT_EQ(ENCL_CALL(&nop_op, &self->run, false), 0);
+	asm("pushfq\n\t"		\
+	    "popq %0\n\t"		\
+	    : "=m"(flags) : : );
+	EXPECT_EEXIT(&self->run);
+	EXPECT_EQ(flags & 0x40400, 0);
+}
+
 /*
  * A section metric is concatenated in a way that @low bits 12-31 define the
  * bits 12-31 of the metric and @high bits 0-19 define the bits 32-51 of the
diff --git a/tools/testing/selftests/sgx/test_encl_bootstrap.S b/tools/testing/selftests/sgx/test_encl_bootstrap.S
index 03ae0f57e..3b69fea61 100644
--- a/tools/testing/selftests/sgx/test_encl_bootstrap.S
+++ b/tools/testing/selftests/sgx/test_encl_bootstrap.S
@@ -57,6 +57,18 @@ encl_entry_core:
 	push	%rcx # push the address after EENTER
 	push	%rbx # push the enclave base address
 
+	# Sanitize CPU state: x86-64 ABI requires RFLAGS.DF=0 on function
+	# entry, and we additionally clear RFLAGS.AC to prevent #AC-fault side
+	# channels.
+	# NOTE: Real-world enclave runtimes should also cleanse extended CPU
+	# state (i.e., x87 FPU and SSE/AVX/...) configuration registers,
+	# preferably using XRSTOR. This is _not_ done below to simplify the
+	# test enclave, which does not use any floating-point instructions.
+	cld
+	pushfq
+	andq $~0x40000, (%rsp)
+	popfq
+
 	call	encl_body
 
 	pop	%rbx # pop the enclave base address
-- 
2.34.1

[PATCH 2/4] selftests/sgx: Store base address and size in test enclave

[Jo Van Bulck]

Make the test enclave aware of its protected virtual address range to allow
untrusted pointer argument range checks.

Add a linker symbol for __enclave_base at the start of the enclave binary.
Similar to real-world enclave runtimes, rely on the untrusted loader to
fill in __enclave_size (measured as part of MRENCLAVE), as the final size
of the enclave image is determined during loading.

Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be>
---
 tools/testing/selftests/sgx/load.c            |  3 +-
 tools/testing/selftests/sgx/main.c            | 32 +++++++++++++++++++
 tools/testing/selftests/sgx/test_encl.lds     |  1 +
 .../selftests/sgx/test_encl_bootstrap.S       | 17 ++++++++++
 4 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/sgx/load.c b/tools/testing/selftests/sgx/load.c
index 94bdeac1c..968a656a3 100644
--- a/tools/testing/selftests/sgx/load.c
+++ b/tools/testing/selftests/sgx/load.c
@@ -60,7 +60,8 @@ static bool encl_map_bin(const char *path, struct encl *encl)
 		goto err;
 	}
 
-	bin = mmap(NULL, sb.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
+	/* NOTE: map read|write to allow __enclave_size to be filled in */
+	bin = mmap(NULL, sb.st_size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);
 	if (bin == MAP_FAILED) {
 		perror("enclave executable mmap()");
 		goto err;
diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
index 9a671dbc6..b1a7988c1 100644
--- a/tools/testing/selftests/sgx/main.c
+++ b/tools/testing/selftests/sgx/main.c
@@ -178,6 +178,7 @@ static bool setup_test_encl(unsigned long heap_size, struct encl *encl,
 	Elf64_Sym *sgx_enter_enclave_sym = NULL;
 	struct vdso_symtab symtab;
 	struct encl_segment *seg;
+	uint64_t encl_size_pt;
 	char maps_line[256];
 	FILE *maps_file;
 	unsigned int i;
@@ -189,6 +190,16 @@ static bool setup_test_encl(unsigned long heap_size, struct encl *encl,
 		return false;
 	}
 
+	/*
+	 * Fill in the expected symbol location with the final size of the
+	 * constructed enclave image.
+	 */
+	encl_size_pt = encl_get_entry(encl, "__enclave_size");
+	if (encl_size_pt) {
+		encl_size_pt += (uint64_t) encl->src;
+		*((uint64_t *) encl_size_pt) = encl->encl_size;
+	}
+
 	if (!encl_measure(encl))
 		goto err;
 
@@ -307,6 +318,27 @@ TEST_F(enclave, unclobbered_vdso)
 	EXPECT_EQ(self->run.user_data, 0);
 }
 
+/*
+ * Sanity check that the loader correctly initializes __enclave_size in the
+ * measured enclave image.
+ */
+TEST_F(enclave, init_size)
+{
+	struct encl_op_get_from_addr get_addr_op;
+
+	ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata));
+
+	memset(&self->run, 0, sizeof(self->run));
+	self->run.tcs = self->encl.encl_base;
+
+	get_addr_op.value = 0;
+	get_addr_op.addr = self->encl.encl_base + encl_get_entry(&self->encl, "__enclave_size");
+	get_addr_op.header.type = ENCL_OP_GET_FROM_ADDRESS;
+	EXPECT_EQ(ENCL_CALL(&get_addr_op, &self->run, false), 0);
+	EXPECT_EEXIT(&self->run);
+	EXPECT_EQ(get_addr_op.value, self->encl.encl_size);
+}
+
 /*
  * Sanity check that the test enclave properly sanitizes untrusted
  * CPU configuration registers.
diff --git a/tools/testing/selftests/sgx/test_encl.lds b/tools/testing/selftests/sgx/test_encl.lds
index a1ec64f7d..ca659db2a 100644
--- a/tools/testing/selftests/sgx/test_encl.lds
+++ b/tools/testing/selftests/sgx/test_encl.lds
@@ -10,6 +10,7 @@ PHDRS
 SECTIONS
 {
 	. = 0;
+        __enclave_base = .;
 	.tcs : {
 		*(.tcs*)
 	} : tcs
diff --git a/tools/testing/selftests/sgx/test_encl_bootstrap.S b/tools/testing/selftests/sgx/test_encl_bootstrap.S
index 3b69fea61..444a075c0 100644
--- a/tools/testing/selftests/sgx/test_encl_bootstrap.S
+++ b/tools/testing/selftests/sgx/test_encl_bootstrap.S
@@ -98,6 +98,23 @@ encl_entry_core:
 	mov	$4, %rax
 	enclu
 
+	.global get_enclave_base
+get_enclave_base:
+	lea __enclave_base(%rip), %rax
+	ret
+
+	.global get_enclave_size
+get_enclave_size:
+	mov __enclave_size(%rip), %rax
+	ret
+
+	# The following 8 bytes (measured as part of MRENCLAVE) will be
+	# filled in by the untrusted loader with the total size of the
+	# loaded enclave.
+	.global __enclave_size
+__enclave_size:
+	.quad 0x0
+
 	.section ".data", "aw"
 
 encl_ssa_tcs1:
-- 
2.34.1

[PATCH 3/4] selftests/sgx: Harden test enclave API

[Jo Van Bulck]

Adhere to enclave programming best practices and prevent confused-deputy
attacks on the test enclave by validating that untrusted pointer arguments
do not fall inside the protected enclave range.

Note that the test enclave deliberately allows arbitrary reads/writes in
enclave memory through the get_from_addr/put_to_addr operations for
explicit testing purposes. Hence, only allow remaining unchecked pointer
dereferences in these functions.

Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be>
---
 tools/testing/selftests/sgx/main.c      |   7 +-
 tools/testing/selftests/sgx/test_encl.c | 190 ++++++++++++++++++------
 2 files changed, 152 insertions(+), 45 deletions(-)

diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
index b1a7988c1..5919f5759 100644
--- a/tools/testing/selftests/sgx/main.c
+++ b/tools/testing/selftests/sgx/main.c
@@ -341,7 +341,7 @@ TEST_F(enclave, init_size)
 
 /*
  * Sanity check that the test enclave properly sanitizes untrusted
- * CPU configuration registers.
+ * CPU configuration registers and pointer arguments.
  */
 TEST_F(enclave, poison_args)
 {
@@ -362,6 +362,11 @@ TEST_F(enclave, poison_args)
 	    : "=m"(flags) : : );
 	EXPECT_EEXIT(&self->run);
 	EXPECT_EQ(flags & 0x40400, 0);
+
+	/* attempt API pointer poisoning */
+	EXPECT_EQ(ENCL_CALL(self->encl.encl_base + self->encl.encl_size - 1, &self->run, false), 0);
+	EXPECT_EQ((&self->run)->function, ERESUME);
+	EXPECT_EQ((&self->run)->exception_vector, 6 /* expect ud2 */);
 }
 
 /*
diff --git a/tools/testing/selftests/sgx/test_encl.c b/tools/testing/selftests/sgx/test_encl.c
index c0d639729..ea24cdf9e 100644
--- a/tools/testing/selftests/sgx/test_encl.c
+++ b/tools/testing/selftests/sgx/test_encl.c
@@ -16,69 +16,148 @@ enum sgx_enclu_function {
 	EMODPE = 0x6,
 };
 
+uint64_t get_enclave_base(void);
+uint64_t get_enclave_size(void);
+
+static void *memcpy(void *dest, const void *src, size_t n)
+{
+	size_t i;
+
+	for (i = 0; i < n; i++)
+		((char *)dest)[i] = ((char *)src)[i];
+
+	return dest;
+}
+
+static void *memset(void *dest, int c, size_t n)
+{
+	size_t i;
+
+	for (i = 0; i < n; i++)
+		((char *)dest)[i] = c;
+
+	return dest;
+}
+
+static int is_outside_enclave(void *addr, size_t len)
+{
+	/* need cast since void pointer arithmetics are undefined in C */
+	size_t start = (size_t) addr;
+	size_t end = start + len - 1;
+	size_t enclave_end = get_enclave_base() + get_enclave_size();
+
+	/* check for integer overflow with untrusted length */
+	if (start > end)
+		return 0;
+
+	return (start > enclave_end || end < get_enclave_base());
+}
+
+static int is_inside_enclave(void *addr, size_t len)
+{
+	/* need cast since void pointer arithmetics are undefined in C */
+	size_t start = (size_t) addr;
+	size_t end = start + len - 1;
+	size_t enclave_end = get_enclave_base() + get_enclave_size();
+
+	/* check for integer overflow with untrusted length */
+	if (start > end)
+		return 0;
+
+	return (start >= get_enclave_base() && end <= enclave_end);
+}
+
+static inline void panic(void)
+{
+	asm("ud2\n\t");
+}
+
+/*
+ * Asserts the buffer @src of @len bytes lies entirely outside the enclave
+ * and copies it to @dst to prevent TOCTOU issues.
+ */
+static inline void copy_inside_enclave(void *dst, void *src, size_t len)
+{
+	if (!is_outside_enclave(src, len))
+		panic();
+
+	memcpy(dst, src, len);
+}
+
+/*
+ * Asserts the buffer @dst of @len bytes lies entirely outside the enclave
+ * and fills it with @len bytes from @src.
+ */
+static inline void copy_outside_enclave(void *dst, void *src, size_t len)
+{
+	if (!is_outside_enclave(dst, len))
+		panic();
+
+	memcpy(dst, src, len);
+}
+
+static inline void assert_inside_enclave(uint64_t arg, size_t len)
+{
+	if (!is_inside_enclave((void *) arg, len))
+		panic();
+}
+
 static void do_encl_emodpe(void *_op)
 {
+	struct encl_op_emodpe op;
 	struct sgx_secinfo secinfo __aligned(sizeof(struct sgx_secinfo)) = {0};
-	struct encl_op_emodpe *op = _op;
 
-	secinfo.flags = op->flags;
+	copy_inside_enclave(&op, _op, sizeof(op));
+	assert_inside_enclave(op.epc_addr, PAGE_SIZE);
+
+	secinfo.flags = op.flags;
 
 	asm volatile(".byte 0x0f, 0x01, 0xd7"
 				:
 				: "a" (EMODPE),
 				  "b" (&secinfo),
-				  "c" (op->epc_addr));
+				  "c" (op.epc_addr));
 }
 
 static void do_encl_eaccept(void *_op)
 {
+	struct encl_op_eaccept op;
 	struct sgx_secinfo secinfo __aligned(sizeof(struct sgx_secinfo)) = {0};
-	struct encl_op_eaccept *op = _op;
 	int rax;
 
-	secinfo.flags = op->flags;
+	copy_inside_enclave(&op, _op, sizeof(op));
+	assert_inside_enclave(op.epc_addr, PAGE_SIZE);
+
+	secinfo.flags = op.flags;
 
 	asm volatile(".byte 0x0f, 0x01, 0xd7"
 				: "=a" (rax)
 				: "a" (EACCEPT),
 				  "b" (&secinfo),
-				  "c" (op->epc_addr));
-
-	op->ret = rax;
-}
-
-static void *memcpy(void *dest, const void *src, size_t n)
-{
-	size_t i;
+				  "c" (op.epc_addr));
 
-	for (i = 0; i < n; i++)
-		((char *)dest)[i] = ((char *)src)[i];
-
-	return dest;
-}
-
-static void *memset(void *dest, int c, size_t n)
-{
-	size_t i;
-
-	for (i = 0; i < n; i++)
-		((char *)dest)[i] = c;
-
-	return dest;
+	op.ret = rax;
+	copy_outside_enclave(_op, &op, sizeof(op));
 }
 
 static void do_encl_init_tcs_page(void *_op)
 {
-	struct encl_op_init_tcs_page *op = _op;
-	void *tcs = (void *)op->tcs_page;
+	struct encl_op_init_tcs_page op;
+	void *tcs;
 	uint32_t val_32;
 
+	copy_inside_enclave(&op, _op, sizeof(op));
+	assert_inside_enclave(get_enclave_base() + op.ssa, PAGE_SIZE);
+	assert_inside_enclave(get_enclave_base() + op.entry, 1);
+	assert_inside_enclave(op.tcs_page, PAGE_SIZE);
+
+	tcs = (void *)op.tcs_page;
 	memset(tcs, 0, 16);			/* STATE and FLAGS */
-	memcpy(tcs + 16, &op->ssa, 8);		/* OSSA */
+	memcpy(tcs + 16, &op.ssa, 8);		/* OSSA */
 	memset(tcs + 24, 0, 4);			/* CSSA */
 	val_32 = 1;
 	memcpy(tcs + 28, &val_32, 4);		/* NSSA */
-	memcpy(tcs + 32, &op->entry, 8);	/* OENTRY */
+	memcpy(tcs + 32, &op.entry, 8);		/* OENTRY */
 	memset(tcs + 40, 0, 24);		/* AEP, OFSBASE, OGSBASE */
 	val_32 = 0xFFFFFFFF;
 	memcpy(tcs + 64, &val_32, 4);		/* FSLIMIT */
@@ -86,32 +165,54 @@ static void do_encl_init_tcs_page(void *_op)
 	memset(tcs + 72, 0, 4024);		/* Reserved */
 }
 
-static void do_encl_op_put_to_buf(void *op)
+static void do_encl_op_put_to_buf(void *_op)
 {
-	struct encl_op_put_to_buf *op2 = op;
+	struct encl_op_get_from_buf op;
 
-	memcpy(&encl_buffer[0], &op2->value, 8);
+	copy_inside_enclave(&op, _op, sizeof(op));
+	memcpy(&encl_buffer[0], &op.value, 8);
+	copy_outside_enclave(_op, &op, sizeof(op));
 }
 
-static void do_encl_op_get_from_buf(void *op)
+static void do_encl_op_get_from_buf(void *_op)
 {
-	struct encl_op_get_from_buf *op2 = op;
+	struct encl_op_get_from_buf op;
 
-	memcpy(&op2->value, &encl_buffer[0], 8);
+	copy_inside_enclave(&op, _op, sizeof(op));
+	memcpy(&op.value, &encl_buffer[0], 8);
+	copy_outside_enclave(_op, &op, sizeof(op));
 }
 
 static void do_encl_op_put_to_addr(void *_op)
 {
-	struct encl_op_put_to_addr *op = _op;
+	struct encl_op_put_to_addr op;
 
-	memcpy((void *)op->addr, &op->value, 8);
+	copy_inside_enclave(&op, _op, sizeof(op));
+
+	/*
+	 * NOTE: not checking is_outside_enclave(op.addr, 8) here
+	 * deliberately allows arbitrary writes to enclave memory for
+	 * testing purposes.
+	 */
+	memcpy((void *)op.addr, &op.value, 8);
+
+	copy_outside_enclave(_op, &op, sizeof(op));
 }
 
 static void do_encl_op_get_from_addr(void *_op)
 {
-	struct encl_op_get_from_addr *op = _op;
+	struct encl_op_get_from_addr op;
+
+	copy_inside_enclave(&op, _op, sizeof(op));
+
+	/*
+	 * NOTE: not checking is_outside_enclave(op.addr, 8) here
+	 * deliberately allows arbitrary reads from enclave memory for
+	 * testing purposes.
+	 */
+	memcpy(&op.value, (void *)op.addr, 8);
 
-	memcpy(&op->value, (void *)op->addr, 8);
+	copy_outside_enclave(_op, &op, sizeof(op));
 }
 
 static void do_encl_op_nop(void *_op)
@@ -131,9 +232,10 @@ void encl_body(void *rdi,  void *rsi)
 		do_encl_emodpe,
 		do_encl_init_tcs_page,
 	};
+	struct encl_op_header op;
 
-	struct encl_op_header *op = (struct encl_op_header *)rdi;
+	copy_inside_enclave(&op, rdi, sizeof(op));
 
-	if (op->type < ENCL_OP_MAX)
-		(*encl_op_array[op->type])(op);
+	if (op.type < ENCL_OP_MAX)
+		(*encl_op_array[op.type])(rdi);
 }
-- 
2.34.1

[PATCH 4/4] selftests/sgx: Fix compiler optimizations in test enclave

[Jo Van Bulck]

Relocate encl_op_array entries at runtime relative to the enclave base to
ensure correct function pointer when compiling the test enclave with -Os.

Declare the secinfo struct as volatile to prevent compiler optimizations
from passing an unaligned pointer to ENCLU.

Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be>
---
 tools/testing/selftests/sgx/test_encl.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/sgx/test_encl.c b/tools/testing/selftests/sgx/test_encl.c
index ea24cdf9e..b580c37f3 100644
--- a/tools/testing/selftests/sgx/test_encl.c
+++ b/tools/testing/selftests/sgx/test_encl.c
@@ -105,7 +105,8 @@ static inline void assert_inside_enclave(uint64_t arg, size_t len)
 static void do_encl_emodpe(void *_op)
 {
 	struct encl_op_emodpe op;
-	struct sgx_secinfo secinfo __aligned(sizeof(struct sgx_secinfo)) = {0};
+	/* declare secinfo volatile to preserve alignment */
+	volatile struct __aligned(sizeof(struct sgx_secinfo)) sgx_secinfo secinfo = {0};
 
 	copy_inside_enclave(&op, _op, sizeof(op));
 	assert_inside_enclave(op.epc_addr, PAGE_SIZE);
@@ -122,8 +123,9 @@ static void do_encl_emodpe(void *_op)
 static void do_encl_eaccept(void *_op)
 {
 	struct encl_op_eaccept op;
-	struct sgx_secinfo secinfo __aligned(sizeof(struct sgx_secinfo)) = {0};
 	int rax;
+	/* declare secinfo volatile to preserve alignment */
+	volatile struct __aligned(sizeof(struct sgx_secinfo)) sgx_secinfo secinfo = {0};
 
 	copy_inside_enclave(&op, _op, sizeof(op));
 	assert_inside_enclave(op.epc_addr, PAGE_SIZE);
@@ -222,7 +224,7 @@ static void do_encl_op_nop(void *_op)
 
 void encl_body(void *rdi,  void *rsi)
 {
-	const void (*encl_op_array[ENCL_OP_MAX])(void *) = {
+	static const void (*encl_op_array[ENCL_OP_MAX])(void *) = {
 		do_encl_op_put_to_buf,
 		do_encl_op_get_from_buf,
 		do_encl_op_put_to_addr,
@@ -237,5 +239,5 @@ void encl_body(void *rdi,  void *rsi)
 	copy_inside_enclave(&op, rdi, sizeof(op));
 
 	if (op.type < ENCL_OP_MAX)
-		(*encl_op_array[op.type])(rdi);
+		(*(get_enclave_base() + encl_op_array[op.type]))(rdi);
 }
-- 
2.34.1

Re: [PATCH v2 0/4] selftests/sgx: Harden test enclave

[Dave Hansen]

On 7/20/23 15:16, Jo Van Bulck wrote:
> While I understand that the bare-metal Intel SGX selftest enclave is
> certainly not intended as a full-featured independent production runtime,
> it has been noted on this mailing list before that "people are likely to
> copy this code for their own enclaves" and that it provides a "great
> starting point if you want to do things from scratch" [1].

I wholeheartedly agree with the desire to spin up enclaves without the
overhead or complexity of the SDK.  I think I'm the one that asked for
this test enclave in the first place.  There *IS* a gap here.  Those who
care about SGX would be wise to close this gap in _some_ way.

But I don't think the kernel should be the place this is done.  The
kernel should not be hosting a real-world (userspace) SGX reference
implementation.

I'd fully support if you'd like to take the selftest code, fork it, and
maintain it.  The SGX ecosystem would be better off if such a project
existed.  If I can help here in some way like (trying to) release the
SGX selftest under a different license, please let me know.

The only patches I want for the kernel are to make the test enclave more
*obviously* insecure.

So, it's a NAK from me for this series.  I won't support merging this
into the kernel.  But at the same time, I'm very sympathetic to your
cause, and I do appreciate your effort here.

Re: [PATCH v2 0/4] selftests/sgx: Harden test enclave

[Jo Van Bulck]

On 21.07.23 02:24, Dave Hansen wrote:
> I wholeheartedly agree with the desire to spin up enclaves without the
> overhead or complexity of the SDK.  I think I'm the one that asked for
> this test enclave in the first place.  There *IS* a gap here.  Those who
> care about SGX would be wise to close this gap in _some_ way.
> 
> But I don't think the kernel should be the place this is done.  The
> kernel should not be hosting a real-world (userspace) SGX reference
> implementation.

Okay, makes sense.

> I'd fully support if you'd like to take the selftest code, fork it, and
> maintain it.  The SGX ecosystem would be better off if such a project
> existed.  If I can help here in some way like (trying to) release the
> SGX selftest under a different license, please let me know.

Thank you! I agree this would benefit the SGX ecosystem and I'll go 
ahead with further developing such a standalone fork when I find time 
probably in the next month or so. For future reference, in case people 
end up reading this discussion thread, I created a placeholder (atm 
emtpy) repo here:

https://github.com/jovanbulck/bare-sgx

Re licensing: no need to re-license, I think GPL would be the best 
license for such a project anyway.

> The only patches I want for the kernel are to make the test enclave more
> *obviously* insecure.

Makes sense. I'll see if I can create a new proposed minimal patch in 
this spirit (e.g., removing existing register cleansing and adding an 
explicit comment) to take away any misguided impression that the test 
enclave would be a representative example of secure code and make its 
real purpose clearer.

> So, it's a NAK from me for this series.  I won't support merging this
> into the kernel.  But at the same time, I'm very sympathetic to your
> cause, and I do appreciate your effort here.

Thank you, appreciated!