Re: [PATCH 1/4] selftests/sgx: Fix segfault upon early test failure

[Reinette Chatre <reinette.chatre@intel.com>]

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