diff mbox series

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

Message ID 20230719142500.13623-2-jo.vanbulck@cs.kuleuven.be (mailing list archive)
State New, archived
Headers show
Series selftests/sgx: Harden test enclave | expand

Commit Message

Jo Van Bulck July 19, 2023, 2:24 p.m. UTC 
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(-)

Comments

Jarkko Sakkinen July 20, 2023, 5:27 p.m. UTC | #1 
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
Jo Van Bulck July 20, 2023, 7:14 p.m. UTC | #2 
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
diff mbox series

Patch

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