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

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

Hi Dave,

On 1/28/2022 11:26 AM, Dave Hansen wrote:
> On 1/28/22 11:22, Reinette Chatre wrote:
>>  	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);
> 
> I was more wondering why the status of heap_seg->src is tied to
> encl->segment_tbl.

Apologies but it is not clear to me what the concern is. Please bear
with me as I first try to create some context and then I hope to
navigate to the issue.

The test creates an SGX enclave from a binary, in the test it is
named test_encl.elf. To create the enclave the test loads the data
from the binary and populates the enclave with it. In order to create
the enclave correctly the binary needs to be loaded via its distinct
segments, initially this is: TCS, TEXT, DATA. This is done to ensure
when the enclave is created it is done with the correct page types
and permissions. For example, the pages from the TCS segment needs
to be loaded in to enclave pages of type TCS with RW permission, the
pages from the TEXT segment needs to be loaded into regular enclave
pages with RX permission, etc.

To ensure the enclave is created correctly the test thus initializes
the data for each segment that will be loaded into the enclave into
that enclave's "segment_tbl".

struct encl {
        ...
        struct encl_segment *segment_tbl;
        ...
};

/*
 * struct encl_segment - parameters that needed for
 * SGX_IOC_ENCLAVE_ADD_PAGES ioctl()
 */
struct encl_segment {
        void *src;
        off_t offset;
        size_t size;
        unsigned int prot;
        unsigned int flags;
        bool measure;
};

Commit 3200505d4de6 ("selftests/sgx: Create a heap for the test
enclave") introduced a new segment, the heap, in support of more
testing. While not loaded from the original binary it is
considered a segment of the enclave and needs all fields in 
struct encl_segment in order to add its pages to the enclave
with appropriate page permissions and flags.

This is thus how heap_seg->src ended up connected to
encl->segment_tbl.

Apologies for being long winded here but I hope with this we could
narrow down where your concerns are.

Reinette