Re: [PATCH v36 22/24] selftests/x86: Add a selftest for SGX
On Thu, Aug 27, 2020 at 08:20:51AM -0700, Sean Christopherson wrote:
> On Thu, Aug 27, 2020 at 12:47:04AM -0400, Nathaniel McCallum wrote:
> > > +int main(int argc, char *argv[], char *envp[])
> > > +{
> > > + struct sgx_enclave_exception exception;
> > > + struct vdso_symtab symtab;
> > > + Elf64_Sym *eenter_sym;
> > > + uint64_t result = 0;
> > > + struct encl encl;
> > > + unsigned int i;
> > > + void *addr;
> > > +
> > > + if (!encl_load("test_encl.elf", ))
> > > + goto err;
> > > +
> > > + if (!encl_measure())
> > > + goto err;
> > > +
> > > + if (!encl_build())
> > > + goto err;
> > > +
> > > + /*
> > > +* An enclave consumer only must do this.
> > > +*/
> > > + for (i = 0; i < encl.nr_segments; i++) {
> > > + struct encl_segment *seg = _tbl[i];
> > > +
> > > + addr = mmap((void *)encl.encl_base + seg->offset,
> > > seg->size,
> > > + seg->prot, MAP_SHARED | MAP_FIXED, encl.fd,
> > > 0);
> >
> > My patch version is a bit behind (v32), but I suspect this still
> > applies. I discovered the following by accident.
> >
> > In the Enarx code base, this invocation succeeds:
> > mmap(0x2000, 0x1000, PROT_READ | PROT_WRITE, MAP_SHARED |
> > MAP_FIXED, sgxfd, 0)
> >
> > However, this one fails with -EINVAL:
> > mmap(0x2000, 0x1000, PROT_READ | PROT_WRITE,
> > MAP_SHARED_VALIDATE | MAP_FIXED, sgxfd, 0)
> >
> > From man mmap:
> >
> >MAP_SHARED_VALIDATE (since Linux 4.15)
> > This flag provides the same behavior as MAP_SHARED
> > except that MAP_SHARED mappings ignore unknown
> > flags in flags. By contrast, when creating a mapping
> > using MAP_SHARED_VALIDATE, the kernel veri‐
> > fies all passed flags are known and fails the
> > mapping with the error EOPNOTSUPP for unknown
> > flags. This mapping type is also required to be able to
> > use some mapping flags (e.g., MAP_SYNC).
> >
> > I can try again on a newer patch set tomorrow if need be. But the
> > documentation of mmap() doesn't match the behavior I'm seeing. A brief
> > look through the patch set didn't turn up anything obvious that could
> > be causing this.
>
> This is a bug in sgx_get_unmapped_area(). EPC must be mapped SHARED, and
> so MAP_PRIVATE is disallowed. The current check is:
>
> if (flags & MAP_PRIVATE)
> return -EINVAL;
>
> and the base "flags" are:
>
> #define MAP_SHARED 0x01/* Share changes */
> #define MAP_PRIVATE 0x02/* Changes are private */
> #define MAP_SHARED_VALIDATE 0x03/* share + validate extension flags
> */
>
> which causes the SGX check to interpret MAP_SHARED_VALIDATE as MAP_PRIVATE.
> The types are just that, types, not flag modifiers. So the SGX code needs
> to be:
>
> if ((flags & MAP_TYPE) == MAP_PRIVATE)
> return -EINVAL;
Updated, thanks.
/Jarkko
Re: [PATCH v36 22/24] selftests/x86: Add a selftest for SGX
On Thu, Aug 27, 2020 at 12:47:04AM -0400, Nathaniel McCallum wrote:
> > +int main(int argc, char *argv[], char *envp[])
> > +{
> > + struct sgx_enclave_exception exception;
> > + struct vdso_symtab symtab;
> > + Elf64_Sym *eenter_sym;
> > + uint64_t result = 0;
> > + struct encl encl;
> > + unsigned int i;
> > + void *addr;
> > +
> > + if (!encl_load("test_encl.elf", ))
> > + goto err;
> > +
> > + if (!encl_measure())
> > + goto err;
> > +
> > + if (!encl_build())
> > + goto err;
> > +
> > + /*
> > +* An enclave consumer only must do this.
> > +*/
> > + for (i = 0; i < encl.nr_segments; i++) {
> > + struct encl_segment *seg = _tbl[i];
> > +
> > + addr = mmap((void *)encl.encl_base + seg->offset, seg->size,
> > + seg->prot, MAP_SHARED | MAP_FIXED, encl.fd, 0);
>
> My patch version is a bit behind (v32), but I suspect this still
> applies. I discovered the following by accident.
>
> In the Enarx code base, this invocation succeeds:
> mmap(0x2000, 0x1000, PROT_READ | PROT_WRITE, MAP_SHARED |
> MAP_FIXED, sgxfd, 0)
>
> However, this one fails with -EINVAL:
> mmap(0x2000, 0x1000, PROT_READ | PROT_WRITE,
> MAP_SHARED_VALIDATE | MAP_FIXED, sgxfd, 0)
>
> From man mmap:
>
>MAP_SHARED_VALIDATE (since Linux 4.15)
> This flag provides the same behavior as MAP_SHARED
> except that MAP_SHARED mappings ignore unknown
> flags in flags. By contrast, when creating a mapping
> using MAP_SHARED_VALIDATE, the kernel veri‐
> fies all passed flags are known and fails the
> mapping with the error EOPNOTSUPP for unknown
> flags. This mapping type is also required to be able to
> use some mapping flags (e.g., MAP_SYNC).
>
> I can try again on a newer patch set tomorrow if need be. But the
> documentation of mmap() doesn't match the behavior I'm seeing. A brief
> look through the patch set didn't turn up anything obvious that could
> be causing this.
This is a bug in sgx_get_unmapped_area(). EPC must be mapped SHARED, and
so MAP_PRIVATE is disallowed. The current check is:
if (flags & MAP_PRIVATE)
return -EINVAL;
and the base "flags" are:
#define MAP_SHARED 0x01/* Share changes */
#define MAP_PRIVATE 0x02/* Changes are private */
#define MAP_SHARED_VALIDATE 0x03/* share + validate extension flags */
which causes the SGX check to interpret MAP_SHARED_VALIDATE as MAP_PRIVATE.
The types are just that, types, not flag modifiers. So the SGX code needs
to be:
if ((flags & MAP_TYPE) == MAP_PRIVATE)
return -EINVAL;
or
unsigned long map_type = (flags & MAP_TYPE);
if (map_type != MAP_SHARED && map_type != MAP_SHARED_VALIDATE)
return -EINVAL;
Side topic, there is at least one existing bug of this nature, in mm/nommu.c.
I'll send a patch for that and look for any other instances of the bad
pattern.
Re: [PATCH v36 22/24] selftests/x86: Add a selftest for SGX
On Thu, Jul 16, 2020 at 9:58 AM Jarkko Sakkinen wrote: > > Add a selftest for SGX. It is a trivial test where a simple enclave > copies one 64-bit word of memory between two memory locations. > > Cc: linux-kselft...@vger.kernel.org > Signed-off-by: Jarkko Sakkinen > --- > tools/testing/selftests/Makefile | 1 + > tools/testing/selftests/sgx/.gitignore| 2 + > tools/testing/selftests/sgx/Makefile | 53 +++ > tools/testing/selftests/sgx/call.S| 54 +++ > tools/testing/selftests/sgx/defines.h | 21 + > tools/testing/selftests/sgx/load.c| 282 + > tools/testing/selftests/sgx/main.c| 199 + > tools/testing/selftests/sgx/main.h| 38 ++ > tools/testing/selftests/sgx/sigstruct.c | 395 ++ > tools/testing/selftests/sgx/test_encl.c | 20 + > tools/testing/selftests/sgx/test_encl.lds | 40 ++ > .../selftests/sgx/test_encl_bootstrap.S | 89 > 12 files changed, 1194 insertions(+) > create mode 100644 tools/testing/selftests/sgx/.gitignore > create mode 100644 tools/testing/selftests/sgx/Makefile > create mode 100644 tools/testing/selftests/sgx/call.S > create mode 100644 tools/testing/selftests/sgx/defines.h > create mode 100644 tools/testing/selftests/sgx/load.c > create mode 100644 tools/testing/selftests/sgx/main.c > create mode 100644 tools/testing/selftests/sgx/main.h > create mode 100644 tools/testing/selftests/sgx/sigstruct.c > create mode 100644 tools/testing/selftests/sgx/test_encl.c > create mode 100644 tools/testing/selftests/sgx/test_encl.lds > create mode 100644 tools/testing/selftests/sgx/test_encl_bootstrap.S > > diff --git a/tools/testing/selftests/Makefile > b/tools/testing/selftests/Makefile > index 1195bd85af38..ec7be6d5a10d 100644 > --- a/tools/testing/selftests/Makefile > +++ b/tools/testing/selftests/Makefile > @@ -64,6 +64,7 @@ TARGETS += user > TARGETS += vm > TARGETS += x86 > TARGETS += zram > +TARGETS += sgx > #Please keep the TARGETS list alphabetically sorted > # Run "make quicktest=1 run_tests" or > # "make quicktest=1 kselftest" from top level Makefile > diff --git a/tools/testing/selftests/sgx/.gitignore > b/tools/testing/selftests/sgx/.gitignore > new file mode 100644 > index ..fbaf0bda9a92 > --- /dev/null > +++ b/tools/testing/selftests/sgx/.gitignore > @@ -0,0 +1,2 @@ > +test_sgx > +test_encl.elf > diff --git a/tools/testing/selftests/sgx/Makefile > b/tools/testing/selftests/sgx/Makefile > new file mode 100644 > index ..95e5c4df8014 > --- /dev/null > +++ b/tools/testing/selftests/sgx/Makefile > @@ -0,0 +1,53 @@ > +top_srcdir = ../../../.. > + > +include ../lib.mk > + > +.PHONY: all clean > + > +CAN_BUILD_X86_64 := $(shell ../x86/check_cc.sh $(CC) \ > + ../x86/trivial_64bit_program.c) > + > +ifndef OBJCOPY > +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 \ > + -fno-stack-protector -mrdrnd $(INCLUDES) > + > +TEST_CUSTOM_PROGS := $(OUTPUT)/test_sgx $(OUTPUT)/test_encl.elf > + > +ifeq ($(CAN_BUILD_X86_64), 1) > +all: $(TEST_CUSTOM_PROGS) > +endif > + > +$(OUTPUT)/test_sgx: $(OUTPUT)/main.o \ > + $(OUTPUT)/load.o \ > + $(OUTPUT)/sigstruct.o \ > + $(OUTPUT)/call.o > + $(CC) $(HOST_CFLAGS) -o $@ $^ -lcrypto > + > +$(OUTPUT)/main.o: main.c > + $(CC) $(HOST_CFLAGS) -c $< -o $@ > + > +$(OUTPUT)/load.o: load.c > + $(CC) $(HOST_CFLAGS) -c $< -o $@ > + > +$(OUTPUT)/sigstruct.o: sigstruct.c > + $(CC) $(HOST_CFLAGS) -c $< -o $@ > + > +$(OUTPUT)/call.o: call.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 $@ > + > +EXTRA_CLEAN := \ > + $(OUTPUT)/test_encl.elf \ > + $(OUTPUT)/load.o \ > + $(OUTPUT)/call.o \ > + $(OUTPUT)/main.o \ > + $(OUTPUT)/sigstruct.o \ > + $(OUTPUT)/test_sgx \ > + $(OUTPUT)/test_sgx.o \ > diff --git a/tools/testing/selftests/sgx/call.S > b/tools/testing/selftests/sgx/call.S > new file mode 100644 > index ..77131e83db42 > --- /dev/null > +++ b/tools/testing/selftests/sgx/call.S > @@ -0,0 +1,54 @@ > +/* SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) */ > +/** > +* Copyright(c) 2016-18 Intel Corporation. > +*/ > + > + .text > + > + .macro ENCLU > + .byte 0x0f, 0x01, 0xd7 > + .endm > + > + .text > + > + .global sgx_call_vdso > +sgx_call_vdso: > + .cfi_startproc > + push%r15 > + .cfi_adjust_cfa_offset 8 > + .cfi_rel_offset %r15, 0 > + push%r14 > + .cfi_adjust_cfa_offset 8 > + .cfi_rel_offset %r14, 0 > +
[PATCH v36 22/24] selftests/x86: Add a selftest for SGX
Add a selftest for SGX. It is a trivial test where a simple enclave copies one 64-bit word of memory between two memory locations. Cc: linux-kselft...@vger.kernel.org Signed-off-by: Jarkko Sakkinen --- tools/testing/selftests/Makefile | 1 + tools/testing/selftests/sgx/.gitignore| 2 + tools/testing/selftests/sgx/Makefile | 53 +++ tools/testing/selftests/sgx/call.S| 54 +++ tools/testing/selftests/sgx/defines.h | 21 + tools/testing/selftests/sgx/load.c| 282 + tools/testing/selftests/sgx/main.c| 199 + tools/testing/selftests/sgx/main.h| 38 ++ tools/testing/selftests/sgx/sigstruct.c | 395 ++ tools/testing/selftests/sgx/test_encl.c | 20 + tools/testing/selftests/sgx/test_encl.lds | 40 ++ .../selftests/sgx/test_encl_bootstrap.S | 89 12 files changed, 1194 insertions(+) create mode 100644 tools/testing/selftests/sgx/.gitignore create mode 100644 tools/testing/selftests/sgx/Makefile create mode 100644 tools/testing/selftests/sgx/call.S create mode 100644 tools/testing/selftests/sgx/defines.h create mode 100644 tools/testing/selftests/sgx/load.c create mode 100644 tools/testing/selftests/sgx/main.c create mode 100644 tools/testing/selftests/sgx/main.h create mode 100644 tools/testing/selftests/sgx/sigstruct.c create mode 100644 tools/testing/selftests/sgx/test_encl.c create mode 100644 tools/testing/selftests/sgx/test_encl.lds create mode 100644 tools/testing/selftests/sgx/test_encl_bootstrap.S diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile index 1195bd85af38..ec7be6d5a10d 100644 --- a/tools/testing/selftests/Makefile +++ b/tools/testing/selftests/Makefile @@ -64,6 +64,7 @@ TARGETS += user TARGETS += vm TARGETS += x86 TARGETS += zram +TARGETS += sgx #Please keep the TARGETS list alphabetically sorted # Run "make quicktest=1 run_tests" or # "make quicktest=1 kselftest" from top level Makefile diff --git a/tools/testing/selftests/sgx/.gitignore b/tools/testing/selftests/sgx/.gitignore new file mode 100644 index ..fbaf0bda9a92 --- /dev/null +++ b/tools/testing/selftests/sgx/.gitignore @@ -0,0 +1,2 @@ +test_sgx +test_encl.elf diff --git a/tools/testing/selftests/sgx/Makefile b/tools/testing/selftests/sgx/Makefile new file mode 100644 index ..95e5c4df8014 --- /dev/null +++ b/tools/testing/selftests/sgx/Makefile @@ -0,0 +1,53 @@ +top_srcdir = ../../../.. + +include ../lib.mk + +.PHONY: all clean + +CAN_BUILD_X86_64 := $(shell ../x86/check_cc.sh $(CC) \ + ../x86/trivial_64bit_program.c) + +ifndef OBJCOPY +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 \ + -fno-stack-protector -mrdrnd $(INCLUDES) + +TEST_CUSTOM_PROGS := $(OUTPUT)/test_sgx $(OUTPUT)/test_encl.elf + +ifeq ($(CAN_BUILD_X86_64), 1) +all: $(TEST_CUSTOM_PROGS) +endif + +$(OUTPUT)/test_sgx: $(OUTPUT)/main.o \ + $(OUTPUT)/load.o \ + $(OUTPUT)/sigstruct.o \ + $(OUTPUT)/call.o + $(CC) $(HOST_CFLAGS) -o $@ $^ -lcrypto + +$(OUTPUT)/main.o: main.c + $(CC) $(HOST_CFLAGS) -c $< -o $@ + +$(OUTPUT)/load.o: load.c + $(CC) $(HOST_CFLAGS) -c $< -o $@ + +$(OUTPUT)/sigstruct.o: sigstruct.c + $(CC) $(HOST_CFLAGS) -c $< -o $@ + +$(OUTPUT)/call.o: call.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 $@ + +EXTRA_CLEAN := \ + $(OUTPUT)/test_encl.elf \ + $(OUTPUT)/load.o \ + $(OUTPUT)/call.o \ + $(OUTPUT)/main.o \ + $(OUTPUT)/sigstruct.o \ + $(OUTPUT)/test_sgx \ + $(OUTPUT)/test_sgx.o \ diff --git a/tools/testing/selftests/sgx/call.S b/tools/testing/selftests/sgx/call.S new file mode 100644 index ..77131e83db42 --- /dev/null +++ b/tools/testing/selftests/sgx/call.S @@ -0,0 +1,54 @@ +/* SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) */ +/** +* Copyright(c) 2016-18 Intel Corporation. +*/ + + .text + + .macro ENCLU + .byte 0x0f, 0x01, 0xd7 + .endm + + .text + + .global sgx_call_vdso +sgx_call_vdso: + .cfi_startproc + push%r15 + .cfi_adjust_cfa_offset 8 + .cfi_rel_offset %r15, 0 + push%r14 + .cfi_adjust_cfa_offset 8 + .cfi_rel_offset %r14, 0 + push%r13 + .cfi_adjust_cfa_offset 8 + .cfi_rel_offset %r13, 0 + push%r12 + .cfi_adjust_cfa_offset 8 + .cfi_rel_offset %r12, 0 + push%rbx + .cfi_adjust_cfa_offset 8 + .cfi_rel_offset %rbx, 0 + push$0 + .cfi_adjust_cfa_offset 8 + push
