Hi Dave,
On 1/28/2022 10:43 AM, Dave Hansen wrote:
On 1/28/22 10:23, Reinette Chatre wrote:
A segfault is encountered if there happens to be an early failure of any of the SGX tests. One way to reproduce this is to remove the enclave binary "test_encl.elf" that will trigger early enclave loading failure followed by a segfault.
The segfault occurs within encl_delete() that cleans up after an enclave by umapping its mapped regions and closing the file descriptor to the SGX driver. As integrated with the kselftest harness encl_delete() is called upon exit from every test, irrespective of test success. encl_delete() is also called to clean up if an error is encountered during enclave loading.
encl_delete() is thus responsible for cleaning any amount of enclave state - including state that has already been cleaned.
encl_delete() starts by accessing encl->segment_tbl that may not have been created yet due to a very early failure or may already be cleaned up because of a failure encountered after encl->segment_tbl was created.
Ensure encl->segment_tbl is valid before attempting to access memory offset from it. The offset with which it is accessed, encl->nr_segments, is initialized after encl->segment_tbl and thus considered valid to use after the encl->segment_tbl check succeeds.
I'm thinking we can be a bit more concise about the problem:
== Background ==
The SGX selftests track parts of the enclave binaries in an array: encl->segment_tbl[]. That array is dynamically allocated early (but not first) in the test's lifetime. The array is referenced at the end of the test in encl_delete().
== Problem ==
encl->segment_tbl[] can be NULL if the test fails before its allocation. That leads to a NULL-pointer-dereference in encl_delete(). This is triggered during early failures of the selftest like if the enclave binary ("test_encl.elf") is deleted.
--
I think it's also best to refer to this as a NULL-pointer problem rather than a segfault. The segfault is really just the fallout from the NULL pointer, *not* the primary problem.
Will do. I plan to resubmit with your changes and just also append the paragraph that documents the fix.
Fixes: 3200505d4de6 ("selftests/sgx: Create a heap for the test enclave") Signed-off-by: Reinette Chatre reinette.chatre@intel.com
tools/testing/selftests/sgx/load.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/sgx/load.c b/tools/testing/selftests/sgx/load.c index 9d4322c946e2..006b464c8fc9 100644 --- a/tools/testing/selftests/sgx/load.c +++ b/tools/testing/selftests/sgx/load.c @@ -21,7 +21,7 @@ void encl_delete(struct encl *encl) {
- struct encl_segment *heap_seg = &encl->segment_tbl[encl->nr_segments - 1];
- struct encl_segment *heap_seg;
if (encl->encl_base) munmap((void *)encl->encl_base, encl->encl_size); @@ -32,10 +32,11 @@ void encl_delete(struct encl *encl) if (encl->fd) close(encl->fd);
- munmap(heap_seg->src, heap_seg->size);
- if (encl->segment_tbl)
- if (encl->segment_tbl) {
heap_seg = &encl->segment_tbl[encl->nr_segments - 1];munmap(heap_seg->src, heap_seg->size);This probably deserves a comment linking heap_seg->src and encl->segment_tbl together. They _look_ independent here.
How about:
diff --git a/tools/testing/selftests/sgx/load.c b/tools/testing/selftests/sgx/load.c index 006b464c8fc9..946eb7c2a253 100644 --- a/tools/testing/selftests/sgx/load.c +++ b/tools/testing/selftests/sgx/load.c @@ -33,6 +33,14 @@ void encl_delete(struct encl *encl) close(encl->fd);
if (encl->segment_tbl) { + /* + * Most segments form part of the enclave binary + * and have their mappings deleted with earlier + * munmap() of encl->bin. + * As a mapping of anonymous memory the heap + * segment is separate from the enclave + * binary and needs its mapping deleted separately. + */ heap_seg = &encl->segment_tbl[encl->nr_segments - 1]; munmap(heap_seg->src, heap_seg->size); free(encl->segment_tbl);
Reinette