Re: [PATCH 3/8] selftests/sgx: Handle relocations in test enclave

[Jarkko Sakkinen]

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