* [PATCH v2 1/2] selftests/sgx: Use rip relative addressing for encl_stack @ 2022-03-22 7:43 Jarkko Sakkinen 2022-03-22 7:43 ` [PATCH v2 2/2] selftests/sgx: Make TCS table relocatable Jarkko Sakkinen 2022-03-28 21:49 ` [PATCH v2 1/2] selftests/sgx: Use rip relative addressing for encl_stack Reinette Chatre 0 siblings, 2 replies; 11+ messages in thread From: Jarkko Sakkinen @ 2022-03-22 7:43 UTC (permalink / raw) To: Shuah Khan Cc: Jarkko Sakkinen, Reinette Chatre, Dave Hansen, Shuah Khan, open list:INTEL SGX, open list:KERNEL SELFTEST FRAMEWORK, open list 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 -- 2.35.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 2/2] selftests/sgx: Make TCS table relocatable 2022-03-22 7:43 [PATCH v2 1/2] selftests/sgx: Use rip relative addressing for encl_stack Jarkko Sakkinen @ 2022-03-22 7:43 ` Jarkko Sakkinen 2022-03-28 21:49 ` [PATCH v2 1/2] selftests/sgx: Use rip relative addressing for encl_stack Reinette Chatre 1 sibling, 0 replies; 11+ messages in thread From: Jarkko Sakkinen @ 2022-03-22 7:43 UTC (permalink / raw) To: Shuah Khan Cc: Jarkko Sakkinen, Reinette Chatre, Dave Hansen, Shuah Khan, open list:INTEL SGX, open list:KERNEL SELFTEST FRAMEWORK, open list Add a PT_NOTE section with n_namesz containg "TCS" and n_descz containing 32-bit offset to the TCS table inside the enclave. This allows to place the TCS segment freely, and thereby make the kselftest binary layout way more robust. Cc: Reinette Chatre <reinette.chatre@intel.com> Cc: Dave Hansen <dave.hansen@linux.intel.com Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> --- v2: * Add RIP relative addressing fix for bootstrap as prepending patch, as this depends on it. * Moved TCS section as the last so that it is easy to add new TCS's, e.g dynamically with EAUG + EMODT, behind it. --- tools/testing/selftests/sgx/load.c | 56 ++++++++++++++----- tools/testing/selftests/sgx/main.c | 37 +++--------- tools/testing/selftests/sgx/main.h | 2 + tools/testing/selftests/sgx/test_encl.lds | 17 ++++-- .../selftests/sgx/test_encl_bootstrap.S | 7 +++ 5 files changed, 72 insertions(+), 47 deletions(-) diff --git a/tools/testing/selftests/sgx/load.c b/tools/testing/selftests/sgx/load.c index 006b464c8fc9..214b9da631bd 100644 --- a/tools/testing/selftests/sgx/load.c +++ b/tools/testing/selftests/sgx/load.c @@ -19,6 +19,9 @@ #include "defines.h" #include "main.h" +const char *TCS_NOTE_NAME = "TCS"; +const unsigned long TCS_NOTE_LEN = 4; + void encl_delete(struct encl *encl) { struct encl_segment *heap_seg; @@ -187,11 +190,31 @@ bool encl_load(const char *path, struct encl *encl, unsigned long heap_size) encl->nr_segments = 1; /* one for the heap */ + /* Count the loadable segments and discover the TCS array. */ for (i = 0; i < ehdr->e_phnum; i++) { Elf64_Phdr *phdr = &phdr_tbl[i]; + Elf64_Nhdr *note; + char *note_name; - if (phdr->p_type == PT_LOAD) + switch (phdr->p_type) { + case PT_LOAD: encl->nr_segments++; + break; + + case PT_NOTE: + note = encl->bin + (phdr->p_offset & PAGE_MASK); + note_name = &((char *)note)[sizeof(*note)]; + + if (note->n_namesz == TCS_NOTE_LEN && + !strncmp(note_name, TCS_NOTE_NAME, TCS_NOTE_LEN)) { + /* 32-bit address. */ + encl->tcs = (struct sgx_tcs *)(unsigned long)(note->n_descsz); + } + break; + + default: + break; + } } encl->segment_tbl = calloc(encl->nr_segments, @@ -215,31 +238,36 @@ bool encl_load(const char *path, struct encl *encl, unsigned long heap_size) goto err; } - if (j == 0 && flags != (PF_R | PF_W)) { - fprintf(stderr, - "TCS has invalid segment flags 0x%02x.\n", - phdr->p_flags); - goto err; - } - if (j == 0) { src_offset = phdr->p_offset & PAGE_MASK; encl->src = encl->bin + src_offset; + } + + seg->offset = (phdr->p_offset & PAGE_MASK) - src_offset; + seg->size = (phdr->p_filesz + PAGE_SIZE - 1) & PAGE_MASK; + seg->src = encl->src + seg->offset; + seg->measure = true; + + if (seg->offset == (unsigned long)encl->tcs) { + if (flags != (PF_R | PF_W)) { + fprintf(stderr, + "TCS has invalid segment flags 0x%02x.\n", + phdr->p_flags); + goto err; + } seg->prot = PROT_READ | PROT_WRITE; seg->flags = SGX_PAGE_TYPE_TCS << 8; } else { + if ((flags & (PF_R | PF_W | PF_X)) == (PF_R | PF_W)) + encl->data_offset = seg->offset; + seg->prot = (phdr->p_flags & PF_R) ? PROT_READ : 0; seg->prot |= (phdr->p_flags & PF_W) ? PROT_WRITE : 0; seg->prot |= (phdr->p_flags & PF_X) ? PROT_EXEC : 0; seg->flags = (SGX_PAGE_TYPE_REG << 8) | seg->prot; } - seg->offset = (phdr->p_offset & PAGE_MASK) - src_offset; - seg->size = (phdr->p_filesz + PAGE_SIZE - 1) & PAGE_MASK; - seg->src = encl->src + seg->offset; - seg->measure = true; - j++; } @@ -322,5 +350,7 @@ bool encl_build(struct encl *encl) return false; } + encl->tcs = (struct sgx_tcs *)((unsigned long)encl->tcs + encl->encl_base); + return true; } diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c index dd74fa42302e..b206548803b4 100644 --- a/tools/testing/selftests/sgx/main.c +++ b/tools/testing/selftests/sgx/main.c @@ -109,25 +109,6 @@ static Elf64_Sym *vdso_symtab_get(struct vdso_symtab *symtab, const char *name) return NULL; } -/* - * Return the offset in the enclave where the data segment can be found. - * The first RW segment loaded is the TCS, skip that to get info on the - * data segment. - */ -static off_t encl_get_data_offset(struct encl *encl) -{ - int i; - - for (i = 1; i < encl->nr_segments; i++) { - struct encl_segment *seg = &encl->segment_tbl[i]; - - if (seg->prot == (PROT_READ | PROT_WRITE)) - return seg->offset; - } - - return -1; -} - FIXTURE(enclave) { struct encl encl; struct sgx_enclave_run run; @@ -248,7 +229,7 @@ TEST_F(enclave, unclobbered_vdso) ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata)); memset(&self->run, 0, sizeof(self->run)); - self->run.tcs = self->encl.encl_base; + self->run.tcs = (__u64)self->encl.tcs; put_op.header.type = ENCL_OP_PUT_TO_BUFFER; put_op.value = MAGIC; @@ -321,7 +302,7 @@ TEST_F(enclave, unclobbered_vdso_oversubscribed) ASSERT_TRUE(setup_test_encl(total_mem, &self->encl, _metadata)); memset(&self->run, 0, sizeof(self->run)); - self->run.tcs = self->encl.encl_base; + self->run.tcs = (__u64)self->encl.tcs; put_op.header.type = ENCL_OP_PUT_TO_BUFFER; put_op.value = MAGIC; @@ -350,7 +331,7 @@ TEST_F(enclave, clobbered_vdso) ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata)); memset(&self->run, 0, sizeof(self->run)); - self->run.tcs = self->encl.encl_base; + self->run.tcs = (__u64)self->encl.tcs; put_op.header.type = ENCL_OP_PUT_TO_BUFFER; put_op.value = MAGIC; @@ -386,7 +367,7 @@ TEST_F(enclave, clobbered_vdso_and_user_function) ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata)); memset(&self->run, 0, sizeof(self->run)); - self->run.tcs = self->encl.encl_base; + self->run.tcs = (__u64)self->encl.tcs; self->run.user_handler = (__u64)test_handler; self->run.user_data = 0xdeadbeef; @@ -419,7 +400,7 @@ TEST_F(enclave, tcs_entry) ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata)); memset(&self->run, 0, sizeof(self->run)); - self->run.tcs = self->encl.encl_base; + self->run.tcs = (__u64)self->encl.tcs; op.type = ENCL_OP_NOP; @@ -431,7 +412,7 @@ TEST_F(enclave, tcs_entry) EXPECT_EQ(self->run.exception_addr, 0); /* Move to the next TCS. */ - self->run.tcs = self->encl.encl_base + PAGE_SIZE; + self->run.tcs = (__u64)self->encl.tcs + PAGE_SIZE; EXPECT_EQ(ENCL_CALL(&op, &self->run, true), 0); @@ -464,11 +445,9 @@ TEST_F(enclave, pte_permissions) ASSERT_TRUE(setup_test_encl(ENCL_HEAP_SIZE_DEFAULT, &self->encl, _metadata)); memset(&self->run, 0, sizeof(self->run)); - self->run.tcs = self->encl.encl_base; + self->run.tcs = (__u64)self->encl.tcs; - data_start = self->encl.encl_base + - encl_get_data_offset(&self->encl) + - PAGE_SIZE; + data_start = self->encl.encl_base + self->encl.data_offset + PAGE_SIZE; /* * Sanity check to ensure it is possible to write to page that will diff --git a/tools/testing/selftests/sgx/main.h b/tools/testing/selftests/sgx/main.h index b45c52ec7ab3..bccb263be8d9 100644 --- a/tools/testing/selftests/sgx/main.h +++ b/tools/testing/selftests/sgx/main.h @@ -29,6 +29,8 @@ struct encl { struct encl_segment *segment_tbl; struct sgx_secs secs; struct sgx_sigstruct sigstruct; + struct sgx_tcs *tcs; + unsigned long data_offset; }; extern unsigned char sign_key[]; diff --git a/tools/testing/selftests/sgx/test_encl.lds b/tools/testing/selftests/sgx/test_encl.lds index a1ec64f7d91f..d76df884d8a4 100644 --- a/tools/testing/selftests/sgx/test_encl.lds +++ b/tools/testing/selftests/sgx/test_encl.lds @@ -2,17 +2,15 @@ OUTPUT_FORMAT(elf64-x86-64) PHDRS { - tcs PT_LOAD; text PT_LOAD; data PT_LOAD; + tcs PT_LOAD; + note PT_NOTE; } SECTIONS { . = 0; - .tcs : { - *(.tcs*) - } : tcs . = ALIGN(4096); .text : { @@ -24,11 +22,20 @@ SECTIONS .data : { *(.data*) + . = ALIGN(4096); } : data + .tcs : { + *(.tcs*) + } : tcs + + .note : { + *(.note.tcs*) + } : note + /DISCARD/ : { *(.comment*) - *(.note*) + *(.note.gnu.*) *(.debug*) *(.eh_frame*) } diff --git a/tools/testing/selftests/sgx/test_encl_bootstrap.S b/tools/testing/selftests/sgx/test_encl_bootstrap.S index 1c1b5c6c4ffe..912b21537532 100644 --- a/tools/testing/selftests/sgx/test_encl_bootstrap.S +++ b/tools/testing/selftests/sgx/test_encl_bootstrap.S @@ -10,6 +10,7 @@ .section ".tcs", "aw" .balign 4096 +encl_tcs: .fill 1, 8, 0 # STATE (set by CPU) .fill 1, 8, 0 # FLAGS .quad encl_ssa_tcs1 # OSSA @@ -90,3 +91,9 @@ encl_stack: .balign 4096 # Stack of TCS #2 .space 4096 + + .section ".note.tcs", "", @progbits + .long 4 + .long encl_tcs + .long 0 + .string "TCS" -- 2.35.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] selftests/sgx: Use rip relative addressing for encl_stack 2022-03-22 7:43 [PATCH v2 1/2] selftests/sgx: Use rip relative addressing for encl_stack Jarkko Sakkinen 2022-03-22 7:43 ` [PATCH v2 2/2] selftests/sgx: Make TCS table relocatable Jarkko Sakkinen @ 2022-03-28 21:49 ` Reinette Chatre 2022-03-30 14:54 ` Jarkko Sakkinen 1 sibling, 1 reply; 11+ messages in thread From: Reinette Chatre @ 2022-03-28 21:49 UTC (permalink / raw) To: Jarkko Sakkinen, Shuah Khan Cc: Dave Hansen, Shuah Khan, open list:INTEL SGX, open list:KERNEL SELFTEST FRAMEWORK, open list 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. Reinette ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] selftests/sgx: Use rip relative addressing for encl_stack 2022-03-28 21:49 ` [PATCH v2 1/2] selftests/sgx: Use rip relative addressing for encl_stack Reinette Chatre @ 2022-03-30 14:54 ` Jarkko Sakkinen 2022-03-30 14:56 ` Jarkko Sakkinen 2022-03-30 17:40 ` Reinette Chatre 0 siblings, 2 replies; 11+ messages in thread From: Jarkko Sakkinen @ 2022-03-30 14:54 UTC (permalink / raw) To: Reinette Chatre Cc: Shuah Khan, Dave Hansen, Shuah Khan, open list:INTEL SGX, open list:KERNEL SELFTEST FRAMEWORK, open list 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. So, no, I won't consider that. This a dead obvious change. BR, Jarkko ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] selftests/sgx: Use rip relative addressing for encl_stack 2022-03-30 14:54 ` Jarkko Sakkinen @ 2022-03-30 14:56 ` Jarkko Sakkinen 2022-03-30 17:40 ` Reinette Chatre 1 sibling, 0 replies; 11+ messages in thread From: Jarkko Sakkinen @ 2022-03-30 14:56 UTC (permalink / raw) To: Reinette Chatre Cc: Shuah Khan, Dave Hansen, Shuah Khan, open list:INTEL SGX, open list:KERNEL SELFTEST FRAMEWORK, open list On Wed, Mar 30, 2022 at 05:54:18PM +0300, 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. > > So, no, I won't consider that. This a dead obvious change. How you organize TCS and stack is completely unrelated topic. BR, Jarkko ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] selftests/sgx: Use rip relative addressing for encl_stack 2022-03-30 14:54 ` Jarkko Sakkinen 2022-03-30 14:56 ` Jarkko Sakkinen @ 2022-03-30 17:40 ` Reinette Chatre [not found] ` <f68d472877b7136c32d8770603a3de38de59c322.camel@kernel.org> 1 sibling, 1 reply; 11+ messages in thread From: Reinette Chatre @ 2022-03-30 17:40 UTC (permalink / raw) To: Jarkko Sakkinen Cc: Shuah Khan, Dave Hansen, Shuah Khan, open list:INTEL SGX, open list:KERNEL SELFTEST FRAMEWORK, open list 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; ^ permalink raw reply related [flat|nested] 11+ messages in thread
[parent not found: <f68d472877b7136c32d8770603a3de38de59c322.camel@kernel.org>]
* Re: [PATCH v2 1/2] selftests/sgx: Use rip relative addressing for encl_stack [not found] ` <f68d472877b7136c32d8770603a3de38de59c322.camel@kernel.org> @ 2022-03-30 19:22 ` Jarkko Sakkinen 2022-03-30 20:05 ` Reinette Chatre 1 sibling, 0 replies; 11+ messages in thread From: Jarkko Sakkinen @ 2022-03-30 19:22 UTC (permalink / raw) To: Reinette Chatre Cc: Shuah Khan, Dave Hansen, Shuah Khan, open list:INTEL SGX, open list:KERNEL SELFTEST FRAMEWORK, open list On Wed, 2022-03-30 at 22:03 +0300, Jarkko Sakkinen wrote: > On Wed, 2022-03-30 at 10:40 -0700, Reinette Chatre wrote: > > Could you please elaborate how the compiler will fix it up? > > Sure. > > Here's the disassembly of the RBX version: > > [0x000021a9]> pi 1 > lea rax, [rbx + loc.encl_stack] > > Here's the same with s/RBX/RIP/: > > [0x000021a9]> pi 5 > lea rax, loc.encl_stack > Compiler will substitute correct offset relative to the RIP, > well, because it can and it makes sense. > > It is treated differently than other registers, e.g. when > LEA is assembled. > > BR, Jarkko To demonstrate this I did a couple of simple sessions with Rizin (fork/continuation/something of Radare2): jarkko@suppilovahvero ~/Downloads (main)> rizin test_encl.rbx.elf -- Temporally drop the verbosity prefixing the commands with ':' [0x00002000]> aaa [x] Analyze all flags starting with sym. and entry0 (aa) [x] Analyze function calls (aac) [x] Analyze len bytes of instructions for references (aar) [x] Check for classes [x] Type matching analysis for all functions (aaft) [x] Propagate noreturn information [x] Use -AA or aaaa to perform additional experimental analysis. [0x00002000]> /ad lea 0x0000206e # 7: lea rax, [rip + 0xf8b] 0x0000206f # 6: lea eax, [rip + 0xf8b] 0x0000207e # 1: leave 0x000020a1 # 7: lea rcx, [rip + 0xf58] 0x000020a2 # 6: lea ecx, [rip + 0xf58] 0x000020b4 # 1: leave 0x000020ee # 1: leave 0x00002128 # 1: leave 0x00002145 # 7: lea rax, [rip - 0x102] 0x00002146 # 6: lea eax, [rip - 0x102] 0x00002150 # 7: lea rax, [rip - 0xd7] 0x00002151 # 6: lea eax, [rip - 0xd7] 0x0000215b # 7: lea rax, [rip - 0xac] 0x0000215c # 6: lea eax, [rip - 0xac] 0x00002166 # 7: lea rax, [rip - 0x7d] 0x00002167 # 6: lea eax, [rip - 0x7d] 0x00002171 # 7: lea rax, [rip - 0x4e] 0x00002172 # 6: lea eax, [rip - 0x4e] 0x000021a7 # 1: leave 0x000021a9 # 7: lea rax, [rbx + loc.encl_stack] 0x000021aa # 6: lea eax, [rbx + loc.encl_stack] [0x00002000]> s 0x21a9 [0x000021a9]> pi 1 lea rax, [rbx + loc.encl_stack] [0x000021a9]> jarkko@suppilovahvero ~/Downloads (main)> rizin test_encl.elf -- Use V! to enter into the visual panels mode (dwm style) [0x00002000]> aaa [x] Analyze all flags starting with sym. and entry0 (aa) [x] Analyze function calls (aac) [x] Analyze len bytes of instructions for references (aar) [x] Check for classes [x] Type matching analysis for all functions (aaft) [x] Propagate noreturn information [x] Use -AA or aaaa to perform additional experimental analysis. [0x00002000]> /ad lea 0x0000206e # 7: lea rax, [rip + 0xf8b] 0x0000206f # 6: lea eax, [rip + 0xf8b] 0x0000207e # 1: leave 0x000020a1 # 7: lea rcx, [rip + 0xf58] 0x000020a2 # 6: lea ecx, [rip + 0xf58] 0x000020b4 # 1: leave 0x000020ee # 1: leave 0x00002128 # 1: leave 0x00002145 # 7: lea rax, [rip - 0x102] 0x00002146 # 6: lea eax, [rip - 0x102] 0x00002150 # 7: lea rax, [rip - 0xd7] 0x00002151 # 6: lea eax, [rip - 0xd7] 0x0000215b # 7: lea rax, [rip - 0xac] 0x0000215c # 6: lea eax, [rip - 0xac] 0x00002166 # 7: lea rax, [rip - 0x7d] 0x00002167 # 6: lea eax, [rip - 0x7d] 0x00002171 # 7: lea rax, [rip - 0x4e] 0x00002172 # 6: lea eax, [rip - 0x4e] 0x000021a7 # 1: leave 0x000021a9 # 7: lea rax, [rip + 0x5e50] 0x000021aa # 6: lea eax, [rip + 0x5e50] [0x00002000]> s 0x21a9 [0x000021a9]> pi 1 lea rax, loc.encl_stack [0x000021a9]> BR, Jarkko ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] selftests/sgx: Use rip relative addressing for encl_stack [not found] ` <f68d472877b7136c32d8770603a3de38de59c322.camel@kernel.org> 2022-03-30 19:22 ` Jarkko Sakkinen @ 2022-03-30 20:05 ` Reinette Chatre 2022-03-30 20:40 ` Jarkko Sakkinen 1 sibling, 1 reply; 11+ messages in thread From: Reinette Chatre @ 2022-03-30 20:05 UTC (permalink / raw) To: Jarkko Sakkinen Cc: Shuah Khan, Dave Hansen, Shuah Khan, open list:INTEL SGX, open list:KERNEL SELFTEST FRAMEWORK, open list On 3/30/2022 12:03 PM, Jarkko Sakkinen wrote: > On Wed, 2022-03-30 at 10:40 -0700, Reinette Chatre wrote: >> Could you please elaborate how the compiler will fix it up? > > Sure. > > Here's the disassembly of the RBX version: > > [0x000021a9]> pi 1 > lea rax, [rbx + loc.encl_stack] > > Here's the same with s/RBX/RIP/: > > [0x000021a9]> pi 5 > lea rax, loc.encl_stack > > Compiler will substitute correct offset relative to the RIP, > well, because it can and it makes sense. It does not make sense to me because, as proven with my test, the two threads end up sharing the same stack memory. Reinette ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] selftests/sgx: Use rip relative addressing for encl_stack 2022-03-30 20:05 ` Reinette Chatre @ 2022-03-30 20:40 ` Jarkko Sakkinen 2022-03-30 21:29 ` Reinette Chatre 0 siblings, 1 reply; 11+ messages in thread From: Jarkko Sakkinen @ 2022-03-30 20:40 UTC (permalink / raw) To: Reinette Chatre Cc: Shuah Khan, Dave Hansen, Shuah Khan, open list:INTEL SGX, open list:KERNEL SELFTEST FRAMEWORK, open list On Wed, 2022-03-30 at 13:05 -0700, Reinette Chatre wrote: > > > On 3/30/2022 12:03 PM, Jarkko Sakkinen wrote: > > On Wed, 2022-03-30 at 10:40 -0700, Reinette Chatre wrote: > > > Could you please elaborate how the compiler will fix it up? > > > > Sure. > > > > Here's the disassembly of the RBX version: > > > > [0x000021a9]> pi 1 > > lea rax, [rbx + loc.encl_stack] > > > > Here's the same with s/RBX/RIP/: > > > > [0x000021a9]> pi 5 > > lea rax, loc.encl_stack > > > > Compiler will substitute correct offset relative to the RIP, > > well, because it can and it makes sense. > > It does not make sense to me because, as proven with my test, > the two threads end up sharing the same stack memory. I see, I need to correct my patch, thanks! RBX gives correct results because of the binary organization, i.e. TCS's are placed to zero offset and forward, and unrelocated symbol is just compiled in as an untranslated offset. RPI is given correct results but how the semantics work right now is incompatible. Still, even for kselftest, I would consider a switch because that way: 1. You can layout binary however you wan and things won't break. 2. You can point to any symbol not just stack, if ever need. I admit it works semantically but it just super unrobust. BR, Jarkko ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] selftests/sgx: Use rip relative addressing for encl_stack 2022-03-30 20:40 ` Jarkko Sakkinen @ 2022-03-30 21:29 ` Reinette Chatre 2022-03-30 22:30 ` Jarkko Sakkinen 0 siblings, 1 reply; 11+ messages in thread From: Reinette Chatre @ 2022-03-30 21:29 UTC (permalink / raw) To: Jarkko Sakkinen Cc: Shuah Khan, Dave Hansen, Shuah Khan, open list:INTEL SGX, open list:KERNEL SELFTEST FRAMEWORK, open list Hi Jarkko, On 3/30/2022 1:40 PM, Jarkko Sakkinen wrote: > On Wed, 2022-03-30 at 13:05 -0700, Reinette Chatre wrote: >> >> >> On 3/30/2022 12:03 PM, Jarkko Sakkinen wrote: >>> On Wed, 2022-03-30 at 10:40 -0700, Reinette Chatre wrote: >>>> Could you please elaborate how the compiler will fix it up? >>> >>> Sure. >>> >>> Here's the disassembly of the RBX version: >>> >>> [0x000021a9]> pi 1 >>> lea rax, [rbx + loc.encl_stack] >>> >>> Here's the same with s/RBX/RIP/: >>> >>> [0x000021a9]> pi 5 >>> lea rax, loc.encl_stack >>> >>> Compiler will substitute correct offset relative to the RIP, >>> well, because it can and it makes sense. >> >> It does not make sense to me because, as proven with my test, >> the two threads end up sharing the same stack memory. > > I see, I need to correct my patch, thanks! > > RBX gives correct results because of the binary organization, > i.e. TCS's are placed to zero offset and forward, and > unrelocated symbol is just compiled in as an untranslated > offset. > > RPI is given correct results but how the semantics work > right now is incompatible. > > Still, even for kselftest, I would consider a switch > because that way: > > 1. You can layout binary however you wan and things > won't break. > 2. You can point to any symbol not just stack, if > ever need. > > I admit it works semantically but it just super > unrobust. I do not think that we need an exceptionally flexible runtime as part of the SGX selftests but instead something that is easy(*) to understand while also sufficient to support the tests. Reinette * I do not actually consider the existing enclave test binary easy to understand (this thread is proof) but keeping its complexity to be minimal would benefit folks needing to ramp up on SGX and/or debug kselftest failures. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] selftests/sgx: Use rip relative addressing for encl_stack 2022-03-30 21:29 ` Reinette Chatre @ 2022-03-30 22:30 ` Jarkko Sakkinen 0 siblings, 0 replies; 11+ messages in thread From: Jarkko Sakkinen @ 2022-03-30 22:30 UTC (permalink / raw) To: Reinette Chatre Cc: Shuah Khan, Dave Hansen, Shuah Khan, open list:INTEL SGX, open list:KERNEL SELFTEST FRAMEWORK, open list On Wed, 2022-03-30 at 14:29 -0700, Reinette Chatre wrote: > Hi Jarkko, > > On 3/30/2022 1:40 PM, Jarkko Sakkinen wrote: > > On Wed, 2022-03-30 at 13:05 -0700, Reinette Chatre wrote: > > > > > > > > > On 3/30/2022 12:03 PM, Jarkko Sakkinen wrote: > > > > On Wed, 2022-03-30 at 10:40 -0700, Reinette Chatre wrote: > > > > > Could you please elaborate how the compiler will fix it up? > > > > > > > > Sure. > > > > > > > > Here's the disassembly of the RBX version: > > > > > > > > [0x000021a9]> pi 1 > > > > lea rax, [rbx + loc.encl_stack] > > > > > > > > Here's the same with s/RBX/RIP/: > > > > > > > > [0x000021a9]> pi 5 > > > > lea rax, loc.encl_stack > > > > > > > > Compiler will substitute correct offset relative to the RIP, > > > > well, because it can and it makes sense. > > > > > > It does not make sense to me because, as proven with my test, > > > the two threads end up sharing the same stack memory. > > > > I see, I need to correct my patch, thanks! > > > > RBX gives correct results because of the binary organization, > > i.e. TCS's are placed to zero offset and forward, and > > unrelocated symbol is just compiled in as an untranslated > > offset. > > > > RPI is given correct results but how the semantics work > > right now is incompatible. > > > > Still, even for kselftest, I would consider a switch > > because that way: > > > > 1. You can layout binary however you wan and things > > won't break. > > 2. You can point to any symbol not just stack, if > > ever need. > > > > I admit it works semantically but it just super > > unrobust. > > I do not think that we need an exceptionally flexible > runtime as part of the SGX selftests but instead something > that is easy(*) to understand while also sufficient to support > the tests. > > Reinette > > * I do not actually consider the existing enclave test binary > easy to understand (this thread is proof) but keeping its > complexity to be minimal would benefit folks needing to > ramp up on SGX and/or debug kselftest failures. Based on you feedback I refined the patch: https://lore.kernel.org/linux-sgx/20220330222834.139769-1-jarkko@kernel.org/T/#u BR, Jarkko BR, Jarkko ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2022-03-30 22:31 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-03-22 7:43 [PATCH v2 1/2] selftests/sgx: Use rip relative addressing for encl_stack Jarkko Sakkinen 2022-03-22 7:43 ` [PATCH v2 2/2] selftests/sgx: Make TCS table relocatable Jarkko Sakkinen 2022-03-28 21:49 ` [PATCH v2 1/2] selftests/sgx: Use rip relative addressing for encl_stack Reinette Chatre 2022-03-30 14:54 ` Jarkko Sakkinen 2022-03-30 14:56 ` Jarkko Sakkinen 2022-03-30 17:40 ` Reinette Chatre [not found] ` <f68d472877b7136c32d8770603a3de38de59c322.camel@kernel.org> 2022-03-30 19:22 ` Jarkko Sakkinen 2022-03-30 20:05 ` Reinette Chatre 2022-03-30 20:40 ` Jarkko Sakkinen 2022-03-30 21:29 ` Reinette Chatre 2022-03-30 22:30 ` Jarkko Sakkinen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).