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