Hi Jarkko, On 3/30/2022 7:54 AM, Jarkko Sakkinen wrote: > On Mon, Mar 28, 2022 at 02:49:04PM -0700, Reinette Chatre wrote: >> Hi Jarkko, >> >> On 3/22/2022 12:43 AM, Jarkko Sakkinen wrote: >>> Simplify the test_encl_bootstrap.S flow by using rip-relative addressing. >>> Compiler does the right thing here, and this removes dependency on where >>> TCS entries need to be located in the binary, i.e. allows the binary layout >>> changed freely in the future. >>> >>> Cc: Reinette Chatre <reinette.chatre@xxxxxxxxx> >>> Cc: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx> >>> Signed-off-by: Jarkko Sakkinen <jarkko@xxxxxxxxxx> >>> --- >>> tools/testing/selftests/sgx/test_encl_bootstrap.S | 6 +----- >>> 1 file changed, 1 insertion(+), 5 deletions(-) >>> >>> diff --git a/tools/testing/selftests/sgx/test_encl_bootstrap.S b/tools/testing/selftests/sgx/test_encl_bootstrap.S >>> index 82fb0dfcbd23..1c1b5c6c4ffe 100644 >>> --- a/tools/testing/selftests/sgx/test_encl_bootstrap.S >>> +++ b/tools/testing/selftests/sgx/test_encl_bootstrap.S >>> @@ -40,11 +40,7 @@ >>> .text >>> >>> encl_entry: >>> - # RBX contains the base address for TCS, which is the first address >>> - # inside the enclave for TCS #1 and one page into the enclave for >>> - # TCS #2. By adding the value of encl_stack to it, we get >>> - # the absolute address for the stack. >>> - lea (encl_stack)(%rbx), %rax >>> + lea (encl_stack)(%rip), %rax >>> xchg %rsp, %rax >>> push %rax >>> >> >> The goal of the above snippet is to set RSP to ensure that each thread has its own stack. >> >> Since EENTER computes RIP as EnclaveBase + TCS.OENTRY, by using offset from RIP this >> would result in all TCS with OENTRY of encl_entry to use the same stack, no? >> >> Could you please consider the following as an alternative: >> https://lore.kernel.org/lkml/65c137c875bd4da675eaba35316ff43d7cfd52f8.1644274683.git.reinette.chatre@xxxxxxxxx/ >> >> The idea in that patch is that a new TCS would always need to be accompanied by a >> dedicated stack so, at least for testing purposes, the TCS and stack can be dynamically >> allocated together with the TCS page following its stack. This seems much simpler >> to me and also makes the following patch unnecessary. > > There's no better alternative than use rip. Compiler will fix it up. Could you please elaborate how the compiler will fix it up? > > So, no, I won't consider that. This a dead obvious change. It is not obvious to me so I attempted to make it obvious by writing a test program that prints RSP from the two different threads. test_encl_bootstrap.S gives each thread, TCS #1 and TCS #2, a page of stack. Before your patch, with the test below printing RSP, this is clear ... the stack used by the two threads are one page apart: # RUN enclave.tcs_entry ... rsp TCS #1 = 0X7FD997D97F68 rsp TCS #2 = 0X7FD997D98F68 # OK enclave.tcs_entry After applying this patch both threads use the same stack memory: # RUN enclave.tcs_entry ... rsp TCS #1 = 0X7FCF778B7F68 rsp TCS #2 = 0X7FCF778B7F68 # OK enclave.tcs_entry Here is the test I used: diff --git a/tools/testing/selftests/sgx/defines.h b/tools/testing/selftests/sgx/defines.h index d8587c971941..08b2765dc2f4 100644 --- a/tools/testing/selftests/sgx/defines.h +++ b/tools/testing/selftests/sgx/defines.h @@ -27,6 +27,7 @@ enum encl_op_type { ENCL_OP_EACCEPT, ENCL_OP_EMODPE, ENCL_OP_INIT_TCS_PAGE, + ENCL_OP_GET_RSP, ENCL_OP_MAX, }; @@ -76,4 +77,10 @@ struct encl_op_init_tcs_page { uint64_t entry; }; +struct encl_op_rsp { + struct encl_op_header header; + uint64_t ret; +}; + + #endif /* DEFINES_H */ diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c index a7543e5561a9..2380944dce71 100644 --- a/tools/testing/selftests/sgx/main.c +++ b/tools/testing/selftests/sgx/main.c @@ -570,12 +573,14 @@ TEST_F(enclave, clobbered_vdso_and_user_function) /* * Sanity check that it is possible to enter either of the two hardcoded TCS */ TEST_F(enclave, tcs_entry) { struct encl_op_header op; + struct encl_op_rsp rsp_op; ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata)); @@ -591,6 +596,17 @@ TEST_F(enclave, tcs_entry) EXPECT_EQ(self->run.exception_error_code, 0); EXPECT_EQ(self->run.exception_addr, 0); + rsp_op.ret = 0; + rsp_op.header.type = ENCL_OP_GET_RSP; + + EXPECT_EQ(ENCL_CALL(&rsp_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); + printf("rsp TCS #1 = 0X%lX \n", rsp_op.ret); + /* Move to the next TCS. */ self->run.tcs = self->encl.encl_base + PAGE_SIZE; @@ -600,6 +616,17 @@ TEST_F(enclave, tcs_entry) EXPECT_EQ(self->run.exception_vector, 0); EXPECT_EQ(self->run.exception_error_code, 0); EXPECT_EQ(self->run.exception_addr, 0); + rsp_op.ret = 0; + rsp_op.header.type = ENCL_OP_GET_RSP; + + EXPECT_EQ(ENCL_CALL(&rsp_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); + printf("rsp TCS #2 = 0X%lX \n", rsp_op.ret); + } /* diff --git a/tools/testing/selftests/sgx/test_encl.c b/tools/testing/selftests/sgx/test_encl.c index c0d6397295e3..b2a94a6d754e 100644 --- a/tools/testing/selftests/sgx/test_encl.c +++ b/tools/testing/selftests/sgx/test_encl.c @@ -119,6 +119,17 @@ static void do_encl_op_nop(void *_op) } +static void do_get_rsp(void *_op) +{ + struct encl_op_rsp *op = _op; + uint64_t rsp; + + asm volatile("mov %%rsp, %0 \n": "=r"(rsp) ::); + + op->ret = rsp; + +} + void encl_body(void *rdi, void *rsi) { const void (*encl_op_array[ENCL_OP_MAX])(void *) = { @@ -130,6 +141,7 @@ void encl_body(void *rdi, void *rsi) do_encl_eaccept, do_encl_emodpe, do_encl_init_tcs_page, + do_get_rsp, }; struct encl_op_header *op = (struct encl_op_header *)rdi;
