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

[PATCH 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]. 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 [2,3]. 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 [4]. 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" [5].
   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://jovanbulck.github.io/files/ccs19-tale.pdf
[3] https://jovanbulck.github.io/files/systex22-abi.pdf
[4] https://jovanbulck.github.io/files/acsac20-fpu.pdf
[5] https://patchwork.kernel.org/comment/23216515/

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            |  55 ++++++
 tools/testing/selftests/sgx/test_encl.c       | 165 +++++++++++++-----
 tools/testing/selftests/sgx/test_encl.lds     |   1 +
 .../selftests/sgx/test_encl_bootstrap.S       |  29 +++
 6 files changed, 214 insertions(+), 41 deletions(-)


base-commit: 1a2945f27157825a561be7840023e3664111ab2f
-- 
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            | 21 +++++++++++++++++++
 .../selftests/sgx/test_encl_bootstrap.S       | 12 +++++++++++
 3 files changed, 34 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..d3c7a39f4 100644
--- a/tools/testing/selftests/sgx/main.c
+++ b/tools/testing/selftests/sgx/main.c
@@ -307,6 +307,27 @@ TEST_F(enclave, unclobbered_vdso)
 	EXPECT_EQ(self->run.user_data, 0);
 }
 
+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            | 29 +++++++++++++++++++
 tools/testing/selftests/sgx/test_encl.lds     |  1 +
 .../selftests/sgx/test_encl_bootstrap.S       | 17 +++++++++++
 4 files changed, 49 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 d3c7a39f4..bad963c79 100644
--- a/tools/testing/selftests/sgx/main.c
+++ b/tools/testing/selftests/sgx/main.c
@@ -182,6 +182,7 @@ static bool setup_test_encl(unsigned long heap_size, struct encl *encl,
 	FILE *maps_file;
 	unsigned int i;
 	void *addr;
+	uint64_t encl_size_addr;
 
 	if (!encl_load("test_encl.elf", encl, heap_size)) {
 		encl_delete(encl);
@@ -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_addr = encl_get_entry(encl, "__enclave_size");
+	if (encl_size_addr) {
+		encl_size_addr += (uint64_t) encl->src;
+		*((uint64_t *) encl_size_addr) = encl->encl_size;
+	}
+
 	if (!encl_measure(encl))
 		goto err;
 
@@ -307,6 +318,24 @@ TEST_F(enclave, unclobbered_vdso)
 	EXPECT_EQ(self->run.user_data, 0);
 }
 
+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;
+
+	/* __enclave_size is initialized by loader in measured enclave image */
+	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);
+}
+
 TEST_F(enclave, poison_args)
 {
 	struct encl_op_header nop_op;
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      |   5 +
 tools/testing/selftests/sgx/test_encl.c | 161 ++++++++++++++++++------
 2 files changed, 128 insertions(+), 38 deletions(-)

diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
index bad963c79..8d60f8dcd 100644
--- a/tools/testing/selftests/sgx/main.c
+++ b/tools/testing/selftests/sgx/main.c
@@ -355,6 +355,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..5531f5d48 100644
--- a/tools/testing/selftests/sgx/test_encl.c
+++ b/tools/testing/selftests/sgx/test_encl.c
@@ -16,37 +16,55 @@ enum sgx_enclu_function {
 	EMODPE = 0x6,
 };
 
-static void do_encl_emodpe(void *_op)
+uint64_t get_enclave_base(void);
+uint64_t get_enclave_size(void);
+
+static int is_outside_enclave(void *addr, size_t len)
 {
-	struct sgx_secinfo secinfo __aligned(sizeof(struct sgx_secinfo)) = {0};
-	struct encl_op_emodpe *op = _op;
+	/* 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();
 
-	secinfo.flags = op->flags;
+	/* check for integer overflow with untrusted length */
+	if (start > end)
+		return 0;
 
-	asm volatile(".byte 0x0f, 0x01, 0xd7"
-				:
-				: "a" (EMODPE),
-				  "b" (&secinfo),
-				  "c" (op->epc_addr));
+	return (start > enclave_end || end < get_enclave_base());
 }
 
-static void do_encl_eaccept(void *_op)
+static int is_inside_enclave(void *addr, size_t len)
 {
-	struct sgx_secinfo secinfo __aligned(sizeof(struct sgx_secinfo)) = {0};
-	struct encl_op_eaccept *op = _op;
-	int rax;
+	/* 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();
 
-	secinfo.flags = op->flags;
+	/* check for integer overflow with untrusted length */
+	if (start > end)
+		return 0;
 
-	asm volatile(".byte 0x0f, 0x01, 0xd7"
-				: "=a" (rax)
-				: "a" (EACCEPT),
-				  "b" (&secinfo),
-				  "c" (op->epc_addr));
-
-	op->ret = rax;
+	return (start >= get_enclave_base() && end <= enclave_end);
 }
 
+#define PANIC()								\
+	asm("ud2\n\t")
+
+#define SAFE_COPY_STRUCT(u_arg, t_cp)					\
+	do {								\
+		/* 1. check if the argument lies entirely outside */	\
+		if (!is_outside_enclave((void *)u_arg, sizeof(*t_cp)))	\
+			PANIC();					\
+		/* 2. copy the argument inside to prevent TOCTOU */	\
+		memcpy(t_cp, u_arg, sizeof(*t_cp));			\
+	} while (0)
+
+#define ASSERT_INSIDE_ENCLAVE(u_arg, size)				\
+	do {								\
+		if (!is_inside_enclave(((void *)(u_arg)), size))	\
+			PANIC();					\
+	} while (0)
+
 static void *memcpy(void *dest, const void *src, size_t n)
 {
 	size_t i;
@@ -67,18 +85,62 @@ static void *memset(void *dest, int c, size_t n)
 	return dest;
 }
 
+static void do_encl_emodpe(void *_op)
+{
+	struct encl_op_emodpe op;
+	struct sgx_secinfo secinfo __aligned(sizeof(struct sgx_secinfo)) = {0};
+
+	SAFE_COPY_STRUCT(_op, &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));
+}
+
+static void do_encl_eaccept(void *_op)
+{
+	struct encl_op_eaccept op;
+	struct sgx_secinfo secinfo __aligned(sizeof(struct sgx_secinfo)) = {0};
+	int rax;
+
+	SAFE_COPY_STRUCT(_op, &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;
+	memcpy(_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;
 
+	SAFE_COPY_STRUCT(_op, &op);
+	tcs = (void *)op.tcs_page;
+	ASSERT_INSIDE_ENCLAVE(tcs, PAGE_SIZE);
+	ASSERT_INSIDE_ENCLAVE(get_enclave_base() + op.ssa, PAGE_SIZE);
+	ASSERT_INSIDE_ENCLAVE(get_enclave_base() + op.entry, 1);
+
 	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 +148,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;
+
+	SAFE_COPY_STRUCT(_op, &op);
 
-	memcpy(&encl_buffer[0], &op2->value, 8);
+	memcpy(&encl_buffer[0], &op.value, 8);
+	memcpy(_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);
+	SAFE_COPY_STRUCT(_op, &op);
+
+	memcpy(&op.value, &encl_buffer[0], 8);
+	memcpy(_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;
+
+	SAFE_COPY_STRUCT(_op, &op);
 
-	memcpy((void *)op->addr, &op->value, 8);
+	/*
+	 * 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);
+	memcpy(_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;
+
+	SAFE_COPY_STRUCT(_op, &op);
 
-	memcpy(&op->value, (void *)op->addr, 8);
+	/*
+	 * 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, &op, sizeof(op));
 }
 
 static void do_encl_op_nop(void *_op)
@@ -131,9 +215,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;
+	SAFE_COPY_STRUCT(rdi, &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 5531f5d48..d78596ccf 100644
--- a/tools/testing/selftests/sgx/test_encl.c
+++ b/tools/testing/selftests/sgx/test_encl.c
@@ -88,7 +88,8 @@ static void *memset(void *dest, int c, size_t n)
 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};
 
 	SAFE_COPY_STRUCT(_op, &op);
 	ASSERT_INSIDE_ENCLAVE(op.epc_addr, PAGE_SIZE);
@@ -105,8 +106,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};
 
 	SAFE_COPY_STRUCT(_op, &op);
 	ASSERT_INSIDE_ENCLAVE(op.epc_addr, PAGE_SIZE);
@@ -205,7 +207,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,
@@ -220,5 +222,5 @@ void encl_body(void *rdi,  void *rsi)
 	SAFE_COPY_STRUCT(rdi, &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 0/4] selftests/sgx: Harden test enclave

[Jarkko Sakkinen]

On Wed Jul 19, 2023 at 5:24 PM EEST, 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]. Thus, proper and
> complete example code is vital for security-sensitive functionality, like the
> selftest example enclave.

If anyone copied the source code for their own enclave, they would have
to publish their source code, given the GPLv2 license.

There's a lot of source code in kselftest, which probably has at least
some security issues.

I'm not sure, at least based on this motivation, why would we care?

BR, Jarkko

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

[Jarkko Sakkinen]

On Wed Jul 19, 2023 at 5:24 PM EEST, Jo Van Bulck wrote:
> 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            | 21 +++++++++++++++++++
>  .../selftests/sgx/test_encl_bootstrap.S       | 12 +++++++++++
>  3 files changed, 34 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..d3c7a39f4 100644
> --- a/tools/testing/selftests/sgx/main.c
> +++ b/tools/testing/selftests/sgx/main.c
> @@ -307,6 +307,27 @@ TEST_F(enclave, unclobbered_vdso)
>  	EXPECT_EQ(self->run.user_data, 0);
>  }
>  

Since the amount of tests is increasing over time, I'd put here:

/*
 * Explanation what the test does and why it exists.
 */
> +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


BR, Jarkko

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

[Jarkko Sakkinen]

On Wed Jul 19, 2023 at 5:24 PM EEST, Jo Van Bulck wrote:
> 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            | 29 +++++++++++++++++++
>  tools/testing/selftests/sgx/test_encl.lds     |  1 +
>  .../selftests/sgx/test_encl_bootstrap.S       | 17 +++++++++++
>  4 files changed, 49 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 d3c7a39f4..bad963c79 100644
> --- a/tools/testing/selftests/sgx/main.c
> +++ b/tools/testing/selftests/sgx/main.c
> @@ -182,6 +182,7 @@ static bool setup_test_encl(unsigned long heap_size, struct encl *encl,
>  	FILE *maps_file;
>  	unsigned int i;
>  	void *addr;
> +	uint64_t encl_size_addr;

Should be the first declaration (reverse xmas tree order).

I'd rename this as encl_end, as the current name is cryptic.

>  
>  	if (!encl_load("test_encl.elf", encl, heap_size)) {
>  		encl_delete(encl);
> @@ -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_addr = encl_get_entry(encl, "__enclave_size");
> +	if (encl_size_addr) {
> +		encl_size_addr += (uint64_t) encl->src;
> +		*((uint64_t *) encl_size_addr) = encl->encl_size;
> +	}
> +
>  	if (!encl_measure(encl))
>  		goto err;
>  
> @@ -307,6 +318,24 @@ TEST_F(enclave, unclobbered_vdso)
>  	EXPECT_EQ(self->run.user_data, 0);
>  }
>  
> +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;
> +
> +	/* __enclave_size is initialized by loader in measured enclave image */
> +	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);
> +}
> +
>  TEST_F(enclave, poison_args)
>  {
>  	struct encl_op_header nop_op;
> 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


BR, Jarkko

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

[Jarkko Sakkinen]

On Wed Jul 19, 2023 at 5:24 PM EEST, Jo Van Bulck wrote:
> 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      |   5 +
>  tools/testing/selftests/sgx/test_encl.c | 161 ++++++++++++++++++------
>  2 files changed, 128 insertions(+), 38 deletions(-)
>
> diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
> index bad963c79..8d60f8dcd 100644
> --- a/tools/testing/selftests/sgx/main.c
> +++ b/tools/testing/selftests/sgx/main.c
> @@ -355,6 +355,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..5531f5d48 100644
> --- a/tools/testing/selftests/sgx/test_encl.c
> +++ b/tools/testing/selftests/sgx/test_encl.c
> @@ -16,37 +16,55 @@ enum sgx_enclu_function {
>  	EMODPE = 0x6,
>  };
>  
> -static void do_encl_emodpe(void *_op)
> +uint64_t get_enclave_base(void);
> +uint64_t get_enclave_size(void);
> +
> +static int is_outside_enclave(void *addr, size_t len)
>  {
> -	struct sgx_secinfo secinfo __aligned(sizeof(struct sgx_secinfo)) = {0};
> -	struct encl_op_emodpe *op = _op;
> +	/* 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();
>  
> -	secinfo.flags = op->flags;
> +	/* check for integer overflow with untrusted length */
> +	if (start > end)
> +		return 0;
>  
> -	asm volatile(".byte 0x0f, 0x01, 0xd7"
> -				:
> -				: "a" (EMODPE),
> -				  "b" (&secinfo),
> -				  "c" (op->epc_addr));
> +	return (start > enclave_end || end < get_enclave_base());
>  }
>  
> -static void do_encl_eaccept(void *_op)
> +static int is_inside_enclave(void *addr, size_t len)
>  {
> -	struct sgx_secinfo secinfo __aligned(sizeof(struct sgx_secinfo)) = {0};
> -	struct encl_op_eaccept *op = _op;
> -	int rax;
> +	/* 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();
>  
> -	secinfo.flags = op->flags;
> +	/* check for integer overflow with untrusted length */
> +	if (start > end)
> +		return 0;
>  
> -	asm volatile(".byte 0x0f, 0x01, 0xd7"
> -				: "=a" (rax)
> -				: "a" (EACCEPT),
> -				  "b" (&secinfo),
> -				  "c" (op->epc_addr));
> -
> -	op->ret = rax;
> +	return (start >= get_enclave_base() && end <= enclave_end);
>  }
>  
> +#define PANIC()								\
> +	asm("ud2\n\t")
> +
> +#define SAFE_COPY_STRUCT(u_arg, t_cp)					\

any reason not to use static inline function?

> +	do {								\
> +		/* 1. check if the argument lies entirely outside */	\
> +		if (!is_outside_enclave((void *)u_arg, sizeof(*t_cp)))	\
> +			PANIC();					\
> +		/* 2. copy the argument inside to prevent TOCTOU */	\
> +		memcpy(t_cp, u_arg, sizeof(*t_cp));			\
> +	} while (0)
> +
> +#define ASSERT_INSIDE_ENCLAVE(u_arg, size)				\

"

> +	do {								\
> +		if (!is_inside_enclave(((void *)(u_arg)), size))	\
> +			PANIC();					\
> +	} while (0)
> +
>  static void *memcpy(void *dest, const void *src, size_t n)
>  {
>  	size_t i;
> @@ -67,18 +85,62 @@ static void *memset(void *dest, int c, size_t n)
>  	return dest;
>  }
>  
> +static void do_encl_emodpe(void *_op)
> +{
> +	struct encl_op_emodpe op;
> +	struct sgx_secinfo secinfo __aligned(sizeof(struct sgx_secinfo)) = {0};
> +
> +	SAFE_COPY_STRUCT(_op, &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));
> +}
> +
> +static void do_encl_eaccept(void *_op)
> +{
> +	struct encl_op_eaccept op;
> +	struct sgx_secinfo secinfo __aligned(sizeof(struct sgx_secinfo)) = {0};
> +	int rax;
> +
> +	SAFE_COPY_STRUCT(_op, &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;
> +	memcpy(_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;
>  
> +	SAFE_COPY_STRUCT(_op, &op);
> +	tcs = (void *)op.tcs_page;
> +	ASSERT_INSIDE_ENCLAVE(tcs, PAGE_SIZE);
> +	ASSERT_INSIDE_ENCLAVE(get_enclave_base() + op.ssa, PAGE_SIZE);
> +	ASSERT_INSIDE_ENCLAVE(get_enclave_base() + op.entry, 1);
> +
>  	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 +148,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;
> +
> +	SAFE_COPY_STRUCT(_op, &op);
>  
> -	memcpy(&encl_buffer[0], &op2->value, 8);
> +	memcpy(&encl_buffer[0], &op.value, 8);
> +	memcpy(_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);
> +	SAFE_COPY_STRUCT(_op, &op);
> +
> +	memcpy(&op.value, &encl_buffer[0], 8);
> +	memcpy(_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;
> +
> +	SAFE_COPY_STRUCT(_op, &op);
>  
> -	memcpy((void *)op->addr, &op->value, 8);
> +	/*
> +	 * 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);
> +	memcpy(_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;
> +
> +	SAFE_COPY_STRUCT(_op, &op);
>  
> -	memcpy(&op->value, (void *)op->addr, 8);
> +	/*
> +	 * 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, &op, sizeof(op));
>  }
>  
>  static void do_encl_op_nop(void *_op)
> @@ -131,9 +215,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;
> +	SAFE_COPY_STRUCT(rdi, &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

BR, Jarkko

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

[Jo Van Bulck]

On 20.07.23 19:25, Jarkko Sakkinen wrote:
> There's a lot of source code in kselftest, which probably has at least
> some security issues.
> 
> I'm not sure, at least based on this motivation, why would we care?

I'd argue that, in general, code examples are often used as templates 
and may thus inherit any vulnerabilities therein. This may be especially 
relevant here as your selftest enclave is in my knowledge the only 
available truly minimal SGX enclave that can be built and extended while 
only relying on standard tools and no heavy frameworks like the Intel 
SGX SDK. Thus, as noted before on this mailing list, it may be an 
attractive start for people who want to build things from scratch.

IMHO the example enclave should do a best effort to reasonably follow 
SGX coding best practices and not have _known_ security vulnerabilities 
in it. Note that these are not advanced microarchitectural attacks with 
ugly LFENCE defenses, but plain, architectural memory-safety exploit 
preventions with minimal sanitization checks, not unlike the existing 
protections against buffer overflow where best practices are followed 
for op->type.

Apart from that, the added checks only enforce correct behavior in the 
test framework, only validating that things are sane and as expected. 
Thus, to some extent, the added checks may even increase resilience of 
the test framework.

Best,
Jo

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

[Jo Van Bulck]

On 20.07.23 19:27, Jarkko Sakkinen wrote:
> 
> Since the amount of tests is increasing over time, I'd put here:
> 
> /*
>   * Explanation what the test does and why it exists.
>   */
>> +TEST_F(enclave, poison_args)

Thank you for the code review! Will add.

Best,
Jo

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

[Jo Van Bulck]

On 20.07.23 19:29, Jarkko Sakkinen wrote:

>> diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
>> index d3c7a39f4..bad963c79 100644
>> --- a/tools/testing/selftests/sgx/main.c
>> +++ b/tools/testing/selftests/sgx/main.c
>> @@ -182,6 +1827 @@ static bool setup_test_encl(unsigned long heap_size, struct encl *encl,
>>   	FILE *maps_file;
>>   	unsigned int i;
>>   	void *addr;
>> +	uint64_t encl_size_addr;
> 
> Should be the first declaration (reverse xmas tree order).

Will do

> I'd rename this as encl_end, as the current name is cryptic.

Thank you, agreed the current name is cryptic. However, encl_end is 
incorrect as this is the location in the enclave that stores the _size_ 
and the location itself is also _not_ at the end of the enclave.

I'll rename this in the next patch revision to encl_size_pt and make it 
of type uint64_t*, which seems more logical to me. Let me know if you 
prefer otherwise!

Best,
Jo

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

[Jo Van Bulck]

On 20.07.23 19:32, Jarkko Sakkinen wrote:

>> +#define PANIC()								\
>> +	asm("ud2\n\t")
> 
> any reason not to use static inline function?
>

Thanks for the suggestion, no reason in this case. Apart perhaps that 
it's only 1 line of code and an inline function may seem a bit like a 
waste (given that gcc does not inline until certain optimization 
levels). I can surely change it to static inline void panic(void) if you 
prefer?

>> +#define SAFE_COPY_STRUCT(u_arg, t_cp)					\ >> +	do {								\
>> +		/* 1. check if the argument lies entirely outside */	\
>> +		if (!is_outside_enclave((void *)u_arg, sizeof(*t_cp)))	\
>> +			PANIC();					\
>> +		/* 2. copy the argument inside to prevent TOCTOU */	\
>> +		memcpy(t_cp, u_arg, sizeof(*t_cp));			\
>> +	} while (0)
>> +

This could be made into a static inline function, but then t_cp would 
have to be type void* and sizeof(*t_cp) won't work anymore and a third 
parameter to pass the sizeof would be needed, which would require the 
caller to pass it correctly. Hence, a macro seems "safer" to me here in 
this instance as it requires only 2 arguments. Agreed?

>> +#define ASSERT_INSIDE_ENCLAVE(u_arg, size)				\
>> +	do {								\
>> +		if (!is_inside_enclave(((void *)(u_arg)), size))	\
>> +			PANIC();					\
>> +	} while (0)

This macro could certainly be turned into a static inline void function 
if preferred.

Best,
Jo

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

[Dave Hansen]

On 7/20/23 12:12, Jo Van Bulck wrote:
> On 20.07.23 19:25, Jarkko Sakkinen wrote:
>> There's a lot of source code in kselftest, which probably has at least
>> some security issues.
>>
>> I'm not sure, at least based on this motivation, why would we care?
> 
> I'd argue that, in general, code examples are often used as templates
> and may thus inherit any vulnerabilities therein. This may be especially
> relevant here as your selftest enclave is in my knowledge the only
> available truly minimal SGX enclave that can be built and extended while
> only relying on standard tools and no heavy frameworks like the Intel
> SGX SDK. Thus, as noted before on this mailing list, it may be an
> attractive start for people who want to build things from scratch.
> 
> IMHO the example enclave should do a best effort to reasonably follow
> SGX coding best practices and not have _known_ security vulnerabilities
> in it.

On the other hand, if we don't leave glaring, known "security"
vulnerabilities in it, even more people will be fooled into trying to
use our example code for something that needs actual security.

I personally don't know the first thing about writing a secure enclave.
I just know it's _really_ hard and I honestly don't expect someone to do
it without the help of the SDK.

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

[Jo Van Bulck]

On 20.07.23 21:56, Dave Hansen wrote:
> On the other hand, if we don't leave glaring, known "security"
> vulnerabilities in it, even more people will be fooled into trying to
> use our example code for something that needs actual security.

I see the reasoning, but I'm afraid it's generally hard to stop people 
from copying good examples as templates for their own projects..

I do believe in the value of clean, minimal open-source example enclave 
code. In this respect, I personally (and others on the past mailing list 
as well, it seems) really like the minimal self-contained Linux selftest 
enclave! I think, with the fixes in this patch series, the Linux 
selftest enclave can continue to bring value to the community and help 
in further diversifying the open-source SGX ecosystem.

FWIW, I'd not call these "glaring" security holes, but rather subtle 
oversights that I think most people who would copy the code today may 
well not be aware of and inherit unknowingly (e.g., reference [2] from 
my original message did a wide-scale study of the open-source SGX 
ecosystem as of 2019 and showed exactly these kinds of ABI/API 
vulnerabilities were widespread and re-occurring in several SGX 
production projects).

> I personally don't know the first thing about writing a secure enclave.
> I just know it's _really_ hard and I honestly don't expect someone to do
> it without the help of the SDK.

Agreed, it _is_ hard indeed. And it has been a moving target over the 
years, especially with software/compiler defenses for different waves of 
microarchitectural vulnerabilities (Spectre, LVI, etc.). That being 
said, I do think that we learned a lot as a community and we have a much 
better grasp on how to write (reasonably) secure enclave software these 
days. Sanitizing the ABI and API remains a core enclave software 
responsibility (whereas microarchitectural attacks can arguably be 
mostly mitigated through hardware silicon/ucode patches and/or automatic 
compiler mitigations).

I do agree that sane end users should use a shielding runtime to 
abstract away most of these concerns, where the Intel SGX SDK is just 
one of many in a growing open-source SGX ecosystem (see for instance 
earlier references [2,3] for an overview).

But there may be good reasons to want to do things from scratch when 
building your own SGX shielding runtime, e.g., support for custom 
languages (Rust, Go) or library OSs. I think the Linux selftest enclave 
helps in further diversifying the open-source SGX ecosystem and can 
provide an excellent starting point (with the fixes in this patch series).

Best,
Jo

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

[Jarkko Sakkinen]

On Thu Jul 20, 2023 at 7:12 PM UTC, Jo Van Bulck wrote:
> On 20.07.23 19:25, Jarkko Sakkinen wrote:
> > There's a lot of source code in kselftest, which probably has at least
> > some security issues.
> > 
> > I'm not sure, at least based on this motivation, why would we care?
>
> I'd argue that, in general, code examples are often used as templates 
> and may thus inherit any vulnerabilities therein. This may be especially 
> relevant here as your selftest enclave is in my knowledge the only 
> available truly minimal SGX enclave that can be built and extended while 
> only relying on standard tools and no heavy frameworks like the Intel 
> SGX SDK. Thus, as noted before on this mailing list, it may be an 
> attractive start for people who want to build things from scratch.

If you use this code as a template,  you have a legal risk in your hands
because of GPLv2 licensing.

> IMHO the example enclave should do a best effort to reasonably follow 
> SGX coding best practices and not have _known_ security vulnerabilities 
> in it. Note that these are not advanced microarchitectural attacks with 
> ugly LFENCE defenses, but plain, architectural memory-safety exploit 
> preventions with minimal sanitization checks, not unlike the existing 
> protections against buffer overflow where best practices are followed 
> for op->type.

I'm not sure what are the "best practices" behavior in the context of a
kselftest instance.

> Apart from that, the added checks only enforce correct behavior in the 
> test framework, only validating that things are sane and as expected. 
> Thus, to some extent, the added checks may even increase resilience of 
> the test framework.

I'm not sure what is "correct" behavior in the context of a kselftest
instance.

> Best,
> Jo

This code is not meant for production. I implemented it specifically for
kselftest, and that is exactly its scope.

BR, Jarkko

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

[Jo Van Bulck]

On 22.07.23 20:10, Jarkko Sakkinen wrote:
> This code is not meant for production. I implemented it specifically for
> kselftest, and that is exactly its scope.

I see, makes sense. As per Dave's suggestion, I'll see if I can submit a 
proposed minimal patch to remove any existing sanitization code that is 
not necessary for kselftest (eg register cleansing) and avoid any 
misguided impressions of the test enclave being representative.

> I'm not sure what is "correct" behavior in the context of a kselftest
> instance.

True. But at least when defining "correct" as passing the selftests, 
then I think it makes sense to merge the compiler optimization fixes. As 
the existing code clearly emits wrong assembly that breaks the selftests 
when switching optimization levels (which may always also be 
incorporated by default in future gcc versions or other compilers like 
clang).

Thus, I'll separate this out and submit another patch to ensure 
correctness with compiler optimizations only.

Best,
Jo

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

[Jarkko Sakkinen]

On Mon Jul 24, 2023 at 10:46 AM UTC, Jo Van Bulck wrote:
> On 22.07.23 20:10, Jarkko Sakkinen wrote:
> > This code is not meant for production. I implemented it specifically for
> > kselftest, and that is exactly its scope.
>
> I see, makes sense. As per Dave's suggestion, I'll see if I can submit a 
> proposed minimal patch to remove any existing sanitization code that is 
> not necessary for kselftest (eg register cleansing) and avoid any 
> misguided impressions of the test enclave being representative.
>
> > I'm not sure what is "correct" behavior in the context of a kselftest
> > instance.
>
> True. But at least when defining "correct" as passing the selftests, 
> then I think it makes sense to merge the compiler optimization fixes. As 
> the existing code clearly emits wrong assembly that breaks the selftests 
> when switching optimization levels (which may always also be 
> incorporated by default in future gcc versions or other compilers like 
> clang).
>
> Thus, I'll separate this out and submit another patch to ensure 
> correctness with compiler optimizations only.
>
> Best,
> Jo

It should be relatively easy to relicense the code as most of the
commits have Intel copyright.

Personally I would not mind because that would give opportunity for
code that I wrote to have a wider audience but it needs to be forked
with some other license first.

BR, Jarkko

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

[Jo Van Bulck]

On 28.07.23 20:54, Jarkko Sakkinen wrote
> It should be relatively easy to relicense the code as most of the
> commits have Intel copyright.
> 
> Personally I would not mind because that would give opportunity for
> code that I wrote to have a wider audience but it needs to be forked
> with some other license first.

> I support also the idea of refining the selftest as a run-time, which
> could perhaps consist of the following steps:
> 
> 1. Create a repository of the self-compiling selftest with GPLv2. You
>    could add also AUTHORS file for the initial content by crawling this
>    data from the git log.
> 2. Create a commit with sob's from the required stakeholders, which
>    changes the license to something more appropriate, and get the
>    sob's with some process.

Thank you Jarkko, appreciated! I plan to start working on the fork from 
next month onwards. However, I think GPL would be the best license for 
this project and I'd prefer to stick to it for the time being.

Best,
Jo

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

[Jarkko Sakkinen]

On Mon Aug 7, 2023 at 9:06 AM EEST, Jo Van Bulck wrote:
> On 28.07.23 20:54, Jarkko Sakkinen wrote
> > It should be relatively easy to relicense the code as most of the
> > commits have Intel copyright.
> > 
> > Personally I would not mind because that would give opportunity for
> > code that I wrote to have a wider audience but it needs to be forked
> > with some other license first.
>
> > I support also the idea of refining the selftest as a run-time, which
> > could perhaps consist of the following steps:
> > 
> > 1. Create a repository of the self-compiling selftest with GPLv2. You
> >    could add also AUTHORS file for the initial content by crawling this
> >    data from the git log.
> > 2. Create a commit with sob's from the required stakeholders, which
> >    changes the license to something more appropriate, and get the
> >    sob's with some process.
>
> Thank you Jarkko, appreciated! I plan to start working on the fork from 
> next month onwards. However, I think GPL would be the best license for 
> this project and I'd prefer to stick to it for the time being.
>
> Best,
> Jo

Ask from me permission when you have things moving forward, and I'll
very likely give permission to all my post-Intel contributions, which
are not that many.

PS. You can quite easily get full authors list with some git magic so
pretty easy to keep things legal wise in shape.

BR, Jarkko