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