Re: [PATCH v7 00/13] selftests/sgx: Fix compilation errors
On 08.11.23 21:46, Dave Hansen wrote: Yes, you've missed something. For your reading pleasure: https://www.kernel.org/doc/html/latest/process/2.Process.html?highlight=merge%20window https://www.kernel.org/doc/html/latest/process/maintainer-tip.html My bad, thank you for pointing out the links! I honestly didn't even think about applying this until Jarkko said something on 23rd. By that point, it was far too late for it to get sorted out for 6.7. So, that puts it in the next merge window. Specifically: The release candidate -rc1 is the starting point for new patches to be applied which are targeted for the next merge window. So wait for the next -rc1, and you'll hopefully see your series get merged. I see, makes sense! Best, Jo
Re: [PATCH v7 00/13] selftests/sgx: Fix compilation errors
On 23.10.23 23:32, Jarkko Sakkinen wrote: On Fri Oct 13, 2023 at 2:45 PM EEST, Jo Van Bulck wrote: On 10.10.23 11:44, Jarkko Sakkinen wrote: Folks (sorry for top posting): I've now taken my old NUC7 out of the dust and tested the series :-) Tested-by: Jarkko Sakkinen Thanks for testing this Jarkko! Not sure on next steps, do you want me to re-post the series with the Tested-by tag for all commits or will you add that? Let me know if something from my side is needed. Dave, can you pick these patches to the x86 tree with my tested-by added? Sorry for latency. It is flu season in Finland and I've been functional varying last week because of that. Just a kind follow-up: from what I can see, this series has not been merged into the x86/sgx branch of tip yet (assuming that's where it should go next)? Apologies if I've overlooked anything, and please let me know if there's something on my end that can help! Best, Jo
Re: [PATCH v7 00/13] selftests/sgx: Fix compilation errors
On 10.10.23 14:11, Jarkko Sakkinen wrote: Dave, since there was already sort of talk about detaching this code from kernel tree so that Jo could work on "pure C" runtime would it make sense to dual-license this first in the kernel tree? E.g. Jo could send a patch once this is merged with a new SPDX platter and we can then ack that? Just a proposal, with the emphasis on minimal amount of work required from each party. Also this would help with possible (and likely) bug fixes, i.e. minimal friction on fixing the same bug. Later on of course, we can consider adding "libsgx-dev" as depedency similarly as today there's a few dependencies like libelf-dev. I'm open for alternative proposals, just throwing something that came up mind. Pitching in here: from my side, I'd also be fine to develop this bare-sgx "pure C" runtime under GPLv2 as is. FWIW, I'd be mostly interested in and see most immediate use cases for such a runtime in research purposes (e.g., low-level benchmarking; rapid prototyping attacks/defenses; etc) and a copyleft license would be a good fit there IMHO. This is not to say that I'm principally opposed to a more permissive (dual) license, especially if there would be a good use case for that. But it seems to me that it may be non-trivial to build on the existing code base and re-license that, whereas GPLv2 would allow to fork immediately and also have any overlapping bug fixes seamlessly merged back upstream as you pointed out. Also open to hearing alternative proposals of course! Best, Jo
Re: [PATCH v7 00/13] selftests/sgx: Fix compilation errors
On 10.10.23 11:44, Jarkko Sakkinen wrote: Folks (sorry for top posting): I've now taken my old NUC7 out of the dust and tested the series :-) Tested-by: Jarkko Sakkinen Thanks for testing this Jarkko! Not sure on next steps, do you want me to re-post the series with the Tested-by tag for all commits or will you add that? Let me know if something from my side is needed. Off-topic: I wish both Intel and AMD straighten up and deliver some "home friendly" development hardware for the latest stuff. Just my stance but the biggest quality risk I see in both TDX and SNP is that the patches are made by an enterprise and reviewed (properly) *only* by other huge enterprises. Yes, I totally agree on this. It is really unfortunate that things like SGX are not (anymore) available on home consumer hardware and you have to buy expensive servers for this, which also change every new CPU generation. Having some kind of "developer boards" like is more the case in embedded systems would be a great and very welcome evolution, if only it were to happen.. I skim status of both from time to time but yeah not much attachment or motivation to do more than that as you either need a cloud access or partnership with Intel or AMD. "Indie" style seems to be disliked these days... You can extrapolate from this that there must be a bunch of maintainers around the Linux kernel that feel the same. Not saying that particularly my contribution would be that important. Sort of even if let's say Intel would provide me a partner access I might not be that interested because I prefer my own (physical) computers. I also understand this and share the concern. FWIW for some things (e.g., uarch attack research) remote access does also not really hold up to bare-metal access IMO. Best, Jo
Re: [PATCH v7 00/13] selftests/sgx: Fix compilation errors
Thank you, Kai! I'm not familiar with any next steps to get this merged upstream, but atm all commits in this series have been reviewed by at least Jarkko. Let me know if anything further is needed from my side! Best, Jo On 05.10.23 23:25, Huang, Kai wrote: Hi Jo, Just FYI I won't review the rest patches in this series. One of the reasons is I am not that familiar with the rest. Jarkko has reviewed anyway :-). On Thu, 2023-10-05 at 17:38 +0200, Jo Van Bulck wrote: Hi, This patch series ensures that all SGX selftests succeed when compiling with optimizations (as tested with -O{0,1,2,3,s} for both gcc 11.3.0 and clang 14.0.0). The aim of the patches is to avoid reliance on undefined, compiler-specific behavior that can make the test results fragile. As far as I see, all commits in this series now have an explicit reviewed-by tag, so hopefully this can get merged upstream? Please let me know if any concerns remain and I'd happily address them. 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 - v7 - Add reviewed-by tag (Jarkko) v6 - Collect final ack/reviewed-by tags (Jarkko, Kai) 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(-)
[PATCH v7 13/13] selftests/sgx: Remove incomplete ABI sanitization code in test enclave
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-aec0ee0d8...@intel.com/ Signed-off-by: Jo Van Bulck Reviewed-by: Jarkko Sakkinen --- .../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. callencl_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
[PATCH v7 10/13] selftests/sgx: Ensure test enclave buffer is entirely preserved
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-a18060f66...@cs.kuleuven.be/ Signed-off-by: Jo Van Bulck Reviewed-by: Jarkko Sakkinen Acked-by: Kai Huang --- 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 v7 09/13] selftests/sgx: Fix linker script asserts
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 Reviewed-by: Jarkko Sakkinen --- 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 v7 07/13] selftests/sgx: Produce static-pie executable for test enclave
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-050d722c6...@cs.kuleuven.be/ Signed-off-by: Jo Van Bulck Reviewed-by: Jarkko Sakkinen Acked-by: Kai Huang --- 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 v7 06/13] selftests/sgx: Remove redundant enclave base address save/restore
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 Reviewed-by: Jarkko Sakkinen Acked-by: Kai Huang --- 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 callencl_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 v7 04/13] selftests/sgx: Separate linker options
Fixes "'linker' input unused [-Wunused-command-line-argument]" errors when compiling with clang. Signed-off-by: Jo Van Bulck Reviewed-by: Jarkko Sakkinen --- 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 v7 02/13] selftests/sgx: Fix uninitialized pointer dereferences in encl_get_entry
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 Reviewed-by: Jarkko Sakkinen --- 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 = [i]; -- 2.25.1
[PATCH v7 08/13] selftests/sgx: Handle relocations in test enclave
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 */ leado_encl_op_put_to_buf(%rip), %rax mov%rax, -0x50(%rbp) leado_encl_op_get_from_buf(%rip), %rax mov%rax,-0x48(%rbp) leado_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 */ leaprepared_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 */ 6000 0008 R_X86_64_RELATIVE 6008 0008 R_X86_64_RELATIVE 6010 0008 R_X86_64_RELATIVE 6018 0008 R_X86_64_RELATIVE 6020 0008 R_X86_64_RELATIVE 6028 0008 R_X86_64_RELATIVE 6030 0008 R_X86_64_RELATIVE 6038 0008 R_X86_64_RELATIVE 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 */ leaencl_op_array(%rip), %rcx lea__encl_base(%rip), %rax add(%rcx,%rdx,8),%rax jmp*%rax Link: https://lore.kernel.org/all/150d8ca8-2c66-60d1-f9fc-8e6279824...@cs.kuleuven.be/ Link: https://lore.kernel.org/all/5c22de5a-4b3b-1f38-9771-409b4ec7f...@cs.kuleuven.be/#r Signed-off-by: Jo Van Bulck Reviewed-by: Jarkko Sakkinen Acked-by: Kai Huang --- 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 ot
[PATCH v7 03/13] selftests/sgx: Include memory clobber for inline asm in test enclave
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 Reviewed-by: Kai Huang Reviewed-by: Jarkko Sakkinen --- 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" (), - "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" (), - "c" (op->epc_addr)); + "c" (op->epc_addr) + : "memory" /* read from secinfo pointer */); op->ret = rax; } -- 2.25.1
[PATCH v7 12/13] selftests/sgx: Discard unsupported ELF sections
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 Reviewed-by: Jarkko Sakkinen --- 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
[PATCH v7 00/13] selftests/sgx: Fix compilation errors
Hi, This patch series ensures that all SGX selftests succeed when compiling with optimizations (as tested with -O{0,1,2,3,s} for both gcc 11.3.0 and clang 14.0.0). The aim of the patches is to avoid reliance on undefined, compiler-specific behavior that can make the test results fragile. As far as I see, all commits in this series now have an explicit reviewed-by tag, so hopefully this can get merged upstream? Please let me know if any concerns remain and I'd happily address them. 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 - v7 - Add reviewed-by tag (Jarkko) v6 - Collect final ack/reviewed-by tags (Jarkko, Kai) 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 v7 01/13] selftests/sgx: Fix uninitialized pointer dereference in error path
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 Reviewed-by: Jarkko Sakkinen Acked-by: Kai Huang --- 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 = >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 v7 05/13] selftests/sgx: Specify freestanding environment for enclave compilation
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 Reviewed-by: Jarkko Sakkinen Acked-by: Kai Huang --- 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
[PATCH v7 11/13] selftests/sgx: Ensure expected location of test enclave buffer
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 Reviewed-by: Jarkko Sakkinen Acked-by: Kai Huang --- 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 v6 00/13] selftests/sgx: Fix compilation errors
Hi, This patch series ensures that all SGX selftests succeed when compiling with optimizations (as tested with -O{0,1,2,3,s} for both gcc 11.3.0 and clang 14.0.0). The aim of the patches is to avoid reliance on undefined, compiler-specific behavior that can make the test results fragile. As far as I see, all commits in this series have now been acked/reviewed. Please let me know if any concerns remain and I'd happily address them. 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 - v6 - Collect final ack/reviewed-by tags (Jarkko, Kai) 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 v6 12/13] selftests/sgx: Discard unsupported ELF sections
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 Reviewed-by: Jarkko Sakkinen --- 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
[PATCH v6 10/13] selftests/sgx: Ensure test enclave buffer is entirely preserved
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-a18060f66...@cs.kuleuven.be/ Signed-off-by: Jo Van Bulck Reviewed-by: Jarkko Sakkinen Acked-by: Kai Huang --- 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 v6 11/13] selftests/sgx: Ensure expected location of test enclave buffer
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 Reviewed-by: Jarkko Sakkinen Acked-by: Kai Huang --- 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 v6 08/13] selftests/sgx: Handle relocations in test enclave
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 */ leado_encl_op_put_to_buf(%rip), %rax mov%rax, -0x50(%rbp) leado_encl_op_get_from_buf(%rip), %rax mov%rax,-0x48(%rbp) leado_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 */ leaprepared_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 */ 6000 0008 R_X86_64_RELATIVE 6008 0008 R_X86_64_RELATIVE 6010 0008 R_X86_64_RELATIVE 6018 0008 R_X86_64_RELATIVE 6020 0008 R_X86_64_RELATIVE 6028 0008 R_X86_64_RELATIVE 6030 0008 R_X86_64_RELATIVE 6038 0008 R_X86_64_RELATIVE 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 */ leaencl_op_array(%rip), %rcx lea__encl_base(%rip), %rax add(%rcx,%rdx,8),%rax jmp*%rax Link: https://lore.kernel.org/all/150d8ca8-2c66-60d1-f9fc-8e6279824...@cs.kuleuven.be/ Link: https://lore.kernel.org/all/5c22de5a-4b3b-1f38-9771-409b4ec7f...@cs.kuleuven.be/#r Signed-off-by: Jo Van Bulck Reviewed-by: Jarkko Sakkinen Acked-by: Kai Huang --- 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 ot
[PATCH v6 05/13] selftests/sgx: Specify freestanding environment for enclave compilation
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 Reviewed-by: Jarkko Sakkinen Acked-by: Kai Huang --- 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
[PATCH v6 01/13] selftests/sgx: Fix uninitialized pointer dereference in error path
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 Reviewed-by: Jarkko Sakkinen Acked-by: Kai Huang --- 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 = >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 v6 06/13] selftests/sgx: Remove redundant enclave base address save/restore
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 Acked-by: Kai Huang --- 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 callencl_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 v6 04/13] selftests/sgx: Separate linker options
Fixes "'linker' input unused [-Wunused-command-line-argument]" errors when compiling with clang. Signed-off-by: Jo Van Bulck Reviewed-by: Jarkko Sakkinen --- 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 v6 13/13] selftests/sgx: Remove incomplete ABI sanitization code in test enclave
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-aec0ee0d8...@intel.com/ Signed-off-by: Jo Van Bulck Reviewed-by: Jarkko Sakkinen --- .../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. callencl_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
[PATCH v6 03/13] selftests/sgx: Include memory clobber for inline asm in test enclave
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 Reviewed-by: Kai Huang Reviewed-by: Jarkko Sakkinen --- 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" (), - "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" (), - "c" (op->epc_addr)); + "c" (op->epc_addr) + : "memory" /* read from secinfo pointer */); op->ret = rax; } -- 2.25.1
[PATCH v6 09/13] selftests/sgx: Fix linker script asserts
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 Reviewed-by: Jarkko Sakkinen --- 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 v6 02/13] selftests/sgx: Fix uninitialized pointer dereferences in encl_get_entry
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 Reviewed-by: Jarkko Sakkinen --- 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 = [i]; -- 2.25.1
[PATCH v6 07/13] selftests/sgx: Produce static-pie executable for test enclave
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-050d722c6...@cs.kuleuven.be/ Signed-off-by: Jo Van Bulck Reviewed-by: Jarkko Sakkinen Acked-by: Kai Huang --- 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