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

[Jo Van Bulck]

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.