lkml.org 
[lkml]   [2022]   [Mar]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
SubjectRe: [PATCH v2 1/2] selftests/sgx: Use rip relative addressing for encl_stack
From
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.1644274683.git.reinette.chatre@intel.com/
>>
>> 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;
\
 
 \ /
  Last update: 2022-03-30 19:40    [W:0.846 / U:0.064 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site