Rename symbols for better clarity:
* 'eenter' might be confused for directly calling ENCLU[EENTER]. It does not. It calls into the VDSO, which actually has the EENTER instruction. * 'sgx_call_vdso' is *only* used for entering the enclave. It's not some generic SGX call into the VDSO.
Signed-off-by: Jarkko Sakkinen jarkko@kernel.org ---
v3: Refine the commit message according to Dave's suggestions.
v2: Refined the renames just a bit.
tools/testing/selftests/sgx/call.S | 6 +++--- tools/testing/selftests/sgx/main.c | 25 +++++++++++++------------ tools/testing/selftests/sgx/main.h | 4 ++-- 3 files changed, 18 insertions(+), 17 deletions(-)
diff --git a/tools/testing/selftests/sgx/call.S b/tools/testing/selftests/sgx/call.S index 4ecadc7490f4..b09a25890f3b 100644 --- a/tools/testing/selftests/sgx/call.S +++ b/tools/testing/selftests/sgx/call.S @@ -5,8 +5,8 @@
.text
- .global sgx_call_vdso -sgx_call_vdso: + .global sgx_enter_enclave +sgx_enter_enclave: .cfi_startproc push %r15 .cfi_adjust_cfa_offset 8 @@ -27,7 +27,7 @@ sgx_call_vdso: .cfi_adjust_cfa_offset 8 push 0x38(%rsp) .cfi_adjust_cfa_offset 8 - call *eenter(%rip) + call *vdso_sgx_enter_enclave(%rip) add $0x10, %rsp .cfi_adjust_cfa_offset -0x10 pop %rbx diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c index d304a4044eb9..43da68388e25 100644 --- a/tools/testing/selftests/sgx/main.c +++ b/tools/testing/selftests/sgx/main.c @@ -21,7 +21,7 @@ #include "../kselftest.h"
static const uint64_t MAGIC = 0x1122334455667788ULL; -vdso_sgx_enter_enclave_t eenter; +vdso_sgx_enter_enclave_t vdso_sgx_enter_enclave;
struct vdso_symtab { Elf64_Sym *elf_symtab; @@ -149,7 +149,7 @@ int main(int argc, char *argv[]) { struct sgx_enclave_run run; struct vdso_symtab symtab; - Elf64_Sym *eenter_sym; + Elf64_Sym *sgx_enter_enclave_sym; uint64_t result = 0; struct encl encl; unsigned int i; @@ -194,29 +194,30 @@ int main(int argc, char *argv[]) if (!vdso_get_symtab(addr, &symtab)) goto err;
- eenter_sym = vdso_symtab_get(&symtab, "__vdso_sgx_enter_enclave"); - if (!eenter_sym) + sgx_enter_enclave_sym = vdso_symtab_get(&symtab, "__vdso_sgx_enter_enclave"); + if (!sgx_enter_enclave_sym) goto err;
- eenter = addr + eenter_sym->st_value; + vdso_sgx_enter_enclave = addr + sgx_enter_enclave_sym->st_value;
- ret = sgx_call_vdso((void *)&MAGIC, &result, 0, EENTER, NULL, NULL, &run); - if (!report_results(&run, ret, result, "sgx_call_vdso")) + ret = sgx_enter_enclave((void *)&MAGIC, &result, 0, EENTER, + NULL, NULL, &run); + if (!report_results(&run, ret, result, "sgx_enter_enclave_unclobbered")) goto err;
/* Invoke the vDSO directly. */ result = 0; - ret = eenter((unsigned long)&MAGIC, (unsigned long)&result, 0, EENTER, - 0, 0, &run); - if (!report_results(&run, ret, result, "eenter")) + ret = vdso_sgx_enter_enclave((unsigned long)&MAGIC, (unsigned long)&result, + 0, EENTER, 0, 0, &run); + if (!report_results(&run, ret, result, "sgx_enter_enclave")) goto err;
/* And with an exit handler. */ run.user_handler = (__u64)user_handler; run.user_data = 0xdeadbeef; - ret = eenter((unsigned long)&MAGIC, (unsigned long)&result, 0, EENTER, - 0, 0, &run); + ret = vdso_sgx_enter_enclave((unsigned long)&MAGIC, (unsigned long)&result, + 0, EENTER, 0, 0, &run); if (!report_results(&run, ret, result, "user_handler")) goto err;
diff --git a/tools/testing/selftests/sgx/main.h b/tools/testing/selftests/sgx/main.h index 67211a708f04..68672fd86cf9 100644 --- a/tools/testing/selftests/sgx/main.h +++ b/tools/testing/selftests/sgx/main.h @@ -35,7 +35,7 @@ bool encl_load(const char *path, struct encl *encl); bool encl_measure(struct encl *encl); bool encl_build(struct encl *encl);
-int sgx_call_vdso(void *rdi, void *rsi, long rdx, u32 function, void *r8, void *r9, - struct sgx_enclave_run *run); +int sgx_enter_enclave(void *rdi, void *rsi, long rdx, u32 function, void *r8, void *r9, + struct sgx_enclave_run *run);
#endif /* MAIN_H */
Migrate to kselftest harness. Use a fixture test with enclave initialized and de-initialized for each of the existing three tests, in other words:
1. One FIXTURE() for managing the enclave life-cycle. 2. Three TEST_F()'s, one for each test case.
Dump lines of /proc/self/maps matching "sgx" in FIXTURE_SETUP() as this can be very useful debugging information later on.
Signed-off-by: Jarkko Sakkinen jarkko@kernel.org ---
v8: * Replace inline function call with ENCL_CALL() macro.
v7: * Implement encl_call() that wraps the logic of invoking the vDSO for the test enclave.
v6: * Remove extra '\n' from TH_LOG() calls.
v5: * Use TH_LOG() for printing enclave address ranges instead of printf(), based on Reinette's remark.
v4: * Refine to take better use of the kselftest harness macros. * Fix: TCS base address was not initialized for a run struct.
v3: * Use helper macros.
v2: * Add the missing string argument to ksft_test_result_pass() and ksft_test_result_fail() calls.
tools/testing/selftests/sgx/load.c | 3 - tools/testing/selftests/sgx/main.c | 177 +++++++++++++++-------------- 2 files changed, 92 insertions(+), 88 deletions(-)
diff --git a/tools/testing/selftests/sgx/load.c b/tools/testing/selftests/sgx/load.c index f441ac34b4d4..00928be57fc4 100644 --- a/tools/testing/selftests/sgx/load.c +++ b/tools/testing/selftests/sgx/load.c @@ -239,9 +239,6 @@ bool encl_load(const char *path, struct encl *encl) seg->offset = (phdr->p_offset & PAGE_MASK) - src_offset; seg->size = (phdr->p_filesz + PAGE_SIZE - 1) & PAGE_MASK;
- printf("0x%016lx 0x%016lx 0x%02x\n", seg->offset, seg->size, - seg->prot); - j++; }
diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c index 43da68388e25..6da19b6bf287 100644 --- a/tools/testing/selftests/sgx/main.c +++ b/tools/testing/selftests/sgx/main.c @@ -17,8 +17,8 @@ #include <sys/types.h> #include <sys/auxv.h> #include "defines.h" +#include "../kselftest_harness.h" #include "main.h" -#include "../kselftest.h"
static const uint64_t MAGIC = 0x1122334455667788ULL; vdso_sgx_enter_enclave_t vdso_sgx_enter_enclave; @@ -107,85 +107,49 @@ static Elf64_Sym *vdso_symtab_get(struct vdso_symtab *symtab, const char *name) return NULL; }
-bool report_results(struct sgx_enclave_run *run, int ret, uint64_t result, - const char *test) -{ - bool valid = true; - - if (ret) { - printf("FAIL: %s() returned: %d\n", test, ret); - valid = false; - } - - if (run->function != EEXIT) { - printf("FAIL: %s() function, expected: %u, got: %u\n", test, EEXIT, - run->function); - valid = false; - } - - if (result != MAGIC) { - printf("FAIL: %s(), expected: 0x%lx, got: 0x%lx\n", test, MAGIC, - result); - valid = false; - } - - if (run->user_data) { - printf("FAIL: %s() user data, expected: 0x0, got: 0x%llx\n", - test, run->user_data); - valid = false; - } - - return valid; -} - -static int user_handler(long rdi, long rsi, long rdx, long ursp, long r8, long r9, - struct sgx_enclave_run *run) -{ - run->user_data = 0; - return 0; -} +FIXTURE(enclave) { + struct encl encl; + struct sgx_enclave_run run; +};
-int main(int argc, char *argv[]) +FIXTURE_SETUP(enclave) { - struct sgx_enclave_run run; + Elf64_Sym *sgx_enter_enclave_sym = NULL; struct vdso_symtab symtab; - Elf64_Sym *sgx_enter_enclave_sym; - uint64_t result = 0; - struct encl encl; + struct encl_segment *seg; unsigned int i; void *addr; - int ret; - - memset(&run, 0, sizeof(run));
- if (!encl_load("test_encl.elf", &encl)) { - encl_delete(&encl); + if (!encl_load("test_encl.elf", &self->encl)) { + encl_delete(&self->encl); ksft_exit_skip("cannot load enclaves\n"); }
- if (!encl_measure(&encl)) + for (i = 0; i < self->encl.nr_segments; i++) { + seg = &self->encl.segment_tbl[i]; + + TH_LOG("0x%016lx 0x%016lx 0x%02x", seg->offset, seg->size, seg->prot); + } + + if (!encl_measure(&self->encl)) goto err;
- if (!encl_build(&encl)) + if (!encl_build(&self->encl)) goto err;
/* * An enclave consumer only must do this. */ - for (i = 0; i < encl.nr_segments; i++) { - struct encl_segment *seg = &encl.segment_tbl[i]; - - addr = mmap((void *)encl.encl_base + seg->offset, seg->size, - seg->prot, MAP_SHARED | MAP_FIXED, encl.fd, 0); - if (addr == MAP_FAILED) { - perror("mmap() segment failed"); - exit(KSFT_FAIL); - } + for (i = 0; i < self->encl.nr_segments; i++) { + struct encl_segment *seg = &self->encl.segment_tbl[i]; + + addr = mmap((void *)self->encl.encl_base + seg->offset, seg->size, + seg->prot, MAP_SHARED | MAP_FIXED, self->encl.fd, 0); + EXPECT_NE(addr, MAP_FAILED); + if (addr == MAP_FAILED) + goto err; }
- memset(&run, 0, sizeof(run)); - run.tcs = encl.encl_base; - /* Get vDSO base address */ addr = (void *)getauxval(AT_SYSINFO_EHDR); if (!addr) @@ -200,32 +164,75 @@ int main(int argc, char *argv[])
vdso_sgx_enter_enclave = addr + sgx_enter_enclave_sym->st_value;
- ret = sgx_enter_enclave((void *)&MAGIC, &result, 0, EENTER, - NULL, NULL, &run); - if (!report_results(&run, ret, result, "sgx_enter_enclave_unclobbered")) - goto err; + memset(&self->run, 0, sizeof(self->run)); + self->run.tcs = self->encl.encl_base;
+err: + if (!sgx_enter_enclave_sym) + encl_delete(&self->encl);
- /* Invoke the vDSO directly. */ - result = 0; - ret = vdso_sgx_enter_enclave((unsigned long)&MAGIC, (unsigned long)&result, - 0, EENTER, 0, 0, &run); - if (!report_results(&run, ret, result, "sgx_enter_enclave")) - goto err; + ASSERT_NE(sgx_enter_enclave_sym, NULL); +}
- /* And with an exit handler. */ - run.user_handler = (__u64)user_handler; - run.user_data = 0xdeadbeef; - ret = vdso_sgx_enter_enclave((unsigned long)&MAGIC, (unsigned long)&result, - 0, EENTER, 0, 0, &run); - if (!report_results(&run, ret, result, "user_handler")) - goto err; +FIXTURE_TEARDOWN(enclave) +{ + encl_delete(&self->encl); +}
- printf("SUCCESS\n"); - encl_delete(&encl); - exit(KSFT_PASS); +#define ENCL_CALL(in, out, run, clobbered) \ + ({ \ + int ret; \ + if ((clobbered)) \ + ret = vdso_sgx_enter_enclave((unsigned long)(in), (unsigned long)(out), 0, \ + EENTER, 0, 0, (run)); \ + else \ + ret = sgx_enter_enclave((void *)(in), (void *)(out), 0, EENTER, NULL, NULL, \ + (run)); \ + ret; \ + }) + +TEST_F(enclave, unclobbered_vdso) +{ + uint64_t result = 0;
-err: - encl_delete(&encl); - exit(KSFT_FAIL); + EXPECT_EQ(ENCL_CALL(&MAGIC, &result, &self->run, false), 0); + + EXPECT_EQ(result, MAGIC); + EXPECT_EQ(self->run.function, EEXIT); + EXPECT_EQ(self->run.user_data, 0); +} + +TEST_F(enclave, clobbered_vdso) +{ + uint64_t result = 0; + + EXPECT_EQ(ENCL_CALL(&MAGIC, &result, &self->run, true), 0); + + EXPECT_EQ(result, MAGIC); + EXPECT_EQ(self->run.function, EEXIT); + EXPECT_EQ(self->run.user_data, 0); } + +static int test_handler(long rdi, long rsi, long rdx, long ursp, long r8, long r9, + struct sgx_enclave_run *run) +{ + run->user_data = 0; + + return 0; +} + +TEST_F(enclave, clobbered_vdso_and_user_function) +{ + uint64_t result = 0; + + self->run.user_handler = (__u64)test_handler; + self->run.user_data = 0xdeadbeef; + + EXPECT_EQ(ENCL_CALL(&MAGIC, &result, &self->run, true), 0); + + EXPECT_EQ(result, MAGIC); + EXPECT_EQ(self->run.function, EEXIT); + EXPECT_EQ(self->run.user_data, 0); +} + +TEST_HARNESS_MAIN
Often, it's useful to check whether /proc/self/maps looks sane when dealing with memory mapped objects, especially when they are JIT'ish dynamically constructed objects. Therefore, dump "/dev/sgx_enclave" matching lines from the memory map in FIXTURE_SETUP().
Signed-off-by: Jarkko Sakkinen jarkko@kernel.org --- tools/testing/selftests/sgx/main.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c index 6da19b6bf287..14030f8b85ff 100644 --- a/tools/testing/selftests/sgx/main.c +++ b/tools/testing/selftests/sgx/main.c @@ -117,6 +117,8 @@ FIXTURE_SETUP(enclave) Elf64_Sym *sgx_enter_enclave_sym = NULL; struct vdso_symtab symtab; struct encl_segment *seg; + char maps_line[256]; + FILE *maps_file; unsigned int i; void *addr;
@@ -167,6 +169,18 @@ FIXTURE_SETUP(enclave) memset(&self->run, 0, sizeof(self->run)); self->run.tcs = self->encl.encl_base;
+ maps_file = fopen("/proc/self/maps", "r"); + if (maps_file != NULL) { + while (fgets(maps_line, sizeof(maps_line), maps_file) != NULL) { + maps_line[strlen(maps_line) - 1] = '\0'; + + if (strstr(maps_line, "/dev/sgx_enclave")) + TH_LOG("%s", maps_line); + } + + fclose(maps_file); + } + err: if (!sgx_enter_enclave_sym) encl_delete(&self->encl);
On 6/10/21 2:30 AM, Jarkko Sakkinen wrote:
Often, it's useful to check whether /proc/self/maps looks sane when dealing with memory mapped objects, especially when they are JIT'ish dynamically constructed objects. Therefore, dump "/dev/sgx_enclave" matching lines from the memory map in FIXTURE_SETUP().
Signed-off-by: Jarkko Sakkinen jarkko@kernel.org
tools/testing/selftests/sgx/main.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c index 6da19b6bf287..14030f8b85ff 100644 --- a/tools/testing/selftests/sgx/main.c +++ b/tools/testing/selftests/sgx/main.c @@ -117,6 +117,8 @@ FIXTURE_SETUP(enclave) Elf64_Sym *sgx_enter_enclave_sym = NULL; struct vdso_symtab symtab; struct encl_segment *seg;
- char maps_line[256];
- FILE *maps_file; unsigned int i; void *addr;
@@ -167,6 +169,18 @@ FIXTURE_SETUP(enclave) memset(&self->run, 0, sizeof(self->run)); self->run.tcs = self->encl.encl_base;
- maps_file = fopen("/proc/self/maps", "r");
I almost applied these. Does this require root access, if so, please add logic to skip the test if non-root user runs it.
Same comments for all other paths that might require root access.
thanks, -- Shuah
On 6/11/21 3:45 PM, Shuah Khan wrote:
@@ -167,6 +169,18 @@ FIXTURE_SETUP(enclave) memset(&self->run, 0, sizeof(self->run)); self->run.tcs = self->encl.encl_base; + maps_file = fopen("/proc/self/maps", "r");
I almost applied these. Does this require root access, if so, please add logic to skip the test if non-root user runs it.
Same comments for all other paths that might require root access.
This specifically doesn't require root. Anything can read its own /proc/self/maps file.
On Fri, Jun 11, 2021 at 04:45:19PM -0600, Shuah Khan wrote:
On 6/10/21 2:30 AM, Jarkko Sakkinen wrote:
Often, it's useful to check whether /proc/self/maps looks sane when dealing with memory mapped objects, especially when they are JIT'ish dynamically constructed objects. Therefore, dump "/dev/sgx_enclave" matching lines from the memory map in FIXTURE_SETUP().
Signed-off-by: Jarkko Sakkinen jarkko@kernel.org
tools/testing/selftests/sgx/main.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c index 6da19b6bf287..14030f8b85ff 100644 --- a/tools/testing/selftests/sgx/main.c +++ b/tools/testing/selftests/sgx/main.c @@ -117,6 +117,8 @@ FIXTURE_SETUP(enclave) Elf64_Sym *sgx_enter_enclave_sym = NULL; struct vdso_symtab symtab; struct encl_segment *seg;
- char maps_line[256];
- FILE *maps_file; unsigned int i; void *addr;
@@ -167,6 +169,18 @@ FIXTURE_SETUP(enclave) memset(&self->run, 0, sizeof(self->run)); self->run.tcs = self->encl.encl_base;
- maps_file = fopen("/proc/self/maps", "r");
I almost applied these. Does this require root access, if so, please add logic to skip the test if non-root user runs it.
Same comments for all other paths that might require root access.
As Dave stated, it does not. A process can inspect its own state through /proc/self path. E.g. Chrome web browser uses /proc/self/exe to initialize multiple instances of itself for browser tabs...
As far as other things go, this patch set does not bind to any other new OS resources.
thanks, -- Shuah
/Jarkko
On 6/11/21 10:27 PM, Jarkko Sakkinen wrote:
On Fri, Jun 11, 2021 at 04:45:19PM -0600, Shuah Khan wrote:
On 6/10/21 2:30 AM, Jarkko Sakkinen wrote:
Often, it's useful to check whether /proc/self/maps looks sane when dealing with memory mapped objects, especially when they are JIT'ish dynamically constructed objects. Therefore, dump "/dev/sgx_enclave" matching lines from the memory map in FIXTURE_SETUP().
Signed-off-by: Jarkko Sakkinen jarkko@kernel.org
tools/testing/selftests/sgx/main.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c index 6da19b6bf287..14030f8b85ff 100644 --- a/tools/testing/selftests/sgx/main.c +++ b/tools/testing/selftests/sgx/main.c @@ -117,6 +117,8 @@ FIXTURE_SETUP(enclave) Elf64_Sym *sgx_enter_enclave_sym = NULL; struct vdso_symtab symtab; struct encl_segment *seg;
- char maps_line[256];
- FILE *maps_file; unsigned int i; void *addr;
@@ -167,6 +169,18 @@ FIXTURE_SETUP(enclave) memset(&self->run, 0, sizeof(self->run)); self->run.tcs = self->encl.encl_base;
- maps_file = fopen("/proc/self/maps", "r");
I almost applied these. Does this require root access, if so, please add logic to skip the test if non-root user runs it.
Same comments for all other paths that might require root access.
As Dave stated, it does not. A process can inspect its own state through /proc/self path. E.g. Chrome web browser uses /proc/self/exe to initialize multiple instances of itself for browser tabs...
Ah yes. I missed the self part. Thanks for clarifying.
As far as other things go, this patch set does not bind to any other new OS resources.
Thank you. I will apply these for 5.14-rc1.
thanks, -- Shuah
On Mon, Jun 14, 2021 at 10:45:43AM -0600, Shuah Khan wrote:
On 6/11/21 10:27 PM, Jarkko Sakkinen wrote:
On Fri, Jun 11, 2021 at 04:45:19PM -0600, Shuah Khan wrote:
On 6/10/21 2:30 AM, Jarkko Sakkinen wrote:
Often, it's useful to check whether /proc/self/maps looks sane when dealing with memory mapped objects, especially when they are JIT'ish dynamically constructed objects. Therefore, dump "/dev/sgx_enclave" matching lines from the memory map in FIXTURE_SETUP().
Signed-off-by: Jarkko Sakkinen jarkko@kernel.org
tools/testing/selftests/sgx/main.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c index 6da19b6bf287..14030f8b85ff 100644 --- a/tools/testing/selftests/sgx/main.c +++ b/tools/testing/selftests/sgx/main.c @@ -117,6 +117,8 @@ FIXTURE_SETUP(enclave) Elf64_Sym *sgx_enter_enclave_sym = NULL; struct vdso_symtab symtab; struct encl_segment *seg;
- char maps_line[256];
- FILE *maps_file; unsigned int i; void *addr;
@@ -167,6 +169,18 @@ FIXTURE_SETUP(enclave) memset(&self->run, 0, sizeof(self->run)); self->run.tcs = self->encl.encl_base;
- maps_file = fopen("/proc/self/maps", "r");
I almost applied these. Does this require root access, if so, please add logic to skip the test if non-root user runs it.
Same comments for all other paths that might require root access.
As Dave stated, it does not. A process can inspect its own state through /proc/self path. E.g. Chrome web browser uses /proc/self/exe to initialize multiple instances of itself for browser tabs...
Ah yes. I missed the self part. Thanks for clarifying.
As far as other things go, this patch set does not bind to any other new OS resources.
Thank you. I will apply these for 5.14-rc1.
Thank you!
thanks, -- Shuah
/Jarkko
Add EXPECT_EEXIT() macro, which will conditionally print the exception information, in addition to
EXPECT_EQ(self->run.function, EEXIT);
Signed-off-by: Jarkko Sakkinen jarkko@kernel.org --- tools/testing/selftests/sgx/main.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c index 14030f8b85ff..bcd0257f48e0 100644 --- a/tools/testing/selftests/sgx/main.c +++ b/tools/testing/selftests/sgx/main.c @@ -205,6 +205,14 @@ FIXTURE_TEARDOWN(enclave) ret; \ })
+#define EXPECT_EEXIT(run) \ + do { \ + EXPECT_EQ((run)->function, EEXIT); \ + if ((run)->function != EEXIT) \ + TH_LOG("0x%02x 0x%02x 0x%016llx", (run)->exception_vector, \ + (run)->exception_error_code, (run)->exception_addr); \ + } while (0) + TEST_F(enclave, unclobbered_vdso) { uint64_t result = 0; @@ -212,7 +220,7 @@ TEST_F(enclave, unclobbered_vdso) EXPECT_EQ(ENCL_CALL(&MAGIC, &result, &self->run, false), 0);
EXPECT_EQ(result, MAGIC); - EXPECT_EQ(self->run.function, EEXIT); + EXPECT_EEXIT(&self->run); EXPECT_EQ(self->run.user_data, 0); }
@@ -223,7 +231,7 @@ TEST_F(enclave, clobbered_vdso) EXPECT_EQ(ENCL_CALL(&MAGIC, &result, &self->run, true), 0);
EXPECT_EQ(result, MAGIC); - EXPECT_EQ(self->run.function, EEXIT); + EXPECT_EEXIT(&self->run); EXPECT_EQ(self->run.user_data, 0); }
@@ -245,7 +253,7 @@ TEST_F(enclave, clobbered_vdso_and_user_function) EXPECT_EQ(ENCL_CALL(&MAGIC, &result, &self->run, true), 0);
EXPECT_EQ(result, MAGIC); - EXPECT_EQ(self->run.function, EEXIT); + EXPECT_EEXIT(&self->run); EXPECT_EQ(self->run.user_data, 0); }
Extend the enclave to have two operations: ENCL_OP_PUT and ENCL_OP_GET. ENCL_OP_PUT stores value inside the enclave address space and ENCL_OP_GET reads it. The internal buffer can be later extended to be variable size, and allow reclaimer tests.
Signed-off-by: Jarkko Sakkinen jarkko@kernel.org --- tools/testing/selftests/sgx/defines.h | 10 ++++ tools/testing/selftests/sgx/main.c | 57 ++++++++++++++++++----- tools/testing/selftests/sgx/test_encl.c | 19 +++++++- tools/testing/selftests/sgx/test_encl.lds | 3 +- 4 files changed, 74 insertions(+), 15 deletions(-)
diff --git a/tools/testing/selftests/sgx/defines.h b/tools/testing/selftests/sgx/defines.h index 0bd73428d2f3..f88562afcaa0 100644 --- a/tools/testing/selftests/sgx/defines.h +++ b/tools/testing/selftests/sgx/defines.h @@ -18,4 +18,14 @@ #include "../../../../arch/x86/include/asm/enclu.h" #include "../../../../arch/x86/include/uapi/asm/sgx.h"
+enum encl_op_type { + ENCL_OP_PUT, + ENCL_OP_GET, +}; + +struct encl_op { + uint64_t type; + uint64_t buffer; +}; + #endif /* DEFINES_H */ diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c index bcd0257f48e0..e252015e0c15 100644 --- a/tools/testing/selftests/sgx/main.c +++ b/tools/testing/selftests/sgx/main.c @@ -193,14 +193,14 @@ FIXTURE_TEARDOWN(enclave) encl_delete(&self->encl); }
-#define ENCL_CALL(in, out, run, clobbered) \ +#define ENCL_CALL(op, run, clobbered) \ ({ \ int ret; \ if ((clobbered)) \ - ret = vdso_sgx_enter_enclave((unsigned long)(in), (unsigned long)(out), 0, \ + ret = vdso_sgx_enter_enclave((unsigned long)(op), 0, 0, \ EENTER, 0, 0, (run)); \ else \ - ret = sgx_enter_enclave((void *)(in), (void *)(out), 0, EENTER, NULL, NULL, \ + ret = sgx_enter_enclave((void *)(op), NULL, 0, EENTER, NULL, NULL, \ (run)); \ ret; \ }) @@ -215,22 +215,44 @@ FIXTURE_TEARDOWN(enclave)
TEST_F(enclave, unclobbered_vdso) { - uint64_t result = 0; + struct encl_op op;
- EXPECT_EQ(ENCL_CALL(&MAGIC, &result, &self->run, false), 0); + op.type = ENCL_OP_PUT; + op.buffer = MAGIC; + + EXPECT_EQ(ENCL_CALL(&op, &self->run, false), 0);
- EXPECT_EQ(result, MAGIC); + EXPECT_EEXIT(&self->run); + EXPECT_EQ(self->run.user_data, 0); + + op.type = ENCL_OP_GET; + op.buffer = 0; + + EXPECT_EQ(ENCL_CALL(&op, &self->run, false), 0); + + EXPECT_EQ(op.buffer, MAGIC); EXPECT_EEXIT(&self->run); EXPECT_EQ(self->run.user_data, 0); }
TEST_F(enclave, clobbered_vdso) { - uint64_t result = 0; + struct encl_op op; + + op.type = ENCL_OP_PUT; + op.buffer = MAGIC; + + EXPECT_EQ(ENCL_CALL(&op, &self->run, true), 0); + + EXPECT_EEXIT(&self->run); + EXPECT_EQ(self->run.user_data, 0); + + op.type = ENCL_OP_GET; + op.buffer = 0;
- EXPECT_EQ(ENCL_CALL(&MAGIC, &result, &self->run, true), 0); + EXPECT_EQ(ENCL_CALL(&op, &self->run, true), 0);
- EXPECT_EQ(result, MAGIC); + EXPECT_EQ(op.buffer, MAGIC); EXPECT_EEXIT(&self->run); EXPECT_EQ(self->run.user_data, 0); } @@ -245,14 +267,25 @@ static int test_handler(long rdi, long rsi, long rdx, long ursp, long r8, long r
TEST_F(enclave, clobbered_vdso_and_user_function) { - uint64_t result = 0; + struct encl_op op;
self->run.user_handler = (__u64)test_handler; self->run.user_data = 0xdeadbeef;
- EXPECT_EQ(ENCL_CALL(&MAGIC, &result, &self->run, true), 0); + op.type = ENCL_OP_PUT; + op.buffer = MAGIC; + + EXPECT_EQ(ENCL_CALL(&op, &self->run, true), 0); + + EXPECT_EEXIT(&self->run); + EXPECT_EQ(self->run.user_data, 0); + + op.type = ENCL_OP_GET; + op.buffer = 0; + + EXPECT_EQ(ENCL_CALL(&op, &self->run, true), 0);
- EXPECT_EQ(result, MAGIC); + EXPECT_EQ(op.buffer, MAGIC); EXPECT_EEXIT(&self->run); EXPECT_EQ(self->run.user_data, 0); } diff --git a/tools/testing/selftests/sgx/test_encl.c b/tools/testing/selftests/sgx/test_encl.c index cf25b5dc1e03..734ea52f9924 100644 --- a/tools/testing/selftests/sgx/test_encl.c +++ b/tools/testing/selftests/sgx/test_encl.c @@ -4,6 +4,8 @@ #include <stddef.h> #include "defines.h"
+static uint8_t encl_buffer[8192] = { 1 }; + static void *memcpy(void *dest, const void *src, size_t n) { size_t i; @@ -14,7 +16,20 @@ static void *memcpy(void *dest, const void *src, size_t n) return dest; }
-void encl_body(void *rdi, void *rsi) +void encl_body(void *rdi, void *rsi) { - memcpy(rsi, rdi, 8); + struct encl_op *op = (struct encl_op *)rdi; + + switch (op->type) { + case ENCL_OP_PUT: + memcpy(&encl_buffer[0], &op->buffer, 8); + break; + + case ENCL_OP_GET: + memcpy(&op->buffer, &encl_buffer[0], 8); + break; + + default: + break; + } } diff --git a/tools/testing/selftests/sgx/test_encl.lds b/tools/testing/selftests/sgx/test_encl.lds index 0fbbda7e665e..a1ec64f7d91f 100644 --- a/tools/testing/selftests/sgx/test_encl.lds +++ b/tools/testing/selftests/sgx/test_encl.lds @@ -18,9 +18,10 @@ SECTIONS .text : { *(.text*) *(.rodata*) + FILL(0xDEADBEEF); + . = ALIGN(4096); } : text
- . = ALIGN(4096); .data : { *(.data*) } : data
On 6/10/21 2:30 AM, Jarkko Sakkinen wrote:
Extend the enclave to have two operations: ENCL_OP_PUT and ENCL_OP_GET. ENCL_OP_PUT stores value inside the enclave address space and ENCL_OP_GET reads it. The internal buffer can be later extended to be variable size, and allow reclaimer tests.
Signed-off-by: Jarkko Sakkinen jarkko@kernel.org
tools/testing/selftests/sgx/defines.h | 10 ++++ tools/testing/selftests/sgx/main.c | 57 ++++++++++++++++++----- tools/testing/selftests/sgx/test_encl.c | 19 +++++++- tools/testing/selftests/sgx/test_encl.lds | 3 +- 4 files changed, 74 insertions(+), 15 deletions(-)
Test output before applying the series:
TAP version 13 1..1 # selftests: sgx: test_sgx # Unable to open /dev/sgx_enclave: No such file or directory # 1..0 # SKIP cannot load enclaves ok 1 selftests: sgx: test_sgx # SKIP
Test output after applying second patch
selftests/sgx: Migrate to kselftest harness
Output changes to the following. It doesn't look like the second patch adds any new tests. What is the point in running the tests that fail if /dev/sgx_enclave is missing.
Unfortunately this series doesn't have a cover letter that explains what this series is doing. I don't like the fact that the test output and behavior changes when migrating the test to kselftest harness. Shouldn't the output stay the same as in skip the tests if /dev/sgx_enclave fails.
TAP version 13 1..1 # selftests: sgx: test_sgx # TAP version 13 # 1..3 # # Starting 3 tests from 2 test cases. # # RUN enclave.unclobbered_vdso ... # Unable to open /dev/sgx_enclave: No such file or directory # ok 2 # SKIP cannot load enclaves # # Planned tests != run tests (3 != 1) # # Totals: pass:0 fail:0 xfail:0 xpass:0 skip:1 error:0 # # unclobbered_vdso: Test failed at step #4 # # FAIL enclave.unclobbered_vdso # not ok 1 enclave.unclobbered_vdso # # RUN enclave.clobbered_vdso ... # Unable to open /dev/sgx_enclave: No such file or directory # ok 3 # SKIP cannot load enclaves # # Planned tests != run tests (3 != 2) # # Totals: pass:0 fail:1 xfail:0 xpass:0 skip:1 error:0 # # clobbered_vdso: Test failed at step #4 # # FAIL enclave.clobbered_vdso # not ok 2 enclave.clobbered_vdso # # RUN enclave.clobbered_vdso_and_user_function ... # Unable to open /dev/sgx_enclave: No such file or directory # ok 4 # SKIP cannot load enclaves # # Totals: pass:0 fail:2 xfail:0 xpass:0 skip:1 error:0 # # clobbered_vdso_and_user_function: Test failed at step #4 # # FAIL enclave.clobbered_vdso_and_user_function # not ok 3 enclave.clobbered_vdso_and_user_function # # FAILED: 0 / 3 tests passed. # # Totals: pass:0 fail:3 xfail:0 xpass:0 skip:0 error:0 not ok 1 selftests: sgx: test_sgx # exit=1
thanks, -- Shuah
On Mon, Jun 14, 2021 at 02:16:15PM -0600, Shuah Khan wrote:
On 6/10/21 2:30 AM, Jarkko Sakkinen wrote:
Extend the enclave to have two operations: ENCL_OP_PUT and ENCL_OP_GET. ENCL_OP_PUT stores value inside the enclave address space and ENCL_OP_GET reads it. The internal buffer can be later extended to be variable size, and allow reclaimer tests.
Signed-off-by: Jarkko Sakkinen jarkko@kernel.org
tools/testing/selftests/sgx/defines.h | 10 ++++ tools/testing/selftests/sgx/main.c | 57 ++++++++++++++++++----- tools/testing/selftests/sgx/test_encl.c | 19 +++++++- tools/testing/selftests/sgx/test_encl.lds | 3 +- 4 files changed, 74 insertions(+), 15 deletions(-)
Test output before applying the series:
TAP version 13 1..1 # selftests: sgx: test_sgx # Unable to open /dev/sgx_enclave: No such file or directory # 1..0 # SKIP cannot load enclaves ok 1 selftests: sgx: test_sgx # SKIP
Test output after applying second patch
selftests/sgx: Migrate to kselftest harness
Output changes to the following. It doesn't look like the second patch adds any new tests. What is the point in running the tests that fail if /dev/sgx_enclave is missing.
Unfortunately this series doesn't have a cover letter that explains what this series is doing. I don't like the fact that the test output and behavior changes when migrating the test to kselftest harness. Shouldn't the output stay the same as in skip the tests if /dev/sgx_enclave fails.
I get what you are saying but actually I do not know how with fixtures I can skip "the rest" when FIXTURE_SETUP() fails.
The reason for the output below is that with fixtures for all tests enclave is initialized for each test case. And it kind of makes sense because all tests start from the clean expected state.
I don't how to do that with zero change in the output.
The reason to do this change is to make it easy to add more tests, and return correct status codes to the framework.
TAP version 13 1..1 # selftests: sgx: test_sgx # TAP version 13 # 1..3 # # Starting 3 tests from 2 test cases. # # RUN enclave.unclobbered_vdso ... # Unable to open /dev/sgx_enclave: No such file or directory # ok 2 # SKIP cannot load enclaves # # Planned tests != run tests (3 != 1) # # Totals: pass:0 fail:0 xfail:0 xpass:0 skip:1 error:0 # # unclobbered_vdso: Test failed at step #4 # # FAIL enclave.unclobbered_vdso # not ok 1 enclave.unclobbered_vdso # # RUN enclave.clobbered_vdso ... # Unable to open /dev/sgx_enclave: No such file or directory # ok 3 # SKIP cannot load enclaves # # Planned tests != run tests (3 != 2) # # Totals: pass:0 fail:1 xfail:0 xpass:0 skip:1 error:0 # # clobbered_vdso: Test failed at step #4 # # FAIL enclave.clobbered_vdso # not ok 2 enclave.clobbered_vdso # # RUN enclave.clobbered_vdso_and_user_function ... # Unable to open /dev/sgx_enclave: No such file or directory # ok 4 # SKIP cannot load enclaves # # Totals: pass:0 fail:2 xfail:0 xpass:0 skip:1 error:0 # # clobbered_vdso_and_user_function: Test failed at step #4 # # FAIL enclave.clobbered_vdso_and_user_function # not ok 3 enclave.clobbered_vdso_and_user_function # # FAILED: 0 / 3 tests passed. # # Totals: pass:0 fail:3 xfail:0 xpass:0 skip:0 error:0 not ok 1 selftests: sgx: test_sgx # exit=1
thanks, -- Shuah
/Jarkko
On Tue, Jun 15, 2021 at 04:14:02PM +0300, Jarkko Sakkinen wrote:
On Mon, Jun 14, 2021 at 02:16:15PM -0600, Shuah Khan wrote:
On 6/10/21 2:30 AM, Jarkko Sakkinen wrote:
Extend the enclave to have two operations: ENCL_OP_PUT and ENCL_OP_GET. ENCL_OP_PUT stores value inside the enclave address space and ENCL_OP_GET reads it. The internal buffer can be later extended to be variable size, and allow reclaimer tests.
Signed-off-by: Jarkko Sakkinen jarkko@kernel.org
tools/testing/selftests/sgx/defines.h | 10 ++++ tools/testing/selftests/sgx/main.c | 57 ++++++++++++++++++----- tools/testing/selftests/sgx/test_encl.c | 19 +++++++- tools/testing/selftests/sgx/test_encl.lds | 3 +- 4 files changed, 74 insertions(+), 15 deletions(-)
Test output before applying the series:
TAP version 13 1..1 # selftests: sgx: test_sgx # Unable to open /dev/sgx_enclave: No such file or directory # 1..0 # SKIP cannot load enclaves ok 1 selftests: sgx: test_sgx # SKIP
Test output after applying second patch
selftests/sgx: Migrate to kselftest harness
Output changes to the following. It doesn't look like the second patch adds any new tests. What is the point in running the tests that fail if /dev/sgx_enclave is missing.
Unfortunately this series doesn't have a cover letter that explains what this series is doing. I don't like the fact that the test output and behavior changes when migrating the test to kselftest harness. Shouldn't the output stay the same as in skip the tests if /dev/sgx_enclave fails.
I get what you are saying but actually I do not know how with fixtures I can skip "the rest" when FIXTURE_SETUP() fails.
The reason for the output below is that with fixtures for all tests enclave is initialized for each test case. And it kind of makes sense because all tests start from the clean expected state.
I don't how to do that with zero change in the output.
The reason to do this change is to make it easy to add more tests, and return correct status codes to the framework.
To add: everything I did I based purely to the existing kernel documentation, following the examples on how to use fixture.
/Jarkko
On 6/15/21 7:15 AM, Jarkko Sakkinen wrote:
On Tue, Jun 15, 2021 at 04:14:02PM +0300, Jarkko Sakkinen wrote:
On Mon, Jun 14, 2021 at 02:16:15PM -0600, Shuah Khan wrote:
On 6/10/21 2:30 AM, Jarkko Sakkinen wrote:
Extend the enclave to have two operations: ENCL_OP_PUT and ENCL_OP_GET. ENCL_OP_PUT stores value inside the enclave address space and ENCL_OP_GET reads it. The internal buffer can be later extended to be variable size, and allow reclaimer tests.
Signed-off-by: Jarkko Sakkinen jarkko@kernel.org
tools/testing/selftests/sgx/defines.h | 10 ++++ tools/testing/selftests/sgx/main.c | 57 ++++++++++++++++++----- tools/testing/selftests/sgx/test_encl.c | 19 +++++++- tools/testing/selftests/sgx/test_encl.lds | 3 +- 4 files changed, 74 insertions(+), 15 deletions(-)
Test output before applying the series:
TAP version 13 1..1 # selftests: sgx: test_sgx # Unable to open /dev/sgx_enclave: No such file or directory # 1..0 # SKIP cannot load enclaves ok 1 selftests: sgx: test_sgx # SKIP
Test output after applying second patch
selftests/sgx: Migrate to kselftest harness
Output changes to the following. It doesn't look like the second patch adds any new tests. What is the point in running the tests that fail if /dev/sgx_enclave is missing.
Unfortunately this series doesn't have a cover letter that explains what this series is doing. I don't like the fact that the test output and behavior changes when migrating the test to kselftest harness. Shouldn't the output stay the same as in skip the tests if /dev/sgx_enclave fails.
I get what you are saying but actually I do not know how with fixtures I can skip "the rest" when FIXTURE_SETUP() fails.
The reason for the output below is that with fixtures for all tests enclave is initialized for each test case. And it kind of makes sense because all tests start from the clean expected state.
I don't how to do that with zero change in the output.
Yeah. I took a look at the FIXTURE. Doesn't look like it is possible.
The reason to do this change is to make it easy to add more tests, and return correct status codes to the framework.
To add: everything I did I based purely to the existing kernel documentation, following the examples on how to use fixture.
I will pick these up and will add a note to the last commit that output changes, so test rings that run kselftest are aware of the change.
thanks, -- Shuah
On Tue, Jun 15, 2021 at 03:55:25PM -0600, Shuah Khan wrote:
On 6/15/21 7:15 AM, Jarkko Sakkinen wrote:
On Tue, Jun 15, 2021 at 04:14:02PM +0300, Jarkko Sakkinen wrote:
On Mon, Jun 14, 2021 at 02:16:15PM -0600, Shuah Khan wrote:
On 6/10/21 2:30 AM, Jarkko Sakkinen wrote:
Extend the enclave to have two operations: ENCL_OP_PUT and ENCL_OP_GET. ENCL_OP_PUT stores value inside the enclave address space and ENCL_OP_GET reads it. The internal buffer can be later extended to be variable size, and allow reclaimer tests.
Signed-off-by: Jarkko Sakkinen jarkko@kernel.org
tools/testing/selftests/sgx/defines.h | 10 ++++ tools/testing/selftests/sgx/main.c | 57 ++++++++++++++++++----- tools/testing/selftests/sgx/test_encl.c | 19 +++++++- tools/testing/selftests/sgx/test_encl.lds | 3 +- 4 files changed, 74 insertions(+), 15 deletions(-)
Test output before applying the series:
TAP version 13 1..1 # selftests: sgx: test_sgx # Unable to open /dev/sgx_enclave: No such file or directory # 1..0 # SKIP cannot load enclaves ok 1 selftests: sgx: test_sgx # SKIP
Test output after applying second patch
selftests/sgx: Migrate to kselftest harness
Output changes to the following. It doesn't look like the second patch adds any new tests. What is the point in running the tests that fail if /dev/sgx_enclave is missing.
Unfortunately this series doesn't have a cover letter that explains what this series is doing. I don't like the fact that the test output and behavior changes when migrating the test to kselftest harness. Shouldn't the output stay the same as in skip the tests if /dev/sgx_enclave fails.
I get what you are saying but actually I do not know how with fixtures I can skip "the rest" when FIXTURE_SETUP() fails.
The reason for the output below is that with fixtures for all tests enclave is initialized for each test case. And it kind of makes sense because all tests start from the clean expected state.
I don't how to do that with zero change in the output.
Yeah. I took a look at the FIXTURE. Doesn't look like it is possible.
The reason to do this change is to make it easy to add more tests, and return correct status codes to the framework.
To add: everything I did I based purely to the existing kernel documentation, following the examples on how to use fixture.
I will pick these up and will add a note to the last commit that output changes, so test rings that run kselftest are aware of the change.
thanks, -- Shuah
OK, great, thank you!
/Jarkko
On 6/10/21 1:30 AM, Jarkko Sakkinen wrote:
Rename symbols for better clarity:
- 'eenter' might be confused for directly calling ENCLU[EENTER]. It does not. It calls into the VDSO, which actually has the EENTER instruction.
- 'sgx_call_vdso' is *only* used for entering the enclave. It's not some generic SGX call into the VDSO.
Signed-off-by: Jarkko Sakkinen jarkko@kernel.org
These all look fine to me. Feel free to add my ack on them.
Since these are pure x86 selftests and the initial code went through the x86 maintainers, should these got through them as well? Or, since this is only selftest code, should Shuah pick them up?
On 6/10/21 9:45 AM, Dave Hansen wrote:
On 6/10/21 1:30 AM, Jarkko Sakkinen wrote:
Rename symbols for better clarity:
- 'eenter' might be confused for directly calling ENCLU[EENTER]. It does not. It calls into the VDSO, which actually has the EENTER instruction.
- 'sgx_call_vdso' is *only* used for entering the enclave. It's not some generic SGX call into the VDSO.
Signed-off-by: Jarkko Sakkinen jarkko@kernel.org
These all look fine to me. Feel free to add my ack on them.
Since these are pure x86 selftests and the initial code went through the x86 maintainers, should these got through them as well? Or, since this is only selftest code, should Shuah pick them up?
I will queue these up for 5.14-rc1
thanks, -- Shuah
On 6/11/21 11:35 AM, Shuah Khan wrote:
On 6/10/21 9:45 AM, Dave Hansen wrote:
On 6/10/21 1:30 AM, Jarkko Sakkinen wrote:
Rename symbols for better clarity:
- 'eenter' might be confused for directly calling ENCLU[EENTER]. It
does not. It calls into the VDSO, which actually has the EENTER instruction.
- 'sgx_call_vdso' is *only* used for entering the enclave. It's not
some generic SGX call into the VDSO.
Signed-off-by: Jarkko Sakkinen jarkko@kernel.org
These all look fine to me. Feel free to add my ack on them.
Since these are pure x86 selftests and the initial code went through the x86 maintainers, should these got through them as well? Or, since this is only selftest code, should Shuah pick them up?
I will queue these up for 5.14-rc1
I almost applied these. Does this require root access, if so, please add logic to skip the test if non-root user runs it.
Please check for code paths that require root access and skip the tests.
thanks, -- Shuah
linux-kselftest-mirror@lists.linaro.org
-
Dave Hansen -
Jarkko Sakkinen -
Shuah Khan