[PATCH 0/4] selftests/sgx: Early enclave loading error path fixes

[PATCH 0/4] selftests/sgx: Early enclave loading error path fixes

[Reinette Chatre]

Hi Everybody,

Please find included a few fixes that address problems encountered after
venturing into the enclave loading error handling code of the SGX
selftests.

Reinette

Reinette Chatre (4):
  selftests/sgx: Fix segfault upon early test failure
  selftests/sgx: Do not attempt enclave build without valid enclave
  selftests/sgx: Ensure enclave data available during debug print
  selftests/sgx: Remove extra newlines in test output

 tools/testing/selftests/sgx/load.c | 9 +++++----
 tools/testing/selftests/sgx/main.c | 9 +++++----
 2 files changed, 10 insertions(+), 8 deletions(-)

-- 
2.25.1

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

[Reinette Chatre]

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.

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);
 		free(encl->segment_tbl);
+	}
 
 	memset(encl, 0, sizeof(*encl));
 }
-- 
2.25.1

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

[Dave Hansen]

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.

> 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.

>  		free(encl->segment_tbl);
> +	}
>  
>  	memset(encl, 0, sizeof(*encl));
>  }

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

[Reinette Chatre]

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

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

[Dave Hansen]

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.

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

[Reinette Chatre]

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

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

[Jarkko Sakkinen]

On Fri, Jan 28, 2022 at 10:23:56AM -0800, 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.

Nit: textwidth=75

Not something I would NAK but just saying.

> 
> 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);
>  		free(encl->segment_tbl);
> +	}
>  
>  	memset(encl, 0, sizeof(*encl));
>  }
> -- 
> 2.25.1
> 

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
Tested-by: Jarkko Sakkinen <jarkko@kernel.org> 

I tested this by a minor code mod to trigger to failure case.

/Jarkko

[PATCH 2/4] selftests/sgx: Do not attempt enclave build without valid enclave

[Reinette Chatre]

It is not possible to build an enclave if it was not possible to load
the binary from which it should be constructed. Do not attempt
to make further progress but instead return with failure.

Fixes: 1b35eb719549 ("selftests/sgx: Encpsulate the test enclave creation")
Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
---
 tools/testing/selftests/sgx/main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
index 370c4995f7c4..a7cd2c3e6f7e 100644
--- a/tools/testing/selftests/sgx/main.c
+++ b/tools/testing/selftests/sgx/main.c
@@ -147,6 +147,7 @@ static bool setup_test_encl(unsigned long heap_size, struct encl *encl,
 	if (!encl_load("test_encl.elf", encl, heap_size)) {
 		encl_delete(encl);
 		TH_LOG("Failed to load the test enclave.\n");
+		return false;
 	}
 
 	if (!encl_measure(encl))
-- 
2.25.1

Re: [PATCH 2/4] selftests/sgx: Do not attempt enclave build without valid enclave

[Dave Hansen]

On 1/28/22 10:23, Reinette Chatre wrote:
> It is not possible to build an enclave if it was not possible to load
> the binary from which it should be constructed. Do not attempt
> to make further progress but instead return with failure.
> 
> Fixes: 1b35eb719549 ("selftests/sgx: Encpsulate the test enclave creation")
> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>

Acked-by: Dave Hansen <dave.hansen@linux.intel.com>

>  tools/testing/selftests/sgx/main.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
> index 370c4995f7c4..a7cd2c3e6f7e 100644
> --- a/tools/testing/selftests/sgx/main.c
> +++ b/tools/testing/selftests/sgx/main.c
> @@ -147,6 +147,7 @@ static bool setup_test_encl(unsigned long heap_size, struct encl *encl,
>  	if (!encl_load("test_encl.elf", encl, heap_size)) {
>  		encl_delete(encl);
>  		TH_LOG("Failed to load the test enclave.\n");
> +		return false;
>  	}
>  
>  	if (!encl_measure(encl))

One more sentence to add to the changelog:

	A "return false" from setup_test_encl() is expected to trip an
	ASSERT_TRUE() and abort the rest of the test.

That at least saves a reviewer from needing to check what the callers see.

Re: [PATCH 2/4] selftests/sgx: Do not attempt enclave build without valid enclave

[Reinette Chatre]

Hi Dave,

On 1/28/2022 11:03 AM, Dave Hansen wrote:
> On 1/28/22 10:23, Reinette Chatre wrote:
>> It is not possible to build an enclave if it was not possible to load
>> the binary from which it should be constructed. Do not attempt
>> to make further progress but instead return with failure.
>>
>> Fixes: 1b35eb719549 ("selftests/sgx: Encpsulate the test enclave creation")
>> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
> 
> Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
> 

Thank you very much.

>>  tools/testing/selftests/sgx/main.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
>> index 370c4995f7c4..a7cd2c3e6f7e 100644
>> --- a/tools/testing/selftests/sgx/main.c
>> +++ b/tools/testing/selftests/sgx/main.c
>> @@ -147,6 +147,7 @@ static bool setup_test_encl(unsigned long heap_size, struct encl *encl,
>>  	if (!encl_load("test_encl.elf", encl, heap_size)) {
>>  		encl_delete(encl);
>>  		TH_LOG("Failed to load the test enclave.\n");
>> +		return false;
>>  	}
>>  
>>  	if (!encl_measure(encl))
> 
> One more sentence to add to the changelog:
> 
> 	A "return false" from setup_test_encl() is expected to trip an
> 	ASSERT_TRUE() and abort the rest of the test.
> 
> That at least saves a reviewer from needing to check what the callers see.

Will do.

Reinette

Re: [PATCH 2/4] selftests/sgx: Do not attempt enclave build without valid enclave

[Jarkko Sakkinen]

On Fri, Jan 28, 2022 at 10:23:57AM -0800, Reinette Chatre wrote:
> It is not possible to build an enclave if it was not possible to load
> the binary from which it should be constructed. Do not attempt
> to make further progress but instead return with failure.
> 
> Fixes: 1b35eb719549 ("selftests/sgx: Encpsulate the test enclave creation")
> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
> ---
>  tools/testing/selftests/sgx/main.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
> index 370c4995f7c4..a7cd2c3e6f7e 100644
> --- a/tools/testing/selftests/sgx/main.c
> +++ b/tools/testing/selftests/sgx/main.c
> @@ -147,6 +147,7 @@ static bool setup_test_encl(unsigned long heap_size, struct encl *encl,
>  	if (!encl_load("test_encl.elf", encl, heap_size)) {
>  		encl_delete(encl);
>  		TH_LOG("Failed to load the test enclave.\n");
> +		return false;
>  	}
>  
>  	if (!encl_measure(encl))
> -- 
> 2.25.1
> 

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

/Jarkko

[PATCH 3/4] selftests/sgx: Ensure enclave data available during debug print

[Reinette Chatre]

In support of debugging the SGX tests print details from
the enclave and its memory mappings if any failure is encountered
during enclave loading.

When a failure is encountered no data is printed because the
printing of the data is preceded by cleanup of the data.

Move the data cleanup after the data print.

Fixes: 147172148909 ("selftests/sgx: Dump segments and /proc/self/maps only on failure")
Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
---
 tools/testing/selftests/sgx/main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
index a7cd2c3e6f7e..b0bd95a4730d 100644
--- a/tools/testing/selftests/sgx/main.c
+++ b/tools/testing/selftests/sgx/main.c
@@ -186,8 +186,6 @@ static bool setup_test_encl(unsigned long heap_size, struct encl *encl,
 	return true;
 
 err:
-	encl_delete(encl);
-
 	for (i = 0; i < encl->nr_segments; i++) {
 		seg = &encl->segment_tbl[i];
 
@@ -208,6 +206,8 @@ static bool setup_test_encl(unsigned long heap_size, struct encl *encl,
 
 	TH_LOG("Failed to initialize the test enclave.\n");
 
+	encl_delete(encl);
+
 	return false;
 }
 
-- 
2.25.1

Re: [PATCH 3/4] selftests/sgx: Ensure enclave data available during debug print

[Dave Hansen]

On 1/28/22 10:23, Reinette Chatre wrote:
> In support of debugging the SGX tests print details from
> the enclave and its memory mappings if any failure is encountered
> during enclave loading.
> 
> When a failure is encountered no data is printed because the
> printing of the data is preceded by cleanup of the data.
> 
> Move the data cleanup after the data print.

Isn't it worse than that?

>  err:
> -	encl_delete(encl);
> -
>  	for (i = 0; i < encl->nr_segments; i++) {
>  		seg = &encl->segment_tbl[i];

encl_delete() does:

	free(encl->segment_tbl);

but doesn't zero encl->nr_segments from what I can see.  That seems like
a use-after-free.

Seems like we need to really run the selftest under valgrind.

Re: [PATCH 3/4] selftests/sgx: Ensure enclave data available during debug print

[Reinette Chatre]

Hi Dave,

On 1/28/2022 11:06 AM, Dave Hansen wrote:
> On 1/28/22 10:23, Reinette Chatre wrote:
>> In support of debugging the SGX tests print details from
>> the enclave and its memory mappings if any failure is encountered
>> during enclave loading.
>>
>> When a failure is encountered no data is printed because the
>> printing of the data is preceded by cleanup of the data.
>>
>> Move the data cleanup after the data print.
> 
> Isn't it worse than that?
> 
>>  err:
>> -	encl_delete(encl);
>> -
>>  	for (i = 0; i < encl->nr_segments; i++) {
>>  		seg = &encl->segment_tbl[i];
> 
> encl_delete() does:
> 
> 	free(encl->segment_tbl);
> 
> but doesn't zero encl->nr_segments from what I can see.  That seems like
> a use-after-free.

encl_delete() ends with:

        memset(encl, 0, sizeof(*encl));

that will zero encl->nr_segments.

Even so, (after this change) the function to which this change belongs
flows as follows:

setup_test_encl()
{
         ...
         if (!encl_load("test_encl.elf", encl, heap_size)) {
                 encl_delete(encl);
                 ...
                 return false;
         }

         <=== /* 
               *  At this point, because encl_load() succeeded, 
               *  encl->segment_tbl and encl->nr_segments will
               *  be valid.
               */
        

         /*
          * Further initialization code, any of which can
          * "goto err" on failure.
          */
         
err:
         /* encl->segment_tbl and encl->nr_segments are valid for use */

         ...
         encl_delete();
         /* encl->segment_tbl and encl->nr_segments are NOT valid for use */
         return false;
}

> 
> Seems like we need to really run the selftest under valgrind.

Reinette

Re: [PATCH 3/4] selftests/sgx: Ensure enclave data available during debug print

[Jarkko Sakkinen]

On Fri, Jan 28, 2022 at 10:23:58AM -0800, Reinette Chatre wrote:
> In support of debugging the SGX tests print details from
> the enclave and its memory mappings if any failure is encountered
> during enclave loading.
> 
> When a failure is encountered no data is printed because the
> printing of the data is preceded by cleanup of the data.
> 
> Move the data cleanup after the data print.
> 
> Fixes: 147172148909 ("selftests/sgx: Dump segments and /proc/self/maps only on failure")
> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
> ---
>  tools/testing/selftests/sgx/main.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
> index a7cd2c3e6f7e..b0bd95a4730d 100644
> --- a/tools/testing/selftests/sgx/main.c
> +++ b/tools/testing/selftests/sgx/main.c
> @@ -186,8 +186,6 @@ static bool setup_test_encl(unsigned long heap_size, struct encl *encl,
>  	return true;
>  
>  err:
> -	encl_delete(encl);
> -
>  	for (i = 0; i < encl->nr_segments; i++) {
>  		seg = &encl->segment_tbl[i];
>  
> @@ -208,6 +206,8 @@ static bool setup_test_encl(unsigned long heap_size, struct encl *encl,
>  
>  	TH_LOG("Failed to initialize the test enclave.\n");
>  
> +	encl_delete(encl);
> +
>  	return false;
>  }
>  
> -- 
> 2.25.1
> 

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

/Jarkko

[PATCH 4/4] selftests/sgx: Remove extra newlines in test output

[Reinette Chatre]

The TH_LOG() macro is an optional debug logging function made
available by kselftest itself. When TH_LOG_ENABLED is set it
prints the provided message with additional information and
formatting that already includes a newline.

Providing a newline to the message printed by TH_LOG() results
in a double newline that produces irregular test output.

Remove the unnecessary newlines from the text provided to
TH_LOG().

Fixes: 1b35eb719549 ("selftests/sgx: Encpsulate the test enclave creation")
Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
---
 tools/testing/selftests/sgx/main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
index b0bd95a4730d..dd74fa42302e 100644
--- a/tools/testing/selftests/sgx/main.c
+++ b/tools/testing/selftests/sgx/main.c
@@ -146,7 +146,7 @@ static bool setup_test_encl(unsigned long heap_size, struct encl *encl,
 
 	if (!encl_load("test_encl.elf", encl, heap_size)) {
 		encl_delete(encl);
-		TH_LOG("Failed to load the test enclave.\n");
+		TH_LOG("Failed to load the test enclave.");
 		return false;
 	}
 
@@ -204,7 +204,7 @@ static bool setup_test_encl(unsigned long heap_size, struct encl *encl,
 		fclose(maps_file);
 	}
 
-	TH_LOG("Failed to initialize the test enclave.\n");
+	TH_LOG("Failed to initialize the test enclave.");
 
 	encl_delete(encl);
 
-- 
2.25.1

Re: [PATCH 4/4] selftests/sgx: Remove extra newlines in test output

[Dave Hansen]

On 1/28/22 10:23, Reinette Chatre wrote:
> The TH_LOG() macro is an optional debug logging function made
> available by kselftest itself. When TH_LOG_ENABLED is set it
> prints the provided message with additional information and
> formatting that already includes a newline.
> 
> Providing a newline to the message printed by TH_LOG() results
> in a double newline that produces irregular test output.
> 
> Remove the unnecessary newlines from the text provided to
> TH_LOG().

Acked-by: Dave Hansen <dave.hansen@linux.intel.com>

Re: [PATCH 4/4] selftests/sgx: Remove extra newlines in test output

[Jarkko Sakkinen]

On Fri, Jan 28, 2022 at 10:23:59AM -0800, Reinette Chatre wrote:
> The TH_LOG() macro is an optional debug logging function made
> available by kselftest itself. When TH_LOG_ENABLED is set it
> prints the provided message with additional information and
> formatting that already includes a newline.
> 
> Providing a newline to the message printed by TH_LOG() results
> in a double newline that produces irregular test output.
> 
> Remove the unnecessary newlines from the text provided to
> TH_LOG().
> 
> Fixes: 1b35eb719549 ("selftests/sgx: Encpsulate the test enclave creation")
> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
> ---
>  tools/testing/selftests/sgx/main.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
> index b0bd95a4730d..dd74fa42302e 100644
> --- a/tools/testing/selftests/sgx/main.c
> +++ b/tools/testing/selftests/sgx/main.c
> @@ -146,7 +146,7 @@ static bool setup_test_encl(unsigned long heap_size, struct encl *encl,
>  
>  	if (!encl_load("test_encl.elf", encl, heap_size)) {
>  		encl_delete(encl);
> -		TH_LOG("Failed to load the test enclave.\n");
> +		TH_LOG("Failed to load the test enclave.");
>  		return false;
>  	}
>  
> @@ -204,7 +204,7 @@ static bool setup_test_encl(unsigned long heap_size, struct encl *encl,
>  		fclose(maps_file);
>  	}
>  
> -	TH_LOG("Failed to initialize the test enclave.\n");
> +	TH_LOG("Failed to initialize the test enclave.");
>  
>  	encl_delete(encl);
>  
> -- 
> 2.25.1
> 

Sorry for the super slow latency. The reason has been job transition, and
also getting SGX remote attestation to work with a non-Intel stack.

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

/Jarkko