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

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

[Reinette Chatre]

Changes since V1:
- V1: https://lore.kernel.org/linux-sgx/cover.1643393473.git.reinette.chatre@intel.com/
- All changes impact the commit messages only, no changes to code.
- Rewrite commit message of 1/4 (Dave).
- Detail in 2/4 commit log what callers will see with this change (Dave).
- Add Acked-by from Dave to 2/4 and 4/4.

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 NULL-pointer-dereference 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(-)


base-commit: 2056e2989bf47ad7274ecc5e9dda2add53c112f9
-- 
2.25.1

[PATCH V2 1/4] selftests/sgx: Fix NULL-pointer-dereference upon early test failure

[Reinette Chatre]

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

== Solution ==

Ensure encl->segment_tbl[] is valid before attempting to access
its members. The offset with which it is accessed, encl->nr_segments,
is initialized before 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>
---
Changes since V1:
- Rewrite commit message (Dave).

 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

[PATCH V2 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. A
"return false" from setup_test_encl() is expected to trip an
ASSERT_TRUE() and abort the rest of the test.

Fixes: 1b35eb719549 ("selftests/sgx: Encpsulate the test enclave creation")
Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
---
Changes since V1:
- Add Acked-by from Dave.
- Detail in commit log what callers will see with this change (Dave).

 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

[PATCH V2 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>
---
No changes since V1.

 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

[PATCH V2 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")
Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
---
Changes since V1:
- Add Acked-by from Dave.

 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 V2 1/4] selftests/sgx: Fix NULL-pointer-dereference upon early test failure

[Shuah Khan]

On 2/1/22 3:47 PM, Reinette Chatre wrote:
> == 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.
> 
> == Solution ==
> 

"==" usage looks a bit odd in the change log.
  
> Ensure encl->segment_tbl[] is valid before attempting to access
> its members. The offset with which it is accessed, encl->nr_segments,
> is initialized before 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>
> ---
> Changes since V1:
> - Rewrite commit message (Dave).
> 
>   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));
>   }
> 

The rest looks good to me. I can take this through kselftest tree, if not,

Acked-by: Shuah Khan <skhan@linuxfoundation.org>

thanks,
-- Shuah

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

[Shuah Khan]

On 2/1/22 3:47 PM, 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. A
> "return false" from setup_test_encl() is expected to trip an
> ASSERT_TRUE() and abort the rest of the test.
> 
> Fixes: 1b35eb719549 ("selftests/sgx: Encpsulate the test enclave creation")
> Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
> ---
> Changes since V1:
> - Add Acked-by from Dave.
> - Detail in commit log what callers will see with this change (Dave).
> 
>   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))
> 
Acked-by: Shuah Khan <skhan@linuxfoundation.org>

thanks,
-- Shuah

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

[Shuah Khan]

On 2/1/22 3:47 PM, 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>
> ---


Acked-by: Shuah Khan <skhan@linuxfoundation.org>

thanks,
-- Shuah

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

[Shuah Khan]

On 2/1/22 3:47 PM, 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")
> Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
> ---
> Changes since V1:
> - Add Acked-by from Dave.
> 
>   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);
>   
> 

Acked-by: Shuah Khan <skhan@linuxfoundation.org>

thanks,
-- Shuah

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

[Shuah Khan]

On 2/1/22 3:47 PM, Reinette Chatre wrote:
> Changes since V1:
> - V1: https://lore.kernel.org/linux-sgx/cover.1643393473.git.reinette.chatre@intel.com/
> - All changes impact the commit messages only, no changes to code.
> - Rewrite commit message of 1/4 (Dave).
> - Detail in 2/4 commit log what callers will see with this change (Dave).
> - Add Acked-by from Dave to 2/4 and 4/4.
> 
> 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 NULL-pointer-dereference 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(-)
> 
> 
> base-commit: 2056e2989bf47ad7274ecc5e9dda2add53c112f9
> 

I can take these through kselftest tree if there are no dependencies on
another tree.

thanks,
-- Shuah

Re: [PATCH V2 1/4] selftests/sgx: Fix NULL-pointer-dereference upon early test failure

[Reinette Chatre]

Hi Shuah and Dave,

On 2/2/2022 10:01 AM, Shuah Khan wrote:
> On 2/1/22 3:47 PM, Reinette Chatre wrote:
>> == 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.
>>
>> == Solution ==
>>
> 
> "==" usage looks a bit odd in the change log.

This is a new trend in the x86/ area and I was asked to modify the commit
message to follow suit in:
https://lore.kernel.org/linux-sgx/df2248d2-eb61-22d6-3a51-d8091f9eaad6@intel.com/


>> Ensure encl->segment_tbl[] is valid before attempting to access
>> its members. The offset with which it is accessed, encl->nr_segments,
>> is initialized before 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>
>> ---
>> Changes since V1:
>> - Rewrite commit message (Dave).
>>
>>   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));
>>   }
>>
> 
> The rest looks good to me. I can take this through kselftest tree, if not,
> 
> Acked-by: Shuah Khan <skhan@linuxfoundation.org>
> 

Thank you very much for reviewing the changes.

None of the patches in this series have external dependencies (all patches
in the "Fixes:" can be found in v5.17-rc1) but my understanding is that Dave
(for now) prefers to take them via the tip.git tree. This is because there
are more SGX features and tests for those features [1] in flight to the
SGX area and at least for now it would make things easier if the changes to
the SGX selftests are contained in the same tree.

Dave: please do correct me if I am wrong.

Reinette

[1] https://lore.kernel.org/linux-sgx/cover.1638381245.git.reinette.chatre@intel.com/

Re: [PATCH V2 1/4] selftests/sgx: Fix NULL-pointer-dereference upon early test failure

[Shuah Khan]

On 2/2/22 11:52 AM, Reinette Chatre wrote:
> Hi Shuah and Dave,
> 
> On 2/2/2022 10:01 AM, Shuah Khan wrote:
>> On 2/1/22 3:47 PM, Reinette Chatre wrote:
>>> == 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.
>>>
>>> == Solution ==
>>>
>>
>> "==" usage looks a bit odd in the change log.
> 
> This is a new trend in the x86/ area and I was asked to modify the commit
> message to follow suit in:
> https://lore.kernel.org/linux-sgx/df2248d2-eb61-22d6-3a51-d8091f9eaad6@intel.com/
> 
> 

Good to know. Thanks for the link.

>>
>> The rest looks good to me. I can take this through kselftest tree, if not,
>>
>> Acked-by: Shuah Khan <skhan@linuxfoundation.org>
>>
> 
> Thank you very much for reviewing the changes.
> 
> None of the patches in this series have external dependencies (all patches
> in the "Fixes:" can be found in v5.17-rc1) but my understanding is that Dave
> (for now) prefers to take them via the tip.git tree. This is because there
> are more SGX features and tests for those features [1] in flight to the
> SGX area and at least for now it would make things easier if the changes to
> the SGX selftests are contained in the same tree.
> 
> Dave: please do correct me if I am wrong.
> 

Sounds good to me.

thanks,
-- Shuah

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

[Jarkko Sakkinen]

On Tue, Feb 01, 2022 at 02:47:04PM -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. A
> "return false" from setup_test_encl() is expected to trip an
> ASSERT_TRUE() and abort the rest of the test.
> 
> Fixes: 1b35eb719549 ("selftests/sgx: Encpsulate the test enclave creation")
> Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
> ---
> Changes since V1:
> - Add Acked-by from Dave.
> - Detail in commit log what callers will see with this change (Dave).
> 
>  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>

BR, Jarkko

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

[Jarkko Sakkinen]

On Tue, Feb 01, 2022 at 02:47:05PM -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>
> ---
> No changes since V1.
> 
>  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
> 

LGTM

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

BR, Jarkko

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

[Jarkko Sakkinen]

On Tue, Feb 01, 2022 at 02:47:06PM -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")
> Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
> ---
> Changes since V1:
> - Add Acked-by from Dave.
> 
>  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
> 


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

BR, Jarkko

Re: [PATCH V2 1/4] selftests/sgx: Fix NULL-pointer-dereference upon early test failure

[Jarkko Sakkinen]

On Tue, Feb 01, 2022 at 02:47:03PM -0800, Reinette Chatre wrote:
> == 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.
> 
> == Solution ==
> 
> Ensure encl->segment_tbl[] is valid before attempting to access
> its members. The offset with which it is accessed, encl->nr_segments,
> is initialized before 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>
> ---
> Changes since V1:
> - Rewrite commit message (Dave).
> 
>  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>

BR, Jarkko

Re: [PATCH V2 1/4] selftests/sgx: Fix NULL-pointer-dereference upon early test failure

[Reinette Chatre]

Hi Jarkko,

On 2/20/2022 12:02 PM, Jarkko Sakkinen wrote:
> On Tue, Feb 01, 2022 at 02:47:03PM -0800, Reinette Chatre wrote:
>> == 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.
>>
>> == Solution ==
>>
>> Ensure encl->segment_tbl[] is valid before attempting to access
>> its members. The offset with which it is accessed, encl->nr_segments,
>> is initialized before 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>
>> ---
>> Changes since V1:
>> - Rewrite commit message (Dave).
>>
>>  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>
> 

Thank you very much for taking a look at these patches. 

V3[1] was submitted (8 February) and merged (11 February) onto x86/sgx
before I received your reviewed-by tags for V1 (15 February) or
V2 (20 February). The merged version thus does not contain your tags.

Reinette

[1] https://lore.kernel.org/linux-sgx/cover.1644355600.git.reinette.chatre@intel.com/

Re: [PATCH V2 1/4] selftests/sgx: Fix NULL-pointer-dereference upon early test failure

[Jarkko Sakkinen]

On Tue, Feb 22, 2022 at 12:05:34PM -0800, Reinette Chatre wrote:
> Hi Jarkko,
> 
> On 2/20/2022 12:02 PM, Jarkko Sakkinen wrote:
> > On Tue, Feb 01, 2022 at 02:47:03PM -0800, Reinette Chatre wrote:
> >> == 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.
> >>
> >> == Solution ==
> >>
> >> Ensure encl->segment_tbl[] is valid before attempting to access
> >> its members. The offset with which it is accessed, encl->nr_segments,
> >> is initialized before 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>
> >> ---
> >> Changes since V1:
> >> - Rewrite commit message (Dave).
> >>
> >>  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>
> > 
> 
> Thank you very much for taking a look at these patches. 
> 
> V3[1] was submitted (8 February) and merged (11 February) onto x86/sgx
> before I received your reviewed-by tags for V1 (15 February) or
> V2 (20 February). The merged version thus does not contain your tags.
> 
> Reinette
> 
> [1] https://lore.kernel.org/linux-sgx/cover.1644355600.git.reinette.chatre@intel.com/

Not a big deal, but thanks for confirming :-)

BR, Jarkko