linux-kernel - Re: [PATCH v3 2/9] selftests/sgx: Produce static-pie executable for 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: <71ad6389da7db8541dada0276db33f98e2a4fdcf.camel@intel.com>
Date:   Tue, 22 Aug 2023 00:26:13 +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 v3 2/9] selftests/sgx: Produce static-pie executable for
 test enclave

On Sat, 2023-08-19 at 11:43 +0200, Jo Van Bulck wrote:
> The current combination of -static and -fPIC creates a static executable
> with position-dependent addresses for global variables. Use -static-pie
> and -fPIE to create a proper static position independent executable that
> can be loaded at any address without a dynamic linker.

IMHO, this patch is also mixing different things together.

Specifically, ...
> 
> Link: https://lore.kernel.org/all/f9c24d89-ed72-7d9e-c650-050d722c6b04@cs.kuleuven.be/
> Signed-off-by: Jo Van Bulck <jo.vanbulck@...kuleuven.be>
> Reviewed-by: Jarkko Sakkinen <jarkko@...nel.org>
> ---
>  tools/testing/selftests/sgx/Makefile              |  2 +-
>  tools/testing/selftests/sgx/test_encl.lds         |  1 +
>  tools/testing/selftests/sgx/test_encl_bootstrap.S | 12 ++++++------
>  3 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/testing/selftests/sgx/Makefile b/tools/testing/selftests/sgx/Makefile
> index 50aab6b57..1d6315a2e 100644
> --- a/tools/testing/selftests/sgx/Makefile
> +++ b/tools/testing/selftests/sgx/Makefile
> @@ -13,7 +13,7 @@ endif
>  
>  INCLUDES := -I$(top_srcdir)/tools/include
>  HOST_CFLAGS := -Wall -Werror -g $(INCLUDES) -fPIC -z noexecstack
> -ENCL_CFLAGS := -Wall -Werror -static -nostdlib -nostartfiles -fPIC \
> +ENCL_CFLAGS := -Wall -Werror -static-pie -nostdlib -nostartfiles -fPIE \
>  	       -fno-stack-protector -mrdrnd $(INCLUDES)

... I think only this build flag change should be done in  this patch, as
described in the changelog.

Because ...
>  
>  TEST_CUSTOM_PROGS := $(OUTPUT)/test_sgx
> diff --git a/tools/testing/selftests/sgx/test_encl.lds b/tools/testing/selftests/sgx/test_encl.lds
> index a1ec64f7d..62d37160f 100644
> --- a/tools/testing/selftests/sgx/test_encl.lds
> +++ b/tools/testing/selftests/sgx/test_encl.lds
> @@ -10,6 +10,7 @@ PHDRS
>  SECTIONS
>  {
>  	. = 0;
> +        __encl_base = .;
>  	.tcs : {
>  		*(.tcs*)
>  	} : tcs
> diff --git a/tools/testing/selftests/sgx/test_encl_bootstrap.S b/tools/testing/selftests/sgx/test_encl_bootstrap.S
> index 03ae0f57e..28fe5d2ac 100644
> --- a/tools/testing/selftests/sgx/test_encl_bootstrap.S
> +++ b/tools/testing/selftests/sgx/test_encl_bootstrap.S
> @@ -42,9 +42,12 @@
>  encl_entry:
>  	# RBX contains the base address for TCS, which is the first address
>  	# inside the enclave for TCS #1 and one page into the enclave for
> -	# TCS #2. By adding the value of encl_stack to it, we get
> -	# the absolute address for the stack.
> -	lea	(encl_stack)(%rbx), %rax
> +	# TCS #2. First make it relative by substracting __encl_base and
> +	# then add the address of encl_stack to get the address for the stack.
> +	lea __encl_base(%rip), %rax
> +	sub %rax, %rbx
> +	lea encl_stack(%rip), %rax
> +	add %rbx, %rax

... if I am not missing anything, this chunk isn't needed for _this_ patch.  The
old code can still produce the correct stack address. __encl_base is only needed
for the next patch, thus the relevant change should be moved to the next patch.

>  	jmp encl_entry_core
>  encl_dyn_entry:
>  	# Entry point for dynamically created TCS page expected to follow
> @@ -55,12 +58,9 @@ encl_entry_core:
>  	push	%rax
>  
>  	push	%rcx # push the address after EENTER
> -	push	%rbx # push the enclave base address
>  
>  	call	encl_body
>  
> -	pop	%rbx # pop the enclave base address
> -
>  	/* Clear volatile GPRs, except RAX (EEXIT function). */
>  	xor     %rcx, %rcx
>  	xor     %rdx, %rdx

I honestly don't understand what's the purpose of this code change.  I believe
this change can be done (because it looks there's no need push/pop %rbx in the
first place), but again it should be in the next patch I suppose.