linux-kernel - Re: [PATCH v4 03/13] selftests/sgx: Handle relocations in test enclave

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <06f20425546aa13310a9fc25f081b8d70a6f1f44.camel@intel.com>
Date:   Mon, 28 Aug 2023 13:15:47 +0000
From:   "Huang, Kai" <kai.huang@...el.com>
To:     "linux-sgx@...r.kernel.org" <linux-sgx@...r.kernel.org>,
        "jarkko@...nel.org" <jarkko@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Van Bulck, Jo" <jo.vanbulck@...kuleuven.be>
CC:     "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>
Subject: Re: [PATCH v4 03/13] selftests/sgx: Handle relocations in test
 enclave

On Fri, 2023-08-25 at 15:32 +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.

Sorry for late reply.

I am wondering is this the right justification for _this_ particular patch?

Even above paragraph is true, the existing code w/o this patch can work because
the generated asm code uses "lea (-xxx)(%rip), %<reg>" to get the right address
and store it to the array on the stack.

It stops to work because you want to use -Os, in which case the generated asm
code instead initializes the array by copying an array (which has function
addresses starting from 0) generated by the compiler/linker.

So to me the true justification should be "using -Os breaks the code".  Or do
you think "the compiler generating code to initialize the array on the stack
using RIP-relative addressing to get the function address" is completely a lucky
thing?

Anyway, it will be very helpful to include the assembly code generated both w/
and w/o using -Os here to the changelog to demonstrate the problem and we need
this patch to fixup.

Without those information, it's basically very hard for people to understand why
this is needed.  This will save maintainer's time, and make git blamer's life
easy in the future.

> 
> Signed-off-by: Jo Van Bulck <jo.vanbulck@...kuleuven.be>
> Reviewed-by: Jarkko Sakkinen <jarkko@...nel.org>
> ---
>  tools/testing/selftests/sgx/test_encl.c | 48 +++++++++++++++++--------
>  1 file changed, 33 insertions(+), 15 deletions(-)
> 
> diff --git a/tools/testing/selftests/sgx/test_encl.c b/tools/testing/selftests/sgx/test_encl.c
> index c0d6397295e3..706c1f7260ff 100644
> --- a/tools/testing/selftests/sgx/test_encl.c
> +++ b/tools/testing/selftests/sgx/test_encl.c
> @@ -119,21 +119,39 @@ 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;
> +
> +typedef void (*encl_op_t)(void *);
> +static 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,
> +};
> +
>  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;
> +
> +	/*
> +	 * The enclave base address needs to be added, as this call site
> +	 * *cannot be* made rip-relative by the compiler, or fixed up by
> +	 * any other possible means.
> +	 */

Is it better to explicitly call out the compiler generates RIP-relative
addressing code to get the address associated with '__encl_base' symbol, so we
can get the actual enclave base during runtime?

Maybe it's obvious, but I am not sure :-)

> +	op = ((uint64_t)&__encl_base) + encl_op_array[header->type];
> +
> +	(*op)(header);
>  }