[PATCH 0/5] selftests/sgx: Fix compilation errors.

[PATCH 0/5] selftests/sgx: Fix compilation errors.

[Jo Van Bulck]

Hi,

This patch series ensures that all SGX selftests succeed when compiling
with optimizations (as tested with -O{0,1,2,3,s} for both gcc 11.3.0 and
clang 14.0.0). The aim of the patches is to avoid reliance on undefined,
compiler-specific behavior that can make the test results fragile.

If deemed useful, I can also include an elementary wrapper shell script to
compile and run the tests for different compilers (gcc/clang) and
optimization levels.

Best,
Jo

Jo Van Bulck (5):
  selftests/sgx: Fix uninitialized pointer dereference in error path.
  selftests/sgx: Fix function pointer relocation in test enclave.
  selftests/sgx: Ensure correct secinfo struct alignment in test
    enclave.
  selftests/sgx: Ensure expected enclave data buffer size and placement.
  selftests/sgx: Enclave freestanding compilation + separate linker
    options.

 tools/testing/selftests/sgx/Makefile            | 12 +++++++-----
 tools/testing/selftests/sgx/sigstruct.c         |  2 +-
 tools/testing/selftests/sgx/test_encl.c         | 17 +++++++++++------
 tools/testing/selftests/sgx/test_encl.lds       |  2 ++
 .../testing/selftests/sgx/test_encl_bootstrap.S |  5 +++++
 5 files changed, 26 insertions(+), 12 deletions(-)


base-commit: ad0dbc1609ab4b01c9c423d3dd6bb6e4f19dee70
-- 
2.34.1

[PATCH 1/5] selftests/sgx: Fix uninitialized pointer dereference in error path.

[Jo Van Bulck]

Ensure ctx is zero-initialized, such that the encl_measure function will
not call EVP_MD_CTX_destroy with an uninitialized ctx pointer in case of an
early error during key generation.

Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be>
---
 tools/testing/selftests/sgx/sigstruct.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/sgx/sigstruct.c b/tools/testing/selftests/sgx/sigstruct.c
index a07896a46364..dd1fdab90e26 100644
--- a/tools/testing/selftests/sgx/sigstruct.c
+++ b/tools/testing/selftests/sgx/sigstruct.c
@@ -318,9 +318,9 @@ bool encl_measure(struct encl *encl)
 	struct sgx_sigstruct *sigstruct = &encl->sigstruct;
 	struct sgx_sigstruct_payload payload;
 	uint8_t digest[SHA256_DIGEST_LENGTH];
+	EVP_MD_CTX *ctx = NULL;
 	unsigned int siglen;
 	RSA *key = NULL;
-	EVP_MD_CTX *ctx;
 	int i;
 
 	memset(sigstruct, 0, sizeof(*sigstruct));
-- 
2.34.1

[PATCH 2/5] selftests/sgx: Fix function pointer relocation in test enclave.

[Jo Van Bulck]

Relocate encl_op_array entries at runtime relative to the enclave base to
ensure correct function pointer when compiling the test enclave with -Os.

Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be>
---
 tools/testing/selftests/sgx/test_encl.c           | 6 ++++--
 tools/testing/selftests/sgx/test_encl.lds         | 1 +
 tools/testing/selftests/sgx/test_encl_bootstrap.S | 5 +++++
 3 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/sgx/test_encl.c b/tools/testing/selftests/sgx/test_encl.c
index c0d6397295e3..4e31a6c3d673 100644
--- a/tools/testing/selftests/sgx/test_encl.c
+++ b/tools/testing/selftests/sgx/test_encl.c
@@ -119,9 +119,11 @@ static void do_encl_op_nop(void *_op)
 
 }
 
+uint64_t get_enclave_base(void);
+
 void encl_body(void *rdi,  void *rsi)
 {
-	const void (*encl_op_array[ENCL_OP_MAX])(void *) = {
+	static 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,
@@ -135,5 +137,5 @@ void encl_body(void *rdi,  void *rsi)
 	struct encl_op_header *op = (struct encl_op_header *)rdi;
 
 	if (op->type < ENCL_OP_MAX)
-		(*encl_op_array[op->type])(op);
+		(*(get_enclave_base() + encl_op_array[op->type]))(op);
 }
diff --git a/tools/testing/selftests/sgx/test_encl.lds b/tools/testing/selftests/sgx/test_encl.lds
index a1ec64f7d91f..ca659db2a534 100644
--- a/tools/testing/selftests/sgx/test_encl.lds
+++ b/tools/testing/selftests/sgx/test_encl.lds
@@ -10,6 +10,7 @@ PHDRS
 SECTIONS
 {
 	. = 0;
+        __enclave_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 03ae0f57e29d..6126dbd7ad1c 100644
--- a/tools/testing/selftests/sgx/test_encl_bootstrap.S
+++ b/tools/testing/selftests/sgx/test_encl_bootstrap.S
@@ -86,6 +86,11 @@ encl_entry_core:
 	mov	$4, %rax
 	enclu
 
+	.global get_enclave_base
+get_enclave_base:
+	lea __enclave_base(%rip), %rax
+	ret
+
 	.section ".data", "aw"
 
 encl_ssa_tcs1:
-- 
2.34.1

[PATCH 3/5] selftests/sgx: Ensure correct secinfo struct alignment in test enclave.

[Jo Van Bulck]

Declare the secinfo struct as volatile to prevent compiler optimizations
from passing an unaligned pointer to ENCLU.

Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be>
---
 tools/testing/selftests/sgx/test_encl.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/sgx/test_encl.c b/tools/testing/selftests/sgx/test_encl.c
index 4e31a6c3d673..aba301abefb8 100644
--- a/tools/testing/selftests/sgx/test_encl.c
+++ b/tools/testing/selftests/sgx/test_encl.c
@@ -18,7 +18,8 @@ enum sgx_enclu_function {
 
 static void do_encl_emodpe(void *_op)
 {
-	struct sgx_secinfo secinfo __aligned(sizeof(struct sgx_secinfo)) = {0};
+	/* declare secinfo volatile to preserve alignment */
+	volatile struct sgx_secinfo secinfo __aligned(sizeof(struct sgx_secinfo)) = {0};
 	struct encl_op_emodpe *op = _op;
 
 	secinfo.flags = op->flags;
@@ -32,7 +33,8 @@ static void do_encl_emodpe(void *_op)
 
 static void do_encl_eaccept(void *_op)
 {
-	struct sgx_secinfo secinfo __aligned(sizeof(struct sgx_secinfo)) = {0};
+	/* declare secinfo volatile to preserve alignment */
+	volatile struct sgx_secinfo secinfo __aligned(sizeof(struct sgx_secinfo)) = {0};
 	struct encl_op_eaccept *op = _op;
 	int rax;
 
-- 
2.34.1

[PATCH 4/5] selftests/sgx: Ensure expected enclave data buffer size and placement.

[Jo Van Bulck]

Do not declare the enclave data buffer static to ensure it is not optimized
away by the compiler, even when not used entirely by the test enclave code.
Use -fPIE to make the compiler access the non-static buffer with
RIP-relative addressing. Place the enclave data buffer in a separate
section that is explicitly placed at the start of the .data segment in the
linker script, as expected by the external tests manipulating page
permissions.

Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be>
---
 tools/testing/selftests/sgx/Makefile      | 2 +-
 tools/testing/selftests/sgx/test_encl.c   | 5 +++--
 tools/testing/selftests/sgx/test_encl.lds | 1 +
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/sgx/Makefile b/tools/testing/selftests/sgx/Makefile
index 50aab6b57da3..c5483445ba28 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 -nostdlib -nostartfiles -fPIE \
 	       -fno-stack-protector -mrdrnd $(INCLUDES)
 
 TEST_CUSTOM_PROGS := $(OUTPUT)/test_sgx
diff --git a/tools/testing/selftests/sgx/test_encl.c b/tools/testing/selftests/sgx/test_encl.c
index aba301abefb8..5c274e517d13 100644
--- a/tools/testing/selftests/sgx/test_encl.c
+++ b/tools/testing/selftests/sgx/test_encl.c
@@ -7,9 +7,10 @@
 /*
  * Data buffer spanning two pages that will be placed first in .data
  * segment. Even if not used internally the second page is needed by
- * external test manipulating page permissions.
+ * external test manipulating page permissions. Do not declare this
+ * buffer as static, so the compiler cannot optimize it out.
  */
-static uint8_t encl_buffer[8192] = { 1 };
+uint8_t __attribute__((section(".data.encl_buffer"))) encl_buffer[8192];
 
 enum sgx_enclu_function {
 	EACCEPT = 0x5,
diff --git a/tools/testing/selftests/sgx/test_encl.lds b/tools/testing/selftests/sgx/test_encl.lds
index ca659db2a534..79b1e41d8d24 100644
--- a/tools/testing/selftests/sgx/test_encl.lds
+++ b/tools/testing/selftests/sgx/test_encl.lds
@@ -24,6 +24,7 @@ SECTIONS
 	} : text
 
 	.data : {
+		*(.data.encl_buffer)
 		*(.data*)
 	} : data
 
-- 
2.34.1

[PATCH 5/5] selftests/sgx: Enclave freestanding compilation + separate linker options.

[Jo Van Bulck]

Fixes "'linker' input unused [-Wunused-command-line-argument]" errors when
compiling with clang.

Additionally pass -ffreestanding to prohibit memset/memcpy stdlib calls for
optimized enclave code.

Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be>
---
 tools/testing/selftests/sgx/Makefile | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/sgx/Makefile b/tools/testing/selftests/sgx/Makefile
index c5483445ba28..aff419615462 100644
--- a/tools/testing/selftests/sgx/Makefile
+++ b/tools/testing/selftests/sgx/Makefile
@@ -12,9 +12,11 @@ OBJCOPY := $(CROSS_COMPILE)objcopy
 endif
 
 INCLUDES := -I$(top_srcdir)/tools/include
-HOST_CFLAGS := -Wall -Werror -g $(INCLUDES) -fPIC -z noexecstack
-ENCL_CFLAGS := -Wall -Werror -static -nostdlib -nostartfiles -fPIE \
-	       -fno-stack-protector -mrdrnd $(INCLUDES)
+HOST_CFLAGS := -Wall -Werror -g $(INCLUDES) -fPIC
+HOST_LDFLAGS := -z noexecstack -lcrypto
+ENCL_CFLAGS := -Wall -Werror -static -nostdlib -ffreestanding \
+	       -nostartfiles -fPIE -fno-stack-protector -mrdrnd $(INCLUDES)
+ENCL_LDFLAGS := -z noexecstack -Wl,--build-id=none
 
 TEST_CUSTOM_PROGS := $(OUTPUT)/test_sgx
 TEST_FILES := $(OUTPUT)/test_encl.elf
@@ -28,7 +30,7 @@ $(OUTPUT)/test_sgx: $(OUTPUT)/main.o \
 		    $(OUTPUT)/sigstruct.o \
 		    $(OUTPUT)/call.o \
 		    $(OUTPUT)/sign_key.o
-	$(CC) $(HOST_CFLAGS) -o $@ $^ -lcrypto
+	$(CC) $(HOST_CFLAGS) -o $@ $^ $(HOST_LDFLAGS)
 
 $(OUTPUT)/main.o: main.c
 	$(CC) $(HOST_CFLAGS) -c $< -o $@
@@ -46,7 +48,7 @@ $(OUTPUT)/sign_key.o: sign_key.S
 	$(CC) $(HOST_CFLAGS) -c $< -o $@
 
 $(OUTPUT)/test_encl.elf: test_encl.lds test_encl.c test_encl_bootstrap.S
-	$(CC) $(ENCL_CFLAGS) -T $^ -o $@ -Wl,--build-id=none
+	$(CC) $(ENCL_CFLAGS) -T $^ -o $@ $(ENCL_LDFLAGS)
 
 EXTRA_CLEAN := \
 	$(OUTPUT)/test_encl.elf \
-- 
2.34.1

Re: [PATCH 0/5] selftests/sgx: Fix compilation errors.

[Jarkko Sakkinen]

On Mon Jul 24, 2023 at 4:58 PM UTC, Jo Van Bulck wrote:
> Hi,
>
> This patch series ensures that all SGX selftests succeed when compiling
> with optimizations (as tested with -O{0,1,2,3,s} for both gcc 11.3.0 and
> clang 14.0.0). The aim of the patches is to avoid reliance on undefined,
> compiler-specific behavior that can make the test results fragile.
>
> If deemed useful, I can also include an elementary wrapper shell script to
> compile and run the tests for different compilers (gcc/clang) and
> optimization levels.

Thank you, this sounds like an appropriate scope for the selftest.

I support also the idea of refining the selftest as a run-time, which
could perhaps consist of the following steps:

1. Create a repository of the self-compiling selftest with GPLv2. You
   could add also AUTHORS file for the initial content by crawling this
   data from the git log.
2. Create a commit with sob's from the required stakeholders, which
   changes the license to something more appropriate, and get the
   sob's with some process.

BR, Jarkko

Re: [PATCH 1/5] selftests/sgx: Fix uninitialized pointer dereference in error path.

[Jarkko Sakkinen]

On Mon Jul 24, 2023 at 4:58 PM UTC, Jo Van Bulck wrote:
> Ensure ctx is zero-initialized, such that the encl_measure function will
> not call EVP_MD_CTX_destroy with an uninitialized ctx pointer in case of an
> early error during key generation.
>
> Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be>
> ---
>  tools/testing/selftests/sgx/sigstruct.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/sgx/sigstruct.c b/tools/testing/selftests/sgx/sigstruct.c
> index a07896a46364..dd1fdab90e26 100644
> --- a/tools/testing/selftests/sgx/sigstruct.c
> +++ b/tools/testing/selftests/sgx/sigstruct.c
> @@ -318,9 +318,9 @@ bool encl_measure(struct encl *encl)
>  	struct sgx_sigstruct *sigstruct = &encl->sigstruct;
>  	struct sgx_sigstruct_payload payload;
>  	uint8_t digest[SHA256_DIGEST_LENGTH];
> +	EVP_MD_CTX *ctx = NULL;
>  	unsigned int siglen;
>  	RSA *key = NULL;
> -	EVP_MD_CTX *ctx;
>  	int i;
>  
>  	memset(sigstruct, 0, sizeof(*sigstruct));
> -- 
> 2.34.1

Add a fixes tag. In other words, find the commit ID that caused the issue
and add the output of the following to this patch before your sob:

git --no-pager log --format='Fixes: %h ("%s")' --abbrev=12 -1 <commit ID>;

BR, Jarkko

Re: [PATCH 1/5] selftests/sgx: Fix uninitialized pointer dereference in error path.

[Jarkko Sakkinen]

On Mon Jul 24, 2023 at 4:58 PM UTC, Jo Van Bulck wrote:
> Ensure ctx is zero-initialized, such that the encl_measure function will
> not call EVP_MD_CTX_destroy with an uninitialized ctx pointer in case of an
> early error during key generation.
>
> Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be>
> ---
>  tools/testing/selftests/sgx/sigstruct.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/sgx/sigstruct.c b/tools/testing/selftests/sgx/sigstruct.c
> index a07896a46364..dd1fdab90e26 100644
> --- a/tools/testing/selftests/sgx/sigstruct.c
> +++ b/tools/testing/selftests/sgx/sigstruct.c
> @@ -318,9 +318,9 @@ bool encl_measure(struct encl *encl)
>  	struct sgx_sigstruct *sigstruct = &encl->sigstruct;
>  	struct sgx_sigstruct_payload payload;
>  	uint8_t digest[SHA256_DIGEST_LENGTH];
> +	EVP_MD_CTX *ctx = NULL;
>  	unsigned int siglen;
>  	RSA *key = NULL;
> -	EVP_MD_CTX *ctx;
>  	int i;
>  
>  	memset(sigstruct, 0, sizeof(*sigstruct));
> -- 
> 2.34.1

Ditto.

BR, Jarkko

Re: [PATCH 2/5] selftests/sgx: Fix function pointer relocation in test enclave.

[Jarkko Sakkinen]

On Mon Jul 24, 2023 at 4:58 PM UTC, Jo Van Bulck wrote:
> Relocate encl_op_array entries at runtime relative to the enclave base to
> ensure correct function pointer when compiling the test enclave with -Os.
>
> Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be>
> ---
>  tools/testing/selftests/sgx/test_encl.c           | 6 ++++--
>  tools/testing/selftests/sgx/test_encl.lds         | 1 +
>  tools/testing/selftests/sgx/test_encl_bootstrap.S | 5 +++++
>  3 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/sgx/test_encl.c b/tools/testing/selftests/sgx/test_encl.c
> index c0d6397295e3..4e31a6c3d673 100644
> --- a/tools/testing/selftests/sgx/test_encl.c
> +++ b/tools/testing/selftests/sgx/test_encl.c
> @@ -119,9 +119,11 @@ static void do_encl_op_nop(void *_op)
>  
>  }
>  
> +uint64_t get_enclave_base(void);
> +
>  void encl_body(void *rdi,  void *rsi)
>  {
> -	const void (*encl_op_array[ENCL_OP_MAX])(void *) = {
> +	static 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,
> @@ -135,5 +137,5 @@ void encl_body(void *rdi,  void *rsi)
>  	struct encl_op_header *op = (struct encl_op_header *)rdi;
>  
>  	if (op->type < ENCL_OP_MAX)
> -		(*encl_op_array[op->type])(op);
> +		(*(get_enclave_base() + encl_op_array[op->type]))(op);
>  }
> diff --git a/tools/testing/selftests/sgx/test_encl.lds b/tools/testing/selftests/sgx/test_encl.lds
> index a1ec64f7d91f..ca659db2a534 100644
> --- a/tools/testing/selftests/sgx/test_encl.lds
> +++ b/tools/testing/selftests/sgx/test_encl.lds
> @@ -10,6 +10,7 @@ PHDRS
>  SECTIONS
>  {
>  	. = 0;
> +        __enclave_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 03ae0f57e29d..6126dbd7ad1c 100644
> --- a/tools/testing/selftests/sgx/test_encl_bootstrap.S
> +++ b/tools/testing/selftests/sgx/test_encl_bootstrap.S
> @@ -86,6 +86,11 @@ encl_entry_core:
>  	mov	$4, %rax
>  	enclu
>  
> +	.global get_enclave_base
> +get_enclave_base:
> +	lea __enclave_base(%rip), %rax
> +	ret
> +
>  	.section ".data", "aw"
>  
>  encl_ssa_tcs1:
> -- 
> 2.34.1

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

BR, Jarkko

Re: [PATCH 3/5] selftests/sgx: Ensure correct secinfo struct alignment in test enclave.

[Jarkko Sakkinen]

On Mon Jul 24, 2023 at 4:58 PM UTC, Jo Van Bulck wrote:
> Declare the secinfo struct as volatile to prevent compiler optimizations
> from passing an unaligned pointer to ENCLU.
>
> Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be>
> ---
>  tools/testing/selftests/sgx/test_encl.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/sgx/test_encl.c b/tools/testing/selftests/sgx/test_encl.c
> index 4e31a6c3d673..aba301abefb8 100644
> --- a/tools/testing/selftests/sgx/test_encl.c
> +++ b/tools/testing/selftests/sgx/test_encl.c
> @@ -18,7 +18,8 @@ enum sgx_enclu_function {
>  
>  static void do_encl_emodpe(void *_op)
>  {
> -	struct sgx_secinfo secinfo __aligned(sizeof(struct sgx_secinfo)) = {0};
> +	/* declare secinfo volatile to preserve alignment */
> +	volatile struct sgx_secinfo secinfo __aligned(sizeof(struct sgx_secinfo)) = {0};
>  	struct encl_op_emodpe *op = _op;
>  
>  	secinfo.flags = op->flags;
> @@ -32,7 +33,8 @@ static void do_encl_emodpe(void *_op)
>  
>  static void do_encl_eaccept(void *_op)
>  {
> -	struct sgx_secinfo secinfo __aligned(sizeof(struct sgx_secinfo)) = {0};
> +	/* declare secinfo volatile to preserve alignment */
> +	volatile struct sgx_secinfo secinfo __aligned(sizeof(struct sgx_secinfo)) = {0};
>  	struct encl_op_eaccept *op = _op;
>  	int rax;
>  
> -- 
> 2.34.1

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

BR, Jarkko

Re: [PATCH 4/5] selftests/sgx: Ensure expected enclave data buffer size and placement.

[Jarkko Sakkinen]

On Mon Jul 24, 2023 at 4:58 PM UTC, Jo Van Bulck wrote:
> Do not declare the enclave data buffer static to ensure it is not optimized
                                         ~~~~~~
					 as static

/enclave data buffer/encl_buffer/

> away by the compiler, even when not used entirely by the test enclave code.

"Declare encl_buffer as global, in order to ensure that it is not
optimized away by the compiler."

So, when exactly is it optimized away by the compiler? This is missing.

BR, Jarkko

Re: [PATCH 5/5] selftests/sgx: Enclave freestanding compilation + separate linker options.

[Jarkko Sakkinen]

On Mon Jul 24, 2023 at 4:58 PM UTC, Jo Van Bulck wrote:
> Fixes "'linker' input unused [-Wunused-command-line-argument]" errors when
> compiling with clang.
>
> Additionally pass -ffreestanding to prohibit memset/memcpy stdlib calls for
> optimized enclave code.

Should be split into two patches. Please describe the motivation for the
second paragraph in the patch, which adds '-ffreestanding'.


BR, Jarkko

Re: [PATCH 1/5] selftests/sgx: Fix uninitialized pointer dereference in error path.

[Huang, Kai]

On Mon, 2023-07-24 at 18:58 +0200, Jo Van Bulck wrote:
> Ensure ctx is zero-initialized, such that the encl_measure function will
> not call EVP_MD_CTX_destroy with an uninitialized ctx pointer in case of an
> early error during key generation.
> 
> Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be>
> ---
>  tools/testing/selftests/sgx/sigstruct.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/sgx/sigstruct.c b/tools/testing/selftests/sgx/sigstruct.c
> index a07896a46364..dd1fdab90e26 100644
> --- a/tools/testing/selftests/sgx/sigstruct.c
> +++ b/tools/testing/selftests/sgx/sigstruct.c
> @@ -318,9 +318,9 @@ bool encl_measure(struct encl *encl)
>  	struct sgx_sigstruct *sigstruct = &encl->sigstruct;
>  	struct sgx_sigstruct_payload payload;
>  	uint8_t digest[SHA256_DIGEST_LENGTH];
> +	EVP_MD_CTX *ctx = NULL;
>  	unsigned int siglen;
>  	RSA *key = NULL;
> -	EVP_MD_CTX *ctx;
>  	int i;
>  
>  	memset(sigstruct, 0, sizeof(*sigstruct));

Is it safe to assume EVP_MD_CTX_destroy() can always handle a NULL ctx?

The manpage says:

EVP_MD_CTX_destroy() cleans up digest context ctx and frees up the space
allocated to it, it should be called only on a context created using
EVP_MD_CTX_create().

Re: [PATCH 2/5] selftests/sgx: Fix function pointer relocation in test enclave.

[Huang, Kai]

On Mon, 2023-07-24 at 18:58 +0200, Jo Van Bulck wrote:
> Relocate encl_op_array entries at runtime relative to the enclave base to
> ensure correct function pointer when compiling the test enclave with -Os.

Putting aside whether we should consider building the selftests using "-Os", it
would be helpful to explain how can the "-Os" break the existing code, so that
we can review why this fix is reasonable.  Perhaps it's obvious to others but
it's not obvious to me what can go wrong here. 

> 
> Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be>
> ---
>  tools/testing/selftests/sgx/test_encl.c           | 6 ++++--
>  tools/testing/selftests/sgx/test_encl.lds         | 1 +
>  tools/testing/selftests/sgx/test_encl_bootstrap.S | 5 +++++
>  3 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/sgx/test_encl.c b/tools/testing/selftests/sgx/test_encl.c
> index c0d6397295e3..4e31a6c3d673 100644
> --- a/tools/testing/selftests/sgx/test_encl.c
> +++ b/tools/testing/selftests/sgx/test_encl.c
> @@ -119,9 +119,11 @@ static void do_encl_op_nop(void *_op)
>  
>  }
>  
> +uint64_t get_enclave_base(void);
> +
>  void encl_body(void *rdi,  void *rsi)
>  {
> -	const void (*encl_op_array[ENCL_OP_MAX])(void *) = {
> +	static 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,
> @@ -135,5 +137,5 @@ void encl_body(void *rdi,  void *rsi)
>  	struct encl_op_header *op = (struct encl_op_header *)rdi;
>  
>  	if (op->type < ENCL_OP_MAX)
> -		(*encl_op_array[op->type])(op);
> +		(*(get_enclave_base() + encl_op_array[op->type]))(op);
>  }
> diff --git a/tools/testing/selftests/sgx/test_encl.lds b/tools/testing/selftests/sgx/test_encl.lds
> index a1ec64f7d91f..ca659db2a534 100644
> --- a/tools/testing/selftests/sgx/test_encl.lds
> +++ b/tools/testing/selftests/sgx/test_encl.lds
> @@ -10,6 +10,7 @@ PHDRS
>  SECTIONS
>  {
>  	. = 0;
> +        __enclave_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 03ae0f57e29d..6126dbd7ad1c 100644
> --- a/tools/testing/selftests/sgx/test_encl_bootstrap.S
> +++ b/tools/testing/selftests/sgx/test_encl_bootstrap.S
> @@ -86,6 +86,11 @@ encl_entry_core:
>  	mov	$4, %rax
>  	enclu
>  
> +	.global get_enclave_base
> +get_enclave_base:
> +	lea __enclave_base(%rip), %rax
> +	ret
> +
>  	.section ".data", "aw"
>  
>  encl_ssa_tcs1:

Re: [PATCH 3/5] selftests/sgx: Ensure correct secinfo struct alignment in test enclave.

[Huang, Kai]

On Mon, 2023-07-24 at 18:58 +0200, Jo Van Bulck wrote:
> Declare the secinfo struct as volatile to prevent compiler optimizations
> from passing an unaligned pointer to ENCLU.

We already have __aligned.  Can you provide more information in what
circumstances that __aligned isn't enough to guarantee the alignment?

We have a lot of kernel code which has __aligned but doesn't have volatile.

> 
> Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be>
> ---
>  tools/testing/selftests/sgx/test_encl.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/sgx/test_encl.c b/tools/testing/selftests/sgx/test_encl.c
> index 4e31a6c3d673..aba301abefb8 100644
> --- a/tools/testing/selftests/sgx/test_encl.c
> +++ b/tools/testing/selftests/sgx/test_encl.c
> @@ -18,7 +18,8 @@ enum sgx_enclu_function {
>  
>  static void do_encl_emodpe(void *_op)
>  {
> -	struct sgx_secinfo secinfo __aligned(sizeof(struct sgx_secinfo)) = {0};
> +	/* declare secinfo volatile to preserve alignment */
> +	volatile struct sgx_secinfo secinfo __aligned(sizeof(struct sgx_secinfo)) = {0};
>  	struct encl_op_emodpe *op = _op;
>  
>  	secinfo.flags = op->flags;
> @@ -32,7 +33,8 @@ static void do_encl_emodpe(void *_op)
>  
>  static void do_encl_eaccept(void *_op)
>  {
> -	struct sgx_secinfo secinfo __aligned(sizeof(struct sgx_secinfo)) = {0};
> +	/* declare secinfo volatile to preserve alignment */
> +	volatile struct sgx_secinfo secinfo __aligned(sizeof(struct sgx_secinfo)) = {0};
>  	struct encl_op_eaccept *op = _op;
>  	int rax;
>  

Re: [PATCH 4/5] selftests/sgx: Ensure expected enclave data buffer size and placement.

[Huang, Kai]

On Mon, 2023-07-24 at 18:58 +0200, Jo Van Bulck wrote:
> Do not declare the enclave data buffer static to ensure it is not optimized
> away by the compiler, even when not used entirely by the test enclave code.

The "encl_buffer" array is initialized to 1 explicitly, which means it should be
in .data section.  As Jarkko commented, can you add more information about in
what cases it can be optimized away?

> Use -fPIE to make the compiler access the non-static buffer with
> RIP-relative addressing. 
> 

I am not quite following how does "RIP-relative addressing" fix any problem?  Is
there any hard relationship between "RIP-relative addressing" and the problem
that you are having?

> Place the enclave data buffer in a separate
> section that is explicitly placed at the start of the .data segment in the
> linker script, as expected by the external tests manipulating page
> permissions.

So this change is not to fix the problem that "the compiler may optimize away
the encl_buffer array", correct?

> 
> Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be>
> ---
>  tools/testing/selftests/sgx/Makefile      | 2 +-
>  tools/testing/selftests/sgx/test_encl.c   | 5 +++--
>  tools/testing/selftests/sgx/test_encl.lds | 1 +
>  3 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/sgx/Makefile b/tools/testing/selftests/sgx/Makefile
> index 50aab6b57da3..c5483445ba28 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 -nostdlib -nostartfiles -fPIE \
>  	       -fno-stack-protector -mrdrnd $(INCLUDES)
>  
>  TEST_CUSTOM_PROGS := $(OUTPUT)/test_sgx
> diff --git a/tools/testing/selftests/sgx/test_encl.c b/tools/testing/selftests/sgx/test_encl.c
> index aba301abefb8..5c274e517d13 100644
> --- a/tools/testing/selftests/sgx/test_encl.c
> +++ b/tools/testing/selftests/sgx/test_encl.c
> @@ -7,9 +7,10 @@
>  /*
>   * Data buffer spanning two pages that will be placed first in .data
>   * segment. Even if not used internally the second page is needed by
> - * external test manipulating page permissions.
> + * external test manipulating page permissions. Do not declare this
> + * buffer as static, so the compiler cannot optimize it out.
>   */
> -static uint8_t encl_buffer[8192] = { 1 };
> +uint8_t __attribute__((section(".data.encl_buffer"))) encl_buffer[8192];
>  
>  enum sgx_enclu_function {
>  	EACCEPT = 0x5,
> diff --git a/tools/testing/selftests/sgx/test_encl.lds b/tools/testing/selftests/sgx/test_encl.lds
> index ca659db2a534..79b1e41d8d24 100644
> --- a/tools/testing/selftests/sgx/test_encl.lds
> +++ b/tools/testing/selftests/sgx/test_encl.lds
> @@ -24,6 +24,7 @@ SECTIONS
>  	} : text
>  
>  	.data : {
> +		*(.data.encl_buffer)
>  		*(.data*)
>  	} : data
>  

Re: [PATCH 1/5] selftests/sgx: Fix uninitialized pointer dereference in error path.

[Jo Van Bulck]

On 28.07.23 21:03, Jarkko Sakkinen wrote:
> Add a fixes tag. In other words, find the commit ID that caused the issue
> and add the output of the following to this patch before your sob:
> 
> git --no-pager log --format='Fixes: %h ("%s")' --abbrev=12 -1 <commit ID>;
> 
> BR, Jarkko

Thank you, added for the next patch revision.

Re: [PATCH 1/5] selftests/sgx: Fix uninitialized pointer dereference in error path.

[Jo Van Bulck]

On 03.08.23 05:51, Huang, Kai wrote:
> Is it safe to assume EVP_MD_CTX_destroy() can always handle a NULL ctx?
> 
> The manpage says:
> 
> EVP_MD_CTX_destroy() cleans up digest context ctx and frees up the space
> allocated to it, it should be called only on a context created using
> EVP_MD_CTX_create().

Thank you for pointing this out. Afais the implementations I've seen can 
handle NULL, and similar error-handling paths exists where 
EVP_MD_CTX_destroy() is called with a NULL pointer exist in several 
places in the openSSL code.

That being said, this indeed not explicit in the specification (unlike 
RSA_free() which is called just after and explicitly specifies that NULL 
is okay). So you're probably right that it's generally safer to not call 
EVP_MD_CTX_destroy() with a NULL pointer.

I'll include an extra check for this in the next patch revision.

Re: [PATCH 2/5] selftests/sgx: Fix function pointer relocation in test enclave.

[Jo Van Bulck]

On 03.08.23 05:58, Huang, Kai wrote:
> Putting aside whether we should consider building the selftests using "-Os", it
> would be helpful to explain how can the "-Os" break the existing code, so that
> we can review why this fix is reasonable.  Perhaps it's obvious to others but
> it's not obvious to me what can go wrong here.

I dug deeper into this and the real problem here is that the enclave ELF 
is linked with -static but also needs to be relocatable, which, in my 
understanding, is not what -static is meant for (i.e., the man pages 
says -static overrides -pie). Particularly, with -static I noticed that 
global variables are hard-coded assuming the ELF is loaded at base 
address zero.

When reading more on this, it seems like the proper thing to do for 
static and relocatable binaries is to compile with -static-pie, which is 
added since gcc 8 [1] (and similarly supported by clang).

As a case in point, to hopefully make this clearer, consider the 
following C function:

extern uint8_t __enclave_base;
void *get_base(void) {
	return &__enclave_base;
}

Compiling with -static and -fPIC hard codes the __enclave_base symbol to 
zero (the start of the ELF enclave image):

00000000000023f4 <get_base>:
     23f4:	f3 0f 1e fa          	endbr64
     23f8:	55                   	push   %rbp
     23f9:	48 89 e5             	mov    %rsp,%rbp
     23fc:	48 c7 c0 00 00 00 00 	mov    $0x0,%rax
     2403:	5d                   	pop    %rbp
     2404:	c3                   	ret

Compiling with -static-pie and -fPIE properly emits a RIP-relative address:

00000000000023f4 <get_base>:
     23f4:	f3 0f 1e fa          	endbr64
     23f8:	55                   	push   %rbp
     23f9:	48 89 e5             	mov    %rsp,%rbp
     23fc:	48 8d 05 fd db ff ff 	lea    -0x2403(%rip),%rax        # 0 
<__enclave_base>
     2403:	5d                   	pop    %rbp
     2404:	c3                   	ret

Now, the fact that it currently *happens* to work is a mere coincidence 
of how the local encl_op_array initialization is compiled without 
optimizations with -static -fPIC:

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 */

When compiling with optimizations with -static -fPIC -Os, encl_op_array 
is instead initialized with a prepared copy from .data:

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 */

Thus, in this optimized code, encl_op_array will have function pointers 
that are *not* relocated. The compilation assumes the -static binary has 
base address zero and is not relocatable:

$ readelf -r test_encl.elf

There are no relocations in this file.

When compiling with -static-pie -PIE -Os, the same code is emitted *but* 
the binary is relocatable:

$ readelf -r test_encl.elf

Relocation section '.rela.dyn' at offset 0x4000 contains 12 entries:
   Offset          Info           Type           Sym. Value    Sym. Name 
+ Addend
# tcs1.ossa
000000000010  000000000008 R_X86_64_RELATIVE                    7000
# tcs1.oentry
000000000020  000000000008 R_X86_64_RELATIVE                    21e3
# tcs2.ossa
000000001010  000000000008 R_X86_64_RELATIVE                    8000
# tcs2.oentry
000000001020  000000000008 R_X86_64_RELATIVE                    21e3
# encl_op_array
000000006000  000000000008 R_X86_64_RELATIVE                    2098
000000006008  000000000008 R_X86_64_RELATIVE                    20ae
000000006010  000000000008 R_X86_64_RELATIVE                    20c4
000000006018  000000000008 R_X86_64_RELATIVE                    20d7
000000006020  000000000008 R_X86_64_RELATIVE                    20ea
000000006028  000000000008 R_X86_64_RELATIVE                    203e
000000006030  000000000008 R_X86_64_RELATIVE                    2000
000000006038  000000000008 R_X86_64_RELATIVE                    20ef

Apparently, for static-pie binaries, glibc includes a 
_dl_relocate_static_pie routine that will perform the relocations as 
part of the startup [2,3]. Since the enclave loading process is 
different and glibc is not included, we cannot rely on these relocations 
to be performed and we need to do them manually. Note: the first 4 
symbols in the relocation table above are from the TCS initialization in 
test_encl_bootstrap.S and should *not* be relocated (as these are 
relative to enclave base as per SGX spec).

Bottom line: the way I see it, the enclave should either ensure no 
relocations are needed, or perform the relocations manually where 
needed, or include a _dl_relocate_static_pie equivalent that parses the 
.rela.dyn ELF section and patches all relocations (minus the TCS 
symbols). Since the latter (most general) approach is likely going to 
make the selftest enclave unnecessarily complex by including ELF parsing 
etc, I propose to simply relocate the function-pointer table manually 
(which is indeed the only place that needs relocations).

I will include code to properly compile the selftest enclave with 
-static-pie as per above and relocate the function-pointer table in the 
next patch revision.

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81498
[2] https://stackoverflow.com/a/62587156
[3] 
https://sourceware.org/git/?p=glibc.git;a=blob;f=elf/dl-reloc-static-pie.c;h=382482fa739f10934aeb916650c65a4db231c481;hb=HEAD

Re: [PATCH 3/5] selftests/sgx: Ensure correct secinfo struct alignment in test enclave.

[Jo Van Bulck]

On 03.08.23 06:00, Huang, Kai wrote:> We already have __aligned.  Can 
you provide more information in what
> circumstances that __aligned isn't enough to guarantee the alignment?
> 
> We have a lot of kernel code which has __aligned but doesn't have volatile.

Thank you. I also dug deeper into this and the proper fix is indeed not 
to make the variable volatile.

The real problem is that the inline assembly does not have the "memory" 
clobber to tell the compiler that the "assembly code performs memory 
reads or writes to items other than those listed in the input and output 
operands (for example, accessing the memory pointed to by one of the 
input parameters)" [1].

I checked that, depending on the optimizations and compiler (gcc vs 
clang), the compiler may indeed reorder the write to secinfo.flags to 
_after_ the inline asm block. Declaring secinfo as volatile fixed that, 
but the proper fix is of course to properly include a "memory" clobber 
for the asm block.

I'll include a fix to include a "memory" clobber for the inline asm 
block in the next patch revision.

[1] 
https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Clobbers-and-Scratch-Registers-1

Re: [PATCH 4/5] selftests/sgx: Ensure expected enclave data buffer size and placement.

[Jo Van Bulck]

On 28.07.23 21:19, Jarkko Sakkinen wrote:
> So, when exactly is it optimized away by the compiler? This is missing.

The problem is that declaring encl_buf as static, implies that it will 
only be used in this file and the compiler is allowed to optimize away 
any entries that are never used within this compilation unit (e.g., when 
optimizing out the memcpy calls).

In reality, the tests outside test_encl.elf rely on both the size and 
exact placement of encl_buf at the start of .data.

For example, clang -Os generates the following (legal) code when 
encl_bug is declared as static:

0000000000001020 <do_encl_op_put_to_buf>:
     mov    0x8(%rdi),%al
     mov    %al,0x1fd7(%rip)   # 3000 <encl_buffer.0>
     mov    0x9(%rdi),%al
     mov    %al,0x8fce(%rip)   # a000 <encl_buffer.1.0>
     mov    0xa(%rdi),%al
     mov    %al,0x8fd5(%rip)   # a010 <encl_buffer.1.1>
     mov    0xb(%rdi),%al
     mov    %al,0x8fce(%rip)   # a012 <encl_buffer.1.2>
     mov    0xc(%rdi),%al
     mov    %al,0x8fd3(%rip)   # a020 <encl_buffer.1.3>
     mov    0xd(%rdi),%al
     mov    %al,0x8fce(%rip)   # a024 <encl_buffer.1.4>
     mov    0xe(%rdi),%al
     mov    %al,0x8fd1(%rip)   # a030 <encl_buffer.1.5>
     mov    0xf(%rdi),%al
     mov    %al,0x8fca(%rip)   # a032 <encl_buffer.1.6>
     ret

Disassembly of section .data:

0000000000003000 <encl_buffer.0>:
     3000:       01 00
         ...
0000000000004000 <encl_ssa_tcs1>:

Thus, this proposed patch fixes both the size and location:

1. removing the static keyword from the encl_bug declaration ensures 
that the _entire_ buffer is preserved with expected size, as the 
compiler cannot anymore assume encl_buf is only used in this file.

2. adding _attribute__((section(".data.encl_buffer"))) ensures that we 
can control the expected location at the start of the .data section. I 
think this is optional, as encl_buf always seems to be placed at the 
start of .data in all my tests. But afaik this is not guaranteed as per 
the C standard and such constraints on exact placement should better be 
explicitly controlled in the linker script(?)

Re: [PATCH 4/5] selftests/sgx: Ensure expected enclave data buffer size and placement.

[Jo Van Bulck]

On 03.08.23 06:22, Huang, Kai wrote:
> The "encl_buffer" array is initialized to 1 explicitly, which means it should be
> in .data section.  As Jarkko commented, can you add more information about in
> what cases it can be optimized away?

Yes it will indeed be in .data, but it may be reduced in size. See my 
reply to Jarkko's question for more information and a concrete example.

> I am not quite following how does "RIP-relative addressing" fix any problem?  Is
> there any hard relationship between "RIP-relative addressing" and the problem
> that you are having?

Good point. I think this is now cleared up with the -static-pie 
discussion in reply to you other point on patch 2/5.

Concretely, the reason -fPIE is needed is that otherwise global 
variables (i.e., not declared as static) would not be addressed 
RIP-relative, but as hard-coded addresses assuming the -static binary is 
loaded at a fixed address.
> So this change is not to fix the problem that "the compiler may optimize away
> the encl_buffer array", correct?

Correct, this fix ensures the expected location. As per my reply to 
Jarkko's question:

> 2. adding _attribute__((section(".data.encl_buffer"))) ensures that we can control the expected location at the start of the .data section. I think this is optional, as encl_buf always seems to be placed at the start of .data in all my tests. But afaik this is not guaranteed as per the C standard and such constraints on exact placement should better be explicitly controlled in the linker script(?) 

Perhaps I better split this into 2 separate patches?

The location-control may also not be needed in practice, but afaik that 
would be undefined C behavior (cf above) and better be avoided?

Best,
Jo


> 
>>
>> Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be>
>> ---
>>   tools/testing/selftests/sgx/Makefile      | 2 +-
>>   tools/testing/selftests/sgx/test_encl.c   | 5 +++--
>>   tools/testing/selftests/sgx/test_encl.lds | 1 +
>>   3 files changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/testing/selftests/sgx/Makefile b/tools/testing/selftests/sgx/Makefile
>> index 50aab6b57da3..c5483445ba28 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 -nostdlib -nostartfiles -fPIE \
>>   	       -fno-stack-protector -mrdrnd $(INCLUDES)
>>   
>>   TEST_CUSTOM_PROGS := $(OUTPUT)/test_sgx
>> diff --git a/tools/testing/selftests/sgx/test_encl.c b/tools/testing/selftests/sgx/test_encl.c
>> index aba301abefb8..5c274e517d13 100644
>> --- a/tools/testing/selftests/sgx/test_encl.c
>> +++ b/tools/testing/selftests/sgx/test_encl.c
>> @@ -7,9 +7,10 @@
>>   /*
>>    * Data buffer spanning two pages that will be placed first in .data
>>    * segment. Even if not used internally the second page is needed by
>> - * external test manipulating page permissions.
>> + * external test manipulating page permissions. Do not declare this
>> + * buffer as static, so the compiler cannot optimize it out.
>>    */
>> -static uint8_t encl_buffer[8192] = { 1 };
>> +uint8_t __attribute__((section(".data.encl_buffer"))) encl_buffer[8192];
>>   
>>   enum sgx_enclu_function {
>>   	EACCEPT = 0x5,
>> diff --git a/tools/testing/selftests/sgx/test_encl.lds b/tools/testing/selftests/sgx/test_encl.lds
>> index ca659db2a534..79b1e41d8d24 100644
>> --- a/tools/testing/selftests/sgx/test_encl.lds
>> +++ b/tools/testing/selftests/sgx/test_encl.lds
>> @@ -24,6 +24,7 @@ SECTIONS
>>   	} : text
>>   
>>   	.data : {
>> +		*(.data.encl_buffer)
>>   		*(.data*)
>>   	} : data
>>   
> 

Re: [PATCH 5/5] selftests/sgx: Enclave freestanding compilation + separate linker options.

[Jo Van Bulck]

On 28.07.23 21:22, Jarkko Sakkinen wrote:
> Should be split into two patches. 

Thanks, will do in the next patch revision.

> Please describe the motivation for the
> second paragraph in the patch, which adds '-ffreestanding'.

Even when passing -nostdlib, the compiler still assumes memset and 
memcpy are present [1].

I found that, when not passing '-ffreestanding', clang seems to optimize 
away the existing memcpy/memset implementations and errors with:

/bin/ld: test_encl.o: in function `do_encl_init_tcs_page':
test_encl.c:(.text+0x17e): undefined reference to `memset'

I will add this information in the next patch revision.

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90701

Best,
Jo

Re: [PATCH 2/5] selftests/sgx: Fix function pointer relocation in test enclave.

[Huang, Kai]

On Mon, 2023-08-07 at 09:13 +0200, Jo Van Bulck wrote:
> On 03.08.23 05:58, Huang, Kai wrote:
> > Putting aside whether we should consider building the selftests using "-Os", it
> > would be helpful to explain how can the "-Os" break the existing code, so that
> > we can review why this fix is reasonable.  Perhaps it's obvious to others but
> > it's not obvious to me what can go wrong here.
> 
> I dug deeper into this and the real problem here is that the enclave ELF 
> is linked with -static but also needs to be relocatable, which, in my 
> understanding, is not what -static is meant for (i.e., the man pages 
> says -static overrides -pie). Particularly, with -static I noticed that 
> global variables are hard-coded assuming the ELF is loaded at base 
> address zero.
> 
> When reading more on this, it seems like the proper thing to do for 
> static and relocatable binaries is to compile with -static-pie, which is 
> added since gcc 8 [1] (and similarly supported by clang).

Thanks for analysing!

> 
> As a case in point, to hopefully make this clearer, consider the 
> following C function:
> 
> extern uint8_t __enclave_base;
> void *get_base(void) {
> 	return &__enclave_base;
> }
> 
> Compiling with -static and -fPIC hard codes the __enclave_base symbol to 
> zero (the start of the ELF enclave image):
> 
> 00000000000023f4 <get_base>:
>      23f4:	f3 0f 1e fa          	endbr64
>      23f8:	55                   	push   %rbp
>      23f9:	48 89 e5             	mov    %rsp,%rbp
>      23fc:	48 c7 c0 00 00 00 00 	mov    $0x0,%rax
>      2403:	5d                   	pop    %rbp
>      2404:	c3                   	ret
> 
> Compiling with -static-pie and -fPIE properly emits a RIP-relative address:
> 
> 00000000000023f4 <get_base>:
>      23f4:	f3 0f 1e fa          	endbr64
>      23f8:	55                   	push   %rbp
>      23f9:	48 89 e5             	mov    %rsp,%rbp
>      23fc:	48 8d 05 fd db ff ff 	lea    -0x2403(%rip),%rax        # 0 
> <__enclave_base>
>      2403:	5d                   	pop    %rbp
>      2404:	c3                   	ret
> 
> Now, the fact that it currently *happens* to work is a mere coincidence 
> of how the local encl_op_array initialization is compiled without 
> optimizations with -static -fPIC:
> 
> 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 */
> 
> When compiling with optimizations with -static -fPIC -Os, encl_op_array 
> is instead initialized with a prepared copy from .data:
> 
> 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 */

How is the "prepared copy" prepared, exactly?  Could you paste the relevant code
here too?  IMHO w/o it it's hard to tell whether the code could be wrong or not
after relocating.

> 
> Thus, in this optimized code, encl_op_array will have function pointers 
> that are *not* relocated. The compilation assumes the -static binary has 
> base address zero and is not relocatable:
> 
> $ readelf -r test_encl.elf
> 
> There are no relocations in this file.
> 
> When compiling with -static-pie -PIE -Os, the same code is emitted *but* 
> the binary is relocatable:
> 
> $ readelf -r test_encl.elf
> 
> Relocation section '.rela.dyn' at offset 0x4000 contains 12 entries:
>    Offset          Info           Type           Sym. Value    Sym. Name 
> + Addend
> # tcs1.ossa
> 000000000010  000000000008 R_X86_64_RELATIVE                    7000
> # tcs1.oentry
> 000000000020  000000000008 R_X86_64_RELATIVE                    21e3
> # tcs2.ossa
> 000000001010  000000000008 R_X86_64_RELATIVE                    8000
> # tcs2.oentry
> 000000001020  000000000008 R_X86_64_RELATIVE                    21e3
> # encl_op_array
> 000000006000  000000000008 R_X86_64_RELATIVE                    2098
> 000000006008  000000000008 R_X86_64_RELATIVE                    20ae
> 000000006010  000000000008 R_X86_64_RELATIVE                    20c4
> 000000006018  000000000008 R_X86_64_RELATIVE                    20d7
> 000000006020  000000000008 R_X86_64_RELATIVE                    20ea
> 000000006028  000000000008 R_X86_64_RELATIVE                    203e
> 000000006030  000000000008 R_X86_64_RELATIVE                    2000
> 000000006038  000000000008 R_X86_64_RELATIVE                    20ef
> 
> Apparently, for static-pie binaries, glibc includes a 
> _dl_relocate_static_pie routine that will perform the relocations as 
> part of the startup [2,3]. 
> 

I am not sure whether all those 'rela.dyn' matters due to the reason you
mentioned below ...

> Since the enclave loading process is 
> different and glibc is not included, we cannot rely on these relocations 
> to be performed and we need to do them manually. 
> 

... here.

Those relocation table are not used by enclave builder anyway.  Only ".tsc"
".text" and ".data" + some heap are built as enclave.

> Note: the first 4 
> symbols in the relocation table above are from the TCS initialization in 
> test_encl_bootstrap.S and should *not* be relocated (as these are 
> relative to enclave base as per SGX spec).

I don't quite follow here.  Per my understanding TCS pages can be any page
within the enclave.  I don't quite understand what does "relocated" mean here.

> 
> Bottom line: the way I see it, the enclave should either ensure no 
> relocations are needed, or perform the relocations manually where 
> needed, or include a _dl_relocate_static_pie equivalent that parses the 
> .rela.dyn ELF section and patches all relocations (minus the TCS 
> symbols). Since the latter (most general) approach is likely going to 
> make the selftest enclave unnecessarily complex by including ELF parsing 
> etc, I propose to simply relocate the function-pointer table manually 
> (which is indeed the only place that needs relocations).

I think we are kinda mixing two things together: 1) the "relocation" supported
by the "non-enclave" normal case, where the compiler/linker generates the PIC
code, and the loader does the "runtime" fixup for those in the "rela.dyn"; 2)
the "relocation" for the enclave case, where the compiler/linker still generates
the PIC code, but the "enclave loader" loads the enclave into a random virtual
address of the process.

Obviously the "enclave loader" (implemented in this selftests/sgx/...) isn't as
powerful as the "real loader" in the normal case.  In fact, reading the code,
IIUC, it simply gathers ".tsc"/".text"/".data" sections from the ELF file (and
plus some heap) and load them into the enclave.

Now the important thing is: those sections are "contiguous" in the enclave. 
That means the kernel needs to build the enclave ELF file with those sections
"contiguously" in the same order too as a single piece, and this single piece
can work at any random address that the "enclave loader" loads the enclave.  Any
address fixing up due to different location of ".data"/".tsc" section at loading
time cannot be generated.

This should be the thing that we need to focus on.

That being said, I think ideally there shouldn't be _ANY_ "rela.dyn" in the
enclave ELF file.

> 
> I will include code to properly compile the selftest enclave with 
> -static-pie as per above and relocate the function-pointer table in the 
> next patch revision.

I agree we should use "-static-pie" + "-fPIE" (or "-fPIC" is also OK??).

However I am yet not convinced the "relocate function-pointer" thing.  If you
can paste the relevant code it would be helpful.


Or am I missing something big?

Re: [PATCH 4/5] selftests/sgx: Ensure expected enclave data buffer size and placement.

[Huang, Kai]

On Mon, 2023-08-07 at 11:41 +0200, Jo Van Bulck wrote:
> On 28.07.23 21:19, Jarkko Sakkinen wrote:
> > So, when exactly is it optimized away by the compiler? This is missing.
> 
> The problem is that declaring encl_buf as static, implies that it will 
> only be used in this file and the compiler is allowed to optimize away 
> any entries that are never used within this compilation unit (e.g., when 
> optimizing out the memcpy calls).
> 
> In reality, the tests outside test_encl.elf rely on both the size and 
> exact placement of encl_buf at the start of .data.
> 
> For example, clang -Os generates the following (legal) code when 
> encl_bug is declared as static:
> 
> 0000000000001020 <do_encl_op_put_to_buf>:
>      mov    0x8(%rdi),%al
>      mov    %al,0x1fd7(%rip)   # 3000 <encl_buffer.0>
>      mov    0x9(%rdi),%al
>      mov    %al,0x8fce(%rip)   # a000 <encl_buffer.1.0>
>      mov    0xa(%rdi),%al
>      mov    %al,0x8fd5(%rip)   # a010 <encl_buffer.1.1>
>      mov    0xb(%rdi),%al
>      mov    %al,0x8fce(%rip)   # a012 <encl_buffer.1.2>
>      mov    0xc(%rdi),%al
>      mov    %al,0x8fd3(%rip)   # a020 <encl_buffer.1.3>
>      mov    0xd(%rdi),%al
>      mov    %al,0x8fce(%rip)   # a024 <encl_buffer.1.4>
>      mov    0xe(%rdi),%al
>      mov    %al,0x8fd1(%rip)   # a030 <encl_buffer.1.5>
>      mov    0xf(%rdi),%al
>      mov    %al,0x8fca(%rip)   # a032 <encl_buffer.1.6>
>      ret
> 
> Disassembly of section .data:
> 
> 0000000000003000 <encl_buffer.0>:
>      3000:       01 00
>          ...
> 0000000000004000 <encl_ssa_tcs1>:
> 
> Thus, this proposed patch fixes both the size and location:
> 
> 1. removing the static keyword from the encl_bug declaration ensures 
> that the _entire_ buffer is preserved with expected size, as the 
> compiler cannot anymore assume encl_buf is only used in this file.

Could we use "used" attribute?

https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html

used 

	This attribute, attached to a variable with static storage, means that 
	the variable must be emitted even if it appears that the variable is 
	not referenced.

	When applied to a static data member of a C++ class template, the 
	attribute also means that the member is instantiated if the class 
	itself is instantiated.
> 
> 2. adding _attribute__((section(".data.encl_buffer"))) ensures that we 
> can control the expected location at the start of the .data section. I 
> think this is optional, as encl_buf always seems to be placed at the 
> start of .data in all my tests. But afaik this is not guaranteed as per 
> the C standard and such constraints on exact placement should better be 
> explicitly controlled in the linker script(?)

This looks sane.

Re: [PATCH 4/5] selftests/sgx: Ensure expected enclave data buffer size and placement.

[Jo Van Bulck]

On 18.08.23 06:07, Huang, Kai wrote:
> Could we use "used" attribute?
> 
> https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html
> 
> used
> 
> 	This attribute, attached to a variable with static storage, means that
> 	the variable must be emitted even if it appears that the variable is
> 	not referenced.
> 
> 	When applied to a static data member of a C++ class template, the
> 	attribute also means that the member is instantiated if the class
> 	itself is instantiated.

Thank you for pointing this out! I was not aware of this attribute, but 
it is indeed exactly what we need in this case and works as expected.

>>
>> 2. adding _attribute__((section(".data.encl_buffer"))) ensures that we
>> can control the expected location at the start of the .data section. I
>> think this is optional, as encl_buf always seems to be placed at the
>> start of .data in all my tests. But afaik this is not guaranteed as per
>> the C standard and such constraints on exact placement should better be
>> explicitly controlled in the linker script(?)
> 
> This looks sane.

Thanks, applying this in a separate commit as discussed.

Best,
Jo

Re: [PATCH 2/5] selftests/sgx: Fix function pointer relocation in test enclave.

[Jo Van Bulck]

On 18.08.23 05:54, Huang, Kai wrote:
> How is the "prepared copy" prepared, exactly?  Could you paste the relevant code
> here too?  IMHO w/o it it's hard to tell whether the code could be wrong or not
> after relocating.

The "prepared copy" resides in .data and simply contains the absolute 
addresses of the functions (i.e., as they appear in objdump with .text 
starting at 0x2000).

Compiled with gcc -static -fPIC -Os, this looks as follows (the 
"prepared copy" starts at 0x5000, immediately following encl_buffer at 
the start of .data):

readelf -x .data test_encl.elf
   0x00005000 64200000 00000000 7a200000 00000000 d ......z ......
   0x00005010 90200000 00000000 a3200000 00000000 . ....... ......
   0x00005020 b6200000 00000000 24200000 00000000 . ......$ ......
   0x00005030 00200000 00000000 bb200000 00000000 . ....... ......

objdump -D test_encl.elf | grep do_encl_
   0000000000002000 <do_encl_emodpe>:
   0000000000002024 <do_encl_eaccept>:
   0000000000002064 <do_encl_op_put_to_buf>:
   000000000000207a <do_encl_op_get_from_buf>:
   0000000000002090 <do_encl_op_put_to_addr>:
   00000000000020a3 <do_encl_op_get_from_addr>:
   00000000000020b6 <do_encl_op_nop>:
   00000000000020bb <do_encl_init_tcs_page>:

For reference, the full encl_body code:

0000000000002175 <encl_body>:
     2175:       f3 0f 1e fa             endbr64
     2179:       49 89 f8                mov    %rdi,%r8
     217c:       48 8d 35 7d 2e 00 00    lea    0x2e7d(%rip),%rsi 
# 5000 <encl_buffer+0x2000>
     2183:       48 8d 7c 24 b8          lea    -0x48(%rsp),%rdi
     2188:       b9 10 00 00 00          mov    $0x10,%ecx
     218d:       f3 a5                   rep movsl %ds:(%rsi),%es:(%rdi)
     218f:       49 8b 00                mov    (%r8),%rax
     2192:       48 83 f8 07             cmp    $0x7,%rax
     2196:       77 0a                   ja     21a2 <encl_body+0x2d>
     2198:       48 8b 44 c4 b8          mov    -0x48(%rsp,%rax,8),%rax
     219d:       4c 89 c7                mov    %r8,%rdi
     21a0:       ff e0                   jmp    *%rax
     21a2:       c3                      ret

Thus, the "prepared copy" with _absolute_ function pointers is loaded 
from .data and copied onto the stack. The code then jumps to the 
_absolute_ function address loaded from the local copy, i.e., _without_ 
first properly relocating the loaded address.

> I am not sure whether all those 'rela.dyn' matters due to the reason you
> mentioned below ...
> 
>> Since the enclave loading process is
>> different and glibc is not included, we cannot rely on these relocations
>> to be performed and we need to do them manually.
>>
> 
> ... here.
> 
> Those relocation table are not used by enclave builder anyway.  Only ".tsc"
> ".text" and ".data" + some heap are built as enclave.

Yes, the relocation table is not used by the (untrusted) enclave 
builder, neither by a (trusted) initialization stub inside the enclave. 
Hence, this commit tries to address this by _manually_ relocating the 
(only) needed relocations.

>> Note: the first 4
>> symbols in the relocation table above are from the TCS initialization in
>> test_encl_bootstrap.S and should *not* be relocated (as these are
>> relative to enclave base as per SGX spec).
> 
> I don't quite follow here.  Per my understanding TCS pages can be any page
> within the enclave.  I don't quite understand what does "relocated" mean here.

Yes, the TCS can be any page in the enclave, as the architectural 
definitions are explicitly position-independent: OSSA and OENTRY are 
specified as a relative _offset_ to the enclave base runtime load address.

The thing is, these fields are initialized in test_encl_bootstrap.S as 
symbols to filled in by the linker:

         .section ".tcs", "aw"
         .balign 4096

         .fill   1, 8, 0                 # STATE (set by CPU)
         .fill   1, 8, 0                 # FLAGS
         .quad   encl_ssa_tcs1           # OSSA
         .fill   1, 4, 0                 # CSSA (set by CPU)
         .fill   1, 4, 1                 # NSSA
         .quad   encl_entry              # OENTRY
	/* snipped */

Thus, when compiling/linking with "-static-pie", the linker (obviously 
not aware of SGX TCS semantics) will treat these symbols as "normal" and 
recognize that they need to be relocated as they are absolute (non-RIP 
relative) references and places them in ".rela.dyn":

Relocation section '.rela.dyn' at offset 0x4000 contains 12 entries:
    Offset          Info           Type           Sym. Value    Sym. Name
+ Addend
# tcs1.ossa
000000000010  000000000008 R_X86_64_RELATIVE                    7000
# tcs1.oentry
000000000020  000000000008 R_X86_64_RELATIVE                    21e3

Thus, my earlier comment says that we can safely ignore these 
apparent/false TCS "relocations".

> I think we are kinda mixing two things together: 1) the "relocation" supported
> by the "non-enclave" normal case, where the compiler/linker generates the PIC
> code, and the loader does the "runtime" fixup for those in the "rela.dyn"; 2)
> the "relocation" for the enclave case, where the compiler/linker still generates
> the PIC code, but the "enclave loader" loads the enclave into a random virtual
> address of the process.
> 
> Obviously the "enclave loader" (implemented in this selftests/sgx/...) isn't as
> powerful as the "real loader" in the normal case.  In fact, reading the code,
> IIUC, it simply gathers ".tsc"/".text"/".data" sections from the ELF file (and
> plus some heap) and load them into the enclave.

Agreed, the "enclave loader" should simply copy the sections into EPC 
memory without being a "real loader". Particularly, it should *not* do 
any relocations as that would change the code and, hence, the MRENCLAVE 
signature.

> 
> Now the important thing is: those sections are "contiguous" in the enclave.
> That means the kernel needs to build the enclave ELF file with those sections
> "contiguously" in the same order too as a single piece, and this single piece
> can work at any random address that the "enclave loader" loads the enclave.  Any
> address fixing up due to different location of ".data"/".tsc" section at loading
> time cannot be generated.
> 
> This should be the thing that we need to focus on.
>

Agreed. That's why any _necessary_ relocations need to happen *inside* 
the enclave, by the application or a small initialization stub, such 
that the enclave MRENCLAVE identity is independent of its load address.

> That being said, I think ideally there shouldn't be _ANY_ "rela.dyn" in the
> enclave ELF file.

Agreed, this would be "ideal". However, I don't see a way to generate 
the function-pointer table without needing a runtime relocation. For all 
other code, we don't have to care about this and we can simply rely on 
-static-pie and -fPIE to emit RIP-relative code without needing 
relocations. Afais, when storing an address in a variable, a relocation 
is needed.

I agree though that we do *not* need a full .rela.dyn parser here and 
can simply manually relocate the only relevant case here, ie encl_op_array.

> I agree we should use "-static-pie" + "-fPIE" (or "-fPIC" is also OK??).

Not sure on the exact difference between -fPIE and -fPIC. I changed to 
-fPIE because gcc mentions in the documentation for "-static-pie" that:

For predictable results, you must also specify the same set of options 
used for compilation (-fpie, -fPIE, or model suboptions) when you 
specify this linker option.

> However I am yet not convinced the "relocate function-pointer" thing.  If you
> can paste the relevant code it would be helpful.

See above. Let me know if you'd like more context or the full binary.

> Or am I missing something big?

Nothing big, but, in my understanding, even PIE code may still need 
(limited) relocations, eg when storing (function-pointer) addresses in a 
C variable. This is normally taken care of by glibc on startup, but we 
need to do this manually here as implemented in this commit.

Best,
Jo

Re: [PATCH 2/5] selftests/sgx: Fix function pointer relocation in test enclave.

[Huang, Kai]

On Fri, 2023-08-18 at 19:30 -0700, Jo Van Bulck wrote:
> On 18.08.23 05:54, Huang, Kai wrote:
> > How is the "prepared copy" prepared, exactly?  Could you paste the relevant code
> > here too?  IMHO w/o it it's hard to tell whether the code could be wrong or not
> > after relocating.
> 
> The "prepared copy" resides in .data and simply contains the absolute 
> addresses of the functions (i.e., as they appear in objdump with .text 
> starting at 0x2000).
> 
> Compiled with gcc -static -fPIC -Os, this looks as follows (the 
> "prepared copy" starts at 0x5000, immediately following encl_buffer at 
> the start of .data):
> 
> readelf -x .data test_encl.elf
>    0x00005000 64200000 00000000 7a200000 00000000 d ......z ......
>    0x00005010 90200000 00000000 a3200000 00000000 . ....... ......
>    0x00005020 b6200000 00000000 24200000 00000000 . ......$ ......
>    0x00005030 00200000 00000000 bb200000 00000000 . ....... ......
> 
> objdump -D test_encl.elf | grep do_encl_
>    0000000000002000 <do_encl_emodpe>:
>    0000000000002024 <do_encl_eaccept>:
>    0000000000002064 <do_encl_op_put_to_buf>:
>    000000000000207a <do_encl_op_get_from_buf>:
>    0000000000002090 <do_encl_op_put_to_addr>:
>    00000000000020a3 <do_encl_op_get_from_addr>:
>    00000000000020b6 <do_encl_op_nop>:
>    00000000000020bb <do_encl_init_tcs_page>:
> 
> For reference, the full encl_body code:
> 
> 0000000000002175 <encl_body>:
>      2175:       f3 0f 1e fa             endbr64
>      2179:       49 89 f8                mov    %rdi,%r8
>      217c:       48 8d 35 7d 2e 00 00    lea    0x2e7d(%rip),%rsi 
> # 5000 <encl_buffer+0x2000>
>      2183:       48 8d 7c 24 b8          lea    -0x48(%rsp),%rdi
>      2188:       b9 10 00 00 00          mov    $0x10,%ecx
>      218d:       f3 a5                   rep movsl %ds:(%rsi),%es:(%rdi)
>      218f:       49 8b 00                mov    (%r8),%rax
>      2192:       48 83 f8 07             cmp    $0x7,%rax
>      2196:       77 0a                   ja     21a2 <encl_body+0x2d>
>      2198:       48 8b 44 c4 b8          mov    -0x48(%rsp,%rax,8),%rax
>      219d:       4c 89 c7                mov    %r8,%rdi
>      21a0:       ff e0                   jmp    *%rax
>      21a2:       c3                      ret
> 
> Thus, the "prepared copy" with _absolute_ function pointers is loaded 
> from .data and copied onto the stack. The code then jumps to the 
> _absolute_ function address loaded from the local copy, i.e., _without_ 
> first properly relocating the loaded address.

Thanks.  You mentioned this was generated by "-static -fPIC -Os" but I think
using "-static-pie -fPIE -Os" would probably generate the same.

> 
> > I am not sure whether all those 'rela.dyn' matters due to the reason you
> > mentioned below ...
> > 
> > > Since the enclave loading process is
> > > different and glibc is not included, we cannot rely on these relocations
> > > to be performed and we need to do them manually.
> > > 
> > 
> > ... here.
> > 
> > Those relocation table are not used by enclave builder anyway.  Only ".tsc"
> > ".text" and ".data" + some heap are built as enclave.
> 
> Yes, the relocation table is not used by the (untrusted) enclave 
> builder, neither by a (trusted) initialization stub inside the enclave. 
> Hence, this commit tries to address this by _manually_ relocating the 
> (only) needed relocations.
> 
> > > Note: the first 4
> > > symbols in the relocation table above are from the TCS initialization in
> > > test_encl_bootstrap.S and should *not* be relocated (as these are
> > > relative to enclave base as per SGX spec).
> > 
> > I don't quite follow here.  Per my understanding TCS pages can be any page
> > within the enclave.  I don't quite understand what does "relocated" mean here.
> 
> Yes, the TCS can be any page in the enclave, as the architectural 
> definitions are explicitly position-independent: OSSA and OENTRY are 
> specified as a relative _offset_ to the enclave base runtime load address.
> 
> The thing is, these fields are initialized in test_encl_bootstrap.S as 
> symbols to filled in by the linker:
> 
>          .section ".tcs", "aw"
>          .balign 4096
> 
>          .fill   1, 8, 0                 # STATE (set by CPU)
>          .fill   1, 8, 0                 # FLAGS
>          .quad   encl_ssa_tcs1           # OSSA
>          .fill   1, 4, 0                 # CSSA (set by CPU)
>          .fill   1, 4, 1                 # NSSA
>          .quad   encl_entry              # OENTRY
> 	/* snipped */
> 
> Thus, when compiling/linking with "-static-pie", the linker (obviously 
> not aware of SGX TCS semantics) will treat these symbols as "normal" and 
> recognize that they need to be relocated as they are absolute (non-RIP 
> relative) references and places them in ".rela.dyn":

Right.  Even for "-static-pie" it's still possible to generate "rela.dyn" for
those have to use absolute addressing.

> 
> Relocation section '.rela.dyn' at offset 0x4000 contains 12 entries:
>     Offset          Info           Type           Sym. Value    Sym. Name
> + Addend
> # tcs1.ossa
> 000000000010  000000000008 R_X86_64_RELATIVE                    7000
> # tcs1.oentry
> 000000000020  000000000008 R_X86_64_RELATIVE                    21e3
> 
> Thus, my earlier comment says that we can safely ignore these 
> apparent/false TCS "relocations".

Yeah.  I guess that's why the test_encl.elf must be built starting from address
0.

> 
> > I think we are kinda mixing two things together: 1) the "relocation" supported
> > by the "non-enclave" normal case, where the compiler/linker generates the PIC
> > code, and the loader does the "runtime" fixup for those in the "rela.dyn"; 2)
> > the "relocation" for the enclave case, where the compiler/linker still generates
> > the PIC code, but the "enclave loader" loads the enclave into a random virtual
> > address of the process.
> > 
> > Obviously the "enclave loader" (implemented in this selftests/sgx/...) isn't as
> > powerful as the "real loader" in the normal case.  In fact, reading the code,
> > IIUC, it simply gathers ".tsc"/".text"/".data" sections from the ELF file (and
> > plus some heap) and load them into the enclave.
> 
> Agreed, the "enclave loader" should simply copy the sections into EPC 
> memory without being a "real loader". Particularly, it should *not* do 
> any relocations as that would change the code and, hence, the MRENCLAVE 
> signature.

Yeah.

> 
> > 
> > Now the important thing is: those sections are "contiguous" in the enclave.
> > That means the kernel needs to build the enclave ELF file with those sections
> > "contiguously" in the same order too as a single piece, and this single piece
> > can work at any random address that the "enclave loader" loads the enclave.  Any
> > address fixing up due to different location of ".data"/".tsc" section at loading
> > time cannot be generated.
> > 
> > This should be the thing that we need to focus on.
> > 
> 
> Agreed. That's why any _necessary_ relocations need to happen *inside* 
> the enclave, by the application or a small initialization stub, such 
> that the enclave MRENCLAVE identity is independent of its load address.
> 
> > That being said, I think ideally there shouldn't be _ANY_ "rela.dyn" in the
> > enclave ELF file.
> 
> Agreed, this would be "ideal". However, I don't see a way to generate 
> the function-pointer table without needing a runtime relocation. For all 
> other code, we don't have to care about this and we can simply rely on 
> -static-pie and -fPIE to emit RIP-relative code without needing 
> relocations. Afais, when storing an address in a variable, a relocation 
> is needed.

Yeah.  Thanks.

> 
> I agree though that we do *not* need a full .rela.dyn parser here and 
> can simply manually relocate the only relevant case here, ie encl_op_array.
> 
> > I agree we should use "-static-pie" + "-fPIE" (or "-fPIC" is also OK??).
> 
> Not sure on the exact difference between -fPIE and -fPIC. I changed to 
> -fPIE because gcc mentions in the documentation for "-static-pie" that:
> 
> For predictable results, you must also specify the same set of options 
> used for compilation (-fpie, -fPIE, or model suboptions) when you 
> specify this linker option.

Yeah I guess we should just use "-fPIE".

[...]

Re: [PATCH 2/5] selftests/sgx: Fix function pointer relocation in test enclave.

[Jo Van Bulck]

On 21.08.23 13:04, Huang, Kai wrote:
> Thanks.  You mentioned this was generated by "-static -fPIC -Os" but I think
> using "-static-pie -fPIE -Os" would probably generate the same.

Thanks, yes, I can confirm that the same code is generated with 
-static-pie -FPIE.

> Right.  Even for "-static-pie" it's still possible to generate "rela.dyn" for
> those have to use absolute addressing.

Indeed.

>> Thus, my earlier comment says that we can safely ignore these
>> apparent/false TCS "relocations".
> 
> Yeah.  I guess that's why the test_encl.elf must be built starting from address
> 0.

True (for the ELF in the linker script, but we still need the runtime 
relocations for the function-pointer table when loading the enclave at a 
non-zero base address).

> Yeah I guess we should just use "-fPIE".

Agreed.

Best,
Jo