| Message ID | 20230808193145.8860-4-jo.vanbulck@cs.kuleuven.be (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | selftests/sgx: Fix compilation errors | expand |
On Tue Aug 8, 2023 at 10:31 PM EEST, Jo Van Bulck wrote: > Static-pie binaries normally include a startup routine to perform any ELF > relocations from .rela.dyn. Since the enclave loading process is different > and glibc is not included, do the necessary relocation for encl_op_array > entries manually at runtime relative to the enclave base to ensure correct > function pointers. > > Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be> What happens if I only apply 1/8 and 2/8 from this patch set? I'm just wondering why there is no mention of "-static-pie" here. > --- > tools/testing/selftests/sgx/test_encl.c | 35 +++++++++++++++-------- > tools/testing/selftests/sgx/test_encl.lds | 3 ++ > 2 files changed, 26 insertions(+), 12 deletions(-) > > diff --git a/tools/testing/selftests/sgx/test_encl.c b/tools/testing/selftests/sgx/test_encl.c > index c0d6397295e3..c71dfbadd2d9 100644 > --- a/tools/testing/selftests/sgx/test_encl.c > +++ b/tools/testing/selftests/sgx/test_encl.c > @@ -119,21 +119,32 @@ static void do_encl_op_nop(void *_op) > > } > > +/* > + * Symbol placed at the start of the enclave image by the linker script. > + * Declare this extern symbol with visibility "hidden" to ensure the > + * compiler does not access it through the GOT. > + */ > +extern uint8_t __attribute__((visibility("hidden"))) __enclave_base; I'd rename this as __encl_base to be consistent with other naming here. You could also declare for convenience and clarity: static const uint64_t encl_base = (uint64_t)&__encl_base; > + > +void (*encl_op_array[ENCL_OP_MAX])(void *) = { > + do_encl_op_put_to_buf, > + do_encl_op_get_from_buf, > + do_encl_op_put_to_addr, > + do_encl_op_get_from_addr, > + do_encl_op_nop, > + do_encl_eaccept, > + do_encl_emodpe, > + do_encl_init_tcs_page, > +}; > + Why you need to drop "const"? The array is not dynamically updated, i.e. there's no reason to move it away from rodata section. If this was kernel code, such modification would be considered as a regression. I would also consider cleaning this up a bit further, while you are refactoring anyway, and declare a typedef: typedef void (*encl_op_t)(void *); const encl_op_t encl_op_array[ENCL_OP_MAX] = { > void encl_body(void *rdi, void *rsi) > { > - const void (*encl_op_array[ENCL_OP_MAX])(void *) = { > - do_encl_op_put_to_buf, > - do_encl_op_get_from_buf, > - do_encl_op_put_to_addr, > - do_encl_op_get_from_addr, > - do_encl_op_nop, > - do_encl_eaccept, > - do_encl_emodpe, > - do_encl_init_tcs_page, > - }; > - > struct encl_op_header *op = (struct encl_op_header *)rdi; > > + /* > + * Manually rebase the loaded function pointer as enclaves cannot > + * rely on startup routines to perform static pie relocations. > + */ This comment is not very useful. I'd consider dropping it. > if (op->type < ENCL_OP_MAX) > - (*encl_op_array[op->type])(op); > + (*(((uint64_t) &__enclave_base) + encl_op_array[op->type]))(op); ~ should not have white space here (coding style) This would be cleaner IMHO: void encl_body(void *rdi, void *rsi) { struct encl_op_header *header = (struct encl_op_header *)rdi; encl_op_t op; if (header->type >= ENCL_OP_MAX) return; /* * "encl_base" needs to be added, as this call site *cannot be* * made rip-relative by the compiler, or fixed up by any other * possible means. */ op = encl_base + encl_op_array[header->type]; (*op)(header); } > } > diff --git a/tools/testing/selftests/sgx/test_encl.lds b/tools/testing/selftests/sgx/test_encl.lds > index ca659db2a534..73d9c8bbe7de 100644 > --- a/tools/testing/selftests/sgx/test_encl.lds > +++ b/tools/testing/selftests/sgx/test_encl.lds > @@ -32,6 +32,9 @@ SECTIONS > *(.note*) > *(.debug*) > *(.eh_frame*) > + /* Dynamic symbol table not supported in enclaves */ I'd drop this comment. > + *(.dyn*) > + *(.gnu.hash) > } > } > > -- > 2.34.1 BR, Jarkko
On 10.08.23 13:32, Jarkko Sakkinen wrote: > What happens if I only apply 1/8 and 2/8 from this patch set? This would work fine for gcc -O0/1/2/3, as encl_op_array happens to be locally initialized: 00000000000023f4 <encl_body>: /* snipped */ 2408: 48 8d 05 ec fe ff ff lea -0x114(%rip),%rax # 22fb <do_encl_op_put_to_buf> 240f: 48 89 45 b0 mov %rax,-0x50(%rbp) 2413: 48 8d 05 18 ff ff ff lea -0xe8(%rip),%rax # 2332 <do_encl_op_get_from_buf> 241a: 48 89 45 b8 mov %rax,-0x48(%rbp) 241e: 48 8d 05 44 ff ff ff lea -0xbc(%rip),%rax # 2369 <do_encl_op_put_to_addr> /* snipped */ However, when compiling with -Os, the initialization proceeds through a prepared copy from .data with hard-coded (ie non RIP-relative) function addresses: > 00000000000021b5 <encl_body>: > /* snipped */ > 21bc: 48 8d 35 3d 2e 00 00 lea 0x2e3d(%rip),%rsi > # 5000 <encl_buffer+0x2000> > 21c3: 48 8d 7c 24 b8 lea -0x48(%rsp),%rdi > 21c8: b9 10 00 00 00 mov $0x10,%ecx > 21cd: f3 a5 rep movsl %ds:(%rsi),%es:(%rdi) > /* snipped */ > I'm just wondering why there is no mention of "-static-pie" here. This patch 3/8 is expected to be applied on top of 2/8 which adds "-static-pie". While "-static-pie" is necessary to generate proper, position-independent code when referencing global variables, there may still be relocations left. These are normally handled by glibc on startup, but we don't have that in the test enclave, so this commit explicitly handles the (only) relocations for encl_op_array. When only applying 2/8, gcc generates correct code with -O0/1/2/3, as the local encl_op_array initialization happens to be initialized in encl_body: >> +extern uint8_t __attribute__((visibility("hidden"))) __enclave_base; > > I'd rename this as __encl_base to be consistent with other naming here. > > You could also declare for convenience and clarity: > > static const uint64_t encl_base = (uint64_t)&__encl_base; > Thanks, makes sense! >> + >> +void (*encl_op_array[ENCL_OP_MAX])(void *) = { >> + do_encl_op_put_to_buf, >> + do_encl_op_get_from_buf, >> + do_encl_op_put_to_addr, >> + do_encl_op_get_from_addr, >> + do_encl_op_nop, >> + do_encl_eaccept, >> + do_encl_emodpe, >> + do_encl_init_tcs_page, >> +}; >> + > > Why you need to drop "const"? The array is not dynamically updated, i.e. > there's no reason to move it away from rodata section. If this was > kernel code, such modification would be considered as a regression. I dropped "const" to work around a clang warning: test_encl.c:130:2: warning: incompatible pointer types initializing 'const void (*)(void *)' with an expression of type 'void (void *)' [-Wincompatible-pointer-types] But I agree dropping const is inferior and it's better to fix the incompatible pointer types as per below. > I would also consider cleaning this up a bit further, while you are > refactoring anyway, and declare a typedef: > > typedef void (*encl_op_t)(void *); > > const encl_op_t encl_op_array[ENCL_OP_MAX] = { Thanks this is indeed cleaner. This also fixes the above clang warning. > >> void encl_body(void *rdi, void *rsi) >> { >> - const void (*encl_op_array[ENCL_OP_MAX])(void *) = { >> - do_encl_op_put_to_buf, >> - do_encl_op_get_from_buf, >> - do_encl_op_put_to_addr, >> - do_encl_op_get_from_addr, >> - do_encl_op_nop, >> - do_encl_eaccept, >> - do_encl_emodpe, >> - do_encl_init_tcs_page, >> - }; >> - >> struct encl_op_header *op = (struct encl_op_header *)rdi; >> >> + /* >> + * Manually rebase the loaded function pointer as enclaves cannot >> + * rely on startup routines to perform static pie relocations. >> + */ > > This comment is not very useful. I'd consider dropping it. Dropped. > >> if (op->type < ENCL_OP_MAX) >> - (*encl_op_array[op->type])(op); >> + (*(((uint64_t) &__enclave_base) + encl_op_array[op->type]))(op); > ~ > should not have white space here (coding style) Thanks for pointing this out. > This would be cleaner IMHO: > > void encl_body(void *rdi, void *rsi) > { > struct encl_op_header *header = (struct encl_op_header *)rdi; > encl_op_t op; > > if (header->type >= ENCL_OP_MAX) > return; > > /* > * "encl_base" needs to be added, as this call site *cannot be* > * made rip-relative by the compiler, or fixed up by any other > * possible means. > */ > op = encl_base + encl_op_array[header->type]; > > (*op)(header); > } Thanks, this is indeed much cleaner! Including this in the next revision. >> + /* Dynamic symbol table not supported in enclaves */ > > I'd drop this comment. Dropped.
On Sat Aug 19, 2023 at 3:32 AM EEST, Jo Van Bulck wrote: > On 10.08.23 13:32, Jarkko Sakkinen wrote: > > What happens if I only apply 1/8 and 2/8 from this patch set? > > This would work fine for gcc -O0/1/2/3, as encl_op_array happens to be > locally initialized: > > 00000000000023f4 <encl_body>: > /* snipped */ > 2408: 48 8d 05 ec fe ff ff lea -0x114(%rip),%rax > # 22fb <do_encl_op_put_to_buf> > 240f: 48 89 45 b0 mov %rax,-0x50(%rbp) > 2413: 48 8d 05 18 ff ff ff lea -0xe8(%rip),%rax > # 2332 <do_encl_op_get_from_buf> > 241a: 48 89 45 b8 mov %rax,-0x48(%rbp) > 241e: 48 8d 05 44 ff ff ff lea -0xbc(%rip),%rax > # 2369 <do_encl_op_put_to_addr> > /* snipped */ > > However, when compiling with -Os, the initialization proceeds through a > prepared copy from .data with hard-coded (ie non RIP-relative) function > addresses: > > > 00000000000021b5 <encl_body>: > > /* snipped */ > > 21bc: 48 8d 35 3d 2e 00 00 lea 0x2e3d(%rip),%rsi > > # 5000 <encl_buffer+0x2000> > > 21c3: 48 8d 7c 24 b8 lea -0x48(%rsp),%rdi > > 21c8: b9 10 00 00 00 mov $0x10,%ecx > > 21cd: f3 a5 rep movsl %ds:(%rsi),%es:(%rdi) > > /* snipped */ Thank you for the explanation. > > I'm just wondering why there is no mention of "-static-pie" here. > > This patch 3/8 is expected to be applied on top of 2/8 which adds > "-static-pie". While "-static-pie" is necessary to generate proper, > position-independent code when referencing global variables, there may > still be relocations left. These are normally handled by glibc on > startup, but we don't have that in the test enclave, so this commit > explicitly handles the (only) relocations for encl_op_array. > > When only applying 2/8, gcc generates correct code with -O0/1/2/3, as > the local encl_op_array initialization happens to be initialized in > encl_body: > > >> +extern uint8_t __attribute__((visibility("hidden"))) __enclave_base; > > > > I'd rename this as __encl_base to be consistent with other naming here. > > > > You could also declare for convenience and clarity: > > > > static const uint64_t encl_base = (uint64_t)&__encl_base; > > > > Thanks, makes sense! Great! > >> + > >> +void (*encl_op_array[ENCL_OP_MAX])(void *) = { > >> + do_encl_op_put_to_buf, > >> + do_encl_op_get_from_buf, > >> + do_encl_op_put_to_addr, > >> + do_encl_op_get_from_addr, > >> + do_encl_op_nop, > >> + do_encl_eaccept, > >> + do_encl_emodpe, > >> + do_encl_init_tcs_page, > >> +}; > >> + > > > > Why you need to drop "const"? The array is not dynamically updated, i.e. > > there's no reason to move it away from rodata section. If this was > > kernel code, such modification would be considered as a regression. > > I dropped "const" to work around a clang warning: > > test_encl.c:130:2: warning: incompatible pointer types initializing > 'const void (*)(void *)' with an expression of type 'void (void *)' > [-Wincompatible-pointer-types] > > But I agree dropping const is inferior and it's better to fix the > incompatible pointer types as per below. > > > I would also consider cleaning this up a bit further, while you are > > refactoring anyway, and declare a typedef: > > > > typedef void (*encl_op_t)(void *); > > > > const encl_op_t encl_op_array[ENCL_OP_MAX] = { > > Thanks this is indeed cleaner. This also fixes the above clang warning. > > > > >> void encl_body(void *rdi, void *rsi) > >> { > >> - const void (*encl_op_array[ENCL_OP_MAX])(void *) = { > >> - do_encl_op_put_to_buf, > >> - do_encl_op_get_from_buf, > >> - do_encl_op_put_to_addr, > >> - do_encl_op_get_from_addr, > >> - do_encl_op_nop, > >> - do_encl_eaccept, > >> - do_encl_emodpe, > >> - do_encl_init_tcs_page, > >> - }; > >> - > >> struct encl_op_header *op = (struct encl_op_header *)rdi; > >> > >> + /* > >> + * Manually rebase the loaded function pointer as enclaves cannot > >> + * rely on startup routines to perform static pie relocations. > >> + */ > > > > This comment is not very useful. I'd consider dropping it. > > Dropped. > > > > >> if (op->type < ENCL_OP_MAX) > >> - (*encl_op_array[op->type])(op); > >> + (*(((uint64_t) &__enclave_base) + encl_op_array[op->type]))(op); > > ~ > > should not have white space here (coding style) > > Thanks for pointing this out. > > > This would be cleaner IMHO: > > > > void encl_body(void *rdi, void *rsi) > > { > > struct encl_op_header *header = (struct encl_op_header *)rdi; > > encl_op_t op; > > > > if (header->type >= ENCL_OP_MAX) > > return; > > > > /* > > * "encl_base" needs to be added, as this call site *cannot be* > > * made rip-relative by the compiler, or fixed up by any other > > * possible means. > > */ > > op = encl_base + encl_op_array[header->type]; > > > > (*op)(header); > > } > > Thanks, this is indeed much cleaner! Including this in the next revision. > > >> + /* Dynamic symbol table not supported in enclaves */ > > > > I'd drop this comment. > > Dropped. Awesome! BR, Jarkko
diff --git a/tools/testing/selftests/sgx/test_encl.c b/tools/testing/selftests/sgx/test_encl.c index c0d6397295e3..c71dfbadd2d9 100644 --- a/tools/testing/selftests/sgx/test_encl.c +++ b/tools/testing/selftests/sgx/test_encl.c @@ -119,21 +119,32 @@ static void do_encl_op_nop(void *_op) } +/* + * Symbol placed at the start of the enclave image by the linker script. + * Declare this extern symbol with visibility "hidden" to ensure the + * compiler does not access it through the GOT. + */ +extern uint8_t __attribute__((visibility("hidden"))) __enclave_base; + +void (*encl_op_array[ENCL_OP_MAX])(void *) = { + do_encl_op_put_to_buf, + do_encl_op_get_from_buf, + do_encl_op_put_to_addr, + do_encl_op_get_from_addr, + do_encl_op_nop, + do_encl_eaccept, + do_encl_emodpe, + do_encl_init_tcs_page, +}; + void encl_body(void *rdi, void *rsi) { - const void (*encl_op_array[ENCL_OP_MAX])(void *) = { - do_encl_op_put_to_buf, - do_encl_op_get_from_buf, - do_encl_op_put_to_addr, - do_encl_op_get_from_addr, - do_encl_op_nop, - do_encl_eaccept, - do_encl_emodpe, - do_encl_init_tcs_page, - }; - struct encl_op_header *op = (struct encl_op_header *)rdi; + /* + * Manually rebase the loaded function pointer as enclaves cannot + * rely on startup routines to perform static pie relocations. + */ if (op->type < ENCL_OP_MAX) - (*encl_op_array[op->type])(op); + (*(((uint64_t) &__enclave_base) + encl_op_array[op->type]))(op); } diff --git a/tools/testing/selftests/sgx/test_encl.lds b/tools/testing/selftests/sgx/test_encl.lds index ca659db2a534..73d9c8bbe7de 100644 --- a/tools/testing/selftests/sgx/test_encl.lds +++ b/tools/testing/selftests/sgx/test_encl.lds @@ -32,6 +32,9 @@ SECTIONS *(.note*) *(.debug*) *(.eh_frame*) + /* Dynamic symbol table not supported in enclaves */ + *(.dyn*) + *(.gnu.hash) } }
Static-pie binaries normally include a startup routine to perform any ELF relocations from .rela.dyn. Since the enclave loading process is different and glibc is not included, do the necessary relocation for encl_op_array entries manually at runtime relative to the enclave base to ensure correct function pointers. Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be> --- tools/testing/selftests/sgx/test_encl.c | 35 +++++++++++++++-------- tools/testing/selftests/sgx/test_encl.lds | 3 ++ 2 files changed, 26 insertions(+), 12 deletions(-)