[PATCH v5 00/13] selftests/sgx: Fix compilation errors

[PATCH v5 00/13] selftests/sgx: Fix compilation errors

[Jo Van Bulck]

Hi,

This is the fifth 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.

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

v5
  - Reorder patches (Jarkko, Kai)
  - Include fixes tag for inline asm memory clobber patch (Kai)
  - Include linker error in static-pie commit message (Kai)
  - Include generated assembly in relocations commit (Kai)

v4
  - Remove redundant -nostartfiles compiler flag (Jarkko)
  - Split dynamic symbol table removal in separate commit (Kai)
  - Split redundant push/pop elimination in separate commit (Kai)
  - Remove (incomplete) register cleansing on enclave exit
  - Fix possibly uninitialized pointer dereferences in load.c

v3
  - Refactor encl_op_array declaration and indexing (Jarkko)
  - Annotate encl_buffer with "used" attribute (Kai)
  - Split encl_buffer size and placement commits (Kai)

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 (13):
  selftests/sgx: Fix uninitialized pointer dereference in error path
  selftests/sgx: Fix uninitialized pointer dereferences in
    encl_get_entry
  selftests/sgx: Include memory clobber for inline asm in test enclave
  selftests/sgx: Separate linker options
  selftests/sgx: Specify freestanding environment for enclave
    compilation
  selftests/sgx: Remove redundant enclave base address save/restore
  selftests/sgx: Produce static-pie executable for test enclave
  selftests/sgx: Handle relocations in test enclave
  selftests/sgx: Fix linker script asserts
  selftests/sgx: Ensure test enclave buffer is entirely preserved
  selftests/sgx: Ensure expected location of test enclave buffer
  selftests/sgx: Discard unsupported ELF sections
  selftests/sgx: Remove incomplete ABI sanitization code in test enclave

 tools/testing/selftests/sgx/Makefile          | 12 ++--
 tools/testing/selftests/sgx/defines.h         |  2 +
 tools/testing/selftests/sgx/load.c            |  9 ++-
 tools/testing/selftests/sgx/sigstruct.c       |  5 +-
 tools/testing/selftests/sgx/test_encl.c       | 67 +++++++++++++------
 tools/testing/selftests/sgx/test_encl.lds     | 10 +--
 .../selftests/sgx/test_encl_bootstrap.S       | 28 +++-----
 7 files changed, 77 insertions(+), 56 deletions(-)

-- 
2.25.1

[PATCH v5 01/13] 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>
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
Acked-by: Kai Huang <kai.huang@intel.com>
---
 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.25.1

[PATCH v5 02/13] selftests/sgx: Fix uninitialized pointer dereferences in encl_get_entry

[Jo Van Bulck]

Ensure sym_tab and sym_names are zero-initialized and add an early-out
condition in the unlikely (erroneous) case that the enclave ELF file would
not contain a symbol table.

This addresses -Werror=maybe-uninitialized compiler warnings for gcc -O2.

Fixes: 33c5aac3bf32 ("selftests/sgx: Test complete changing of page type flow")
Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be>
---
 tools/testing/selftests/sgx/load.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/sgx/load.c b/tools/testing/selftests/sgx/load.c
index 94bdeac1cf04..c9f658e44de6 100644
--- a/tools/testing/selftests/sgx/load.c
+++ b/tools/testing/selftests/sgx/load.c
@@ -136,11 +136,11 @@ static bool encl_ioc_add_pages(struct encl *encl, struct encl_segment *seg)
  */
 uint64_t encl_get_entry(struct encl *encl, const char *symbol)
 {
+	Elf64_Sym *symtab = NULL;
+	char *sym_names = NULL;
 	Elf64_Shdr *sections;
-	Elf64_Sym *symtab;
 	Elf64_Ehdr *ehdr;
-	char *sym_names;
-	int num_sym;
+	int num_sym = 0;
 	int i;
 
 	ehdr = encl->bin;
@@ -161,6 +161,9 @@ uint64_t encl_get_entry(struct encl *encl, const char *symbol)
 		}
 	}
 
+	if (!symtab || !sym_names)
+		return 0;
+
 	for (i = 0; i < num_sym; i++) {
 		Elf64_Sym *sym = &symtab[i];
 
-- 
2.25.1

Re: [PATCH v5 02/13] selftests/sgx: Fix uninitialized pointer dereferences in encl_get_entry

[Jarkko Sakkinen]

On Thu Aug 31, 2023 at 4:41 PM EEST, Jo Van Bulck wrote:
> Ensure sym_tab and sym_names are zero-initialized and add an early-out
> condition in the unlikely (erroneous) case that the enclave ELF file would
> not contain a symbol table.
>
> This addresses -Werror=maybe-uninitialized compiler warnings for gcc -O2.
>
> Fixes: 33c5aac3bf32 ("selftests/sgx: Test complete changing of page type flow")
> Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be>
> ---
>  tools/testing/selftests/sgx/load.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/tools/testing/selftests/sgx/load.c b/tools/testing/selftests/sgx/load.c
> index 94bdeac1cf04..c9f658e44de6 100644
> --- a/tools/testing/selftests/sgx/load.c
> +++ b/tools/testing/selftests/sgx/load.c
> @@ -136,11 +136,11 @@ static bool encl_ioc_add_pages(struct encl *encl, struct encl_segment *seg)
>   */
>  uint64_t encl_get_entry(struct encl *encl, const char *symbol)
>  {
> +	Elf64_Sym *symtab = NULL;
> +	char *sym_names = NULL;
>  	Elf64_Shdr *sections;
> -	Elf64_Sym *symtab;
>  	Elf64_Ehdr *ehdr;
> -	char *sym_names;
> -	int num_sym;
> +	int num_sym = 0;
>  	int i;
>  
>  	ehdr = encl->bin;
> @@ -161,6 +161,9 @@ uint64_t encl_get_entry(struct encl *encl, const char *symbol)
>  		}
>  	}
>  
> +	if (!symtab || !sym_names)
> +		return 0;
> +
>  	for (i = 0; i < num_sym; i++) {
>  		Elf64_Sym *sym = &symtab[i];
>  
> -- 
> 2.25.1

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

BR, Jarkko

[PATCH v5 03/13] 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.

Fixes: 20404a808593 ("selftests/sgx: Add test for EPCM permission changes")
Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be>
Reviewed-by: Kai Huang <kai.huang@intel.com>
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 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 c0d6397295e3..ae791df3e5a5 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.25.1

[PATCH v5 04/13] 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>
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 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 50aab6b57da3..dcdd04b322f8 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 -fPIC \
+HOST_CFLAGS := -Wall -Werror -g $(INCLUDES) -fPIC
+HOST_LDFLAGS := -z noexecstack -lcrypto
+ENCL_CFLAGS += -Wall -Werror -static -nostdlib -nostartfiles -fPIC \
 	       -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.25.1

[PATCH v5 05/13] 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.

Still need to instruct the linker from using standard system startup
functions, but drop -nostartfiles as it is implied by -nostdlib.

Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be>
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 tools/testing/selftests/sgx/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/sgx/Makefile b/tools/testing/selftests/sgx/Makefile
index dcdd04b322f8..7eb890bdd3f0 100644
--- a/tools/testing/selftests/sgx/Makefile
+++ b/tools/testing/selftests/sgx/Makefile
@@ -14,7 +14,7 @@ endif
 INCLUDES := -I$(top_srcdir)/tools/include
 HOST_CFLAGS := -Wall -Werror -g $(INCLUDES) -fPIC
 HOST_LDFLAGS := -z noexecstack -lcrypto
-ENCL_CFLAGS += -Wall -Werror -static -nostdlib -nostartfiles -fPIC \
+ENCL_CFLAGS += -Wall -Werror -static -nostdlib -ffreestanding -fPIC \
 	       -fno-stack-protector -mrdrnd $(INCLUDES)
 ENCL_LDFLAGS := -Wl,-T,test_encl.lds,--build-id=none
 
-- 
2.25.1

Re: [PATCH v5 05/13] selftests/sgx: Specify freestanding environment for enclave compilation

[Huang, Kai]

On Thu, 2023-08-31 at 15:41 +0200, Jo Van Bulck wrote:
> 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.
> 
> Still need to instruct the linker from using standard system startup
> functions, but drop -nostartfiles as it is implied by -nostdlib.
> 
> Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be>
> Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

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

[PATCH v5 06/13] selftests/sgx: Remove redundant enclave base address save/restore

[Jo Van Bulck]

Remove redundant push/pop pair that stores and restores the enclave base
address in the test enclave, as it is never used after the pop and can
anyway be easily retrieved via the __encl_base symbol.

Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be>
Acked-by: Kai Huang <kai.huang@intel.com>
---
 tools/testing/selftests/sgx/test_encl_bootstrap.S | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/tools/testing/selftests/sgx/test_encl_bootstrap.S b/tools/testing/selftests/sgx/test_encl_bootstrap.S
index 03ae0f57e29d..e0ce993d3f2c 100644
--- a/tools/testing/selftests/sgx/test_encl_bootstrap.S
+++ b/tools/testing/selftests/sgx/test_encl_bootstrap.S
@@ -55,12 +55,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.25.1

[PATCH v5 07/13] 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.

When building the original "lea (encl_stack)(%rbx), %rax" assembly code
with -static-pie -fPIE, the linker complains about a relocation it cannot
resolve:

/usr/local/bin/ld: /tmp/cchIWyfG.o: relocation R_X86_64_32S against
`.data' can not be used when making a PIE object; recompile with -fPIE
collect2: error: ld returned 1 exit status

Thus, since only RIP-relative addressing is legit for local symbols, use
"encl_stack(%rip)" and declare an explicit "__encl_base" symbol at the
start of the linker script to be able to calculate the stack address
relative to the current TCS in the enclave assembly entry code.

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>
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
Acked-by: Kai Huang <kai.huang@intel.com>
---
 tools/testing/selftests/sgx/Makefile              | 2 +-
 tools/testing/selftests/sgx/test_encl.lds         | 1 +
 tools/testing/selftests/sgx/test_encl_bootstrap.S | 9 ++++++---
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/sgx/Makefile b/tools/testing/selftests/sgx/Makefile
index 7eb890bdd3f0..8d2ba6adc92b 100644
--- a/tools/testing/selftests/sgx/Makefile
+++ b/tools/testing/selftests/sgx/Makefile
@@ -14,7 +14,7 @@ endif
 INCLUDES := -I$(top_srcdir)/tools/include
 HOST_CFLAGS := -Wall -Werror -g $(INCLUDES) -fPIC
 HOST_LDFLAGS := -z noexecstack -lcrypto
-ENCL_CFLAGS += -Wall -Werror -static -nostdlib -ffreestanding -fPIC \
+ENCL_CFLAGS += -Wall -Werror -static-pie -nostdlib -ffreestanding -fPIE \
 	       -fno-stack-protector -mrdrnd $(INCLUDES)
 ENCL_LDFLAGS := -Wl,-T,test_encl.lds,--build-id=none
 
diff --git a/tools/testing/selftests/sgx/test_encl.lds b/tools/testing/selftests/sgx/test_encl.lds
index a1ec64f7d91f..62d37160f59b 100644
--- a/tools/testing/selftests/sgx/test_encl.lds
+++ b/tools/testing/selftests/sgx/test_encl.lds
@@ -10,6 +10,7 @@ PHDRS
 SECTIONS
 {
 	. = 0;
+        __encl_base = .;
 	.tcs : {
 		*(.tcs*)
 	} : tcs
diff --git a/tools/testing/selftests/sgx/test_encl_bootstrap.S b/tools/testing/selftests/sgx/test_encl_bootstrap.S
index e0ce993d3f2c..28fe5d2ac0af 100644
--- a/tools/testing/selftests/sgx/test_encl_bootstrap.S
+++ b/tools/testing/selftests/sgx/test_encl_bootstrap.S
@@ -42,9 +42,12 @@
 encl_entry:
 	# RBX contains the base address for TCS, which is the first address
 	# inside the enclave for TCS #1 and one page into the enclave for
-	# TCS #2. By adding the value of encl_stack to it, we get
-	# the absolute address for the stack.
-	lea	(encl_stack)(%rbx), %rax
+	# TCS #2. First make it relative by substracting __encl_base and
+	# then add the address of encl_stack to get the address for the stack.
+	lea __encl_base(%rip), %rax
+	sub %rax, %rbx
+	lea encl_stack(%rip), %rax
+	add %rbx, %rax
 	jmp encl_entry_core
 encl_dyn_entry:
 	# Entry point for dynamically created TCS page expected to follow
-- 
2.25.1

[PATCH v5 08/13] 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.

When keeping encl_op_array as a local variable on the stack, gcc without
optimizations generates code that explicitly gets the right function
addresses and stores them to create the array on the stack:

encl_body:
    /* snipped */
    lea    do_encl_op_put_to_buf(%rip), %rax
    mov    %rax, -0x50(%rbp)
    lea    do_encl_op_get_from_buf(%rip), %rax
    mov    %rax,-0x48(%rbp)
    lea    do_encl_op_put_to_addr(%rip), %rax
    /* snipped */

However, gcc -Os or clang generate more efficient code that initializes
encl_op_array by copying a "prepared copy" containing the absolute
addresses of the functions (i.e., relative to the image base starting from
0) generated by the compiler/linker:

encl_body:
    /* snipped */
    lea    prepared_copy(%rip), %rsi
    lea    -0x48(%rsp), %rdi
    mov    $0x10,%ecx
    rep movsl %ds:(%rsi),%es:(%rdi)
    /* snipped */

When building the enclave with -static-pie, the compiler/linker includes
relocation entries for the function symbols in the "prepared copy":

Relocation section '.rela.dyn' at offset 0x4000 contains 12 entries:
  Offset          Info           Type         Symbol
/* snipped; "prepared_copy" starts at 0x6000 */
000000006000  000000000008 R_X86_64_RELATIVE  <do_encl_emodpe>
000000006008  000000000008 R_X86_64_RELATIVE  <do_encl_eaccept>
000000006010  000000000008 R_X86_64_RELATIVE  <do_encl_op_put_to_buf>
000000006018  000000000008 R_X86_64_RELATIVE  <do_encl_op_get_from_buf>
000000006020  000000000008 R_X86_64_RELATIVE  <do_encl_op_put_to_addr>
000000006028  000000000008 R_X86_64_RELATIVE  <do_encl_op_get_from_addr>
000000006030  000000000008 R_X86_64_RELATIVE  <do_encl_op_nop>
000000006038  000000000008 R_X86_64_RELATIVE  <do_encl_init_tcs_page>

Static-pie binaries normally include a glibc "_dl_relocate_static_pie"
routine that will perform these relocations as part of the startup.
However, since the enclave loading process is different and glibc is not
included, we cannot rely on these relocations to be performed. Without
relocations, the code would erroneously jump to the _absolute_ function
address loaded from the local copy.

Thus, declare "encl_op_array" as global and manually relocate the loaded
function-pointer entries relative to the enclave base at runtime. This
generates the following code:

encl_body:
    /* snipped */
    lea    encl_op_array(%rip), %rcx
    lea    __encl_base(%rip), %rax
    add    (%rcx,%rdx,8),%rax
    jmp    *%rax
    ret

Link: https://lore.kernel.org/all/150d8ca8-2c66-60d1-f9fc-8e6279824e94@cs.kuleuven.be/
Link: https://lore.kernel.org/all/5c22de5a-4b3b-1f38-9771-409b4ec7f96d@cs.kuleuven.be/#r
Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be>
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 tools/testing/selftests/sgx/test_encl.c | 50 +++++++++++++++++--------
 1 file changed, 35 insertions(+), 15 deletions(-)

diff --git a/tools/testing/selftests/sgx/test_encl.c b/tools/testing/selftests/sgx/test_encl.c
index ae791df3e5a5..649604c526e7 100644
--- a/tools/testing/selftests/sgx/test_encl.c
+++ b/tools/testing/selftests/sgx/test_encl.c
@@ -121,21 +121,41 @@ 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 and generates position-independent
+ * addressing as __encl_base(%rip), so we can get the actual enclave base
+ * during runtime.
+ */
+extern const uint8_t __attribute__((visibility("hidden"))) __encl_base;
+
+typedef void (*encl_op_t)(void *);
+static const encl_op_t encl_op_array[ENCL_OP_MAX] = {
+	do_encl_op_put_to_buf,
+	do_encl_op_get_from_buf,
+	do_encl_op_put_to_addr,
+	do_encl_op_get_from_addr,
+	do_encl_op_nop,
+	do_encl_eaccept,
+	do_encl_emodpe,
+	do_encl_init_tcs_page,
+};
+
 void encl_body(void *rdi,  void *rsi)
 {
-	const void (*encl_op_array[ENCL_OP_MAX])(void *) = {
-		do_encl_op_put_to_buf,
-		do_encl_op_get_from_buf,
-		do_encl_op_put_to_addr,
-		do_encl_op_get_from_addr,
-		do_encl_op_nop,
-		do_encl_eaccept,
-		do_encl_emodpe,
-		do_encl_init_tcs_page,
-	};
-
-	struct encl_op_header *op = (struct encl_op_header *)rdi;
-
-	if (op->type < ENCL_OP_MAX)
-		(*encl_op_array[op->type])(op);
+	struct encl_op_header *header = (struct encl_op_header *)rdi;
+	encl_op_t op;
+
+	if (header->type >= ENCL_OP_MAX)
+		return;
+
+	/*
+	 * The enclave base address needs to be added, as this call site
+	 * *cannot be* made rip-relative by the compiler, or fixed up by
+	 * any other possible means.
+	 */
+	op = ((uint64_t)&__encl_base) + encl_op_array[header->type];
+
+	(*op)(header);
 }
-- 
2.25.1

Re: [PATCH v5 08/13] selftests/sgx: Handle relocations in test enclave

[Huang, Kai]

On Thu, 2023-08-31 at 15:41 +0200, Jo Van Bulck wrote:
> Static-pie binaries normally include a startup routine to perform any ELF
> relocations from .rela.dyn. Since the enclave loading process is different
> and glibc is not included, do the necessary relocation for encl_op_array
> entries manually at runtime relative to the enclave base to ensure correct
> function pointers.
> 
> When keeping encl_op_array as a local variable on the stack, gcc without
> optimizations generates code that explicitly gets the right function
> addresses and stores them to create the array on the stack:
> 
> encl_body:
>     /* snipped */
>     lea    do_encl_op_put_to_buf(%rip), %rax
>     mov    %rax, -0x50(%rbp)
>     lea    do_encl_op_get_from_buf(%rip), %rax
>     mov    %rax,-0x48(%rbp)
>     lea    do_encl_op_put_to_addr(%rip), %rax
>     /* snipped */
> 
> However, gcc -Os or clang generate more efficient code that initializes
> encl_op_array by copying a "prepared copy" containing the absolute
> addresses of the functions (i.e., relative to the image base starting from
> 0) generated by the compiler/linker:
> 
> encl_body:
>     /* snipped */
>     lea    prepared_copy(%rip), %rsi
>     lea    -0x48(%rsp), %rdi
>     mov    $0x10,%ecx
>     rep movsl %ds:(%rsi),%es:(%rdi)
>     /* snipped */
> 
> When building the enclave with -static-pie, the compiler/linker includes
> relocation entries for the function symbols in the "prepared copy":
> 
> Relocation section '.rela.dyn' at offset 0x4000 contains 12 entries:
>   Offset          Info           Type         Symbol
> /* snipped; "prepared_copy" starts at 0x6000 */
> 000000006000  000000000008 R_X86_64_RELATIVE  <do_encl_emodpe>
> 000000006008  000000000008 R_X86_64_RELATIVE  <do_encl_eaccept>
> 000000006010  000000000008 R_X86_64_RELATIVE  <do_encl_op_put_to_buf>
> 000000006018  000000000008 R_X86_64_RELATIVE  <do_encl_op_get_from_buf>
> 000000006020  000000000008 R_X86_64_RELATIVE  <do_encl_op_put_to_addr>
> 000000006028  000000000008 R_X86_64_RELATIVE  <do_encl_op_get_from_addr>
> 000000006030  000000000008 R_X86_64_RELATIVE  <do_encl_op_nop>
> 000000006038  000000000008 R_X86_64_RELATIVE  <do_encl_init_tcs_page>
> 
> Static-pie binaries normally include a glibc "_dl_relocate_static_pie"
> routine that will perform these relocations as part of the startup.
> However, since the enclave loading process is different and glibc is not
> included, we cannot rely on these relocations to be performed. Without
> relocations, the code would erroneously jump to the _absolute_ function
> address loaded from the local copy.
> 
> Thus, declare "encl_op_array" as global and manually relocate the loaded
> function-pointer entries relative to the enclave base at runtime. This
> generates the following code:
> 
> encl_body:
>     /* snipped */
>     lea    encl_op_array(%rip), %rcx
>     lea    __encl_base(%rip), %rax
>     add    (%rcx,%rdx,8),%rax
>     jmp    *%rax

call *%rax

?

>     ret
> 
> Link: https://lore.kernel.org/all/150d8ca8-2c66-60d1-f9fc-8e6279824e94@cs.kuleuven.be/
> Link: https://lore.kernel.org/all/5c22de5a-4b3b-1f38-9771-409b4ec7f96d@cs.kuleuven.be/#r
> Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be>
> Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

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

Re: [PATCH v5 08/13] selftests/sgx: Handle relocations in test enclave

[Jo Van Bulck]

On 01.09.23 01:13, Huang, Kai wrote:
>> encl_body:
>>      /* snipped */
>>      lea    encl_op_array(%rip), %rcx
>>      lea    __encl_base(%rip), %rax
>>      add    (%rcx,%rdx,8),%rax
>>      jmp    *%rax
> 
> call *%rax
> 
> ?
> 
>>      ret

Good catch, but this is indeed the code as generated with gcc -Os (for 
readability). It seems the compiler choose a JMP over a CALL (as the 
stack is untouched and the callee can immediately return to the caller).

Somehow, the compiler still emits a RET after the JMP (which is not 
supposed to return here) though. I agree this is unnecessarily confusing 
and can simply remove the RET from the commit message.

Best,
Jo

Re: [PATCH v5 08/13] selftests/sgx: Handle relocations in test enclave

[Huang, Kai]

On Fri, 2023-09-01 at 01:26 +0200, Jo Van Bulck wrote:
> On 01.09.23 01:13, Huang, Kai wrote:
> > > encl_body:
> > >      /* snipped */
> > >      lea    encl_op_array(%rip), %rcx
> > >      lea    __encl_base(%rip), %rax
> > >      add    (%rcx,%rdx,8),%rax
> > >      jmp    *%rax
> > 
> > call *%rax
> > 
> > ?
> > 
> > >      ret
> 
> Good catch, but this is indeed the code as generated with gcc -Os (for 
> readability). It seems the compiler choose a JMP over a CALL (as the 
> stack is untouched and the callee can immediately return to the caller).

Ah, OK.

I thought it's a typo, but obviously the compiler is smarter. :-)

> 
> Somehow, the compiler still emits a RET after the JMP (which is not 
> supposed to return here) though. I agree this is unnecessarily confusing 
> and can simply remove the RET from the commit message.

The RET is for encl_body itself I suppose.

> 
> Best,
> Jo

[PATCH v5 09/13] 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>
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 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 62d37160f59b..6ffdfc9fb4cf 100644
--- a/tools/testing/selftests/sgx/test_encl.lds
+++ b/tools/testing/selftests/sgx/test_encl.lds
@@ -35,8 +35,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.25.1

[PATCH v5 10/13] selftests/sgx: Ensure test enclave buffer is entirely preserved

[Jo Van Bulck]

Attach the "used" attribute to instruct the compiler to preserve the static
encl_buffer, even if it appears it is not entirely referenced in the enclave
code, as expected by the external tests manipulating page permissions.

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>
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
Acked-by: Kai Huang <kai.huang@intel.com>
---
 tools/testing/selftests/sgx/defines.h   | 1 +
 tools/testing/selftests/sgx/test_encl.c | 9 +++++----
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/sgx/defines.h b/tools/testing/selftests/sgx/defines.h
index d8587c971941..b8f482667ce1 100644
--- a/tools/testing/selftests/sgx/defines.h
+++ b/tools/testing/selftests/sgx/defines.h
@@ -13,6 +13,7 @@
 
 #define __aligned(x) __attribute__((__aligned__(x)))
 #define __packed __attribute__((packed))
+#define __used __attribute__((used))
 
 #include "../../../../arch/x86/include/asm/sgx.h"
 #include "../../../../arch/x86/include/asm/enclu.h"
diff --git a/tools/testing/selftests/sgx/test_encl.c b/tools/testing/selftests/sgx/test_encl.c
index 649604c526e7..7465f121fb74 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. Even if not used internally the second page is needed by external
+ * test manipulating page permissions, so mark encl_buffer as "used" to make
+ * sure it is entirely preserved by the compiler.
  */
-static uint8_t encl_buffer[8192] = { 1 };
+static uint8_t __used encl_buffer[8192] = { 1 };
 
 enum sgx_enclu_function {
 	EACCEPT = 0x5,
-- 
2.25.1

[PATCH v5 11/13] selftests/sgx: Ensure expected location of test enclave buffer

[Jo Van Bulck]

The external tests manipulating page permissions expect encl_buffer to be
placed at the start of the test enclave's .data section. As this is not
guaranteed per the C standard, explicitly 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.

Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be>
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
Acked-by: Kai Huang <kai.huang@intel.com>
---
 tools/testing/selftests/sgx/defines.h     | 1 +
 tools/testing/selftests/sgx/test_encl.c   | 8 ++++----
 tools/testing/selftests/sgx/test_encl.lds | 1 +
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/sgx/defines.h b/tools/testing/selftests/sgx/defines.h
index b8f482667ce1..402f8787a71c 100644
--- a/tools/testing/selftests/sgx/defines.h
+++ b/tools/testing/selftests/sgx/defines.h
@@ -14,6 +14,7 @@
 #define __aligned(x) __attribute__((__aligned__(x)))
 #define __packed __attribute__((packed))
 #define __used __attribute__((used))
+#define __section(x)__attribute__((__section__(x)))
 
 #include "../../../../arch/x86/include/asm/sgx.h"
 #include "../../../../arch/x86/include/asm/enclu.h"
diff --git a/tools/testing/selftests/sgx/test_encl.c b/tools/testing/selftests/sgx/test_encl.c
index 7465f121fb74..2c4d709cce2d 100644
--- a/tools/testing/selftests/sgx/test_encl.c
+++ b/tools/testing/selftests/sgx/test_encl.c
@@ -6,11 +6,11 @@
 
 /*
  * Data buffer spanning two pages that will be placed first in the .data
- * segment. Even if not used internally the second page is needed by external
- * test manipulating page permissions, so mark encl_buffer as "used" to make
- * sure it is entirely preserved by the compiler.
+ * segment via the linker script. Even if not used internally the second page
+ * is needed by external test manipulating page permissions, so mark
+ * encl_buffer as "used" to make sure it is entirely preserved by the compiler.
  */
-static uint8_t __used encl_buffer[8192] = { 1 };
+static uint8_t __used __section(".data.encl_buffer") encl_buffer[8192] = { 1 };
 
 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 6ffdfc9fb4cf..333a3e78fdc9 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.25.1

[PATCH v5 12/13] selftests/sgx: Discard unsupported ELF sections

[Jo Van Bulck]

Building the test enclave with -static-pie may produce a dynamic symbol
table, but this is not supported for enclaves and any relocations need to
happen manually (e.g., as for "encl_op_array"). Thus, opportunistically
discard ".dyn*" and ".gnu.hash" which the enclave loader cannot handle.

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

diff --git a/tools/testing/selftests/sgx/test_encl.lds b/tools/testing/selftests/sgx/test_encl.lds
index 333a3e78fdc9..ffe851a1cac4 100644
--- a/tools/testing/selftests/sgx/test_encl.lds
+++ b/tools/testing/selftests/sgx/test_encl.lds
@@ -33,6 +33,8 @@ SECTIONS
 		*(.note*)
 		*(.debug*)
 		*(.eh_frame*)
+		*(.dyn*)
+		*(.gnu.hash)
 	}
 }
 
-- 
2.25.1

Re: [PATCH v5 12/13] selftests/sgx: Discard unsupported ELF sections

[Jarkko Sakkinen]

On Thu Aug 31, 2023 at 4:41 PM EEST, Jo Van Bulck wrote:
> Building the test enclave with -static-pie may produce a dynamic symbol
> table, but this is not supported for enclaves and any relocations need to
> happen manually (e.g., as for "encl_op_array"). Thus, opportunistically
> discard ".dyn*" and ".gnu.hash" which the enclave loader cannot handle.
>
> Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be>
> ---
>  tools/testing/selftests/sgx/test_encl.lds | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/tools/testing/selftests/sgx/test_encl.lds b/tools/testing/selftests/sgx/test_encl.lds
> index 333a3e78fdc9..ffe851a1cac4 100644
> --- a/tools/testing/selftests/sgx/test_encl.lds
> +++ b/tools/testing/selftests/sgx/test_encl.lds
> @@ -33,6 +33,8 @@ SECTIONS
>  		*(.note*)
>  		*(.debug*)
>  		*(.eh_frame*)
> +		*(.dyn*)
> +		*(.gnu.hash)
>  	}
>  }
>  
> -- 
> 2.25.1

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

Also, for any patches (not that many) I've done post-Intel: I hereby
give permission to license that code with MIT or compatible license
as the upper bound.

For any code I've produced while at Intel the copyright is owned by
Intel so perhaps Dave could help with that part?

I'm happy if this code ends up to wider use than just kselftest for
sure.

BR, Jarkko

[PATCH v5 13/13] selftests/sgx: Remove incomplete ABI sanitization code in test enclave

[Jo Van Bulck]

As the selftest enclave is *not* intended for production, simplify the
code by not initializing CPU configuration registers as expected by the
ABI on enclave entry or cleansing caller-save registers on enclave exit.

Link: https://lore.kernel.org/all/da0cfb1e-e347-f7f2-ac72-aec0ee0d867d@intel.com/
Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be>
---
 .../testing/selftests/sgx/test_encl_bootstrap.S  | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/tools/testing/selftests/sgx/test_encl_bootstrap.S b/tools/testing/selftests/sgx/test_encl_bootstrap.S
index 28fe5d2ac0af..d8c4ac94e032 100644
--- a/tools/testing/selftests/sgx/test_encl_bootstrap.S
+++ b/tools/testing/selftests/sgx/test_encl_bootstrap.S
@@ -59,21 +59,11 @@ encl_entry_core:
 
 	push	%rcx # push the address after EENTER
 
+	# NOTE: as the selftest enclave is *not* intended for production,
+	# simplify the code by not initializing ABI registers on entry or
+	# cleansing caller-save registers on exit.
 	call	encl_body
 
-	/* Clear volatile GPRs, except RAX (EEXIT function). */
-	xor     %rcx, %rcx
-	xor     %rdx, %rdx
-	xor     %rdi, %rdi
-	xor     %rsi, %rsi
-	xor     %r8, %r8
-	xor     %r9, %r9
-	xor     %r10, %r10
-	xor     %r11, %r11
-
-	# Reset status flags.
-	add     %rdx, %rdx # OF = SF = AF = CF = 0; ZF = PF = 1
-
 	# Prepare EEXIT target by popping the address of the instruction after
 	# EENTER to RBX.
 	pop	%rbx
-- 
2.25.1

Re: [PATCH v5 13/13] selftests/sgx: Remove incomplete ABI sanitization code in test enclave

[Jarkko Sakkinen]

On Thu Aug 31, 2023 at 4:41 PM EEST, Jo Van Bulck wrote:
> As the selftest enclave is *not* intended for production, simplify the
> code by not initializing CPU configuration registers as expected by the
> ABI on enclave entry or cleansing caller-save registers on enclave exit.
>
> Link: https://lore.kernel.org/all/da0cfb1e-e347-f7f2-ac72-aec0ee0d867d@intel.com/
> Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be>
> ---
>  .../testing/selftests/sgx/test_encl_bootstrap.S  | 16 +++-------------
>  1 file changed, 3 insertions(+), 13 deletions(-)
>
> diff --git a/tools/testing/selftests/sgx/test_encl_bootstrap.S b/tools/testing/selftests/sgx/test_encl_bootstrap.S
> index 28fe5d2ac0af..d8c4ac94e032 100644
> --- a/tools/testing/selftests/sgx/test_encl_bootstrap.S
> +++ b/tools/testing/selftests/sgx/test_encl_bootstrap.S
> @@ -59,21 +59,11 @@ encl_entry_core:
>  
>  	push	%rcx # push the address after EENTER
>  
> +	# NOTE: as the selftest enclave is *not* intended for production,
> +	# simplify the code by not initializing ABI registers on entry or
> +	# cleansing caller-save registers on exit.
>  	call	encl_body
>  
> -	/* Clear volatile GPRs, except RAX (EEXIT function). */
> -	xor     %rcx, %rcx
> -	xor     %rdx, %rdx
> -	xor     %rdi, %rdi
> -	xor     %rsi, %rsi
> -	xor     %r8, %r8
> -	xor     %r9, %r9
> -	xor     %r10, %r10
> -	xor     %r11, %r11
> -
> -	# Reset status flags.
> -	add     %rdx, %rdx # OF = SF = AF = CF = 0; ZF = PF = 1
> -
>  	# Prepare EEXIT target by popping the address of the instruction after
>  	# EENTER to RBX.
>  	pop	%rbx
> -- 
> 2.25.1

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

BR, Jarkko