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

["Jarkko Sakkinen" <jarkko@kernel.org>]

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