| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Yw7HOIfnaO0IcDiX@kernel.org>
Date: Wed, 31 Aug 2022 05:28:08 +0300
From: Jarkko Sakkinen <jarkko@...nel.org>
To: Reinette Chatre <reinette.chatre@...el.com>
Cc: linux-sgx@...r.kernel.org,
Haitao Huang <haitao.huang@...ux.intel.com>,
Vijay Dhanraj <vijay.dhanraj@...el.com>,
Dave Hansen <dave.hansen@...ux.intel.com>,
Shuah Khan <shuah@...nel.org>,
"open list:KERNEL SELFTEST FRAMEWORK"
<linux-kselftest@...r.kernel.org>,
open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 4/6] selftests/sgx: Add SGX selftest
augment_via_eaccept_long
On Tue, Aug 30, 2022 at 03:55:47PM -0700, Reinette Chatre wrote:
> Hi Vijay and Jarkko,
>
> On 8/29/2022 8:12 PM, Jarkko Sakkinen wrote:
> > From: Vijay Dhanraj <vijay.dhanraj@...el.com>
> >
> > Add a new test case which is same as augment_via_eaccept but adds a
> > larger number of EPC pages to stress test EAUG via EACCEPT.
> >
> > Signed-off-by: Vijay Dhanraj <vijay.dhanraj@...el.com>
> > Co-developed-by: Jarkko Sakkinen <jarkko@...nel.org>
> > Signed-off-by: Jarkko Sakkinen <jarkko@...nel.org>
> > ---
> > v2:
> > - Addressed Reinette's feedback:
> > https://lore.kernel.org/linux-sgx/24bd8e42-ff4e-0090-d9e1-cd81e4807f21@intel.com/
> > ---
> > tools/testing/selftests/sgx/load.c | 5 +-
> > tools/testing/selftests/sgx/main.c | 141 +++++++++++++++++++++---
> > tools/testing/selftests/sgx/main.h | 3 +-
> > tools/testing/selftests/sgx/sigstruct.c | 2 +-
> > 4 files changed, 129 insertions(+), 22 deletions(-)
>
> There seems to be at least three patches merged into one here:
> 1) Update SGX selftests to create enclaves with provided size dedicated
> to EDMM (this change causes a lot of noise and distracts from the test
> addition).
> 2) The mrenclave_ecreate() fix (which is still incomplete).
> 3) The actual test addition.
I would agree on this on a kernel patch but not for kselftest patch. It
does not really give useful value here. This adds a test and that is a
good enough granularity in my opinion, unless some major architecture
work is required as precursory. It is not the case here.
> > diff --git a/tools/testing/selftests/sgx/load.c b/tools/testing/selftests/sgx/load.c
> > index 94bdeac1cf04..7de1b15c90b1 100644
> > --- a/tools/testing/selftests/sgx/load.c
> > +++ b/tools/testing/selftests/sgx/load.c
> > @@ -171,7 +171,8 @@ uint64_t encl_get_entry(struct encl *encl, const char *symbol)
> > return 0;
> > }
> >
> > -bool encl_load(const char *path, struct encl *encl, unsigned long heap_size)
> > +bool encl_load(const char *path, struct encl *encl, unsigned long heap_size,
> > + unsigned long edmm_size)
> > {
>
> checkpatch.pl informs about alignment issues above and also a few other places.
Weird. I did run checkpatch through these. Will revisit.
>
> > const char device_path[] = "/dev/sgx_enclave";
> > struct encl_segment *seg;
> > @@ -300,7 +301,7 @@ bool encl_load(const char *path, struct encl *encl, unsigned long heap_size)
> >
> > encl->src_size = encl->segment_tbl[j].offset + encl->segment_tbl[j].size;
> >
> > - for (encl->encl_size = 4096; encl->encl_size < encl->src_size; )
> > + for (encl->encl_size = 4096; encl->encl_size < encl->src_size + edmm_size;)
> > encl->encl_size <<= 1;
> >
> > return true;
> > diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
> > index 9820b3809c69..867e98502120 100644
> > --- a/tools/testing/selftests/sgx/main.c
> > +++ b/tools/testing/selftests/sgx/main.c
> > @@ -21,8 +21,15 @@
> > #include "../kselftest_harness.h"
> > #include "main.h"
> >
> > +/*
> > + * The size was chosen based on a bug report:
> > + * https://lore.kernel.org/linux-sgx/DM8PR11MB55912A7F47A84EC9913A6352F6999@DM8PR11MB5591.namprd11.prod.outlook.com/
> > + */
> > +static const unsigned long EDMM_SIZE_LONG = 8L * 1024L * 1024L * 1024L; // 8 GB
> > +static const unsigned long TIMEOUT_LONG = 900; /* seconds */
>
> It is interesting that in this small snippet there are three different
> comment styles (multi-line comment preceding code, tail comment using /**/,
> and tail comment using //) :)
Oops. Will fix.
>
> > static const uint64_t MAGIC = 0x1122334455667788ULL;
> > static const uint64_t MAGIC2 = 0x8877665544332211ULL;
> > +
>
> Addition of empty line here.
Will remove.
>
> > vdso_sgx_enter_enclave_t vdso_sgx_enter_enclave;
> >
> > /*
> > @@ -173,7 +180,8 @@ FIXTURE(enclave) {
> > };
> >
> > static bool setup_test_encl(unsigned long heap_size, struct encl *encl,
> > - struct __test_metadata *_metadata)
> > + struct __test_metadata *_metadata,
> > + unsigned long edmm_size)
> > {
> > Elf64_Sym *sgx_enter_enclave_sym = NULL;
> > struct vdso_symtab symtab;
> > @@ -183,7 +191,7 @@ static bool setup_test_encl(unsigned long heap_size, struct encl *encl,
> > unsigned int i;
> > void *addr;
> >
> > - if (!encl_load("test_encl.elf", encl, heap_size)) {
> > + if (!encl_load("test_encl.elf", encl, heap_size, edmm_size)) {
> > encl_delete(encl);
> > TH_LOG("Failed to load the test enclave.");
> > return false;
> > @@ -284,7 +292,7 @@ TEST_F(enclave, unclobbered_vdso)
> > struct encl_op_get_from_buf get_op;
> > struct encl_op_put_to_buf put_op;
> >
> > - ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata));
> > + ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata, 0));
> >
> > memset(&self->run, 0, sizeof(self->run));
> > self->run.tcs = self->encl.encl_base;
> > @@ -357,7 +365,7 @@ TEST_F(enclave, unclobbered_vdso_oversubscribed)
> >
> > total_mem = get_total_epc_mem();
> > ASSERT_NE(total_mem, 0);
> > - ASSERT_TRUE(setup_test_encl(total_mem, &self->encl, _metadata));
> > + ASSERT_TRUE(setup_test_encl(total_mem, &self->encl, _metadata, 0));
> >
>
> The support for EDMM size and all the call site changes could be moved to a separate
> patch to make the changes easier to parse.
>
>
> ...
>
> > @@ -1210,6 +1218,103 @@ TEST_F(enclave, augment_via_eaccept)
> > munmap(addr, PAGE_SIZE);
> > }
> >
> > +/*
> > + * Test for the addition of large number of pages to an initialized enclave via
> > + * a pre-emptive run of EACCEPT on every page to be added.
> > + */
> > +TEST_F_TIMEOUT(enclave, augment_via_eaccept_long, TIMEOUT_LONG)
> > +{
> > + struct encl_op_get_from_addr get_addr_op;
> > + struct encl_op_put_to_addr put_addr_op;
> > + struct encl_op_eaccept eaccept_op;
> > + size_t total_size = 0;
> > + unsigned long i;
> > + void *addr;
> > +
> > + if (!sgx2_supported())
> > + SKIP(return, "SGX2 not supported");
> > +
> > + ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata,
> > + EDMM_SIZE_LONG));
> > +
> > + memset(&self->run, 0, sizeof(self->run));
> > + self->run.tcs = self->encl.encl_base;
> > +
> > + for (i = 0; i < self->encl.nr_segments; i++) {
> > + struct encl_segment *seg = &self->encl.segment_tbl[i];
> > +
> > + total_size += seg->size;
> > + }
> > +
> > + /*
> > + * mmap() every page at end of existing enclave to be used for
> > + * EDMM.
> > + */
> > + addr = mmap((void *)self->encl.encl_base + total_size, EDMM_SIZE_LONG,
> > + PROT_READ | PROT_WRITE | PROT_EXEC, MAP_SHARED | MAP_FIXED,
> > + self->encl.fd, 0);
> > + EXPECT_NE(addr, MAP_FAILED);
> > +
> > + self->run.exception_vector = 0;
> > + self->run.exception_error_code = 0;
> > + self->run.exception_addr = 0;
> > +
> > + /*
> > + * Run EACCEPT on new page to trigger the #PF->EAUG->EACCEPT(again
> > + * without a #PF). All should be transparent to userspace.
> > + */
>
> s/on new page/on every page/ ?
+1
>
> > + eaccept_op.flags = SGX_SECINFO_R | SGX_SECINFO_W | SGX_SECINFO_REG | SGX_SECINFO_PENDING;
> > + eaccept_op.ret = 0;
> > + eaccept_op.header.type = ENCL_OP_EACCEPT;
> > +
> > + for (i = 0; i < EDMM_SIZE_LONG; i += 4096) {
> > + eaccept_op.epc_addr = (uint64_t)(addr + i);
> > +
> > + EXPECT_EQ(ENCL_CALL(&eaccept_op, &self->run, true), 0);
> > + if (self->run.exception_vector == 14 &&
> > + self->run.exception_error_code == 4 &&
> > + self->run.exception_addr == self->encl.encl_base) {
> > + munmap(addr, EDMM_SIZE_LONG);
> > + SKIP(return, "Kernel does not support adding pages to initialized enclave");
> > + }
> > +
> > + EXPECT_EQ(self->run.exception_vector, 0);
> > + EXPECT_EQ(self->run.exception_error_code, 0);
> > + EXPECT_EQ(self->run.exception_addr, 0);
> > + ASSERT_EQ(eaccept_op.ret, 0);
> > + ASSERT_EQ(self->run.function, EEXIT);
> > + }
> > +
>
> I see that you removed all comments here after the discussion about what
> comment would be appropriate. This may be ok but it does seem awkward that
> there are two parts to this section and the second part still has a comment
> while the first does not. How about the comment I provided in
> https://lore.kernel.org/linux-sgx/69a5e350-ded9-30c9-dc41-d08c01dd05dc@intel.com/ :
>
> /*
> * Pool of pages were successfully added to enclave. Perform sanity
> * check on first page of the pool only to ensure data can be written
> * to and read from a dynamically added enclave page.
> */
+1
>
> > + put_addr_op.value = MAGIC;
> > + put_addr_op.addr = (unsigned long)addr;
> > + put_addr_op.header.type = ENCL_OP_PUT_TO_ADDRESS;
> > +
> > + EXPECT_EQ(ENCL_CALL(&put_addr_op, &self->run, true), 0);
> > +
> > + EXPECT_EEXIT(&self->run);
> > + EXPECT_EQ(self->run.exception_vector, 0);
> > + EXPECT_EQ(self->run.exception_error_code, 0);
> > + EXPECT_EQ(self->run.exception_addr, 0);
> > +
> > + /*
> > + * Read memory from newly added page that was just written to,
> > + * confirming that data previously written (MAGIC) is present.
> > + */
> > + get_addr_op.value = 0;
> > + get_addr_op.addr = (unsigned long)addr;
> > + get_addr_op.header.type = ENCL_OP_GET_FROM_ADDRESS;
> > +
> > + EXPECT_EQ(ENCL_CALL(&get_addr_op, &self->run, true), 0);
> > +
> > + EXPECT_EQ(get_addr_op.value, MAGIC);
> > + EXPECT_EEXIT(&self->run);
> > + EXPECT_EQ(self->run.exception_vector, 0);
> > + EXPECT_EQ(self->run.exception_error_code, 0);
> > + EXPECT_EQ(self->run.exception_addr, 0);
> > +
> > + munmap(addr, EDMM_SIZE_LONG);
> > +}
> > +
> > /*
> > * SGX2 page type modification test in two phases:
> > * Phase 1:
>
> ...
>
> > diff --git a/tools/testing/selftests/sgx/sigstruct.c b/tools/testing/selftests/sgx/sigstruct.c
> > index a07896a46364..decd767434d5 100644
> > --- a/tools/testing/selftests/sgx/sigstruct.c
> > +++ b/tools/testing/selftests/sgx/sigstruct.c
> > @@ -349,7 +349,7 @@ bool encl_measure(struct encl *encl)
> > if (!ctx)
> > goto err;
> >
> > - if (!mrenclave_ecreate(ctx, encl->src_size))
> > + if (!mrenclave_ecreate(ctx, encl->encl_size))
> > goto err;
> >
> > for (i = 0; i < encl->nr_segments; i++) {
>
> As I mentioned in feedback to previous version, even though
> encl_size is now provided, this misses that mrenclave_ecreate()
> continues to recompute it within.
>
> Reinette
>
I simply forgot this one, sorry. Will fix.
BR, Jarkko
Powered by blists - more mailing lists