Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <0031a4f0-75f6-3104-1825-dcc2528f61b0@intel.com>
Date:   Wed, 30 Mar 2022 10:40:19 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Firefox/91.0 Thunderbird/91.7.0
Subject: Re: [PATCH v2 1/2] selftests/sgx: Use rip relative addressing for
 encl_stack
Content-Language: en-US
To:     Jarkko Sakkinen <jarkko@kernel.org>
CC:     Shuah Khan <skhan@linuxfoundation.org>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Shuah Khan <shuah@kernel.org>,
        "open list:INTEL SGX" <linux-sgx@vger.kernel.org>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>
References: <20220322074313.7444-1-jarkko@kernel.org>
 <7b7732ec-c7ff-cf92-510f-64c83ed985cd@intel.com> <YkRvFkEO8Ic1VdQ8@iki.fi>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <YkRvFkEO8Ic1VdQ8@iki.fi>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?djJuZ0FES1JLMDE3amE5L1hMUlJqMXJTK1k1ZzdYMFg5Y2ozbkY2UkhkQktp?=
 =?utf-8?B?b2pmYnFMYjF3NjZmR2srL0NXYW1IdHV2Ull5UHVobTZ2TUFMVi9CWXFOUFlD?=
 =?utf-8?B?Rm9MVkxNSGxOdytJVXB4N2JvQnAxdEYvekFXYi9YMGlPcGRFMkc2MS9raU5v?=
 =?utf-8?B?b3BQMFUzOGRpT0FxN2ZIR0lUVHNkZmJaV2paREQrSytSdWZodXJ5d2lZR2J1?=
 =?utf-8?B?OE1EVFVBWXUwODlRdkxBbUV3RWpGL3lQeit6VHNhUW5qWE0vbWl1Z3J0NmNY?=
 =?utf-8?B?S0VMd1ZhV00xeFhDYjQ2dlVCeVY4MFhJTmpOTnlzaDI3R1VUTzBjdFo3eFJX?=
 =?utf-8?B?aWVLZjFoUTQ0QWo0UGk3RUlmV1p1Mzc2THVhU3RsRVlDYXZIZlZsQzVnSW5M?=
 =?utf-8?B?b0ZsY2JxY1lHek45RGRKWWZwRHo4S1B2UHludVVwWjVTYkptWHgxREpoZ2tt?=
 =?utf-8?B?VVNPWUhBdlNuMXVhUk9DbW5KejJIaEVrT3VWeC9yalJVaGl1SUVEdGxpbUhG?=
 =?utf-8?B?V2dYbFFpeG9aTG4rUVlTdXFUeGs3ZDFwenZyWUQ5ZC94bXBPZHFvUjN5RTh2?=
 =?utf-8?B?Y29yMlZoQlR6N2tpQ2xQazVvYnlKTThrdjBJYW9zOHhZOFdhdFRlTzZTdzVw?=
 =?utf-8?B?UCs0MG5kalJkWjRSMjJ3c3U2emZhVDM3SHhGTDc0VXRNR3IyNUYvOW5VVGU5?=
 =?utf-8?B?V1dua09nMXNCa2o1RGFqYjlTVWhrYTJ6WVRmaERuSEpVa3owd1Fpc1J2MTFM?=
 =?utf-8?B?SWRydWFsN1BzTFo1OVBLclc2NGRCeUtiSU90WFhOYXhucGJtYWRBdzUvT0s1?=
 =?utf-8?B?dHhlYkNYc1ZQSGx6RDhiYTgzS2Y5SW42SGFXc3lIUDR6bGcrU1JOZEF2TkdB?=
 =?utf-8?B?Q3dpTG51TkZ0ZTQzSUFDOWVudnY2MHEvdFh1Zk1wNnFHTkJmQVlFaHNGNGpZ?=
 =?utf-8?B?czhJZGYvUitSN1VCTVhqNFgyNU9UWXFDM3prYlhrRnhWaEhuU1ZwOCtwRGlZ?=
 =?utf-8?B?TWx3VDVjSEdocFJVb0RRa0FraHgweHgrZURkNmVFNFd1YVlHRHFEL3hDMFY1?=
 =?utf-8?B?ak5SZDJlTUw4ZnNQNGdVZ002TGlOZitwbWUyRkFvV3dWb3lDYXVrUTZQMmJ1?=
 =?utf-8?B?ZmdsVzdVNk4zc1l3UmJlZnFXSFh6bEpJY3FzYlZVZ3hKRlEyaGhjelJDZER6?=
 =?utf-8?B?NC9IOUxlM0RUNHoxYVQ0VzJRTXZwNC9XU0FWYUUyY2NucHBMVzg3WUFmR1cy?=
 =?utf-8?B?SEk4UUpWYjNKdHloNE13MFNVUnNYK2UzMHBMNVNTeDMrQkFhUFJ2UmZFQkQw?=
 =?utf-8?B?MDZnK1NDNUd4YlpuUld5RENoOWlFSFoxbzVJWEhJZm1XaklXSHp6Q3U0L2gv?=
 =?utf-8?B?aS9wd3czOGx6Nk40aXpvRkVtM3VibkF0clBSSSthMlZJZ2M2dnI4WUU5TzFR?=
 =?utf-8?B?MER6alN2NHFFM0o3RXAwZExFU3FhNG0rbFg5dXBzMVlySFZ4Qk5Zbm95NS94?=
 =?utf-8?B?TlZ4RmhKZzIraFoxcEQwQmtma0gyRENHMHhmMDlwR0dHQU9xNEVsZFdCUm5I?=
 =?utf-8?B?Q1ZUMXd3TTJKR1ZHMkVnbTE0TTVlbHVidDRLbkdFVExuNlhFUC9GNUREWHhw?=
 =?utf-8?B?WXg3Q0dTdDVUSUNtNy9DcG5sVVVlYXFRdHptZ2l2SlBYR2hPcVk0emI5Q3JI?=
 =?utf-8?B?VlNXUUxIUlVyNlIrTWJSR2FjeU0xb2hLWWV0MEVHa0xEaWdTV2N3bUlVZ2lv?=
 =?utf-8?B?VEh1dlQydklwY3hxYmwrZktZeW5HNmhTbXl1K1VSaEQvRWsrQ3UwcEV1QmNJ?=
 =?utf-8?B?aWNEWVBZQ0RNSUI4eEdnRzBySWxOL1YrRXY5RFZqcE83RWJHUXNzbDRHMGlh?=
 =?utf-8?B?MXZGZFJTN0VkemNlWFdFMEZqdFRsTUZYU0dMRW9HM0NkUTk4QXdUYTJ2bUxK?=
 =?utf-8?B?b0J2MFlqV3JndStTTElHN2lKR1ZROStYNExIcVhwLy9oVnNIR0l0b2ZRTWdR?=
 =?utf-8?B?Q2hXWC8ya2FYc1NFL3FqRWp0R3ROSSt6V1F4ZUcrNHJHRXB5VUlYeFZwQTFu?=
 =?utf-8?B?dS9YNFRjZkZuUGhmVU0wQm5FM1dFL2pkcHE3TGxSak93TzlnUGRoclNyZFMz?=
 =?utf-8?B?TjJuK3UyQllROTkwRFBhaUhYSHBVZTJTdWdTa29IVkFlWHJmS3VLNzFDbWdj?=
 =?utf-8?B?VTk0OXRad1dGYlJJaTNGQ1ZxZWwvVFFkZFl3Tk4ySndTSCt6VUhoSXljcDFE?=
 =?utf-8?B?bHZiT25UcHNHb2QrUW83SUNBeUtrTGFhM256Uk53YisyZHRicWRVUUJ0NVVi?=
 =?utf-8?B?dlRtbHduaU1lMVB0MXNRUHJuc0ovRFE1YzY1bEFKMWpaVGhaQnMwczNHdml1?=
 =?utf-8?Q?YrRbbCkIAK2oDDSE=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 5c1f924c-7755-4718-45ce-08da12745e0f
X-MS-Exchange-CrossTenant-AuthSource: CH0PR11MB5740.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 Mar 2022 17:40:23.4755
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 80ION2YnLod4vmipuoob1k4vP3Z8QEkgEH9blz2Yjwf0loasCjQHTF7h5mA/fMkaQJwnGLrQ3rTqIleKdmbfVruYgaYNx1AJPMnFmqvTzuc=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR11MB4756
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.0 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
	DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,RDNS_NONE,SPF_HELO_NONE,
	T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
	lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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;