| Message ID | ddb6554a95b0978aa018740fbfb32f786bcbd284.1643393473.git.reinette.chatre@intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 2db703fc3b15e7ef68c82eca613a3c00d43d70af |
| Headers | show |
| Series | selftests/sgx: Early enclave loading error path fixes | expand |
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.
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
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
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; }
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(-)