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 > +
Re: [PATCH v36 21/24] x86/vdso: Implement a vDSO for Intel SGX enclave call
On Tue, Aug 18, 2020 at 12:44 PM Jarkko Sakkinen wrote: > > On Tue, Aug 18, 2020 at 11:15:32AM -0400, Nathaniel McCallum wrote: > > That seems like overkill to me. I'm just asking for one additional mov > > instruction. :) > > I started to consider eBPF since the complexity and constraints of the > callback look like an overkill and without doubt will be a burden to > maintain. That feels to me like more complexity just to move the existing complexity from one place to another.
Re: [PATCH v36 21/24] x86/vdso: Implement a vDSO for Intel SGX enclave call
That seems like overkill to me. I'm just asking for one additional mov instruction. :) On Tue, Aug 18, 2020 at 11:06 AM Jarkko Sakkinen wrote: > > On Tue, Aug 18, 2020 at 05:52:41PM +0300, Jarkko Sakkinen wrote: > > On Mon, Aug 10, 2020 at 03:23:17PM -0700, Sean Christopherson wrote: > > > > This can be done implicitly by wrapping the struct > > > > sgx_enclave_exception in another structure and then using techniques > > > > like container_of() to find another field. However, this is made more > > > > difficult by the fact that the sgx_enclave_exit_handler_t is not > > > > really using the x86_64 sysv calling convention. Therefore, the > > > > sgx_enclave_exit_handler_t MUST be written in assembly. > > > > > > What bits of the x86-64 ABI require writing the handler in assembly? > > > There > > > are certainly restrictions on what the handler can do without needing an > > > assembly trampoline, but I was under the impression that vanilla C code is > > > compatible with the exit handler patch. Is Rust more picky about calling > > > convention? > > > > > > Side topic, the documentation for vdso_sgx_enter_enclave_t is wrong, it > > > states the EFLAGS.DF is not cleared before invoking the handler, but > > > that's > > > a lie. > > > > If handler requires the use of setjmp/longjmp API for sudden exits, that > > is considered bad even with C++, as it is not compatible with stack > > unwinding. The handler has a lot of constraints for its environment, and > > is somewhat unappealing to use. > > > > That's why I started today thinking a possibility of using a bpf program > > as a middle-man. BPF programs can be used to execute code by the kernel > > in behalf of user in a domain defined sandbox. The execution context is > > just a buffer passed in R1 to the BPF interpreter. It can be defined by > > application. > > Something like > > 1. An exception is triggered. > 2. Kernel executes an eBPF program behalf of the caller, if one was >given. > 3. vDSO calls a fixed exit handler that based on the outcome calls >ERESUME/EENTER. > > Possibly an ioctl could be used to attach an eBPF program to an > enclave and vDSO would only get a context struct. > > /Jarkko >
Re: [PATCH v36 21/24] x86/vdso: Implement a vDSO for Intel SGX enclave call
On Mon, Aug 10, 2020 at 7:09 PM Andy Lutomirski wrote: > > On Thu, Aug 6, 2020 at 7:55 AM Nathaniel McCallum > wrote: > > > > In a past revision of this patch, I had requested a void *misc > > parameter that could be passed through vdso_sgx_enter_enclave_t into > > sgx_enclave_exit_handler_t. This request encountered some push back > > and I dropped the issue. However, I'd like to revisit it or something > > similar. > > Why do you need an exit handler at all? IIRC way back when I > suggested that we simply not support it at all. If you want to > call__vdso_sgx_enter_enclave() in a loop, call it in a loop. If you > want to wrap it intelligently in Rust, you don't want a callback > anyway -- that forces you have an FFI (or non-Rust, anyway) frame on > the stack, which interacts poorly with panic handling and prevents you > from using await in your Rust callback handler. If, on the other > hand, you just call __vdso_sg_enter_enclave() in a loop, all these > problems go away and, if you really want, you can pass in a callback > in Rust and call the callback from Rust. > > What am I missing? I still don't really understand why we are > supporting this mechanism at all. Just the asm code to invoke the > callback seems to be about half of the entire function. There are three ways to pass state between the enclave and the outside world: 1. A pre-allocated memory block at enclave creation time. 2. A contract for pushing values onto the stack during entry/exit. 3. All registers and flags besides rax, rbx, and rcx. Under the current vDSO function: #1 is completely possible without a handler. The challenge is how to communicate the address of this memory to the enclave. This can be accomplished by a parameter in a measured block or by convention. Otherwise, it needs to use #2 or #3 to communicate the address of the block. #2 requires a handler written in assembly. The semantics are well known. #3 is possible without a handler, but only for the subset of the registers allowed by the calling convention. However, with a handler written in assembly you can pass both in and out the full range of registers. To do this, the assembly handler needs a pointer to a buffer to save the registers into. How does it get said pointer? Currently: offsetof, which Rust doesn't support. So when I want to write a general purpose library to expose this, which method should I expose? In particular, I want to give a single "best practice" workflow to library consumers so that the startup process is easy. Since #1 requires #3 to avoid a bad developer experience, I'm inclined to provide #3. I can provide the calling convention registers easily. But this limits the output registers to two (practically, one for a variety of reasons). I might just accept that. But with a misc pointer to the handler, I could expand the one or two return registers to 11 (rdi, rsi, rdx, r8-r15). Put simply: I think the one extra line of assembly is worth the flexibility it gives to consumers of this interface. In particular, it improves the FFI experience greatly since one doesn't have to resort to offsetof tricks to get some additional context into the handler.
Re: [PATCH v36 21/24] x86/vdso: Implement a vDSO for Intel SGX enclave call
In a past revision of this patch, I had requested a void *misc
parameter that could be passed through vdso_sgx_enter_enclave_t into
sgx_enclave_exit_handler_t. This request encountered some push back
and I dropped the issue. However, I'd like to revisit it or something
similar.
One way to create a generic interface to SGX is to pass a structure
that captures the relevant CPU state from the handler so that it can
be evaluated in C code before reentry. Precedent for this approach can
be found in struct kvm_run[0]. Currently, however, there is no way to
pass a pointer to such a structure directly into the handler.
This can be done implicitly by wrapping the struct
sgx_enclave_exception in another structure and then using techniques
like container_of() to find another field. However, this is made more
difficult by the fact that the sgx_enclave_exit_handler_t is not
really using the x86_64 sysv calling convention. Therefore, the
sgx_enclave_exit_handler_t MUST be written in assembly. This also
implies that we can't use techniques like container_of() and must
calculate all the offsets manually, which is tedious, error prone and
fragile.
This is further complicated by the fact that I'm using Rust (as are a
number of other consumers), which has no native offsetof support (yes,
there is a crate for it, but it requires a number of complex
strategies to defeat the compiler which aren't great) and therefore no
container_of() support.
We could design a standard struct for this (similar to struct
kvm_run). But in order to keep performance in check we'd have to
define a limited ABI surface (to avoid things like xsave) which
wouldn't have the full flexibility of the current approach. This would
allow for a kernel provided vDSO function with a normal calling
convention, however (which does have some non-trivial value). I think
this is a trade-off we should consider (perhaps making it optional?).
But at the least, allowing a pass-through void *misc would reduce the
complexity of the assembly calculations.
[0]: https://github.com/torvalds/linux/blob/master/include/uapi/linux/kvm.h#L263
On Thu, Jul 16, 2020 at 9:58 AM Jarkko Sakkinen
wrote:
>
> From: Sean Christopherson
>
> An SGX runtime must be aware of the exceptions, which happen inside an
> enclave. Introduce a vDSO call that wraps EENTER/ERESUME cycle and returns
> the CPU exception back to the caller exactly when it happens.
>
> Kernel fixups the exception information to RDI, RSI and RDX. The SGX call
> vDSO handler fills this information to the user provided buffer or
> alternatively trigger user provided callback at the time of the exception.
>
> The calling convention is custom and does not follow System V x86-64 ABI.
>
> Suggested-by: Andy Lutomirski
> Acked-by: Jethro Beekman
> Tested-by: Jethro Beekman
> Signed-off-by: Sean Christopherson
> Co-developed-by: Cedric Xing
> Signed-off-by: Cedric Xing
> Signed-off-by: Jarkko Sakkinen
> ---
> arch/x86/entry/vdso/Makefile | 2 +
> arch/x86/entry/vdso/vdso.lds.S | 1 +
> arch/x86/entry/vdso/vsgx_enter_enclave.S | 131 +++
> arch/x86/include/asm/enclu.h | 8 ++
> arch/x86/include/uapi/asm/sgx.h | 98 +
> 5 files changed, 240 insertions(+)
> create mode 100644 arch/x86/entry/vdso/vsgx_enter_enclave.S
> create mode 100644 arch/x86/include/asm/enclu.h
>
> diff --git a/arch/x86/entry/vdso/Makefile b/arch/x86/entry/vdso/Makefile
> index ebe82b7aecda..f71ad5ebd0c4 100644
> --- a/arch/x86/entry/vdso/Makefile
> +++ b/arch/x86/entry/vdso/Makefile
> @@ -29,6 +29,7 @@ VDSO32-$(CONFIG_IA32_EMULATION) := y
> vobjs-y := vdso-note.o vclock_gettime.o vgetcpu.o
> vobjs32-y := vdso32/note.o vdso32/system_call.o vdso32/sigreturn.o
> vobjs32-y += vdso32/vclock_gettime.o
> +vobjs-$(VDSO64-y) += vsgx_enter_enclave.o
>
> # files to link into kernel
> obj-y += vma.o extable.o
> @@ -100,6 +101,7 @@ $(vobjs): KBUILD_CFLAGS := $(filter-out
> $(GCC_PLUGINS_CFLAGS) $(RETPOLINE_CFLAGS
> CFLAGS_REMOVE_vclock_gettime.o = -pg
> CFLAGS_REMOVE_vdso32/vclock_gettime.o = -pg
> CFLAGS_REMOVE_vgetcpu.o = -pg
> +CFLAGS_REMOVE_vsgx_enter_enclave.o = -pg
>
> #
> # X32 processes use x32 vDSO to access 64bit kernel data.
> diff --git a/arch/x86/entry/vdso/vdso.lds.S b/arch/x86/entry/vdso/vdso.lds.S
> index 36b644e16272..4bf48462fca7 100644
> --- a/arch/x86/entry/vdso/vdso.lds.S
> +++ b/arch/x86/entry/vdso/vdso.lds.S
> @@ -27,6 +27,7 @@ VERSION {
> __vdso_time;
> clock_getres;
> __vdso_clock_getres;
> + __vdso_sgx_enter_enclave;
> local: *;
> };
> }
> diff --git a/arch/x86/entry/vdso/vsgx_enter_enclave.S
> b/arch/x86/entry/vdso/vsgx_enter_enclave.S
> new file mode 100644
> index ..be7e467e1efb
> --- /dev/null
> +++ b/arch/x86/entry/vdso/vsgx_enter_enclave.S
> @@ -0,0 +1,131 @@
> +/* SPDX-License-Identifier: GPL-2.0
Re: [PATCH v29 00/20] Intel SGX foundations
On Thu, May 14, 2020 at 5:17 AM Dr. Greg wrote: > > On Fri, May 08, 2020 at 12:56:35PM -0700, Sean Christopherson wrote: > > Good morning, I hope the week is proceeding well for everyone. > > > On Fri, May 08, 2020 at 02:02:26PM -0500, Dr. Greg wrote: > > > On Thu, May 07, 2020 at 02:41:30AM +0200, Thomas Gleixner wrote: > > > > The diffstat of your patch is irrelevant. What's relevant is the > > > > fact that it is completely unreviewed and that it creates yet > > > > another user space visible ABI with a noticable lack of > > > > documentation. > > > > ... > > > > > As to lack of review, we would certainly welcome technical and API > > > comments but we cannot force them. > > > Thomas' point isn't that your code needs to be reviewed, it's that > > trying to squeeze it into the initial merge will, not might, _will_ > > push out the acceptance of SGX. The same is true for other features > > we have lined up, e.g. KVM and cgroup support. It's not a slight on > > your code, it's just reality at this point. > > For the record and for whatever it is worth at this point. The patch > has been available in its present form and function for around 14 > months. > > So there was plenty of time for its consideration and integration into > what is being prepared as the final merge candidate. > > > > In fact, anyone who reviews the patch will see that the current driver > > > creates a pointer in the ioctl call to pass downward into the hardware > > > initialization routines. Our code simply replaces that pointer with > > > one supplied by userspace. > > > Personally, I'm in favor of adding a reserved placeholder for a > > token so as to avoid adding a second ioctl() in the event that > > something gets upstreamed that wants the token. Ditto for a file > > descriptor for the backing store in sgx_enclave_create. > > That would be a very low cost and forward looking addition. > > > > At the very least a modular form of the driver should be > > > considered that would allow alternate implementations. Sean > > > indicated that there was a 'kludgy' approach that would allow an > > > alternate modular implementation alongside the in-kernel driver. > > > I believe that Andy has already indicated that may not be an > > > advisable approach. > > > Modularizing the the driver does nothing for your use case. The > > "driver" is a relatively lightweight wrapper, removing that is akin > > to making /dev/sgx/enclave inaccessible, i.e. it doesn't make the > > EPC tracking and core code go away. Modularizing the whole thing is > > flat out not an option, as doing so is not compatible with an EPC > > cgroup and adds unnecessary complexity to KVM enabling, both of > > which are highly desired features by pretty much everyone that plans > > on using SGX. > > All well taken technical points, but they don't speak directly to the > issue at hand. The potential security issue in all of this is access > to /dev/sgx/enclave, not the EPC tracking and core code. > > Here in a nutshell is the paradox the kernel faces: > > No one seems to be disputing the fact that the primary focus of this > driver will be to support the notion of 'runtime encryption' and > Confidential Computing. The whole premise of the concept and economic > predicate of the initiative is that the owner/manager of the platform > has no visibility into what is being done on the platform. > > If the Linux community believes that standard platform security > controls can make qualitatively good judgements on what enclave based > execution is doing, it is effectively making the statement that the > very concept of runtime encryption and by extension Confidential > Computing is invalid. > > If we accept the concept that Confidential Computing is valid then we > admit the technology provides the opportunity for unsupervised code > execution and data manipulation. > > Our premise in all of this is that one page of code implementing three > linked lists is a small price to pay to provide platform owners with > the opportunity to optionally implement what is effectively 2-factor > authentication over this process. > > Going forward, if we are intellectually honest, all of this suggests > that adding complexity to the driver with LSM controls makes little > sense technically. Since, by definition and economic intent, the > technology provides tools and infrastructure that allows these > controls to be evaded. > > The notion that entities with adversarial intent would not try and > take advantage of this flies in the face of everything we know about > security. > > > Andy is right to caution against playing games to squish in-kernel > > things, but the virtualization snafu is a completely different > > beast. E.g. SGX doesn't require fiddling with CR4, won't corrupt > > random memory across kexec(), doesn't block INIT, etc... And I'm > > not advocating long-term shenanigans, I just wanted to point out > > that there are options for running out-of-tree drivers in the short
Re: [PATCH v29 00/20] Intel SGX foundations
On Thu, May 7, 2020 at 1:03 AM Haitao Huang wrote: > > On Wed, 06 May 2020 17:14:22 -0500, Sean Christopherson > wrote: > > > On Wed, May 06, 2020 at 05:42:42PM -0400, Nathaniel McCallum wrote: > >> Tested on Enarx. This requires a patch[0] for v29 support. > >> > >> Tested-by: Nathaniel McCallum > >> > >> However, we did uncover a small usability issue. See below. > >> > >> [0]: > >> https://github.com/enarx/enarx/pull/507/commits/80da2352aba46aa7bc6b4d1fccf20fe1bda58662 > > > > ... > > > >> > * Disallow mmap(PROT_NONE) from /dev/sgx. Any mapping (e.g. > >> anonymous) can > >> > be used to reserve the address range. Now /dev/sgx supports only > >> opaque > >> > mappings to the (initialized) enclave data. > >> > >> The statement "Any mapping..." isn't actually true. > >> > >> Enarx creates a large enclave (currently 64GiB). This worked when we > >> created a file-backed mapping on /dev/sgx/enclave. However, switching > >> to an anonymous mapping fails with ENOMEM. We suspect this is because > >> the kernel attempts to allocate all the pages and zero them but there > >> is insufficient RAM available. We currently work around this by > >> creating a shared mapping on /dev/zero. > > > > Hmm, the kernel shouldn't actually allocate physical pages unless they're > > written. I'll see if I can reproduce. > > > > For larger size mmap, I think it requires enabling vm overcommit mode 1: > echo 1 | sudo tee /proc/sys/vm/overcommit_memory Which means the default experience isn't good.
Re: [PATCH v29 00/20] Intel SGX foundations
Tested on Enarx. This requires a patch[0] for v29 support.
Tested-by: Nathaniel McCallum
However, we did uncover a small usability issue. See below.
[0]:
https://github.com/enarx/enarx/pull/507/commits/80da2352aba46aa7bc6b4d1fccf20fe1bda58662
On Tue, Apr 21, 2020 at 5:53 PM Jarkko Sakkinen
wrote:
>
> Intel(R) SGX is a set of CPU instructions that can be used by applications
> to set aside private regions of code and data. The code outside the enclave
> is disallowed to access the memory inside the enclave by the CPU access
> control.
>
> There is a new hardware unit in the processor called Memory Encryption
> Engine (MEE) starting from the Skylake microacrhitecture. BIOS can define
> one or many MEE regions that can hold enclave data by configuring them with
> PRMRR registers.
>
> The MEE automatically encrypts the data leaving the processor package to
> the MEE regions. The data is encrypted using a random key whose life-time
> is exactly one power cycle.
>
> The current implementation requires that the firmware sets
> IA32_SGXLEPUBKEYHASH* MSRs as writable so that ultimately the kernel can
> decide what enclaves it wants run. The implementation does not create
> any bottlenecks to support read-only MSRs later on.
>
> You can tell if your CPU supports SGX by looking into /proc/cpuinfo:
>
> cat /proc/cpuinfo | grep sgx
>
> v29:
> * The selftest has been moved to selftests/sgx. Because SGX is an execution
> environment of its own, it really isn't a great fit with more "standard"
> x86 tests.
>
> The RSA key is now generated on fly and the whole signing process has
> been made as part of the enclave loader instead of signing the enclave
> during the compilation time.
>
> Finally, the enclave loader loads now the test enclave directly from its
> ELF file, which means that ELF file does not need to be coverted as raw
> binary during the build process.
> * Version the mm_list instead of using on synchronize_mm() when adding new
> entries. We hold the write lock for the mm_struct, and dup_mm() can thus
> deadlock with the page reclaimer, which could hold the lock for the old
> mm_struct.
> * Disallow mmap(PROT_NONE) from /dev/sgx. Any mapping (e.g. anonymous) can
> be used to reserve the address range. Now /dev/sgx supports only opaque
> mappings to the (initialized) enclave data.
The statement "Any mapping..." isn't actually true.
Enarx creates a large enclave (currently 64GiB). This worked when we
created a file-backed mapping on /dev/sgx/enclave. However, switching
to an anonymous mapping fails with ENOMEM. We suspect this is because
the kernel attempts to allocate all the pages and zero them but there
is insufficient RAM available. We currently work around this by
creating a shared mapping on /dev/zero.
If we want to keep this mmap() strategy, we probably don't want to
advise mmap(ANON) if it allocates all the memory for the enclave ahead
of time, even if it won't be used. This would be wasteful.
OTOH, having to mmap("/dev/zero") seems a bit awkward.
Currently, the selftest uses mmap(ANON) and the patch message above
recommends it.
> * Make the vDSO callable directly from C by preserving RBX and taking leaf
> from RCX.
>
> v28:
> * Documented to Documentation/x86/sgx.rst how the kernel manages the
> enclave ownership.
> * Removed non-LC flow from sgx_einit().
> * Removed struct sgx_einittoken since only the size of the corresponding
> microarchitectural structure is used in the series ATM.
>
> v27:
> * Disallow RIE processes to use enclaves as there could a permission
> conflict between VMA and enclave permissions.
> * In the documentation, replace "grep /proc/cpuinfo" with
> "grep sgx /proc/cpuinfo".
>
> v26:
> * Fixed the commit author in "x86/sgx: Linux Enclave Driver", which was
> changed in v25 by mistake.
> * Addressed a bunch of grammar mistakes in sgx.rst (thanks Randy once
> again for such a detailed feedback).
> * Added back the MAINTAINERS update commit, which was mistakenly removed
> in v25.
> * EREMOVE's for SECS cannot be done while sanitizing an EPC section. The
> CPU does not allow to remove a SECS page before all of its children have
> been removed, and a child page can be in some other section than the one
> currently being processed. Thus, removed special SECS processing from
> sgx_sanitize_page() and instead put sections through it twice. In the
> 2nd round the lists should only contain SECS pages.
>
> v25:
> * Fix a double-free issue when SGX_IOC_ENCLAVE_ADD_PAGES
> fails on executing ENCLS[EADD]. The rollback path executed
> radix_tree_delete() on the same address twice when this happened.
> * Return -EINTR instead of -
SEV Command Privilege Separation
I've been working on wrapping various SEV kernel APIs for userspace consumption. There does not appear to be any privilege separation for these commands: you can run them all or none of them. This is less than ideal because it means that a compromise of the code which launches VMs could make permanent changes to the SEV certificate chain. These commands are required to attest the VM environment: SEV_PDH_CERT_EXPORT SEV_PLATFORM_STATUS SEV_GET_ID These commands manage the SEV certificate chain: SEV_PEK_CERT_IMPORT SEV_FACTORY_RESET SEV_PEK_GEN SEV_PEK_CSR SEV_PDH_GEN I would expect the first group of commands to be able to be called by whatever actor launches VMs. The latter group of commands should be able to be called by whatever actor manages the SEV environment. I don't have strong opinions on how this privilege separation should happen. It might be sufficient to distinguish these based on the mode of the open() call. This could then be managed with filesystem permissions. But I'm open to other ideas.
Re: [PATCH] crypto: ccp: introduce SEV_GET_ID2 command
I'm a little concerned that this immediately disables SEV_GET_ID.
IMHO, we should continue to support both for a period of time. One
justification for immediate disablement would be that if keeping it
around is likely to enabled incorrect or insecure userspace behavior
with a firmware change. Given that this value is passed directly to
the AMD server, I don't think either of these is the case and it can
probably be worked around on the server side.
On Wed, Feb 13, 2019 at 1:24 PM Singh, Brijesh wrote:
>
> The current definition and implementation of the SEV_GET_ID command
> does not provide the length of the unique ID returned by the firmware.
> As per the firmware specification, the firmware may return an ID
> length that is not restricted to 64 bytes as assumed by the SEV_GET_ID
> command.
>
> Introduce the SEV_GET_ID2 command to allow for querying and returing
> the length of the ID. Deprecate the SEV_GET_ID in the favor of
> SEV_GET_ID2.
>
> Cc: Janakarajan Natarajan
> Cc: Herbert Xu
> Cc: Gary Hook
> Cc: Tom Lendacky
> Signed-off-by: Brijesh Singh
> ---
> drivers/crypto/ccp/psp-dev.c | 65 +---
> include/uapi/linux/psp-sev.h | 18 +++---
> 2 files changed, 60 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
> index b16be8a11d92..b510900a9a83 100644
> --- a/drivers/crypto/ccp/psp-dev.c
> +++ b/drivers/crypto/ccp/psp-dev.c
> @@ -584,40 +584,63 @@ static int sev_ioctl_do_pek_import(struct sev_issue_cmd
> *argp)
>
> static int sev_ioctl_do_get_id(struct sev_issue_cmd *argp)
> {
> + struct sev_user_data_get_id2 input;
> struct sev_data_get_id *data;
> - u64 data_size, user_size;
> - void *id_blob, *mem;
> + void *id_blob = NULL;
> int ret;
>
> - /* SEV GET_ID available from SEV API v0.16 and up */
> + /* SEV GET_ID is available from SEV API v0.16 and up */
> if (!SEV_VERSION_GREATER_OR_EQUAL(0, 16))
> return -ENOTSUPP;
>
> - /* SEV FW expects the buffer it fills with the ID to be
> -* 8-byte aligned. Memory allocated should be enough to
> -* hold data structure + alignment padding + memory
> -* where SEV FW writes the ID.
> -*/
> - data_size = ALIGN(sizeof(struct sev_data_get_id), 8);
> - user_size = sizeof(struct sev_user_data_get_id);
> + if (copy_from_user(, (void __user *)argp->data, sizeof(input)))
> + return -EFAULT;
>
> - mem = kzalloc(data_size + user_size, GFP_KERNEL);
> - if (!mem)
> + /* Check if we have write access to the userspace buffer */
> + if (input.address &&
> + input.length &&
> + !access_ok(input.address, input.length))
> + return -EFAULT;
> +
> + data = kzalloc(sizeof(*data), GFP_KERNEL);
> + if (!data)
> return -ENOMEM;
>
> - data = mem;
> - id_blob = mem + data_size;
> + if (input.address && input.length) {
> + id_blob = kmalloc(input.length, GFP_KERNEL);
> + if (!id_blob) {
> + kfree(data);
> + return -ENOMEM;
> + }
>
> - data->address = __psp_pa(id_blob);
> - data->len = user_size;
> + data->address = __psp_pa(id_blob);
> + data->len = input.length;
> + }
>
> ret = __sev_do_cmd_locked(SEV_CMD_GET_ID, data, >error);
> - if (!ret) {
> - if (copy_to_user((void __user *)argp->data, id_blob,
> data->len))
> +
> + /*
> +* Firmware will return the length of the ID value (either the minimum
> +* required length or the actual length written), return it to the
> user.
> +*/
> + input.length = data->len;
> +
> + if (copy_to_user((void __user *)argp->data, , sizeof(input))) {
> + ret = -EFAULT;
> + goto e_free;
> + }
> +
> + if (id_blob) {
> + if (copy_to_user((void __user *)input.address,
> +id_blob, data->len)) {
> ret = -EFAULT;
> + goto e_free;
> + }
> }
>
> - kfree(mem);
> +e_free:
> + kfree(id_blob);
> + kfree(data);
>
> return ret;
> }
> @@ -760,6 +783,10 @@ static long sev_ioctl(struct file *file, unsigned int
> ioctl, unsigned long arg)
> ret = sev_ioctl_do_pdh_export();
> break;
> case SEV_GET_ID:
> + /* SEV_GET_ID is deprecated */
> + ret = -ENOTSUPP;
> + break;
> + case SEV_GET_ID2:
> ret = sev_ioctl_do_get_id();
> break;
> default:
> diff --git a/include/uapi/linux/psp-sev.h b/include/uapi/linux/psp-sev.h
> index ac8c60bcc83b..43521d500c2b 100644
> --- a/include/uapi/linux/psp-sev.h
> +++ b/include/uapi/linux/psp-sev.h
> @@
Re: [intel-sgx-kernel-dev] [PATCH v11 13/13] intel_sgx: in-kernel launch enclave
On Tue, Jun 26, 2018 at 4:44 AM Jarkko Sakkinen wrote: > > On Mon, 2018-06-25 at 08:45 -0700, Andy Lutomirski wrote: > > I'm personally rather strongly in favor of the vastly simpler model in > > which we first merge SGX without LE support at all. Instead we use > > the approach where we just twiddle the MSRs to launch normal enclaves > > without an init token at all, which is probably considerably faster > > and will remove several thousand lines of code. If and when a bona > > fide use case for LE support shows up, we can work out the details and > > merge it. > > Andy, I was going to propose exactly the same :-) > > We can upstream SGX that supports only unlocked MSRs and that does > not preventing to upstream support for locked MSRs later. Even if > we had a consensus for locked MSRs, making two milestones for the > mainline would make perfect sense. > > I came into this conclusion last night because all the other review > comments not concerning the launch control are easily sorted out. +1. Let's do this and get it merged without launch enclave support lockdown now. We can revisit this once we have hands on experience with the technology.
Re: [intel-sgx-kernel-dev] [PATCH v11 13/13] intel_sgx: in-kernel launch enclave
On Tue, Jun 26, 2018 at 4:44 AM Jarkko Sakkinen wrote: > > On Mon, 2018-06-25 at 08:45 -0700, Andy Lutomirski wrote: > > I'm personally rather strongly in favor of the vastly simpler model in > > which we first merge SGX without LE support at all. Instead we use > > the approach where we just twiddle the MSRs to launch normal enclaves > > without an init token at all, which is probably considerably faster > > and will remove several thousand lines of code. If and when a bona > > fide use case for LE support shows up, we can work out the details and > > merge it. > > Andy, I was going to propose exactly the same :-) > > We can upstream SGX that supports only unlocked MSRs and that does > not preventing to upstream support for locked MSRs later. Even if > we had a consensus for locked MSRs, making two milestones for the > mainline would make perfect sense. > > I came into this conclusion last night because all the other review > comments not concerning the launch control are easily sorted out. +1. Let's do this and get it merged without launch enclave support lockdown now. We can revisit this once we have hands on experience with the technology.
Re: [intel-sgx-kernel-dev] [PATCH v11 13/13] intel_sgx: in-kernel launch enclave
On Mon, Jun 25, 2018 at 11:45 AM Andy Lutomirski wrote: > > On Mon, Jun 25, 2018 at 2:41 AM Jarkko Sakkinen > wrote: > > > > On Thu, 2018-06-21 at 08:32 -0400, Nathaniel McCallum wrote: > > > This implies that it should be possible to create MSR activation (and > > > an embedded launch enclave?) entirely as a UEFI module. The kernel > > > would still get to manage who has access to /dev/sgx and other > > > important non-cryptographic policy details. Users would still be able > > > to control the cryptographic policy details (via BIOS Secure Boot > > > configuration that exists today). Distributions could still control > > > cryptographic policy details via signing of the UEFI module with their > > > own Secure Boot key (or using something like shim). The UEFI module > > > (and possibly the external launch enclave) could be distributed via > > > linux-firmware. > > > > > > Andy/Neil, does this work for you? > > > > Nothing against having UEFI module for MSR activation step. > > > > And we would move the existing in-kernel LE to firmware so that it is > > avaible for locked-in-to-non-Intel-values case? > > > > This is a hell of a lot of complexity. To get it right we'd need an > actual formal spec of what firmware is supposed to do and how it > integrates with the kernel, and we'd need a reason why it's useful. What do you want the kernel's behavior to be in the case where an FLC CPU is present, but the MSRs have been locked by the BIOS? Because I think the workflow for the UEFI module idea is the same. > I'm personally rather strongly in favor of the vastly simpler model in > which we first merge SGX without LE support at all. Instead we use > the approach where we just twiddle the MSRs to launch normal enclaves > without an init token at all, which is probably considerably faster > and will remove several thousand lines of code. If and when a bona > fide use case for LE support shows up, we can work out the details and > merge it. I'm also okay with an incremental approach, BTW. I just want us to think through the issues. And I do think that SGX root kits are a serious threat. But I'm willing to move in stages. In fact, if we can merge something without any LE support faster, I'm in favor of doing so. > Right now, we're talking about a lot of design considerations, a lot > of interoperability considerations, and a lot of code to support a use > case that doesn't clearly exist. I agree.
Re: [intel-sgx-kernel-dev] [PATCH v11 13/13] intel_sgx: in-kernel launch enclave
On Mon, Jun 25, 2018 at 11:45 AM Andy Lutomirski wrote: > > On Mon, Jun 25, 2018 at 2:41 AM Jarkko Sakkinen > wrote: > > > > On Thu, 2018-06-21 at 08:32 -0400, Nathaniel McCallum wrote: > > > This implies that it should be possible to create MSR activation (and > > > an embedded launch enclave?) entirely as a UEFI module. The kernel > > > would still get to manage who has access to /dev/sgx and other > > > important non-cryptographic policy details. Users would still be able > > > to control the cryptographic policy details (via BIOS Secure Boot > > > configuration that exists today). Distributions could still control > > > cryptographic policy details via signing of the UEFI module with their > > > own Secure Boot key (or using something like shim). The UEFI module > > > (and possibly the external launch enclave) could be distributed via > > > linux-firmware. > > > > > > Andy/Neil, does this work for you? > > > > Nothing against having UEFI module for MSR activation step. > > > > And we would move the existing in-kernel LE to firmware so that it is > > avaible for locked-in-to-non-Intel-values case? > > > > This is a hell of a lot of complexity. To get it right we'd need an > actual formal spec of what firmware is supposed to do and how it > integrates with the kernel, and we'd need a reason why it's useful. What do you want the kernel's behavior to be in the case where an FLC CPU is present, but the MSRs have been locked by the BIOS? Because I think the workflow for the UEFI module idea is the same. > I'm personally rather strongly in favor of the vastly simpler model in > which we first merge SGX without LE support at all. Instead we use > the approach where we just twiddle the MSRs to launch normal enclaves > without an init token at all, which is probably considerably faster > and will remove several thousand lines of code. If and when a bona > fide use case for LE support shows up, we can work out the details and > merge it. I'm also okay with an incremental approach, BTW. I just want us to think through the issues. And I do think that SGX root kits are a serious threat. But I'm willing to move in stages. In fact, if we can merge something without any LE support faster, I'm in favor of doing so. > Right now, we're talking about a lot of design considerations, a lot > of interoperability considerations, and a lot of code to support a use > case that doesn't clearly exist. I agree.
Re: [intel-sgx-kernel-dev] [PATCH v11 13/13] intel_sgx: in-kernel launch enclave
On Mon, Jun 25, 2018 at 5:28 AM Jarkko Sakkinen wrote: > > On Wed, 2018-06-20 at 12:28 -0400, Nathaniel McCallum wrote: > > As I understand it, the current policy models under discussion look like > > this: > > > > 1. SGX w/o FLC (not being merged) looks like this: > > Intel CPU => (Intel signed) launch enclave => enclaves > > > > 2. SGX w/ FLC, looks like this: > > Intel CPU => kernel => launch enclave => enclaves > > > > 3. Andy is proposing this: > > Intel CPU => kernel => enclaves > > What if MSRs are not writable after hand over to the OS? It is a legit > configuration at least according to the SDM. It seems to me that "set the MSRs in the BIOS" and "set the MSRs in a UEFI module" are functionally equivalent.
Re: [intel-sgx-kernel-dev] [PATCH v11 13/13] intel_sgx: in-kernel launch enclave
On Mon, Jun 25, 2018 at 5:28 AM Jarkko Sakkinen wrote: > > On Wed, 2018-06-20 at 12:28 -0400, Nathaniel McCallum wrote: > > As I understand it, the current policy models under discussion look like > > this: > > > > 1. SGX w/o FLC (not being merged) looks like this: > > Intel CPU => (Intel signed) launch enclave => enclaves > > > > 2. SGX w/ FLC, looks like this: > > Intel CPU => kernel => launch enclave => enclaves > > > > 3. Andy is proposing this: > > Intel CPU => kernel => enclaves > > What if MSRs are not writable after hand over to the OS? It is a legit > configuration at least according to the SDM. It seems to me that "set the MSRs in the BIOS" and "set the MSRs in a UEFI module" are functionally equivalent.
Re: [intel-sgx-kernel-dev] [PATCH v11 13/13] intel_sgx: in-kernel launch enclave
On Thu, Jun 21, 2018 at 6:49 PM Andy Lutomirski wrote: > > On Thu, Jun 21, 2018 at 12:11 PM Nathaniel McCallum > wrote: > > > > If this is acceptable for everyone, my hope is the following: > > > > 1. Intel would split the existing code into one of the following > > schemas (I don't care which): > > A. three parts: UEFI module, FLC-only kernel driver and user-space > > launch enclave > > B. two parts: UEFI module (including launch enclave) and FLC-only > > kernel driver > > > > 2. Intel would release a reproducible build of the GPL UEFI module > > sources signed with a SecureBoot trusted key and provide an > > acceptable[0] binary redistribution license. > > > > 3. The kernel community would agree to merge the kernel driver given > > the above criteria (and, obviously, acceptable kernel code). > > > > The question of how to distribute the UEFI module and possible launch > > enclave remains open. I see two options: independent distribution and > > bundling it in linux-firmware. The former may be a better > > technological fit since the UEFI module will likely need to be run > > before the kernel (and the boot loader; and shim). However, the latter > > has the benefit of already being a well-known entity to our downstream > > distributors. I could go either way on this. > > This is a lot of complication and effort for a gain that is not > entirely clear. Root kits and evil maid attacks are two worth considering. > I really really really do *not* want to see Intel or > anyone else start enforcing policy on which programs can and cannot > run using this mechanism. We already do this. It is called SecureBoot. > (This is exactly why non-FLC systems aren't > about to be supported upstream.) So my preference is to not merge > anything that supports this type of use case unless there is > compelling evidence that it is (a) genuinely useful, (b) will be used > to improve security and (c) won't be abused for, say, revenue > purposes. I think there are benefits for (a) and (b). I agree with you about (c). But, again, we already have SecureBoot.
Re: [intel-sgx-kernel-dev] [PATCH v11 13/13] intel_sgx: in-kernel launch enclave
On Thu, Jun 21, 2018 at 6:49 PM Andy Lutomirski wrote: > > On Thu, Jun 21, 2018 at 12:11 PM Nathaniel McCallum > wrote: > > > > If this is acceptable for everyone, my hope is the following: > > > > 1. Intel would split the existing code into one of the following > > schemas (I don't care which): > > A. three parts: UEFI module, FLC-only kernel driver and user-space > > launch enclave > > B. two parts: UEFI module (including launch enclave) and FLC-only > > kernel driver > > > > 2. Intel would release a reproducible build of the GPL UEFI module > > sources signed with a SecureBoot trusted key and provide an > > acceptable[0] binary redistribution license. > > > > 3. The kernel community would agree to merge the kernel driver given > > the above criteria (and, obviously, acceptable kernel code). > > > > The question of how to distribute the UEFI module and possible launch > > enclave remains open. I see two options: independent distribution and > > bundling it in linux-firmware. The former may be a better > > technological fit since the UEFI module will likely need to be run > > before the kernel (and the boot loader; and shim). However, the latter > > has the benefit of already being a well-known entity to our downstream > > distributors. I could go either way on this. > > This is a lot of complication and effort for a gain that is not > entirely clear. Root kits and evil maid attacks are two worth considering. > I really really really do *not* want to see Intel or > anyone else start enforcing policy on which programs can and cannot > run using this mechanism. We already do this. It is called SecureBoot. > (This is exactly why non-FLC systems aren't > about to be supported upstream.) So my preference is to not merge > anything that supports this type of use case unless there is > compelling evidence that it is (a) genuinely useful, (b) will be used > to improve security and (c) won't be abused for, say, revenue > purposes. I think there are benefits for (a) and (b). I agree with you about (c). But, again, we already have SecureBoot.
Re: [intel-sgx-kernel-dev] [PATCH v11 13/13] intel_sgx: in-kernel launch enclave
On Thu, Jun 21, 2018 at 5:21 PM Sean Christopherson wrote: > > On Thu, Jun 21, 2018 at 03:11:18PM -0400, Nathaniel McCallum wrote: > > If this is acceptable for everyone, my hope is the following: > > > > 1. Intel would split the existing code into one of the following > > schemas (I don't care which): > > A. three parts: UEFI module, FLC-only kernel driver and user-space > > launch enclave > > B. two parts: UEFI module (including launch enclave) and FLC-only > > kernel driver > > To make sure I understand correctly... > > The UEFI module would lock the LE MSRs with a public key hardcoded > into both the UEFI module and the kernel at build time? > > And for the kernel, it would only load its SGX driver if FLC is > supported and the MSRs are locked to the expected key? > > IIUC, this approach will cause problems for virtualization. Running > VMs with different LE keys would require the bare metal firmware to > configure the LE MSRs to be unlocked, which would effectively make > using SGX in the host OS mutually exlusive with exposing SGX to KVM > guests. Theoretically it would be possible for KVM to emulate the > guest's LE and use the host's LE to generate EINIT tokens, but > emulating an enclave would likely require a massive amount of code > and/or complexity. How is this different from any other scenario where you lock the LE MSRs? Unless Intel provides hardware support between the LE MSRs and the VMX instructions, I don't see any way around this besides letting any launch enclave run. > > 2. Intel would release a reproducible build of the GPL UEFI module > > sources signed with a SecureBoot trusted key and provide an > > acceptable[0] binary redistribution license. > > > > 3. The kernel community would agree to merge the kernel driver given > > the above criteria (and, obviously, acceptable kernel code). > > > > The question of how to distribute the UEFI module and possible launch > > enclave remains open. I see two options: independent distribution and > > bundling it in linux-firmware. The former may be a better > > technological fit since the UEFI module will likely need to be run > > before the kernel (and the boot loader; and shim). However, the latter > > has the benefit of already being a well-known entity to our downstream > > distributors. I could go either way on this. > > Writing and locks the LE MSRs effectively needs to be done before > running the bootloader/kernel/etc... Delaying activation would > require, at a minimum, leaving IA32_FEATURE_CONTROL unlocked since > IA32_FEATURE_CONTROL's SGX bits can't be set until SGX is activated. > > > I know this plan is more work for everyone involved, but I think it > > manages to actually maximize both security and freedom. > > > > [0]: details here - > > https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/README#n19 > > On Thu, Jun 21, 2018 at 11:29 AM Neil Horman wrote: > > > > > > On Thu, Jun 21, 2018 at 08:32:25AM -0400, Nathaniel McCallum wrote: > > > > On Wed, Jun 20, 2018 at 5:02 PM Sean Christopherson > > > > wrote: > > > > > > > > > > On Wed, Jun 20, 2018 at 11:39:00AM -0700, Jethro Beekman wrote: > > > > > > On 2018-06-20 11:16, Jethro Beekman wrote: > > > > > > > > This last bit is also repeated in different words in Table 35-2 > > > > > > > > and > > > > > > > > Section 42.2.2. The MSRs are *not writable* before the > > > > > > > > write-lock bit > > > > > > > > itself is locked. Meaning the MSRs are either locked with > > > > > > > > Intel's key > > > > > > > > hash, or not locked at all. > > > > > > > > > > > > Actually, this might be a documentation bug. I have some test > > > > > > hardware and I > > > > > > was able to configure the MSRs in the BIOS and then read the MSRs > > > > > > after boot > > > > > > like this: > > > > > > > > > > > > MSR 0x3a 0x00040005 > > > > > > MSR 0x8c 0x20180620 > > > > > > MSR 0x8d 0x20180620 > > > > > > MSR 0x8e 0x20180620 > > > > > > MSR 0x8f 0x20180620 > > > > > > > > > > > > Since this is not production hardware, it could also be a CPU bug > > > > > > of course. > > > > > > > > > > > > If it is indeed possible to con
Re: [intel-sgx-kernel-dev] [PATCH v11 13/13] intel_sgx: in-kernel launch enclave
On Thu, Jun 21, 2018 at 5:21 PM Sean Christopherson wrote: > > On Thu, Jun 21, 2018 at 03:11:18PM -0400, Nathaniel McCallum wrote: > > If this is acceptable for everyone, my hope is the following: > > > > 1. Intel would split the existing code into one of the following > > schemas (I don't care which): > > A. three parts: UEFI module, FLC-only kernel driver and user-space > > launch enclave > > B. two parts: UEFI module (including launch enclave) and FLC-only > > kernel driver > > To make sure I understand correctly... > > The UEFI module would lock the LE MSRs with a public key hardcoded > into both the UEFI module and the kernel at build time? > > And for the kernel, it would only load its SGX driver if FLC is > supported and the MSRs are locked to the expected key? > > IIUC, this approach will cause problems for virtualization. Running > VMs with different LE keys would require the bare metal firmware to > configure the LE MSRs to be unlocked, which would effectively make > using SGX in the host OS mutually exlusive with exposing SGX to KVM > guests. Theoretically it would be possible for KVM to emulate the > guest's LE and use the host's LE to generate EINIT tokens, but > emulating an enclave would likely require a massive amount of code > and/or complexity. How is this different from any other scenario where you lock the LE MSRs? Unless Intel provides hardware support between the LE MSRs and the VMX instructions, I don't see any way around this besides letting any launch enclave run. > > 2. Intel would release a reproducible build of the GPL UEFI module > > sources signed with a SecureBoot trusted key and provide an > > acceptable[0] binary redistribution license. > > > > 3. The kernel community would agree to merge the kernel driver given > > the above criteria (and, obviously, acceptable kernel code). > > > > The question of how to distribute the UEFI module and possible launch > > enclave remains open. I see two options: independent distribution and > > bundling it in linux-firmware. The former may be a better > > technological fit since the UEFI module will likely need to be run > > before the kernel (and the boot loader; and shim). However, the latter > > has the benefit of already being a well-known entity to our downstream > > distributors. I could go either way on this. > > Writing and locks the LE MSRs effectively needs to be done before > running the bootloader/kernel/etc... Delaying activation would > require, at a minimum, leaving IA32_FEATURE_CONTROL unlocked since > IA32_FEATURE_CONTROL's SGX bits can't be set until SGX is activated. > > > I know this plan is more work for everyone involved, but I think it > > manages to actually maximize both security and freedom. > > > > [0]: details here - > > https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/README#n19 > > On Thu, Jun 21, 2018 at 11:29 AM Neil Horman wrote: > > > > > > On Thu, Jun 21, 2018 at 08:32:25AM -0400, Nathaniel McCallum wrote: > > > > On Wed, Jun 20, 2018 at 5:02 PM Sean Christopherson > > > > wrote: > > > > > > > > > > On Wed, Jun 20, 2018 at 11:39:00AM -0700, Jethro Beekman wrote: > > > > > > On 2018-06-20 11:16, Jethro Beekman wrote: > > > > > > > > This last bit is also repeated in different words in Table 35-2 > > > > > > > > and > > > > > > > > Section 42.2.2. The MSRs are *not writable* before the > > > > > > > > write-lock bit > > > > > > > > itself is locked. Meaning the MSRs are either locked with > > > > > > > > Intel's key > > > > > > > > hash, or not locked at all. > > > > > > > > > > > > Actually, this might be a documentation bug. I have some test > > > > > > hardware and I > > > > > > was able to configure the MSRs in the BIOS and then read the MSRs > > > > > > after boot > > > > > > like this: > > > > > > > > > > > > MSR 0x3a 0x00040005 > > > > > > MSR 0x8c 0x20180620 > > > > > > MSR 0x8d 0x20180620 > > > > > > MSR 0x8e 0x20180620 > > > > > > MSR 0x8f 0x20180620 > > > > > > > > > > > > Since this is not production hardware, it could also be a CPU bug > > > > > > of course. > > > > > > > > > > > > If it is indeed possible to con
Re: [intel-sgx-kernel-dev] [PATCH v11 13/13] intel_sgx: in-kernel launch enclave
If this is acceptable for everyone, my hope is the following: 1. Intel would split the existing code into one of the following schemas (I don't care which): A. three parts: UEFI module, FLC-only kernel driver and user-space launch enclave B. two parts: UEFI module (including launch enclave) and FLC-only kernel driver 2. Intel would release a reproducible build of the GPL UEFI module sources signed with a SecureBoot trusted key and provide an acceptable[0] binary redistribution license. 3. The kernel community would agree to merge the kernel driver given the above criteria (and, obviously, acceptable kernel code). The question of how to distribute the UEFI module and possible launch enclave remains open. I see two options: independent distribution and bundling it in linux-firmware. The former may be a better technological fit since the UEFI module will likely need to be run before the kernel (and the boot loader; and shim). However, the latter has the benefit of already being a well-known entity to our downstream distributors. I could go either way on this. I know this plan is more work for everyone involved, but I think it manages to actually maximize both security and freedom. [0]: details here - https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/README#n19 On Thu, Jun 21, 2018 at 11:29 AM Neil Horman wrote: > > On Thu, Jun 21, 2018 at 08:32:25AM -0400, Nathaniel McCallum wrote: > > On Wed, Jun 20, 2018 at 5:02 PM Sean Christopherson > > wrote: > > > > > > On Wed, Jun 20, 2018 at 11:39:00AM -0700, Jethro Beekman wrote: > > > > On 2018-06-20 11:16, Jethro Beekman wrote: > > > > > > This last bit is also repeated in different words in Table 35-2 and > > > > > > Section 42.2.2. The MSRs are *not writable* before the write-lock > > > > > > bit > > > > > > itself is locked. Meaning the MSRs are either locked with Intel's > > > > > > key > > > > > > hash, or not locked at all. > > > > > > > > Actually, this might be a documentation bug. I have some test hardware > > > > and I > > > > was able to configure the MSRs in the BIOS and then read the MSRs after > > > > boot > > > > like this: > > > > > > > > MSR 0x3a 0x00040005 > > > > MSR 0x8c 0x20180620 > > > > MSR 0x8d 0x20180620 > > > > MSR 0x8e 0x20180620 > > > > MSR 0x8f 0x20180620 > > > > > > > > Since this is not production hardware, it could also be a CPU bug of > > > > course. > > > > > > > > If it is indeed possible to configure AND lock the MSR values to > > > > non-Intel > > > > values, I'm very much in favor of Nathaniels proposal to treat the > > > > launch > > > > enclave like any other firmware blob. > > > > > > It's not a CPU or documentation bug (though the latter is arguable). > > > SGX has an activation step that is triggered by doing a WRMSR(0x7a) > > > with bit 0 set. Until SGX is activated, the SGX related bits in > > > IA32_FEATURE_CONTROL cannot be set, i.e. SGX can't be enabled. But, > > > the LE hash MSRs are fully writable prior to activation, e.g. to > > > allow firmware to lock down the LE key with a non-Intel value. > > > > > > So yes, it's possible to lock the MSRs to a non-Intel value. The > > > obvious caveat is that whatever blob is used to write the MSRs would > > > need be executed prior to activation. > > > > This implies that it should be possible to create MSR activation (and > > an embedded launch enclave?) entirely as a UEFI module. The kernel > > would still get to manage who has access to /dev/sgx and other > > important non-cryptographic policy details. Users would still be able > > to control the cryptographic policy details (via BIOS Secure Boot > > configuration that exists today). Distributions could still control > > cryptographic policy details via signing of the UEFI module with their > > own Secure Boot key (or using something like shim). The UEFI module > > (and possibly the external launch enclave) could be distributed via > > linux-firmware. > > > > Andy/Neil, does this work for you? > > > I need some time to digest it. Who in your mind is writing the UEFI module. > Is > that the firmware vendor or IHV? > > Neil > > > > As for the SDM, it's a documentation... omission? SGX activation > > > is intentionally omitted from the SDM. The intended usage model is > > > that firmware will always do the activation (if it wants SGX enabled), > > > i.e. post-firmware software will only ever "see" SGX as disabled or > > > in the fully activated state, and so the SDM doesn't describe SGX > > > behavior prior to activation. I believe the activation process, or > > > at least what is required from firmware, is documented in the BIOS > > > writer's guide. > > > > > > > Jethro Beekman | Fortanix > > > > > > > > > >
Re: [intel-sgx-kernel-dev] [PATCH v11 13/13] intel_sgx: in-kernel launch enclave
If this is acceptable for everyone, my hope is the following: 1. Intel would split the existing code into one of the following schemas (I don't care which): A. three parts: UEFI module, FLC-only kernel driver and user-space launch enclave B. two parts: UEFI module (including launch enclave) and FLC-only kernel driver 2. Intel would release a reproducible build of the GPL UEFI module sources signed with a SecureBoot trusted key and provide an acceptable[0] binary redistribution license. 3. The kernel community would agree to merge the kernel driver given the above criteria (and, obviously, acceptable kernel code). The question of how to distribute the UEFI module and possible launch enclave remains open. I see two options: independent distribution and bundling it in linux-firmware. The former may be a better technological fit since the UEFI module will likely need to be run before the kernel (and the boot loader; and shim). However, the latter has the benefit of already being a well-known entity to our downstream distributors. I could go either way on this. I know this plan is more work for everyone involved, but I think it manages to actually maximize both security and freedom. [0]: details here - https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/README#n19 On Thu, Jun 21, 2018 at 11:29 AM Neil Horman wrote: > > On Thu, Jun 21, 2018 at 08:32:25AM -0400, Nathaniel McCallum wrote: > > On Wed, Jun 20, 2018 at 5:02 PM Sean Christopherson > > wrote: > > > > > > On Wed, Jun 20, 2018 at 11:39:00AM -0700, Jethro Beekman wrote: > > > > On 2018-06-20 11:16, Jethro Beekman wrote: > > > > > > This last bit is also repeated in different words in Table 35-2 and > > > > > > Section 42.2.2. The MSRs are *not writable* before the write-lock > > > > > > bit > > > > > > itself is locked. Meaning the MSRs are either locked with Intel's > > > > > > key > > > > > > hash, or not locked at all. > > > > > > > > Actually, this might be a documentation bug. I have some test hardware > > > > and I > > > > was able to configure the MSRs in the BIOS and then read the MSRs after > > > > boot > > > > like this: > > > > > > > > MSR 0x3a 0x00040005 > > > > MSR 0x8c 0x20180620 > > > > MSR 0x8d 0x20180620 > > > > MSR 0x8e 0x20180620 > > > > MSR 0x8f 0x20180620 > > > > > > > > Since this is not production hardware, it could also be a CPU bug of > > > > course. > > > > > > > > If it is indeed possible to configure AND lock the MSR values to > > > > non-Intel > > > > values, I'm very much in favor of Nathaniels proposal to treat the > > > > launch > > > > enclave like any other firmware blob. > > > > > > It's not a CPU or documentation bug (though the latter is arguable). > > > SGX has an activation step that is triggered by doing a WRMSR(0x7a) > > > with bit 0 set. Until SGX is activated, the SGX related bits in > > > IA32_FEATURE_CONTROL cannot be set, i.e. SGX can't be enabled. But, > > > the LE hash MSRs are fully writable prior to activation, e.g. to > > > allow firmware to lock down the LE key with a non-Intel value. > > > > > > So yes, it's possible to lock the MSRs to a non-Intel value. The > > > obvious caveat is that whatever blob is used to write the MSRs would > > > need be executed prior to activation. > > > > This implies that it should be possible to create MSR activation (and > > an embedded launch enclave?) entirely as a UEFI module. The kernel > > would still get to manage who has access to /dev/sgx and other > > important non-cryptographic policy details. Users would still be able > > to control the cryptographic policy details (via BIOS Secure Boot > > configuration that exists today). Distributions could still control > > cryptographic policy details via signing of the UEFI module with their > > own Secure Boot key (or using something like shim). The UEFI module > > (and possibly the external launch enclave) could be distributed via > > linux-firmware. > > > > Andy/Neil, does this work for you? > > > I need some time to digest it. Who in your mind is writing the UEFI module. > Is > that the firmware vendor or IHV? > > Neil > > > > As for the SDM, it's a documentation... omission? SGX activation > > > is intentionally omitted from the SDM. The intended usage model is > > > that firmware will always do the activation (if it wants SGX enabled), > > > i.e. post-firmware software will only ever "see" SGX as disabled or > > > in the fully activated state, and so the SDM doesn't describe SGX > > > behavior prior to activation. I believe the activation process, or > > > at least what is required from firmware, is documented in the BIOS > > > writer's guide. > > > > > > > Jethro Beekman | Fortanix > > > > > > > > > >
Re: [intel-sgx-kernel-dev] [PATCH v11 13/13] intel_sgx: in-kernel launch enclave
On Wed, Jun 20, 2018 at 5:02 PM Sean Christopherson wrote: > > On Wed, Jun 20, 2018 at 11:39:00AM -0700, Jethro Beekman wrote: > > On 2018-06-20 11:16, Jethro Beekman wrote: > > > > This last bit is also repeated in different words in Table 35-2 and > > > > Section 42.2.2. The MSRs are *not writable* before the write-lock bit > > > > itself is locked. Meaning the MSRs are either locked with Intel's key > > > > hash, or not locked at all. > > > > Actually, this might be a documentation bug. I have some test hardware and I > > was able to configure the MSRs in the BIOS and then read the MSRs after boot > > like this: > > > > MSR 0x3a 0x00040005 > > MSR 0x8c 0x20180620 > > MSR 0x8d 0x20180620 > > MSR 0x8e 0x20180620 > > MSR 0x8f 0x20180620 > > > > Since this is not production hardware, it could also be a CPU bug of course. > > > > If it is indeed possible to configure AND lock the MSR values to non-Intel > > values, I'm very much in favor of Nathaniels proposal to treat the launch > > enclave like any other firmware blob. > > It's not a CPU or documentation bug (though the latter is arguable). > SGX has an activation step that is triggered by doing a WRMSR(0x7a) > with bit 0 set. Until SGX is activated, the SGX related bits in > IA32_FEATURE_CONTROL cannot be set, i.e. SGX can't be enabled. But, > the LE hash MSRs are fully writable prior to activation, e.g. to > allow firmware to lock down the LE key with a non-Intel value. > > So yes, it's possible to lock the MSRs to a non-Intel value. The > obvious caveat is that whatever blob is used to write the MSRs would > need be executed prior to activation. This implies that it should be possible to create MSR activation (and an embedded launch enclave?) entirely as a UEFI module. The kernel would still get to manage who has access to /dev/sgx and other important non-cryptographic policy details. Users would still be able to control the cryptographic policy details (via BIOS Secure Boot configuration that exists today). Distributions could still control cryptographic policy details via signing of the UEFI module with their own Secure Boot key (or using something like shim). The UEFI module (and possibly the external launch enclave) could be distributed via linux-firmware. Andy/Neil, does this work for you? > As for the SDM, it's a documentation... omission? SGX activation > is intentionally omitted from the SDM. The intended usage model is > that firmware will always do the activation (if it wants SGX enabled), > i.e. post-firmware software will only ever "see" SGX as disabled or > in the fully activated state, and so the SDM doesn't describe SGX > behavior prior to activation. I believe the activation process, or > at least what is required from firmware, is documented in the BIOS > writer's guide. > > > Jethro Beekman | Fortanix > > > >
Re: [intel-sgx-kernel-dev] [PATCH v11 13/13] intel_sgx: in-kernel launch enclave
On Wed, Jun 20, 2018 at 5:02 PM Sean Christopherson wrote: > > On Wed, Jun 20, 2018 at 11:39:00AM -0700, Jethro Beekman wrote: > > On 2018-06-20 11:16, Jethro Beekman wrote: > > > > This last bit is also repeated in different words in Table 35-2 and > > > > Section 42.2.2. The MSRs are *not writable* before the write-lock bit > > > > itself is locked. Meaning the MSRs are either locked with Intel's key > > > > hash, or not locked at all. > > > > Actually, this might be a documentation bug. I have some test hardware and I > > was able to configure the MSRs in the BIOS and then read the MSRs after boot > > like this: > > > > MSR 0x3a 0x00040005 > > MSR 0x8c 0x20180620 > > MSR 0x8d 0x20180620 > > MSR 0x8e 0x20180620 > > MSR 0x8f 0x20180620 > > > > Since this is not production hardware, it could also be a CPU bug of course. > > > > If it is indeed possible to configure AND lock the MSR values to non-Intel > > values, I'm very much in favor of Nathaniels proposal to treat the launch > > enclave like any other firmware blob. > > It's not a CPU or documentation bug (though the latter is arguable). > SGX has an activation step that is triggered by doing a WRMSR(0x7a) > with bit 0 set. Until SGX is activated, the SGX related bits in > IA32_FEATURE_CONTROL cannot be set, i.e. SGX can't be enabled. But, > the LE hash MSRs are fully writable prior to activation, e.g. to > allow firmware to lock down the LE key with a non-Intel value. > > So yes, it's possible to lock the MSRs to a non-Intel value. The > obvious caveat is that whatever blob is used to write the MSRs would > need be executed prior to activation. This implies that it should be possible to create MSR activation (and an embedded launch enclave?) entirely as a UEFI module. The kernel would still get to manage who has access to /dev/sgx and other important non-cryptographic policy details. Users would still be able to control the cryptographic policy details (via BIOS Secure Boot configuration that exists today). Distributions could still control cryptographic policy details via signing of the UEFI module with their own Secure Boot key (or using something like shim). The UEFI module (and possibly the external launch enclave) could be distributed via linux-firmware. Andy/Neil, does this work for you? > As for the SDM, it's a documentation... omission? SGX activation > is intentionally omitted from the SDM. The intended usage model is > that firmware will always do the activation (if it wants SGX enabled), > i.e. post-firmware software will only ever "see" SGX as disabled or > in the fully activated state, and so the SDM doesn't describe SGX > behavior prior to activation. I believe the activation process, or > at least what is required from firmware, is documented in the BIOS > writer's guide. > > > Jethro Beekman | Fortanix > > > >
Re: [intel-sgx-kernel-dev] [PATCH v11 13/13] intel_sgx: in-kernel launch enclave
On Wed, Jun 20, 2018 at 2:16 PM Jethro Beekman wrote: > > On 2018-06-20 09:28, Nathaniel McCallum wrote: > > As I understand it, the current policy models under discussion look like > > this: > > > > 1. SGX w/o FLC (not being merged) looks like this: > >Intel CPU => (Intel signed) launch enclave => enclaves > > I think you mean: > > Intel CPU => kernel => (Intel signed) launch enclave => enclaves I get what you mean. But it wasn't what I intended. I'm referring to cryptographic policy. While it is true that the kernel would provide hardware support and would still enforce other non-cryptographic policy under this model (such as filesystem permissions to /dev/sgx), the kernel doesn't verify signatures or pick the key used to verify signatures. The Intel CPU validates the signature of the launch enclave using a hard-coded key. Since the kernel doesn't get to pick the key, it has no say in the cryptographic policy. > > 2. SGX w/ FLC, looks like this: > >Intel CPU => kernel => launch enclave => enclaves In this case, the kernel picks the key used to verify the signature of the LE and writes it to the appropriate MSRs. So it has a say in the policy chain. > > 3. Andy is proposing this: > >Intel CPU => kernel => enclaves In this case, the kernel basically throws away the concept of LEs based on the fact that proposal #2 doesn't actually reduce the attack surface and therefore adds complexity without security. So the kernel takes over the evaluation of the cryptographic policy. > > This proposal is based on the fact that if the kernel can write to the > > MSRs then a kernel compromise allows an attacker to run their own > > launch enclave and therefore having an independent launch enclave adds > > only complexity but not security. > > > > Is it possible to restrict the ability of the kernel to change the > > MSRs? For example, could a UEFI module manage the MSRs? Could the > > launch enclave live entirely within that UEFI module? > > On 2017-03-17 09:15, Jethro Beekman wrote: > > While collecting my thoughts about the issue I read through the > > documentation again and it seems that it will not be possible for a > > platform to lock in a non-Intel hash at all. From Section 39.1.4 of the > > latest Intel SDM: > > > > > IA32_SGXLEPUBKEYHASH defaults to digest of Intel’s launch enclave > > > signing key after reset. > > > > > > IA32_FEATURE_CONTROL bit 17 controls the permissions on the > > > IA32_SGXLEPUBKEYHASH MSRs when CPUID.(EAX=12H, ECX=00H):EAX[0] = 1. > > > If IA32_FEATURE_CONTROL is locked with bit 17 set, > > > IA32_SGXLEPUBKEYHASH MSRs are reconfigurable (writeable). If either > > > IA32_FEATURE_CONTROL is not locked or bit 17 is clear, the MSRs are > > > read only. > > > > This last bit is also repeated in different words in Table 35-2 and > > Section 42.2.2. The MSRs are *not writable* before the write-lock bit > > itself is locked. Meaning the MSRs are either locked with Intel's key > > hash, or not locked at all. > > > > > 4. I am suggesting this: > >Intel CPU => UEFI module => enclaves > > > > Under this architecture, the kernel isn't involved in policy at all > > and users get exactly the same freedoms they already have with Secure > > Boot. Further, the launch enclave can be independently updated and > > could be distributed in linux-firmware. The UEFI module can also be > > shared across operating systems. If I want to have my own enclave > > policy, then I can build the UEFI module myself, with my > > modifications, and I can disable Secure Boot. Alternatively, > > distributions that want to set their own policies can build their own > > UEFI module and sign it with their vendor key. > > Jethro Beekman | Fortanix >
Re: [intel-sgx-kernel-dev] [PATCH v11 13/13] intel_sgx: in-kernel launch enclave
On Wed, Jun 20, 2018 at 2:16 PM Jethro Beekman wrote: > > On 2018-06-20 09:28, Nathaniel McCallum wrote: > > As I understand it, the current policy models under discussion look like > > this: > > > > 1. SGX w/o FLC (not being merged) looks like this: > >Intel CPU => (Intel signed) launch enclave => enclaves > > I think you mean: > > Intel CPU => kernel => (Intel signed) launch enclave => enclaves I get what you mean. But it wasn't what I intended. I'm referring to cryptographic policy. While it is true that the kernel would provide hardware support and would still enforce other non-cryptographic policy under this model (such as filesystem permissions to /dev/sgx), the kernel doesn't verify signatures or pick the key used to verify signatures. The Intel CPU validates the signature of the launch enclave using a hard-coded key. Since the kernel doesn't get to pick the key, it has no say in the cryptographic policy. > > 2. SGX w/ FLC, looks like this: > >Intel CPU => kernel => launch enclave => enclaves In this case, the kernel picks the key used to verify the signature of the LE and writes it to the appropriate MSRs. So it has a say in the policy chain. > > 3. Andy is proposing this: > >Intel CPU => kernel => enclaves In this case, the kernel basically throws away the concept of LEs based on the fact that proposal #2 doesn't actually reduce the attack surface and therefore adds complexity without security. So the kernel takes over the evaluation of the cryptographic policy. > > This proposal is based on the fact that if the kernel can write to the > > MSRs then a kernel compromise allows an attacker to run their own > > launch enclave and therefore having an independent launch enclave adds > > only complexity but not security. > > > > Is it possible to restrict the ability of the kernel to change the > > MSRs? For example, could a UEFI module manage the MSRs? Could the > > launch enclave live entirely within that UEFI module? > > On 2017-03-17 09:15, Jethro Beekman wrote: > > While collecting my thoughts about the issue I read through the > > documentation again and it seems that it will not be possible for a > > platform to lock in a non-Intel hash at all. From Section 39.1.4 of the > > latest Intel SDM: > > > > > IA32_SGXLEPUBKEYHASH defaults to digest of Intel’s launch enclave > > > signing key after reset. > > > > > > IA32_FEATURE_CONTROL bit 17 controls the permissions on the > > > IA32_SGXLEPUBKEYHASH MSRs when CPUID.(EAX=12H, ECX=00H):EAX[0] = 1. > > > If IA32_FEATURE_CONTROL is locked with bit 17 set, > > > IA32_SGXLEPUBKEYHASH MSRs are reconfigurable (writeable). If either > > > IA32_FEATURE_CONTROL is not locked or bit 17 is clear, the MSRs are > > > read only. > > > > This last bit is also repeated in different words in Table 35-2 and > > Section 42.2.2. The MSRs are *not writable* before the write-lock bit > > itself is locked. Meaning the MSRs are either locked with Intel's key > > hash, or not locked at all. > > > > > 4. I am suggesting this: > >Intel CPU => UEFI module => enclaves > > > > Under this architecture, the kernel isn't involved in policy at all > > and users get exactly the same freedoms they already have with Secure > > Boot. Further, the launch enclave can be independently updated and > > could be distributed in linux-firmware. The UEFI module can also be > > shared across operating systems. If I want to have my own enclave > > policy, then I can build the UEFI module myself, with my > > modifications, and I can disable Secure Boot. Alternatively, > > distributions that want to set their own policies can build their own > > UEFI module and sign it with their vendor key. > > Jethro Beekman | Fortanix >
Re: [intel-sgx-kernel-dev] [PATCH v11 13/13] intel_sgx: in-kernel launch enclave
As I understand it, the current policy models under discussion look like this: 1. SGX w/o FLC (not being merged) looks like this: Intel CPU => (Intel signed) launch enclave => enclaves 2. SGX w/ FLC, looks like this: Intel CPU => kernel => launch enclave => enclaves 3. Andy is proposing this: Intel CPU => kernel => enclaves This proposal is based on the fact that if the kernel can write to the MSRs then a kernel compromise allows an attacker to run their own launch enclave and therefore having an independent launch enclave adds only complexity but not security. Is it possible to restrict the ability of the kernel to change the MSRs? For example, could a UEFI module manage the MSRs? Could the launch enclave live entirely within that UEFI module? 4. I am suggesting this: Intel CPU => UEFI module => enclaves Under this architecture, the kernel isn't involved in policy at all and users get exactly the same freedoms they already have with Secure Boot. Further, the launch enclave can be independently updated and could be distributed in linux-firmware. The UEFI module can also be shared across operating systems. If I want to have my own enclave policy, then I can build the UEFI module myself, with my modifications, and I can disable Secure Boot. Alternatively, distributions that want to set their own policies can build their own UEFI module and sign it with their vendor key. On Mon, Jun 18, 2018 at 5:59 PM Andy Lutomirski wrote: > > On Tue, Jun 12, 2018 at 10:45 AM Neil Horman wrote: > > > > On Mon, Jun 11, 2018 at 09:55:29PM -0700, Andy Lutomirski wrote: > > > On Mon, Jun 11, 2018 at 4:52 AM Neil Horman wrote: > > > > > > > > On Sun, Jun 10, 2018 at 10:17:13PM -0700, Andy Lutomirski wrote: > > > > > > On Jun 9, 2018, at 10:39 PM, Andy Lutomirski > > > > > > wrote: > > > > > > > > > > > > On Fri, Jun 8, 2018 at 10:32 AM Jarkko Sakkinen > > > > > > wrote: > > > > > >> > > > > > >> The Launch Enclave (LE) generates cryptographic launch tokens for > > > > > >> user > > > > > >> enclaves. A launch token is used by EINIT to check whether the > > > > > >> enclave > > > > > >> is authorized to launch or not. By having its own launch enclave, > > > > > >> Linux > > > > > >> has full control of the enclave launch process. > > > > > >> > > > > > >> LE is wrapped into a user space proxy program that reads enclave > > > > > >> signatures outputs launch tokens. The kernel-side glue code is > > > > > >> implemented by using the user space helper framework. The IPC > > > > > >> between > > > > > >> the LE proxy program and kernel is handled with an anonymous inode. > > > > > >> > > > > > >> The commit also adds enclave signing tool that is used by kbuild to > > > > > >> measure and sign the launch enclave. CONFIG_INTEL_SGX_SIGNING_KEY > > > > > >> points > > > > > >> to a PEM-file for the 3072-bit RSA key that is used as the LE > > > > > >> public key > > > > > >> pair. The default location is: > > > > > >> > > > > > >> drivers/platform/x86/intel_sgx/sgx_signing_key.pem > > > > > >> > > > > > >> If the default key does not exist kbuild will generate a random > > > > > >> key and > > > > > >> place it to this location. KBUILD_SGX_SIGN_PIN can be used to > > > > > >> specify > > > > > >> the passphrase for the LE public key. > > > > > > > > > > > > It seems to me that it might be more useful to just commit a key > > > > > > pair > > > > > > into the kernel. As far as I know, there is no security whatsoever > > > > > > gained by keeping the private key private, so why not make > > > > > > reproducible builds easier by simply fixing the key? > > > > > > > > > > Having thought about this some more, I think that you should > > > > > completely remove support for specifying a key. Provide a fixed key > > > > > pair, hard code the cache, and call it a day. If you make the key > > > > > configurable, every vendor that has any vendor keys (Debian, Ubuntu, > > > > > Fedora, Red Hat, SuSE, Clear Linux, etc) will see that config option > > > > > and set up their own key pair for no gain whatsoever. Instead, it'll > > > > > give some illusion of security and it'll slow down operations in a VM > > > > > guest due to swapping out the values of the MSRs. And, if the code to > > > > > support a locked MSR that just happens to have the right value stays > > > > > in the kernel, then we'll risk having vendors actually ship one > > > > > distro's public key hash, and that will seriously suck. > > > > > > > > > If you hard code the key pair however, doesn't that imply that anyone > > > > can sign a > > > > user space binary as a launch enclave, and potentially gain control of > > > > the token > > > > granting process? > > > > > > Yes and no. > > > > > > First of all, the kernel driver shouldn't be allowing user code to > > > launch a launch enclave regardless of signature. I haven't gotten far > > > enough in reviewing the code to see whether that's the case, but if > > > it's not, it should be fixed before
Re: [intel-sgx-kernel-dev] [PATCH v11 13/13] intel_sgx: in-kernel launch enclave
As I understand it, the current policy models under discussion look like this: 1. SGX w/o FLC (not being merged) looks like this: Intel CPU => (Intel signed) launch enclave => enclaves 2. SGX w/ FLC, looks like this: Intel CPU => kernel => launch enclave => enclaves 3. Andy is proposing this: Intel CPU => kernel => enclaves This proposal is based on the fact that if the kernel can write to the MSRs then a kernel compromise allows an attacker to run their own launch enclave and therefore having an independent launch enclave adds only complexity but not security. Is it possible to restrict the ability of the kernel to change the MSRs? For example, could a UEFI module manage the MSRs? Could the launch enclave live entirely within that UEFI module? 4. I am suggesting this: Intel CPU => UEFI module => enclaves Under this architecture, the kernel isn't involved in policy at all and users get exactly the same freedoms they already have with Secure Boot. Further, the launch enclave can be independently updated and could be distributed in linux-firmware. The UEFI module can also be shared across operating systems. If I want to have my own enclave policy, then I can build the UEFI module myself, with my modifications, and I can disable Secure Boot. Alternatively, distributions that want to set their own policies can build their own UEFI module and sign it with their vendor key. On Mon, Jun 18, 2018 at 5:59 PM Andy Lutomirski wrote: > > On Tue, Jun 12, 2018 at 10:45 AM Neil Horman wrote: > > > > On Mon, Jun 11, 2018 at 09:55:29PM -0700, Andy Lutomirski wrote: > > > On Mon, Jun 11, 2018 at 4:52 AM Neil Horman wrote: > > > > > > > > On Sun, Jun 10, 2018 at 10:17:13PM -0700, Andy Lutomirski wrote: > > > > > > On Jun 9, 2018, at 10:39 PM, Andy Lutomirski > > > > > > wrote: > > > > > > > > > > > > On Fri, Jun 8, 2018 at 10:32 AM Jarkko Sakkinen > > > > > > wrote: > > > > > >> > > > > > >> The Launch Enclave (LE) generates cryptographic launch tokens for > > > > > >> user > > > > > >> enclaves. A launch token is used by EINIT to check whether the > > > > > >> enclave > > > > > >> is authorized to launch or not. By having its own launch enclave, > > > > > >> Linux > > > > > >> has full control of the enclave launch process. > > > > > >> > > > > > >> LE is wrapped into a user space proxy program that reads enclave > > > > > >> signatures outputs launch tokens. The kernel-side glue code is > > > > > >> implemented by using the user space helper framework. The IPC > > > > > >> between > > > > > >> the LE proxy program and kernel is handled with an anonymous inode. > > > > > >> > > > > > >> The commit also adds enclave signing tool that is used by kbuild to > > > > > >> measure and sign the launch enclave. CONFIG_INTEL_SGX_SIGNING_KEY > > > > > >> points > > > > > >> to a PEM-file for the 3072-bit RSA key that is used as the LE > > > > > >> public key > > > > > >> pair. The default location is: > > > > > >> > > > > > >> drivers/platform/x86/intel_sgx/sgx_signing_key.pem > > > > > >> > > > > > >> If the default key does not exist kbuild will generate a random > > > > > >> key and > > > > > >> place it to this location. KBUILD_SGX_SIGN_PIN can be used to > > > > > >> specify > > > > > >> the passphrase for the LE public key. > > > > > > > > > > > > It seems to me that it might be more useful to just commit a key > > > > > > pair > > > > > > into the kernel. As far as I know, there is no security whatsoever > > > > > > gained by keeping the private key private, so why not make > > > > > > reproducible builds easier by simply fixing the key? > > > > > > > > > > Having thought about this some more, I think that you should > > > > > completely remove support for specifying a key. Provide a fixed key > > > > > pair, hard code the cache, and call it a day. If you make the key > > > > > configurable, every vendor that has any vendor keys (Debian, Ubuntu, > > > > > Fedora, Red Hat, SuSE, Clear Linux, etc) will see that config option > > > > > and set up their own key pair for no gain whatsoever. Instead, it'll > > > > > give some illusion of security and it'll slow down operations in a VM > > > > > guest due to swapping out the values of the MSRs. And, if the code to > > > > > support a locked MSR that just happens to have the right value stays > > > > > in the kernel, then we'll risk having vendors actually ship one > > > > > distro's public key hash, and that will seriously suck. > > > > > > > > > If you hard code the key pair however, doesn't that imply that anyone > > > > can sign a > > > > user space binary as a launch enclave, and potentially gain control of > > > > the token > > > > granting process? > > > > > > Yes and no. > > > > > > First of all, the kernel driver shouldn't be allowing user code to > > > launch a launch enclave regardless of signature. I haven't gotten far > > > enough in reviewing the code to see whether that's the case, but if > > > it's not, it should be fixed before
Authenticating Unix Socket Connections by PGID
I apologize if this is the wrong mailing list for this query. I wasn't sure where else to post it. In user space, I have a process hierarchy. There is a root process which spawns a child which in turn may spawn more children. Hence, I have a standard hierarchy with a root process, zero or more branch processes and one or more leaf processes. I need a mechanism by which the leaf processes communicate with the root process. However, I also need to prevent parallel hierarchies from communicating with the root process: only descendants of the root process should have access to the root process. Further, I'd like to avoid the complication of proxying through branch processes. I'm already using a process group so that the root process can killpg() for cleanup. So I was wondering: can I use the PGID for authentication as well? Basically, I'd just use a well-known AF_UNIX/SOCK_STREAM socket. The root process would then check that new connections were in the process group by using: getpgid(getsockopt(SO_PEERCRED).pid) == getpid(). I inquired with several very knowledgeable people before sending this email, and they were all somewhat uncomfortable with this scheme but couldn't detail why. This system depends on a few features. First, it depends on the documented requirement that setpgid() fails if attempting to move to a different session. Since I create my own session (via setsid()) in the root process, it would seem that spoofing a child is impossible. This requirement is documented in the man page for setpgid() as failing with EPERM. I also confirmed this behavior by reviewing the kernel code for implementing the setpgid() syscall. Second, it depends on being able to properly determine the PGID of the unix socket connection peer. One concern that was voiced to me was a fear that this wouldn't work across namespaces; so I tested it. In child namespaces, getsockopt(SO_PEERCRED).pid returns the mapped PID in the parent. In parallel namespaces, the same approach returns PID = 0. Thus, AFAICS, this requirement is also met: we can accurately detect the PGID of a unix socket peer. Obviously, branch/leaf processes need to be sure not to call setpgid()/setsid(). But the worst case here is that they get authentication denied. Am shooting myself in the foot? Might there be some corner case that I'm missing?
Authenticating Unix Socket Connections by PGID
I apologize if this is the wrong mailing list for this query. I wasn't sure where else to post it. In user space, I have a process hierarchy. There is a root process which spawns a child which in turn may spawn more children. Hence, I have a standard hierarchy with a root process, zero or more branch processes and one or more leaf processes. I need a mechanism by which the leaf processes communicate with the root process. However, I also need to prevent parallel hierarchies from communicating with the root process: only descendants of the root process should have access to the root process. Further, I'd like to avoid the complication of proxying through branch processes. I'm already using a process group so that the root process can killpg() for cleanup. So I was wondering: can I use the PGID for authentication as well? Basically, I'd just use a well-known AF_UNIX/SOCK_STREAM socket. The root process would then check that new connections were in the process group by using: getpgid(getsockopt(SO_PEERCRED).pid) == getpid(). I inquired with several very knowledgeable people before sending this email, and they were all somewhat uncomfortable with this scheme but couldn't detail why. This system depends on a few features. First, it depends on the documented requirement that setpgid() fails if attempting to move to a different session. Since I create my own session (via setsid()) in the root process, it would seem that spoofing a child is impossible. This requirement is documented in the man page for setpgid() as failing with EPERM. I also confirmed this behavior by reviewing the kernel code for implementing the setpgid() syscall. Second, it depends on being able to properly determine the PGID of the unix socket connection peer. One concern that was voiced to me was a fear that this wouldn't work across namespaces; so I tested it. In child namespaces, getsockopt(SO_PEERCRED).pid returns the mapped PID in the parent. In parallel namespaces, the same approach returns PID = 0. Thus, AFAICS, this requirement is also met: we can accurately detect the PGID of a unix socket peer. Obviously, branch/leaf processes need to be sure not to call setpgid()/setsid(). But the worst case here is that they get authentication denied. Am shooting myself in the foot? Might there be some corner case that I'm missing?
