lkml.org 
[lkml]   [2022]   [Aug]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRE: [PATCH 2/2] selftests/sgx: Add SGX selftest augment_via_eaccept_long
    Date
    Hi Reinette,

    > -----Original Message-----
    > From: Chatre, Reinette <reinette.chatre@intel.com>
    > Sent: Tuesday, August 16, 2022 9:35 PM
    > To: Dhanraj, Vijay <vijay.dhanraj@intel.com>; Jarkko Sakkinen
    > <jarkko@kernel.org>
    > Cc: Dave Hansen <dave.hansen@linux.intel.com>; linux-
    > sgx@vger.kernel.org; Shuah Khan <shuah@kernel.org>; open list:KERNEL
    > SELFTEST FRAMEWORK <linux-kselftest@vger.kernel.org>; open list <linux-
    > kernel@vger.kernel.org>
    > Subject: Re: [PATCH 2/2] selftests/sgx: Add SGX selftest
    > augment_via_eaccept_long
    >
    > Hi Vijay,
    >
    > On 8/16/2022 6:27 PM, Dhanraj, Vijay wrote:
    > > Hi Jarkko, Reinette,
    > >
    > >> -----Original Message-----
    > >> From: Jarkko Sakkinen <jarkko@kernel.org>
    > >> Sent: Tuesday, August 16, 2022 4:34 PM
    > >> To: Chatre, Reinette <reinette.chatre@intel.com>
    > >> Cc: Dave Hansen <dave.hansen@linux.intel.com>; linux-
    > >> sgx@vger.kernel.org; Dhanraj, Vijay <vijay.dhanraj@intel.com>; Shuah
    > >> Khan <shuah@kernel.org>; open list:KERNEL SELFTEST FRAMEWORK
    > <linux-
    > >> kselftest@vger.kernel.org>; open list <linux-kernel@vger.kernel.org>
    > >> Subject: Re: [PATCH 2/2] selftests/sgx: Add SGX selftest
    > >> augment_via_eaccept_long
    > >>
    > >> On Tue, Aug 16, 2022 at 09:26:40AM -0700, Reinette Chatre wrote:
    > >>> Hi Vijay,
    > >>>
    > >>> Thank you very much for digging into this. A few comments below.
    > >>>
    > >>> On 8/15/2022 4:39 PM, Jarkko Sakkinen wrote:
    >
    > ...
    >
    > >>>> @@ -25,6 +25,8 @@ static const uint64_t MAGIC =
    > >>>> 0x1122334455667788ULL; static const uint64_t MAGIC2 =
    > >>>> 0x8877665544332211ULL; vdso_sgx_enter_enclave_t
    > >>>> vdso_sgx_enter_enclave;
    > >>>>
    > >>>> +static const unsigned long edmm_size = 8589934592; //8G
    > >>>> +
    > >>>
    > >>> Could you please elaborate how this constant was chosen? I
    > >>> understand that this test helped to uncover a bug and it is useful
    > >>> to add to the kernel. When doing so this test will be run on systems
    > >>> with a variety of SGX memory sizes, could you please elaborate (and
    > >>> add a
    > >>> snippet) how 8GB is the right value for all systems?
    > >>
    > >> It is the only constant I know for sure that some people (Vijay and
    > >> Haitao) have been able to reproduce the bug.
    > >>
    > >> Unless someone can show that the same bug reproduces with a smaller
    > >> constant, changing it would make the whole test irrelevant.
    > >
    > > I tried with 2GB and it always succeed and with 4GB was able to repro
    > sporadically. But with 8GB failure was consistent. One thing to note is even
    > with 8GB Haitao couldn't reproduce this every time. So not sure if it good for
    > all the systems but on my ICX system, I was able to consistently repro with
    > this value.
    > >
    >
    > Could all of this information be placed in a description of this constant? At this
    > time it appears to be arbitrary.

    Yes it makes sense to record the reason for this constant.
    >
    > >>>> +
    > >>>> + if (!sgx2_supported())
    > >>>> + SKIP(return, "SGX2 not supported");
    > >>>> +
    > >>>> + ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self-
    > >>> encl,
    > >>>> +_metadata));
    > >>>> +
    > >>>> + 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;
    > >>>> + TH_LOG("test enclave: total_size = %ld, seg->size = %ld",
    > >> total_size, seg->size);
    > >>>> + }
    > >>>> +
    > >>>> + /*
    > >>>> + * Actual enclave size is expected to be larger than the loaded
    > >>>> + * test enclave since enclave size must be a power of 2 in bytes while
    > >>>> + * test_encl does not consume it all.
    > >>>> + */
    > >>>> + EXPECT_LT(total_size + edmm_size, self->encl.encl_size);
    > >>>
    > >>> Will this test ever fail?
    > >>
    > >> With a *quick* look: no.
    > >>
    > >> Vijay, what was the point of this check?
    > >
    > > Yes we can remove this check. I tried to copy from `augment_via_eaccept`
    > and just changed the request size.
    > >
    >
    > In augment_via_eaccept the check is required since augment_via_eaccept
    > assumes that there is enough address space in the existing enclave for
    > dynamic memory addition without needing to change the enclave size. If
    > anybody later changes the test enclave to break this assumption then that
    > check will pick it up.


    Got it, thanks. Yes this check is can be removed.

    >
    > In this new test the enclave size is set to accommodate the planned dynamic
    > memory addition and thus adding a test to check if the enclave has enough
    > space for the dynamic memory is not needed.
    >
    > >>>> + TH_LOG("Entering enclave to run EACCEPT for each page of %zd
    > >> bytes may take a while ...",
    > >>>> + edmm_size);
    > >>>> + 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; 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);
    > >>>> + 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);
    > >>>> + }
    > >>>> +
    > >>>> + /*
    > >>>> + * New page should be accessible from within enclave - attempt to
    > >>>> + * write to it.
    > >>>> + */
    > >>>
    > >>> This portion below was also copied from previous test and by only
    > >>> testing a write to the first page of the range the purpose is not
    > >>> clear. Could you please elaborate if the intention is to only test
    > >>> accessibility of the first page and why that is sufficient?
    > >>
    > >> It is sufficient because the test reproduces the bug. It would have
    > >> to be rather elaborated why you would possibly want to do more than
    > that.
    >
    > That is fair. An accurate comment (currently an inaccurate copy&paste)
    > would help to explain this part of the test.
    >
    > >>>> + 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);
    > >>>> +}
    > >>>> +
    > >>>> /*
    > >>>> * SGX2 page type modification test in two phases:
    > >>>> * Phase 1:
    > >>>> diff --git a/tools/testing/selftests/sgx/main.h
    > >>>> b/tools/testing/selftests/sgx/main.h
    > >>>> index fc585be97e2f..fe5d39ac0e1e 100644
    > >>>> --- a/tools/testing/selftests/sgx/main.h
    > >>>> +++ b/tools/testing/selftests/sgx/main.h
    > >>>> @@ -35,7 +35,8 @@ extern unsigned char sign_key[]; extern unsigned
    > >>>> char sign_key_end[];
    > >>>>
    > >>>> void encl_delete(struct encl *ctx); -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);
    > >>>> bool encl_measure(struct encl *encl); bool encl_build(struct encl
    > >>>> *encl); uint64_t encl_get_entry(struct encl *encl, const char
    > >>>> *symbol); diff --git a/tools/testing/selftests/sgx/sigstruct.c
    > >>>> b/tools/testing/selftests/sgx/sigstruct.c
    > >>>> index 50c5ab1aa6fa..6000cf0e4975 100644
    > >>>> --- a/tools/testing/selftests/sgx/sigstruct.c
    > >>>> +++ b/tools/testing/selftests/sgx/sigstruct.c
    > >>>> @@ -343,7 +343,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++) {
    > >>>
    > >>>
    > >>> Looking at mrenclave_ecreate() the above snippet seems separate from
    > >>> this test and incomplete since it now obtains encl->encl_size but
    > >>> continues to compute it again internally. Should this be a separate fix?
    > >>
    > >> I would remove this part completely but this also needs comment from
    > Vijay.
    > >
    > > If we restrict the large enclave size just for this test, then the above change
    > can be reverted. Calling ` mrenclave_ecreate` with src_size esults in EINIT
    > failure and I think the reason is because of incorrect MRenclave.
    >
    > From what I understand this change is needed since the enclave size is no
    > longer just the size of all the segments at enclave creation. I think it is
    > incomplete though since it still recomputes the enclave size even though it is
    > now provided as parameter.
    > This change does not need to be part of this test addition.

    I see your point and this change can be removed from the test.
    >
    > Reinette

    Regards, Vijay
    \
     
     \ /
      Last update: 2022-08-17 18:17    [W:2.554 / U:0.156 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site