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@intel.com Cc: Dave Hansen dave.hansen@linux.intel.com Signed-off-by: Jarkko Sakkinen jarkko@kernel.org
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.164427...
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;