[PATCH v3 0/9] selftests/sgx: Fix compilation errors

[PATCH v3 0/9] selftests/sgx: Fix compilation errors

[Jo Van Bulck]

Hi,

This is the third 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
---------

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 (9):
  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 test enclave buffer is entirely preserved
  selftests/sgx: Ensure expected location of test enclave buffer
  selftests/sgx: Separate linker options
  selftests/sgx: Specify freestanding environment for enclave
    compilation

 tools/testing/selftests/sgx/Makefile          | 14 ++--
 tools/testing/selftests/sgx/defines.h         |  2 +
 tools/testing/selftests/sgx/sigstruct.c       |  5 +-
 tools/testing/selftests/sgx/test_encl.c       | 66 ++++++++++++-------
 tools/testing/selftests/sgx/test_encl.lds     | 10 +--
 .../selftests/sgx/test_encl_bootstrap.S       | 12 ++--
 6 files changed, 68 insertions(+), 41 deletions(-)

-- 
2.25.1

[PATCH v3 1/9] 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 a07896a46..d73b29bec 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 v3 2/9] 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>
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 tools/testing/selftests/sgx/Makefile              |  2 +-
 tools/testing/selftests/sgx/test_encl.lds         |  1 +
 tools/testing/selftests/sgx/test_encl_bootstrap.S | 12 ++++++------
 3 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/sgx/Makefile b/tools/testing/selftests/sgx/Makefile
index 50aab6b57..1d6315a2e 100644
--- a/tools/testing/selftests/sgx/Makefile
+++ b/tools/testing/selftests/sgx/Makefile
@@ -13,7 +13,7 @@ endif
 
 INCLUDES := -I$(top_srcdir)/tools/include
 HOST_CFLAGS := -Wall -Werror -g $(INCLUDES) -fPIC -z noexecstack
-ENCL_CFLAGS := -Wall -Werror -static -nostdlib -nostartfiles -fPIC \
+ENCL_CFLAGS := -Wall -Werror -static-pie -nostdlib -nostartfiles -fPIE \
 	       -fno-stack-protector -mrdrnd $(INCLUDES)
 
 TEST_CUSTOM_PROGS := $(OUTPUT)/test_sgx
diff --git a/tools/testing/selftests/sgx/test_encl.lds b/tools/testing/selftests/sgx/test_encl.lds
index a1ec64f7d..62d37160f 100644
--- a/tools/testing/selftests/sgx/test_encl.lds
+++ b/tools/testing/selftests/sgx/test_encl.lds
@@ -10,6 +10,7 @@ PHDRS
 SECTIONS
 {
 	. = 0;
+        __encl_base = .;
 	.tcs : {
 		*(.tcs*)
 	} : tcs
diff --git a/tools/testing/selftests/sgx/test_encl_bootstrap.S b/tools/testing/selftests/sgx/test_encl_bootstrap.S
index 03ae0f57e..28fe5d2ac 100644
--- a/tools/testing/selftests/sgx/test_encl_bootstrap.S
+++ b/tools/testing/selftests/sgx/test_encl_bootstrap.S
@@ -42,9 +42,12 @@
 encl_entry:
 	# RBX contains the base address for TCS, which is the first address
 	# inside the enclave for TCS #1 and one page into the enclave for
-	# TCS #2. By adding the value of encl_stack to it, we get
-	# the absolute address for the stack.
-	lea	(encl_stack)(%rbx), %rax
+	# TCS #2. First make it relative by substracting __encl_base and
+	# then add the address of encl_stack to get the address for the stack.
+	lea __encl_base(%rip), %rax
+	sub %rax, %rbx
+	lea encl_stack(%rip), %rax
+	add %rbx, %rax
 	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.25.1

[PATCH v3 3/9] 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   | 49 ++++++++++++++++-------
 tools/testing/selftests/sgx/test_encl.lds |  2 +
 2 files changed, 36 insertions(+), 15 deletions(-)

diff --git a/tools/testing/selftests/sgx/test_encl.c b/tools/testing/selftests/sgx/test_encl.c
index c0d639729..7633fb7cb 100644
--- a/tools/testing/selftests/sgx/test_encl.c
+++ b/tools/testing/selftests/sgx/test_encl.c
@@ -119,21 +119,40 @@ static void do_encl_op_nop(void *_op)
 
 }
 
+/*
+ * Symbol placed at the start of the enclave image by the linker script.
+ * Declare this extern symbol with visibility "hidden" to ensure the
+ * compiler does not access it through the GOT.
+ */
+extern const uint8_t __attribute__((visibility("hidden"))) __encl_base;
+static const uint64_t encl_base = (uint64_t)&__encl_base;
+
+typedef void (*encl_op_t)(void *);
+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;
+
+	/*
+	 * "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 62d37160f..b86c86060 100644
--- a/tools/testing/selftests/sgx/test_encl.lds
+++ b/tools/testing/selftests/sgx/test_encl.lds
@@ -32,6 +32,8 @@ SECTIONS
 		*(.note*)
 		*(.debug*)
 		*(.eh_frame*)
+		*(.dyn*)
+		*(.gnu.hash)
 	}
 }
 
-- 
2.25.1

[PATCH v3 4/9] 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 b86c86060..13144b045 100644
--- a/tools/testing/selftests/sgx/test_encl.lds
+++ b/tools/testing/selftests/sgx/test_encl.lds
@@ -37,8 +37,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 v3 5/9] 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>
Reviewed-by: Kai Huang <kai.huang@intel.com>
---
 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 7633fb7cb..b09550cb3 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 v3 6/9] 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>
---
 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 d8587c971..b8f482667 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 b09550cb3..c7bcbc85b 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 v3 7/9] 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>
---
 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 b8f482667..402f8787a 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 c7bcbc85b..151600353 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 13144b045..ffe851a1c 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 v3 8/9] 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 1d6315a2e..2de970f72 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.25.1

[PATCH v3 9/9] 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 2de970f72..19a07e890 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.25.1

Re: [PATCH v3 6/9] selftests/sgx: Ensure test enclave buffer is entirely preserved

[Huang, Kai]

On Sat, 2023-08-19 at 11:43 +0200, Jo Van Bulck wrote:
> 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>
> ---
>  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 d8587c971..b8f482667 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 b09550cb3..c7bcbc85b 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,

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

Re: [PATCH v3 7/9] selftests/sgx: Ensure expected location of test enclave buffer

[Huang, Kai]

On Sat, 2023-08-19 at 11:43 +0200, Jo Van Bulck wrote:
> 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>
> ---
>  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 b8f482667..402f8787a 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 c7bcbc85b..151600353 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 13144b045..ffe851a1c 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
>  

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

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

[Huang, Kai]

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

IMHO, this patch is also mixing different things together.

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

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

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

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

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

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

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

[Huang, Kai]

On Sat, 2023-08-19 at 11:43 +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.
> 
> Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be>
> ---
>  tools/testing/selftests/sgx/test_encl.c   | 49 ++++++++++++++++-------
>  tools/testing/selftests/sgx/test_encl.lds |  2 +
>  2 files changed, 36 insertions(+), 15 deletions(-)
> 
> diff --git a/tools/testing/selftests/sgx/test_encl.c b/tools/testing/selftests/sgx/test_encl.c
> index c0d639729..7633fb7cb 100644
> --- a/tools/testing/selftests/sgx/test_encl.c
> +++ b/tools/testing/selftests/sgx/test_encl.c
> @@ -119,21 +119,40 @@ static void do_encl_op_nop(void *_op)
>  
>  }
>  
> +/*
> + * Symbol placed at the start of the enclave image by the linker script.
> + * Declare this extern symbol with visibility "hidden" to ensure the
> + * compiler does not access it through the GOT.
> + */
> +extern const uint8_t __attribute__((visibility("hidden"))) __encl_base;
> +static const uint64_t encl_base = (uint64_t)&__encl_base;

I had hard time to understand this.  The __encl_base is a symbol which is a
fixed value set by the compiler/linker.  encl_base has the real storage in the
.data section, but the value is also build-time fixed.  IIUC we need some code
to explicitly override it, but I don't see where it's done.  Perhaps I missed
something?

> +
> +typedef void (*encl_op_t)(void *);
> +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,
> +};

Any reason it cannot be 'static'?

> +
>  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;
> +
> +	/*
> +	 * "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 62d37160f..b86c86060 100644
> --- a/tools/testing/selftests/sgx/test_encl.lds
> +++ b/tools/testing/selftests/sgx/test_encl.lds
> @@ -32,6 +32,8 @@ SECTIONS
>  		*(.note*)
>  		*(.debug*)
>  		*(.eh_frame*)
> +		*(.dyn*)
> +		*(.gnu.hash)

This looks can be in a separate patch, because it's not directly related to what
you are trying to fix.  

But I don't want to make things unnecessarily complicated for selftests, so fine
to me if you still want to keep it.  But if you do, perhaps you can add some
justification to the changelog saying something like: opportunistically discard
".dyn*" and ".gnu.hash" which the enclave loader cannot handle.  Anyway, still
better to make a separate patch for such purpose IMHO.

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

[Jarkko Sakkinen]

On Sat Aug 19, 2023 at 12:43 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>
> ---
>  tools/testing/selftests/sgx/test_encl.c   | 49 ++++++++++++++++-------
>  tools/testing/selftests/sgx/test_encl.lds |  2 +
>  2 files changed, 36 insertions(+), 15 deletions(-)
>
> diff --git a/tools/testing/selftests/sgx/test_encl.c b/tools/testing/selftests/sgx/test_encl.c
> index c0d639729..7633fb7cb 100644
> --- a/tools/testing/selftests/sgx/test_encl.c
> +++ b/tools/testing/selftests/sgx/test_encl.c
> @@ -119,21 +119,40 @@ static void do_encl_op_nop(void *_op)
>  
>  }
>  
> +/*
> + * Symbol placed at the start of the enclave image by the linker script.
> + * Declare this extern symbol with visibility "hidden" to ensure the
> + * compiler does not access it through the GOT.
> + */
> +extern const uint8_t __attribute__((visibility("hidden"))) __encl_base;
> +static const uint64_t encl_base = (uint64_t)&__encl_base;
> +
> +typedef void (*encl_op_t)(void *);
> +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;
> +
> +	/*
> +	 * "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 62d37160f..b86c86060 100644
> --- a/tools/testing/selftests/sgx/test_encl.lds
> +++ b/tools/testing/selftests/sgx/test_encl.lds
> @@ -32,6 +32,8 @@ SECTIONS
>  		*(.note*)
>  		*(.debug*)
>  		*(.eh_frame*)
> +		*(.dyn*)
> +		*(.gnu.hash)
>  	}
>  }
>  
> -- 
> 2.25.1

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

BR, Jarkko

Re: [PATCH v3 4/9] selftests/sgx: Fix linker script asserts

[Jarkko Sakkinen]

On Sat Aug 19, 2023 at 12:43 PM EEST, Jo Van Bulck wrote:
> 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 b86c86060..13144b045 100644
> --- a/tools/testing/selftests/sgx/test_encl.lds
> +++ b/tools/testing/selftests/sgx/test_encl.lds
> @@ -37,8 +37,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

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

BR, Jarkko

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

[Jarkko Sakkinen]

On Sat Aug 19, 2023 at 12:43 PM EEST, 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>
> Reviewed-by: Kai Huang <kai.huang@intel.com>
> ---
>  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 7633fb7cb..b09550cb3 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

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

BR, Jarkko

Re: [PATCH v3 6/9] selftests/sgx: Ensure test enclave buffer is entirely preserved

[Jarkko Sakkinen]

On Sat Aug 19, 2023 at 12:43 PM EEST, Jo Van Bulck wrote:
> 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>
> ---
>  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 d8587c971..b8f482667 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 b09550cb3..c7bcbc85b 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

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

BR, Jarkko

Re: [PATCH v3 7/9] selftests/sgx: Ensure expected location of test enclave buffer

[Jarkko Sakkinen]

On Sat Aug 19, 2023 at 12:43 PM EEST, Jo Van Bulck wrote:
> 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>
> ---
>  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 b8f482667..402f8787a 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 c7bcbc85b..151600353 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 13144b045..ffe851a1c 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

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

BR, Jarkko

Re: [PATCH v3 8/9] selftests/sgx: Separate linker options

[Jarkko Sakkinen]

On Sat Aug 19, 2023 at 12:43 PM EEST, Jo Van Bulck wrote:
> 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 1d6315a2e..2de970f72 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.25.1

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

BR, Jarkko

Re: [PATCH v3 9/9] selftests/sgx: Specify freestanding environment for enclave compilation

[Jarkko Sakkinen]

On Sat Aug 19, 2023 at 12:43 PM EEST, 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.
>
> 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 2de970f72..19a07e890 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.25.1

Do you still need nostdfiles and nostartfiles with freestanding?

BR, Jarkko

Re: [PATCH v3 0/9] selftests/sgx: Fix compilation errors

[Jarkko Sakkinen]

On Sat Aug 19, 2023 at 12:43 PM EEST, Jo Van Bulck wrote:
> Hi,
>
> This is the third 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
> ---------
>
> 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 (9):
>   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 test enclave buffer is entirely preserved
>   selftests/sgx: Ensure expected location of test enclave buffer
>   selftests/sgx: Separate linker options
>   selftests/sgx: Specify freestanding environment for enclave
>     compilation
>
>  tools/testing/selftests/sgx/Makefile          | 14 ++--
>  tools/testing/selftests/sgx/defines.h         |  2 +
>  tools/testing/selftests/sgx/sigstruct.c       |  5 +-
>  tools/testing/selftests/sgx/test_encl.c       | 66 ++++++++++++-------
>  tools/testing/selftests/sgx/test_encl.lds     | 10 +--
>  .../selftests/sgx/test_encl_bootstrap.S       | 12 ++--
>  6 files changed, 68 insertions(+), 41 deletions(-)
>
> -- 
> 2.25.1

Looks "almost there" to me. Just had one query about freestanding.

BR, Jarkko

Re: [PATCH v3 9/9] selftests/sgx: Specify freestanding environment for enclave compilation

[Jo Van Bulck]

On 22.08.23 12:14, Jarkko Sakkinen wrote:
> Do you still need nostdfiles and nostartfiles with freestanding?

Thanks, good question. I tested that compiling with only -ffreestanding 
yields:

/* snipped */
/usr/local/bin/ld: 
/usr/lib/gcc/x86_64-linux-gnu/9/../../../x86_64-linux-gnu/rcrt1.o: in 
function `_start':
(.text+0x24): undefined reference to `main'
/* snipped */

So we definitely still need -nostartfiles to prevent the compiler/linker 
from introducing the standard system startup functions. However, in my 
understanding, -nostdlib (which is what I assume you mean with 
nostdfiles) already implies the individual options -nodefaultlibs and 
-nostartfiles.

Thus, we definitely still need -nostartfiles and I'm not 100% sure we 
don't need -nostdlib (though it compiles fine for me with only 
-nostartfiles). Gcc only specifies:

-ffreestanding
Assert that compilation targets a freestanding environment.  This 
implies -fno-builtin. A freestanding environment is one in which the 
standard library may not exist, and program startup may not necessarily 
be at "main".

Bottom line: I suggest to keep -nostdlib to be sure and remove 
-nostartfiles (as it is redundant). I'll include this in the next  patch 
iteration.

Best,
Jo

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

[Jo Van Bulck]

On 22.08.23 02:26, Huang, Kai wrote:

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

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

I understand the confusion, but the reason I included this small change 
already in this commit is to make sure the commit compiles standalone. 
That is, when building the original assembly statement "lea 
(encl_stack)(%rbx), %rax" 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

The problem is that only RIP-relative addressing is legit for local 
symbols, ie "encl_stack(%rip)". Hence, __encl_base is already needed 
here to calculate the stack address in the updated asm sequence in this 
patch.

Hope this helps clarifying!

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

Thanks, the purpose indeed was merely to remove redundant code that is 
not needed. I see that it would be better to include this in a separate 
patch, so I'll update this in the next patch revision.

FWIW: if this is okay, while I'm on it, I'll also take a shot at 
removing remaining (unnecessary) assembly register cleansing code to 
make it more obvious that the test enclave is *not* exemplary secure, as 
per our earlier discussions. Ie in response to Dave's earlier comments 
that "The only patches I want for the kernel are to make the test 
enclave more *obviously* insecure." [1].

Best,
Jo

[1] 
https://lore.kernel.org/all/da0cfb1e-e347-f7f2-ac72-aec0ee0d867d@intel.com/

Re: [PATCH v3 9/9] selftests/sgx: Specify freestanding environment for enclave compilation

[Jarkko Sakkinen]

On Wed Aug 23, 2023 at 3:57 PM EEST, Jo Van Bulck wrote:
> On 22.08.23 12:14, Jarkko Sakkinen wrote:
> > Do you still need nostdfiles and nostartfiles with freestanding?
>
> Thanks, good question. I tested that compiling with only -ffreestanding 
> yields:
>
> /* snipped */
> /usr/local/bin/ld: 
> /usr/lib/gcc/x86_64-linux-gnu/9/../../../x86_64-linux-gnu/rcrt1.o: in 
> function `_start':
> (.text+0x24): undefined reference to `main'
> /* snipped */
>
> So we definitely still need -nostartfiles to prevent the compiler/linker 
> from introducing the standard system startup functions. However, in my 
> understanding, -nostdlib (which is what I assume you mean with 
> nostdfiles) already implies the individual options -nodefaultlibs and 
> -nostartfiles.
>
> Thus, we definitely still need -nostartfiles and I'm not 100% sure we 
> don't need -nostdlib (though it compiles fine for me with only 
> -nostartfiles). Gcc only specifies:
>
> -ffreestanding
> Assert that compilation targets a freestanding environment.  This 
> implies -fno-builtin. A freestanding environment is one in which the 
> standard library may not exist, and program startup may not necessarily 
> be at "main".
>
> Bottom line: I suggest to keep -nostdlib to be sure and remove 
> -nostartfiles (as it is redundant). I'll include this in the next  patch 
> iteration.
>
> Best,
> Jo

OK, cool, sounds like plan.

BR, Jarkko

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

[Jo Van Bulck]

On 22.08.23 03:11, Huang, Kai wrote:>> +/*
>> + * Symbol placed at the start of the enclave image by the linker script.
>> + * Declare this extern symbol with visibility "hidden" to ensure the
>> + * compiler does not access it through the GOT.
>> + */
>> +extern const uint8_t __attribute__((visibility("hidden"))) __encl_base;
>> +static const uint64_t encl_base = (uint64_t)&__encl_base;
> 
> I had hard time to understand this.  The __encl_base is a symbol which is a
> fixed value set by the compiler/linker.  encl_base has the real storage in the
> .data section, but the value is also build-time fixed.  IIUC we need some code
> to explicitly override it, but I don't see where it's done.  Perhaps I missed
> something?

Thank you for catching this. Such initialization would indeed have to be 
explicitly overridden at runtime and I somehow overlooked this (it seems 
I left the line to actually run the tests commented out after 
compilation in my test script for all versions; this is now fixed). 
Apologies for the confusion, my bad! I've reverted this back to an 
explicit (uit64_t)&__encl_base cast in the next patch iteration to avoid 
such confusion.

>> +
>> +typedef void (*encl_op_t)(void *);
>> +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,
>> +};
> 
> Any reason it cannot be 'static'?

I tested static indeed also works and will include this in the next 
iteration.

>> diff --git a/tools/testing/selftests/sgx/test_encl.lds b/tools/testing/selftests/sgx/test_encl.lds
>> index 62d37160f..b86c86060 100644
>> --- a/tools/testing/selftests/sgx/test_encl.lds
>> +++ b/tools/testing/selftests/sgx/test_encl.lds
>> @@ -32,6 +32,8 @@ SECTIONS
>>   		*(.note*)
>>   		*(.debug*)
>>   		*(.eh_frame*)
>> +		*(.dyn*)
>> +		*(.gnu.hash)
> 
> This looks can be in a separate patch, because it's not directly related to what
> you are trying to fix.
> 
> But I don't want to make things unnecessarily complicated for selftests, so fine
> to me if you still want to keep it.  But if you do, perhaps you can add some
> justification to the changelog saying something like: opportunistically discard
> ".dyn*" and ".gnu.hash" which the enclave loader cannot handle.  Anyway, still
> better to make a separate patch for such purpose IMHO.

Thanks, splitting this off in a separate commit for the next iteration.

Best,
Jo