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
