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

["Huang, Kai" <kai.huang@intel.com>]

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.