| Message ID | cover.1643754040.git.reinette.chatre@intel.com |
|---|---|
| Headers | show |
| Series | selftests/sgx: Early enclave loading error path fixes | expand |
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
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
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
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/
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