[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@xxxxxxxxxxxxxx>
---
 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