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