Changes since V2: - V2: https://lore.kernel.org/linux-sgx/cover.1643754040.git.reinette.chatre@intel... - Added Acked-by from Shuah to all patches in series. - Shuah is ok with these entering the kernel via tip.git: https://lore.kernel.org/linux-sgx/e188c62e-f75e-2bec-b544-76063098611d@linux... - Include x86@kernel.org and more x86 maintainers since goal is for inclusion into tip.git.
Changes since V1: - V1: https://lore.kernel.org/linux-sgx/cover.1643393473.git.reinette.chatre@intel... - 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(-)
== 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") Acked-by: Shuah Khan skhan@linuxfoundation.org Signed-off-by: Reinette Chatre reinette.chatre@intel.com --- Changes since V2: - Add Acked by from Shuah.
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)); }
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 Acked-by: Shuah Khan skhan@linuxfoundation.org Signed-off-by: Reinette Chatre reinette.chatre@intel.com --- Changes since V2: - Add Acked-by from Shuah.
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))
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") Acked-by: Shuah Khan skhan@linuxfoundation.org Signed-off-by: Reinette Chatre reinette.chatre@intel.com --- Changes since V2: - Add Acked-by from Shuah.
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; }
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 Acked-by: Shuah Khan skhan@linuxfoundation.org Signed-off-by: Reinette Chatre reinette.chatre@intel.com --- Changes since V2: - Add Acked-by from Shuah.
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);
linux-kselftest-mirror@lists.linaro.org
-
Reinette Chatre