| Message ID | 20230819094332.8535-4-jo.vanbulck@cs.kuleuven.be (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | selftests/sgx: Fix compilation errors | expand |
On Sat, 2023-08-19 at 11:43 +0200, 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> > --- > tools/testing/selftests/sgx/test_encl.c | 49 ++++++++++++++++------- > tools/testing/selftests/sgx/test_encl.lds | 2 + > 2 files changed, 36 insertions(+), 15 deletions(-) > > diff --git a/tools/testing/selftests/sgx/test_encl.c b/tools/testing/selftests/sgx/test_encl.c > index c0d639729..7633fb7cb 100644 > --- a/tools/testing/selftests/sgx/test_encl.c > +++ b/tools/testing/selftests/sgx/test_encl.c > @@ -119,21 +119,40 @@ 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 const uint8_t __attribute__((visibility("hidden"))) __encl_base; > +static const uint64_t encl_base = (uint64_t)&__encl_base; I had hard time to understand this. The __encl_base is a symbol which is a fixed value set by the compiler/linker. encl_base has the real storage in the .data section, but the value is also build-time fixed. IIUC we need some code to explicitly override it, but I don't see where it's done. Perhaps I missed something? > + > +typedef void (*encl_op_t)(void *); > +const encl_op_t encl_op_array[ENCL_OP_MAX] = { > + 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, > +}; Any reason it cannot be 'static'? > + > 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; > - > - if (op->type < ENCL_OP_MAX) > - (*encl_op_array[op->type])(op); > + 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 62d37160f..b86c86060 100644 > --- a/tools/testing/selftests/sgx/test_encl.lds > +++ b/tools/testing/selftests/sgx/test_encl.lds > @@ -32,6 +32,8 @@ SECTIONS > *(.note*) > *(.debug*) > *(.eh_frame*) > + *(.dyn*) > + *(.gnu.hash) This looks can be in a separate patch, because it's not directly related to what you are trying to fix. But I don't want to make things unnecessarily complicated for selftests, so fine to me if you still want to keep it. But if you do, perhaps you can add some justification to the changelog saying something like: opportunistically discard ".dyn*" and ".gnu.hash" which the enclave loader cannot handle. Anyway, still better to make a separate patch for such purpose IMHO.
On Sat Aug 19, 2023 at 12:43 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> > --- > tools/testing/selftests/sgx/test_encl.c | 49 ++++++++++++++++------- > tools/testing/selftests/sgx/test_encl.lds | 2 + > 2 files changed, 36 insertions(+), 15 deletions(-) > > diff --git a/tools/testing/selftests/sgx/test_encl.c b/tools/testing/selftests/sgx/test_encl.c > index c0d639729..7633fb7cb 100644 > --- a/tools/testing/selftests/sgx/test_encl.c > +++ b/tools/testing/selftests/sgx/test_encl.c > @@ -119,21 +119,40 @@ 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 const uint8_t __attribute__((visibility("hidden"))) __encl_base; > +static const uint64_t encl_base = (uint64_t)&__encl_base; > + > +typedef void (*encl_op_t)(void *); > +const encl_op_t encl_op_array[ENCL_OP_MAX] = { > + 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; > - > - if (op->type < ENCL_OP_MAX) > - (*encl_op_array[op->type])(op); > + 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 62d37160f..b86c86060 100644 > --- a/tools/testing/selftests/sgx/test_encl.lds > +++ b/tools/testing/selftests/sgx/test_encl.lds > @@ -32,6 +32,8 @@ SECTIONS > *(.note*) > *(.debug*) > *(.eh_frame*) > + *(.dyn*) > + *(.gnu.hash) > } > } > > -- > 2.25.1 Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org> BR, Jarkko
On 22.08.23 03:11, Huang, Kai wrote:>> +/* >> + * 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 const uint8_t __attribute__((visibility("hidden"))) __encl_base; >> +static const uint64_t encl_base = (uint64_t)&__encl_base; > > I had hard time to understand this. The __encl_base is a symbol which is a > fixed value set by the compiler/linker. encl_base has the real storage in the > .data section, but the value is also build-time fixed. IIUC we need some code > to explicitly override it, but I don't see where it's done. Perhaps I missed > something? Thank you for catching this. Such initialization would indeed have to be explicitly overridden at runtime and I somehow overlooked this (it seems I left the line to actually run the tests commented out after compilation in my test script for all versions; this is now fixed). Apologies for the confusion, my bad! I've reverted this back to an explicit (uit64_t)&__encl_base cast in the next patch iteration to avoid such confusion. >> + >> +typedef void (*encl_op_t)(void *); >> +const encl_op_t encl_op_array[ENCL_OP_MAX] = { >> + 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, >> +}; > > Any reason it cannot be 'static'? I tested static indeed also works and will include this in the next iteration. >> diff --git a/tools/testing/selftests/sgx/test_encl.lds b/tools/testing/selftests/sgx/test_encl.lds >> index 62d37160f..b86c86060 100644 >> --- a/tools/testing/selftests/sgx/test_encl.lds >> +++ b/tools/testing/selftests/sgx/test_encl.lds >> @@ -32,6 +32,8 @@ SECTIONS >> *(.note*) >> *(.debug*) >> *(.eh_frame*) >> + *(.dyn*) >> + *(.gnu.hash) > > This looks can be in a separate patch, because it's not directly related to what > you are trying to fix. > > But I don't want to make things unnecessarily complicated for selftests, so fine > to me if you still want to keep it. But if you do, perhaps you can add some > justification to the changelog saying something like: opportunistically discard > ".dyn*" and ".gnu.hash" which the enclave loader cannot handle. Anyway, still > better to make a separate patch for such purpose IMHO. Thanks, splitting this off in a separate commit for the next iteration. Best, Jo
diff --git a/tools/testing/selftests/sgx/test_encl.c b/tools/testing/selftests/sgx/test_encl.c index c0d639729..7633fb7cb 100644 --- a/tools/testing/selftests/sgx/test_encl.c +++ b/tools/testing/selftests/sgx/test_encl.c @@ -119,21 +119,40 @@ 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 const uint8_t __attribute__((visibility("hidden"))) __encl_base; +static const uint64_t encl_base = (uint64_t)&__encl_base; + +typedef void (*encl_op_t)(void *); +const encl_op_t encl_op_array[ENCL_OP_MAX] = { + 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; - - if (op->type < ENCL_OP_MAX) - (*encl_op_array[op->type])(op); + 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 62d37160f..b86c86060 100644 --- a/tools/testing/selftests/sgx/test_encl.lds +++ b/tools/testing/selftests/sgx/test_encl.lds @@ -32,6 +32,8 @@ SECTIONS *(.note*) *(.debug*) *(.eh_frame*) + *(.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 | 49 ++++++++++++++++------- tools/testing/selftests/sgx/test_encl.lds | 2 + 2 files changed, 36 insertions(+), 15 deletions(-)