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

["Jarkko Sakkinen" <jarkko@kernel.org>]

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