| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <0031a4f0-75f6-3104-1825-dcc2528f61b0@intel.com>
Date: Wed, 30 Mar 2022 10:40:19 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Jarkko Sakkinen <jarkko@...nel.org>
CC: Shuah Khan <skhan@...uxfoundation.org>,
Dave Hansen <dave.hansen@...ux.intel.com>,
Shuah Khan <shuah@...nel.org>,
"open list:INTEL SGX" <linux-sgx@...r.kernel.org>,
"open list:KERNEL SELFTEST FRAMEWORK"
<linux-kselftest@...r.kernel.org>,
open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/2] selftests/sgx: Use rip relative addressing for
encl_stack
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@...el.com>
>>> Cc: Dave Hansen <dave.hansen@...ux.intel.com>
>>> Signed-off-by: Jarkko Sakkinen <jarkko@...nel.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;
Powered by blists - more mailing lists