Re: [PATCH v2 1/2] selftests/sgx: Use rip relative addressing for encl_stack

[Reinette Chatre <reinette.chatre@intel.com>]

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;