| Message ID | 89824888783fd8e770bfc64530c7549650a41851.1643754040.git.reinette.chatre@intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 2d03861e0d1d1ee81efc59338101cdd86a7474f6 |
| 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
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
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
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/
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
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)); }
== 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(-)