[PATCH v2 0/8] selftests/sgx: Fix compilation errors

[PATCH v2 0/8] selftests/sgx: Fix compilation errors

[Jo Van Bulck]

Hi,

This is the second iteration of a patch series to ensure 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 useful, I can also include an elementary wrapper shell script to compile and
run the tests for different compilers (gcc/clang) and optimization levels.
Reference output below:

.. Testing gcc   -O0    [OK]
.. Testing gcc   -O1    [OK]
.. Testing gcc   -O2    [OK]
.. Testing gcc   -O3    [OK]
.. Testing gcc   -Os    [OK]
.. Testing gcc   -Ofast [OK]
.. Testing gcc   -Og    [OK]
.. Testing clang -O0    [OK]
.. Testing clang -O1    [OK]
.. Testing clang -O2    [OK]
.. Testing clang -O3    [OK]
.. Testing clang -Os    [OK]
.. Testing clang -Ofast [OK]
.. Testing clang -Og    [OK]

Changelog
---------

v2
  - Add additional check for NULL pointer (Kai)
  - Refine to produce proper static-pie executable
  - Fix linker script assertions
  - Specify memory clobber for inline asm instead of volatile (Kai)
  - Clarify why encl_buffer non-static (Jarkko, Kai)
  - Clarify -ffreestanding (Jarkko)

Best,
Jo

Jo Van Bulck (8):
  selftests/sgx: Fix uninitialized pointer dereference in error path
  selftests/sgx: Produce static-pie executable for test enclave
  selftests/sgx: Handle relocations in test enclave
  selftests/sgx: Fix linker script asserts
  selftests/sgx: Include memory clobber for inline asm in test enclave
  selftests/sgx: Ensure expected enclave data buffer size and placement
  selftests/sgx: Separate linker options
  selftests/sgx: Specify freestanding environment for enclave
    compilation

 tools/testing/selftests/sgx/Makefile          | 14 ++---
 tools/testing/selftests/sgx/sigstruct.c       |  5 +-
 tools/testing/selftests/sgx/test_encl.c       | 52 ++++++++++++-------
 tools/testing/selftests/sgx/test_encl.lds     | 11 ++--
 .../selftests/sgx/test_encl_bootstrap.S       | 12 ++---
 5 files changed, 56 insertions(+), 38 deletions(-)

-- 
2.34.1

[PATCH 1/8] 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.

Fixes: 2adcba79e69d ("selftests/x86: Add a selftest for SGX")
Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be>
---
 tools/testing/selftests/sgx/sigstruct.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/sgx/sigstruct.c b/tools/testing/selftests/sgx/sigstruct.c
index a07896a46364..d73b29becf5b 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));
@@ -384,7 +384,8 @@ bool encl_measure(struct encl *encl)
 	return true;
 
 err:
-	EVP_MD_CTX_destroy(ctx);
+	if (ctx)
+		EVP_MD_CTX_destroy(ctx);
 	RSA_free(key);
 	return false;
 }
-- 
2.34.1

[PATCH 2/8] selftests/sgx: Produce static-pie executable for test enclave

[Jo Van Bulck]

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.

Link: https://lore.kernel.org/all/f9c24d89-ed72-7d9e-c650-050d722c6b04@cs.kuleuven.be/
Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be>
---
 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 50aab6b57da3..1d6315a2e5f5 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)
 
 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 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..c91743f14312 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 __enclave_base and
+	# then add the address of encl_stack to get the address for the stack.
+	lea __enclave_base(%rip), %rax
+	sub %rax, %rbx
+	lea encl_stack(%rip), %rax
+	add %rbx, %rax
 	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
-- 
2.34.1

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

[Jo Van Bulck]

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   | 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;
+
+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,
+};
+
 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.
+	 */
 	if (op->type < ENCL_OP_MAX)
-		(*encl_op_array[op->type])(op);
+		(*(((uint64_t) &__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 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 */
+		*(.dyn*)
+		*(.gnu.hash)
 	}
 }
 
-- 
2.34.1

[PATCH 4/8] selftests/sgx: Fix linker script asserts

[Jo Van Bulck]

DEFINED only considers symbols, not section names. Hence, replace the
check for .got.plt with the _GLOBAL_OFFSET_TABLE_ symbol and remove other
(non-essential) asserts.

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

diff --git a/tools/testing/selftests/sgx/test_encl.lds b/tools/testing/selftests/sgx/test_encl.lds
index 73d9c8bbe7de..27c2527ecbc4 100644
--- a/tools/testing/selftests/sgx/test_encl.lds
+++ b/tools/testing/selftests/sgx/test_encl.lds
@@ -38,8 +38,4 @@ SECTIONS
 	}
 }
 
-ASSERT(!DEFINED(.altinstructions), "ALTERNATIVES are not supported in enclaves")
-ASSERT(!DEFINED(.altinstr_replacement), "ALTERNATIVES are not supported in enclaves")
-ASSERT(!DEFINED(.discard.retpoline_safe), "RETPOLINE ALTERNATIVES are not supported in enclaves")
-ASSERT(!DEFINED(.discard.nospec), "RETPOLINE ALTERNATIVES are not supported in enclaves")
-ASSERT(!DEFINED(.got.plt), "Libcalls are not supported in enclaves")
+ASSERT(!DEFINED(_GLOBAL_OFFSET_TABLE_), "Libcalls through GOT are not supported in enclaves")
-- 
2.34.1

[PATCH 5/8] selftests/sgx: Include memory clobber for inline asm in test enclave

[Jo Van Bulck]

Add the "memory" clobber to the EMODPE and EACCEPT asm blocks to tell the
compiler the assembly code accesses to the secinfo struct. This ensures
the compiler treats the asm block as a memory barrier and the write to
secinfo will be visible to ENCLU.

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

diff --git a/tools/testing/selftests/sgx/test_encl.c b/tools/testing/selftests/sgx/test_encl.c
index c71dfbadd2d9..5b758eaf808c 100644
--- a/tools/testing/selftests/sgx/test_encl.c
+++ b/tools/testing/selftests/sgx/test_encl.c
@@ -24,10 +24,11 @@ static void do_encl_emodpe(void *_op)
 	secinfo.flags = op->flags;
 
 	asm volatile(".byte 0x0f, 0x01, 0xd7"
-				:
+				: /* no outputs */
 				: "a" (EMODPE),
 				  "b" (&secinfo),
-				  "c" (op->epc_addr));
+				  "c" (op->epc_addr)
+				: "memory" /* read from secinfo pointer */);
 }
 
 static void do_encl_eaccept(void *_op)
@@ -42,7 +43,8 @@ static void do_encl_eaccept(void *_op)
 				: "=a" (rax)
 				: "a" (EACCEPT),
 				  "b" (&secinfo),
-				  "c" (op->epc_addr));
+				  "c" (op->epc_addr)
+				: "memory" /* read from secinfo pointer */);
 
 	op->ret = rax;
 }
-- 
2.34.1

[PATCH 6/8] selftests/sgx: Ensure expected enclave data buffer size and placement

[Jo Van Bulck]

Ensure the compiler respects the size and placement of encl_buffer as
expected by the external tests manipulating page permissions:

1. Declare encl_buffer as global, in order to ensure that it is not
   optimized away by the compiler, even when not used entirely by the test
   enclave code.

2. Place encl_buffer in a separate section that is explicitly placed
   at the start of the .data segment in the linker script to avoid the
   compiler placing it somewhere else in .data.

Link: https://lore.kernel.org/all/a2732938-f3db-a0af-3d68-a18060f66e79@cs.kuleuven.be/
Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be>
---
 tools/testing/selftests/sgx/test_encl.c   | 9 +++++----
 tools/testing/selftests/sgx/test_encl.lds | 1 +
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/sgx/test_encl.c b/tools/testing/selftests/sgx/test_encl.c
index 5b758eaf808c..02a9e8c55e82 100644
--- a/tools/testing/selftests/sgx/test_encl.c
+++ b/tools/testing/selftests/sgx/test_encl.c
@@ -5,11 +5,12 @@
 #include "defines.h"
 
 /*
- * 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.
+ * Data buffer spanning two pages that will be placed first in the .data
+ * segment via the linker script. Even if not used internally the second page
+ * is needed by external test manipulating page permissions, so do not declare
+ * encl_buffer as static to make sure it is entirely preserved by the compiler.
  */
-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 27c2527ecbc4..2ec29340ba94 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 7/8] selftests/sgx: Separate linker options

[Jo Van Bulck]

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

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

diff --git a/tools/testing/selftests/sgx/Makefile b/tools/testing/selftests/sgx/Makefile
index 1d6315a2e5f5..2de970f7237c 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
+HOST_CFLAGS := -Wall -Werror -g $(INCLUDES) -fPIC
+HOST_LDFLAGS := -z noexecstack -lcrypto
 ENCL_CFLAGS := -Wall -Werror -static-pie -nostdlib -nostartfiles -fPIE \
 	       -fno-stack-protector -mrdrnd $(INCLUDES)
+ENCL_LDFLAGS := -Wl,-T,test_encl.lds,--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 $@
@@ -45,8 +47,8 @@ $(OUTPUT)/call.o: call.S
 $(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
+$(OUTPUT)/test_encl.elf: test_encl.c test_encl_bootstrap.S
+	$(CC) $(ENCL_CFLAGS) $^ -o $@ $(ENCL_LDFLAGS)
 
 EXTRA_CLEAN := \
 	$(OUTPUT)/test_encl.elf \
-- 
2.34.1

[PATCH 8/8] selftests/sgx: Specify freestanding environment for enclave compilation

[Jo Van Bulck]

Use -ffreestanding to assert the enclave compilation targets a
freestanding environment (i.e., without "main" or standard libraries).
This fixes clang reporting "undefined reference to `memset'" after
erroneously optimizing away the provided memset/memcpy implementations.

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

diff --git a/tools/testing/selftests/sgx/Makefile b/tools/testing/selftests/sgx/Makefile
index 2de970f7237c..19a07e890009 100644
--- a/tools/testing/selftests/sgx/Makefile
+++ b/tools/testing/selftests/sgx/Makefile
@@ -14,8 +14,8 @@ endif
 INCLUDES := -I$(top_srcdir)/tools/include
 HOST_CFLAGS := -Wall -Werror -g $(INCLUDES) -fPIC
 HOST_LDFLAGS := -z noexecstack -lcrypto
-ENCL_CFLAGS := -Wall -Werror -static-pie -nostdlib -nostartfiles -fPIE \
-	       -fno-stack-protector -mrdrnd $(INCLUDES)
+ENCL_CFLAGS := -Wall -Werror -static-pie -nostdlib -ffreestanding -fPIE \
+		-nostartfiles -fno-stack-protector -mrdrnd $(INCLUDES)
 ENCL_LDFLAGS := -Wl,-T,test_encl.lds,--build-id=none
 
 TEST_CUSTOM_PROGS := $(OUTPUT)/test_sgx
-- 
2.34.1

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

[Jarkko Sakkinen]

On Tue Aug 8, 2023 at 10:31 PM EEST, 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.
>
> Fixes: 2adcba79e69d ("selftests/x86: Add a selftest for SGX")
> Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be>
> ---
>  tools/testing/selftests/sgx/sigstruct.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/sgx/sigstruct.c b/tools/testing/selftests/sgx/sigstruct.c
> index a07896a46364..d73b29becf5b 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));
> @@ -384,7 +384,8 @@ bool encl_measure(struct encl *encl)
>  	return true;
>  
>  err:
> -	EVP_MD_CTX_destroy(ctx);
> +	if (ctx)
> +		EVP_MD_CTX_destroy(ctx);
>  	RSA_free(key);
>  	return false;
>  }
> -- 
> 2.34.1

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

BR, Jarkko

Re: [PATCH 2/8] selftests/sgx: Produce static-pie executable for test enclave

[Jarkko Sakkinen]

On Tue Aug 8, 2023 at 10:31 PM EEST, 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.
>
> Link: https://lore.kernel.org/all/f9c24d89-ed72-7d9e-c650-050d722c6b04@cs.kuleuven.be/
> Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be>
> ---
>  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 50aab6b57da3..1d6315a2e5f5 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)
>  
>  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 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..c91743f14312 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 __enclave_base and
> +	# then add the address of encl_stack to get the address for the stack.
> +	lea __enclave_base(%rip), %rax
> +	sub %rax, %rbx
> +	lea encl_stack(%rip), %rax
> +	add %rbx, %rax
>  	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
> -- 
> 2.34.1

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

BR, Jarkko

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

[Jarkko Sakkinen]

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

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

[Huang, Kai]

On Tue, 2023-08-08 at 12:31 -0700, 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.
> 
> Fixes: 2adcba79e69d ("selftests/x86: Add a selftest for SGX")
> Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be>
> ---
>  tools/testing/selftests/sgx/sigstruct.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/sgx/sigstruct.c b/tools/testing/selftests/sgx/sigstruct.c
> index a07896a46364..d73b29becf5b 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));
> @@ -384,7 +384,8 @@ bool encl_measure(struct encl *encl)
>  	return true;
>  
>  err:
> -	EVP_MD_CTX_destroy(ctx);
> +	if (ctx)
> +		EVP_MD_CTX_destroy(ctx);
>  	RSA_free(key);
>  	return false;
>  }

Acked-by: Kai Huang <kai.huang@intel.com>

Re: [PATCH 5/8] selftests/sgx: Include memory clobber for inline asm in test enclave

[Huang, Kai]

On Tue, 2023-08-08 at 12:31 -0700, Jo Van Bulck wrote:
> Add the "memory" clobber to the EMODPE and EACCEPT asm blocks to tell the
> compiler the assembly code accesses to the secinfo struct. This ensures
> the compiler treats the asm block as a memory barrier and the write to
> secinfo will be visible to ENCLU.
> 
> Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be>
> ---
>  tools/testing/selftests/sgx/test_encl.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/sgx/test_encl.c b/tools/testing/selftests/sgx/test_encl.c
> index c71dfbadd2d9..5b758eaf808c 100644
> --- a/tools/testing/selftests/sgx/test_encl.c
> +++ b/tools/testing/selftests/sgx/test_encl.c
> @@ -24,10 +24,11 @@ static void do_encl_emodpe(void *_op)
>  	secinfo.flags = op->flags;
>  
>  	asm volatile(".byte 0x0f, 0x01, 0xd7"
> -				:
> +				: /* no outputs */
>  				: "a" (EMODPE),
>  				  "b" (&secinfo),
> -				  "c" (op->epc_addr));
> +				  "c" (op->epc_addr)
> +				: "memory" /* read from secinfo pointer */);
>  }
>  
>  static void do_encl_eaccept(void *_op)
> @@ -42,7 +43,8 @@ static void do_encl_eaccept(void *_op)
>  				: "=a" (rax)
>  				: "a" (EACCEPT),
>  				  "b" (&secinfo),
> -				  "c" (op->epc_addr));
> +				  "c" (op->epc_addr)
> +				: "memory" /* read from secinfo pointer */);
>  
>  	op->ret = rax;
>  }

Reviewed-by: Kai Huang <kai.huang@intel.com>

Re: [PATCH 6/8] selftests/sgx: Ensure expected enclave data buffer size and placement

[Huang, Kai]

On Tue, 2023-08-08 at 12:31 -0700, Jo Van Bulck wrote:
> Ensure the compiler respects the size and placement of encl_buffer as
> expected by the external tests manipulating page permissions:
> 
> 1. Declare encl_buffer as global, in order to ensure that it is not
>    optimized away by the compiler, even when not used entirely by the test
>    enclave code.
> 
> 2. Place encl_buffer in a separate section that is explicitly placed
>    at the start of the .data segment in the linker script to avoid the
>    compiler placing it somewhere else in .data.

Firstly, these two problems are independent. Could you split this into two
patches?  One to preserve the entire buffer, the other to always place the
buffer at the beginning.

Secondly, as replied to v1, I think we can use "used" gcc attribute to always
preserve the buffer?

> 
> Link: https://lore.kernel.org/all/a2732938-f3db-a0af-3d68-a18060f66e79@cs.kuleuven.be/
> Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be>
> ---
>  tools/testing/selftests/sgx/test_encl.c   | 9 +++++----
>  tools/testing/selftests/sgx/test_encl.lds | 1 +
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/testing/selftests/sgx/test_encl.c b/tools/testing/selftests/sgx/test_encl.c
> index 5b758eaf808c..02a9e8c55e82 100644
> --- a/tools/testing/selftests/sgx/test_encl.c
> +++ b/tools/testing/selftests/sgx/test_encl.c
> @@ -5,11 +5,12 @@
>  #include "defines.h"
>  
>  /*
> - * 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.
> + * Data buffer spanning two pages that will be placed first in the .data
> + * segment via the linker script. Even if not used internally the second page
> + * is needed by external test manipulating page permissions, so do not declare
> + * encl_buffer as static to make sure it is entirely preserved by the compiler.
>   */
> -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 27c2527ecbc4..2ec29340ba94 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 3/8] selftests/sgx: Handle relocations in test enclave

[Jo Van Bulck]

On 10.08.23 13:32, Jarkko Sakkinen wrote:
> What happens if I only apply 1/8 and 2/8 from this patch set?

This would work fine for gcc -O0/1/2/3, as encl_op_array happens to be 
locally initialized:

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

However, when compiling with -Os, the initialization proceeds through a 
prepared copy from .data with hard-coded (ie non RIP-relative) function 
addresses:

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

> I'm just wondering why there is no mention of "-static-pie" here.

This patch 3/8 is expected to be applied on top of 2/8 which adds 
"-static-pie". While "-static-pie" is necessary to generate proper, 
position-independent code when referencing global variables, there may 
still be relocations left. These are normally handled by glibc on 
startup, but we don't have that in the test enclave, so this commit 
explicitly handles the (only) relocations for encl_op_array.

When only applying 2/8, gcc generates correct code with -O0/1/2/3, as 
the local encl_op_array initialization happens to be initialized in 
encl_body:

>> +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;
> 

Thanks, makes sense!

>> +
>> +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 dropped "const" to work around a clang warning:

test_encl.c:130:2: warning: incompatible pointer types initializing 
'const void (*)(void *)' with an expression of type 'void (void *)' 
[-Wincompatible-pointer-types]

But I agree dropping const is inferior and it's better to fix the 
incompatible pointer types as per below.

> 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] = {

Thanks this is indeed cleaner. This also fixes the above clang warning.

> 
>>   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.

Dropped.

> 
>>   	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)

Thanks for pointing this out.

> 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);
> }

Thanks, this is indeed much cleaner! Including this in the next revision.

>> +		/* Dynamic symbol table not supported in enclaves */
> 
> I'd drop this comment.

Dropped.

Re: [PATCH 6/8] selftests/sgx: Ensure expected enclave data buffer size and placement

[Jo Van Bulck]

On 18.08.23 06:10, Huang, Kai wrote:
> Firstly, these two problems are independent. Could you split this into two
> patches?  One to preserve the entire buffer, the other to always place the
> buffer at the beginning.

Makes sense, doing this for the next revision.

> Secondly, as replied to v1, I think we can use "used" gcc attribute to always
> preserve the buffer?

Thanks for the suggestion. Yes, I was not aware of this attribute, but 
it is indeed exactly what we need in this case. Applying this in the 
next revision.

Best,
Jo

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

[Jarkko Sakkinen]

On Sat Aug 19, 2023 at 3:32 AM EEST, Jo Van Bulck wrote:
> On 10.08.23 13:32, Jarkko Sakkinen wrote:
> > What happens if I only apply 1/8 and 2/8 from this patch set?
>
> This would work fine for gcc -O0/1/2/3, as encl_op_array happens to be 
> locally initialized:
>
> 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 */
>
> However, when compiling with -Os, the initialization proceeds through a 
> prepared copy from .data with hard-coded (ie non RIP-relative) function 
> addresses:
>
>  > 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 */

Thank you for the explanation.

> > I'm just wondering why there is no mention of "-static-pie" here.
>
> This patch 3/8 is expected to be applied on top of 2/8 which adds 
> "-static-pie". While "-static-pie" is necessary to generate proper, 
> position-independent code when referencing global variables, there may 
> still be relocations left. These are normally handled by glibc on 
> startup, but we don't have that in the test enclave, so this commit 
> explicitly handles the (only) relocations for encl_op_array.
>
> When only applying 2/8, gcc generates correct code with -O0/1/2/3, as 
> the local encl_op_array initialization happens to be initialized in 
> encl_body:
>
> >> +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;
> > 
>
> Thanks, makes sense!

Great!

> >> +
> >> +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 dropped "const" to work around a clang warning:
>
> test_encl.c:130:2: warning: incompatible pointer types initializing 
> 'const void (*)(void *)' with an expression of type 'void (void *)' 
> [-Wincompatible-pointer-types]
>
> But I agree dropping const is inferior and it's better to fix the 
> incompatible pointer types as per below.
>
> > 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] = {
>
> Thanks this is indeed cleaner. This also fixes the above clang warning.
>
> > 
> >>   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.
>
> Dropped.
>
> > 
> >>   	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)
>
> Thanks for pointing this out.
>
> > 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);
> > }
>
> Thanks, this is indeed much cleaner! Including this in the next revision.
>
> >> +		/* Dynamic symbol table not supported in enclaves */
> > 
> > I'd drop this comment.
>
> Dropped.

Awesome!

BR, Jarkko