[RFC PATCH v3 00/12] security: x86/sgx: SGX vs. LSM, round 3

[RFC PATCH v3 00/12] security: x86/sgx: SGX vs. LSM, round 3

[Sean Christopherson]

My original plan was for my next RFC to be an implementation of Andy's
proposed "dynamic tracking" model.  I actually finished the tracking
portion, but was completely flummoxed by the auditing[1].  Since Cedric's
RFC is essentially a variation of the dynamic tracking model, it too has
the same auditing complexities.  End result, I ended back at the "make
userspace state its intentions" approach.

Except for patch 12 (see below), the SGX changes have been fully tested,
including updating the kernel's selftest as well as my own fork of (an old
version of) Intel's SDK to use the new UAPI.  The LSM changes have been
smoke tested, but I haven't actually configured AppArmor or SELinux to
verify the permissions work as intended.

Patches 1-3 are bug fixes that should go into v21 regardless of what we
end up doing for LSM support.  They're included here as the actual LSM
RFC patches are essentially untestable without them.

Patches 4-11 are the meat of the RFC.

Patch 12 is purely to show how we might implement SGX2 support.  It's not
intended to be included in v21.


This series is a delta to Jarkko's ongoing SGX series and applies on
Jarkko's current master at https://github.com/jsakkine-intel/linux-sgx.git:

  4cc5d411db1b ("docs: x86/sgx: Document the enclave API")

The basic gist of the approach is to track an enclave's page protections
separately from any vmas that map the page, and separate from the hardware
enforced protections.  The SGX UAPI is modified to require userspace to
explicitly define the protections for each enclave page, i.e. the ioctl
to add pages to an enclave is extended to take PROT_{READ,WRITE,EXEC}
flags.

An enclave page's protections are the maximal protections that userspace
can use to map the page, e.g. mprotect() and mmap() are rejected if the
protections for the vma would be more permissible than those of the
associated enclave page.

Tracking protections for an enclave page (in additional to vmas) allows
SGX to invoke LSM upcalls while the enclave is being built.  This is
critical to enabling LSMs to implement policies for enclave pages that
are functionally equivalent to existing policies for normal pages.


[1] https://lkml.kernel.org/r/20190614003759.GE18385@linux.intel.com

v1: https://lkml.kernel.org/r/20190531233159.30992-1-sean.j.christopherson@intel.com

v2: https://lkml.kernel.org/r/20190606021145.12604-1-sean.j.christopherson@intel.com

  - Dropped the patch(es) to extend the SGX UAPI to allow adding multiple
    enclave pages in a single syscall [Jarkko].

  - Reject ioctl() immediately on LSM denial [Stephen].

  - Rework SELinux code to avoid checking EXEMEM multiple times [Stephen].

  - Adding missing equivalents to existing selinux_file_protect() checks
    [Stephen].

  - Hold mmap_sem across copy_to_user() to prevent a TOCTOU race when
    checking the source vma [Stephen].

  - Stubify security_enclave_load() if !CONFIG_SECURITY [Stephen].

  - Make flags a 32-bit field [Andy].

  - Don't validate the SECINFO protection flags against the enclave
    page's protection flags [Andy].

  - Rename mprotect() hook to may_mprotect() [Andy].

  - Test 'vma->vm_flags & VM_MAYEXEC' instead of manually checking for
    a noexec path [Jarkko].

  - Drop the SGX defined flags (use PROT_*) [Jarkko].

  - Improve comments and changelogs [Jarkko].

v3:
  - Clear VM_MAY* flags instead of using .may_mprotect() to enforce
    maximal enclave page protections.

  - Update the SGX selftest to work with the new API.

  - Rewrite SELinux code to use SGX specific permissions, with the goal
    of addressing Andy's feedback regarding what people will actually
    care about when it comes to SGX, e.g. add permissions for restricing
    unmeasured code and stop trying to infer permissions from the source
    of each enclave page.

  - Add a (very minimal) AppArmor patch.

  - Show line of sight to SGX2 support.

  - Rebased to Jarkko's latest code base.

Sean Christopherson (12):
  x86/sgx: Add mm to enclave at mmap()
  x86/sgx: Do not naturally align MAP_FIXED address
  selftests: x86/sgx: Mark the enclave loader as not needing an exec
    stack
  x86/sgx: Require userspace to define enclave pages' protection bits
  x86/sgx: Enforce noexec filesystem restriction for enclaves
  mm: Introduce vm_ops->may_mprotect()
  LSM: x86/sgx: Introduce ->enclave_map() hook for Intel SGX
  security/selinux: Require SGX_EXECMEM to map enclave page WX
  LSM: x86/sgx: Introduce ->enclave_load() hook for Intel SGX
  security/selinux: Add enclave_load() implementation
  security/apparmor: Add enclave_load() implementation
  LSM: x86/sgx: Show line of sight to LSM support SGX2's EAUG

 arch/x86/include/uapi/asm/sgx.h          |  5 +-
 arch/x86/kernel/cpu/sgx/driver/ioctl.c   | 69 +++++++++++------
 arch/x86/kernel/cpu/sgx/driver/main.c    | 94 +++++++++++++++++++++++-
 arch/x86/kernel/cpu/sgx/encl.c           | 81 ++++++++++++--------
 arch/x86/kernel/cpu/sgx/encl.h           |  7 +-
 include/linux/lsm_hooks.h                | 20 +++++
 include/linux/mm.h                       |  2 +
 include/linux/security.h                 | 18 +++++
 mm/mprotect.c                            | 15 +++-
 security/apparmor/include/audit.h        |  2 +
 security/apparmor/lsm.c                  | 14 ++++
 security/security.c                      | 12 +++
 security/selinux/hooks.c                 | 72 ++++++++++++++++++
 security/selinux/include/classmap.h      |  6 +-
 tools/testing/selftests/x86/sgx/Makefile |  2 +-
 tools/testing/selftests/x86/sgx/main.c   | 32 +++++++-
 16 files changed, 384 insertions(+), 67 deletions(-)

-- 
2.21.0

[RFC PATCH v3 01/12] x86/sgx: Add mm to enclave at mmap()

[Sean Christopherson]

The enclave mm tracking is currently broken:

  - Adding current->mm during ECREATE is wrong as there is no guarantee
    that the current process has mmap()'d the enclave, i.e. there may
    never be an associated sgx_vma_close() to drop the encl_mm.

  - Adding mm's at sgx_vma_open() is wrong as vm_ops->open is called
    only when splitting or duplicating a vma.  If userspace performs a
    single mmap() on the enclave then SGX will fail to track the mm.
    This bug is partially hidden by tracking current->mm at ECREATE.

Rework the tracking to get/add the mm at mmap().  A side effect of the
bug fix is that sgx_vma_{open,close}() should never encounter a vma with
an associated enclave and no associated encl_mm, i.e. WARN if an encl_mm
cannot be found in either condition.

Change the WARN() on a non-empty mm_list to a WARN_ONCE().  The warning
will fire over and over (and over) if the mm tracking is broken, which
hampers debug/triage far more than it helps.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kernel/cpu/sgx/driver/ioctl.c | 14 ----------
 arch/x86/kernel/cpu/sgx/driver/main.c  | 26 ++++++++++++++++++
 arch/x86/kernel/cpu/sgx/encl.c         | 38 +++++---------------------
 arch/x86/kernel/cpu/sgx/encl.h         |  4 +--
 4 files changed, 35 insertions(+), 47 deletions(-)

diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
index d17c60dca114..3552d642b26f 100644
--- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
@@ -276,7 +276,6 @@ static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs)
 {
 	unsigned long encl_size = secs->size + PAGE_SIZE;
 	struct sgx_epc_page *secs_epc;
-	struct sgx_encl_mm *encl_mm;
 	unsigned long ssaframesize;
 	struct sgx_pageinfo pginfo;
 	struct sgx_secinfo secinfo;
@@ -311,12 +310,6 @@ static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs)
 
 	INIT_WORK(&encl->work, sgx_add_page_worker);
 
-	encl_mm = sgx_encl_mm_add(encl, current->mm);
-	if (IS_ERR(encl_mm)) {
-		ret = PTR_ERR(encl_mm);
-		goto err_out;
-	}
-
 	secs_epc = sgx_alloc_page(&encl->secs, true);
 	if (IS_ERR(secs_epc)) {
 		ret = PTR_ERR(secs_epc);
@@ -369,13 +362,6 @@ static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs)
 		encl->backing = NULL;
 	}
 
-	if (!list_empty(&encl->mm_list)) {
-		encl_mm = list_first_entry(&encl->mm_list, struct sgx_encl_mm,
-					   list);
-		list_del(&encl_mm->list);
-		kfree(encl_mm);
-	}
-
 	mutex_unlock(&encl->lock);
 	return ret;
 }
diff --git a/arch/x86/kernel/cpu/sgx/driver/main.c b/arch/x86/kernel/cpu/sgx/driver/main.c
index 87735ce8b5ba..03eca4bccee1 100644
--- a/arch/x86/kernel/cpu/sgx/driver/main.c
+++ b/arch/x86/kernel/cpu/sgx/driver/main.c
@@ -60,9 +60,35 @@ static long sgx_compat_ioctl(struct file *filep, unsigned int cmd,
 }
 #endif
 
+static int sgx_encl_mm_add(struct sgx_encl *encl, struct mm_struct *mm)
+{
+	struct sgx_encl_mm *encl_mm;
+
+	encl_mm = kzalloc(sizeof(*encl_mm), GFP_KERNEL);
+	if (!encl_mm)
+		return -ENOMEM;
+
+	encl_mm->encl = encl;
+	encl_mm->mm = mm;
+	kref_init(&encl_mm->refcount);
+
+	spin_lock(&encl->mm_lock);
+	list_add(&encl_mm->list, &encl->mm_list);
+	spin_unlock(&encl->mm_lock);
+
+	return 0;
+}
+
 static int sgx_mmap(struct file *file, struct vm_area_struct *vma)
 {
 	struct sgx_encl *encl = file->private_data;
+	int ret;
+
+	if (!sgx_encl_get_mm(encl, vma->vm_mm)) {
+		ret = sgx_encl_mm_add(encl, vma->vm_mm);
+		if (ret)
+			return ret;
+	}
 
 	vma->vm_ops = &sgx_vm_ops;
 	vma->vm_flags |= VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP | VM_IO;
diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
index 6b190eccd02e..6e9f3a41a40d 100644
--- a/arch/x86/kernel/cpu/sgx/encl.c
+++ b/arch/x86/kernel/cpu/sgx/encl.c
@@ -132,26 +132,6 @@ static struct sgx_encl_page *sgx_encl_load_page(struct sgx_encl *encl,
 	return entry;
 }
 
-struct sgx_encl_mm *sgx_encl_mm_add(struct sgx_encl *encl,
-				    struct mm_struct *mm)
-{
-	struct sgx_encl_mm *encl_mm;
-
-	encl_mm = kzalloc(sizeof(*encl_mm), GFP_KERNEL);
-	if (!encl_mm)
-		return ERR_PTR(-ENOMEM);
-
-	encl_mm->encl = encl;
-	encl_mm->mm = mm;
-	kref_init(&encl_mm->refcount);
-
-	spin_lock(&encl->mm_lock);
-	list_add(&encl_mm->list, &encl->mm_list);
-	spin_unlock(&encl->mm_lock);
-
-	return encl_mm;
-}
-
 void sgx_encl_mm_release(struct kref *ref)
 {
 	struct sgx_encl_mm *encl_mm =
@@ -164,8 +144,8 @@ void sgx_encl_mm_release(struct kref *ref)
 	kfree(encl_mm);
 }
 
-static struct sgx_encl_mm *sgx_encl_get_mm(struct sgx_encl *encl,
-					   struct mm_struct *mm)
+struct sgx_encl_mm *sgx_encl_get_mm(struct sgx_encl *encl,
+				    struct mm_struct *mm)
 {
 	struct sgx_encl_mm *encl_mm = NULL;
 	struct sgx_encl_mm *prev_mm = NULL;
@@ -190,10 +170,10 @@ static struct sgx_encl_mm *sgx_encl_get_mm(struct sgx_encl *encl,
 	return NULL;
 }
 
+
 static void sgx_vma_open(struct vm_area_struct *vma)
 {
 	struct sgx_encl *encl = vma->vm_private_data;
-	struct sgx_encl_mm *encl_mm;
 
 	if (!encl)
 		return;
@@ -201,12 +181,8 @@ static void sgx_vma_open(struct vm_area_struct *vma)
 	if (encl->flags & SGX_ENCL_DEAD)
 		goto error;
 
-	encl_mm = sgx_encl_get_mm(encl, vma->vm_mm);
-	if (!encl_mm) {
-		encl_mm = sgx_encl_mm_add(encl, vma->vm_mm);
-		if (IS_ERR(encl_mm))
-			goto error;
-	}
+	if (WARN_ON_ONCE(!sgx_encl_get_mm(encl, vma->vm_mm)))
+		goto error;
 
 	kref_get(&encl->refcount);
 	return;
@@ -224,7 +200,7 @@ static void sgx_vma_close(struct vm_area_struct *vma)
 		return;
 
 	encl_mm = sgx_encl_get_mm(encl, vma->vm_mm);
-	if (encl_mm) {
+	if (!WARN_ON_ONCE(!encl_mm)) {
 		kref_put(&encl_mm->refcount, sgx_encl_mm_release);
 
 		/* Release kref for the VMA. */
@@ -468,7 +444,7 @@ void sgx_encl_release(struct kref *ref)
 	if (encl->backing)
 		fput(encl->backing);
 
-	WARN(!list_empty(&encl->mm_list), "sgx: mm_list non-empty");
+	WARN_ONCE(!list_empty(&encl->mm_list), "sgx: mm_list non-empty");
 
 	kfree(encl);
 }
diff --git a/arch/x86/kernel/cpu/sgx/encl.h b/arch/x86/kernel/cpu/sgx/encl.h
index c557f0374d74..7b339334d875 100644
--- a/arch/x86/kernel/cpu/sgx/encl.h
+++ b/arch/x86/kernel/cpu/sgx/encl.h
@@ -120,9 +120,9 @@ pgoff_t sgx_encl_get_index(struct sgx_encl *encl, struct sgx_encl_page *page);
 struct page *sgx_encl_get_backing_page(struct sgx_encl *encl, pgoff_t index);
 struct sgx_encl_mm *sgx_encl_next_mm(struct sgx_encl *encl,
 				     struct sgx_encl_mm *encl_mm, int *iter);
-struct sgx_encl_mm *sgx_encl_mm_add(struct sgx_encl *encl,
-				    struct mm_struct *mm);
 void sgx_encl_mm_release(struct kref *ref);
+struct sgx_encl_mm *sgx_encl_get_mm(struct sgx_encl *encl,
+				    struct mm_struct *mm);
 int sgx_encl_test_and_clear_young(struct mm_struct *mm,
 				  struct sgx_encl_page *page);
 struct sgx_encl_page *sgx_encl_reserve_page(struct sgx_encl *encl,
-- 
2.21.0

[RFC PATCH v3 02/12] x86/sgx: Do not naturally align MAP_FIXED address

[Sean Christopherson]

SGX enclaves have an associated Enclave Linear Range (ELRANGE) that is
tracked and enforced by the CPU using a base+mask approach, similar to
how hardware range registers such as the variable MTRRs.  As a result,
the ELRANGE must be naturally sized and aligned.

To reduce boilerplate code that would be needed in every userspace
enclave loader, the SGX driver naturally aligns the mmap() address and
also requires the range to be naturally sized.  Unfortunately, SGX fails
to grant a waiver to the MAP_FIXED case, e.g. incorrectly rejects mmap()
if userspace is attempting to map a small slice of an existing enclave.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kernel/cpu/sgx/driver/main.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/sgx/driver/main.c b/arch/x86/kernel/cpu/sgx/driver/main.c
index 03eca4bccee1..d7de4d9aea87 100644
--- a/arch/x86/kernel/cpu/sgx/driver/main.c
+++ b/arch/x86/kernel/cpu/sgx/driver/main.c
@@ -105,7 +105,13 @@ static unsigned long sgx_get_unmapped_area(struct file *file,
 					   unsigned long pgoff,
 					   unsigned long flags)
 {
-	if (len < 2 * PAGE_SIZE || len & (len - 1) || flags & MAP_PRIVATE)
+	if (flags & MAP_PRIVATE)
+		return -EINVAL;
+
+	if (flags & MAP_FIXED)
+		return addr;
+
+	if (len < 2 * PAGE_SIZE || len & (len - 1))
 		return -EINVAL;
 
 	addr = current->mm->get_unmapped_area(file, addr, 2 * len, pgoff,
-- 
2.21.0

[RFC PATCH v3 03/12] selftests: x86/sgx: Mark the enclave loader as not needing an exec stack

[Sean Christopherson]

The SGX enclave loader doesn't need an executable stack, but linkers
will assume it does due to the lack of .note.GNU-stack sections in the
loader's assembly code.  As a result, the kernel tags the loader as
having "read implies exec", and so adds PROT_EXEC to all mmap()s, even
those for mapping EPC regions.  This will cause problems in the future
when userspace needs to explicit state a page's protection bits when the
page is added to an enclave, e.g. adding TCS pages as R+W will cause
mmap() to fail when the kernel tacks on +X.

Explicitly tell the linker that an executable stack is not needed.
Alternatively, each .S file could add .note.GNU-stack, but the loader
should never need an executable stack so zap it in one fell swoop.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 tools/testing/selftests/x86/sgx/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/x86/sgx/Makefile b/tools/testing/selftests/x86/sgx/Makefile
index 1fd6f2708e81..10136b73096b 100644
--- a/tools/testing/selftests/x86/sgx/Makefile
+++ b/tools/testing/selftests/x86/sgx/Makefile
@@ -2,7 +2,7 @@ top_srcdir = ../../../../..
 
 include ../../lib.mk
 
-HOST_CFLAGS := -Wall -Werror -g $(INCLUDES) -fPIC
+HOST_CFLAGS := -Wall -Werror -g $(INCLUDES) -fPIC -z noexecstack
 ENCL_CFLAGS := -Wall -Werror -static -nostdlib -nostartfiles -fPIC \
 	       -fno-stack-protector -mrdrnd $(INCLUDES)
 
-- 
2.21.0

[RFC PATCH v3 04/12] x86/sgx: Require userspace to define enclave pages' protection bits

[Sean Christopherson]

Existing Linux Security Module policies restrict userspace's ability to
map memory, e.g. may require priveleged permissions to map a page that
is simultaneously writable and executable.  Said permissions are often
tied to the file which backs the mapped memory, i.e. vm_file.

For reasons explained below, SGX does not allow LSMs to enforce policies
using existing LSM hooks such as file_mprotect().  Explicitly track the
protection bits for an enclave page (separate from the vma/pte bits) and
require userspace to explicit define a page's protection bits when the
page is added to the enclave.  Enclave page protection bits paves the
way to adding security_enclave_load() LSM hook as an SGX equivalent to
security_file_mprotect(), e.g. SGX can pass the page's protection bits
and source vma to LSMs.   The source vma will allow LSMs to tie
permissions to files, e.g. the file containing the enclave's code and
initial data, and the protection bits will allow LSMs to make decisions
based on the capabilities of the process, e.g. if a process is allowed
to load unmeasured code or load code from anonymous memory.

Due to the nature of the Enclave Page Cache, and because the EPC is
manually managed by SGX, all enclave vmas are backed by the same file,
i.e. /dev/sgx/enclave.  Specifically, a single file allows SGX to use
file op hooks to move pages in/out of the EPC.

Furthermore, EPC pages for any given enclave are fundamentally shared
between processes, i.e. CoW semantics are not possible with EPC pages
due to hardware restrictions such as 1:1 mappings between virtual and
physical addresses (within the enclave).

Lastly, all real world enclaves will need read, write and execute
permissions to EPC pages.

As a result, SGX does not play nice with existing LSM behavior as it is
impossible to apply policies to enclaves with reasonable granularity,
e.g. an LSM can deny access to EPC altogether, but can't deny
potentially unwanted behavior such as mapping pages WX, loading code
from anonymous memory, loading unmeasured code, etc...

For example, because all (practical) enclaves need RW pages for data and
RX pages for code, SELinux's existing policies will require all enclaves
to have FILE__READ, FILE__WRITE and FILE__EXECUTE permissions on
/dev/sgx/enclave.  Witholding FILE__WRITE or FILE__EXECUTE in an attempt
to deny RW->RX or RWX would prevent running *any* enclave, even those
that cleanly separate RW and RX pages.  And because /dev/sgx/enclave
requires MAP_SHARED, the anonymous/CoW checks that would trigger
FILE__EXECMOD or PROCESS__EXECMEM permissions will never fire.

Taking protection bits has a second use in that it can be used to
prevent loading an enclave from a noexec file system.  On SGX2 hardware,
regardless of kernel support for SGX2, userspace could EADD a page from
a noexec path using read-only permissions and later mprotect() and
ENCLU[EMODPE] the page to gain execute permissions.  By requiring
the enclave's page protections up front, SGX will be able to enforce
noexec paths when building enclaves.

To prevent userspace from circumventing the allowed protections, do not
allow PROT_{READ,WRITE,EXEC} mappings to an enclave without an
associated enclave page, i.e. prevent creating a mapping with unchecked
protection bits.

Many alternatives[1][2] have been explored, most notably the concept of
having SGX check (at load time) and save the permissions of the enclave
loader.  The permissions would then be enforced by SGX at run time, e.g.
via mmap()/mprotect() hooks of some form.  The basic functionality of
pre-checking permissions is relatively straightforward, but supporting
LSM auditing is complex and fraught with pitfalls.  If auditing is done
at the time of denial then the audit logs will potentially show a large
number of false positives.  Auditing when the denial is enforced, e.g.
at mprotect(), suffers from its own problems, e.g.:

  - Requires LSMs to pre-generate audit messages so that they can be
    replayed by SGX when the denial is actually enforced.

  - System changes can result in stale audit messages, e.g. if files
    are removed from the system, an LSM profile is modified, etc...

  - A process could log what is essentially a false positive denial,
    e.g. if the current process has the requisite capability but the
    original enclave loader did not.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/include/uapi/asm/sgx.h        |  5 +--
 arch/x86/kernel/cpu/sgx/driver/ioctl.c | 15 +++++---
 arch/x86/kernel/cpu/sgx/driver/main.c  | 49 ++++++++++++++++++++++++++
 arch/x86/kernel/cpu/sgx/encl.h         |  1 +
 tools/testing/selftests/x86/sgx/main.c | 32 +++++++++++++++--
 5 files changed, 93 insertions(+), 9 deletions(-)

diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
index 6dba9f282232..4144242ab690 100644
--- a/arch/x86/include/uapi/asm/sgx.h
+++ b/arch/x86/include/uapi/asm/sgx.h
@@ -35,15 +35,16 @@ struct sgx_enclave_create  {
  * @src:	address for the page data
  * @secinfo:	address for the SECINFO data
  * @mrmask:	bitmask for the measured 256 byte chunks
+ * @flags:	flags, e.g. PROT_{READ,WRITE,EXEC}
  */
 struct sgx_enclave_add_page {
 	__u64	addr;
 	__u64	src;
 	__u64	secinfo;
-	__u64	mrmask;
+	__u32	mrmask;
+	__u32	flags;
 };
 
-
 /**
  * struct sgx_enclave_init - parameter structure for the
  *                           %SGX_IOC_ENCLAVE_INIT ioctl
diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
index 3552d642b26f..8e95e45411f2 100644
--- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
@@ -2,6 +2,7 @@
 // Copyright(c) 2016-19 Intel Corporation.
 
 #include <asm/mman.h>
+#include <linux/mman.h>
 #include <linux/delay.h>
 #include <linux/file.h>
 #include <linux/hashtable.h>
@@ -235,7 +236,8 @@ static int sgx_validate_secs(const struct sgx_secs *secs,
 }
 
 static struct sgx_encl_page *sgx_encl_page_alloc(struct sgx_encl *encl,
-						 unsigned long addr)
+						 unsigned long addr,
+						 unsigned long prot)
 {
 	struct sgx_encl_page *encl_page;
 	int ret;
@@ -247,6 +249,7 @@ static struct sgx_encl_page *sgx_encl_page_alloc(struct sgx_encl *encl,
 		return ERR_PTR(-ENOMEM);
 	encl_page->desc = addr;
 	encl_page->encl = encl;
+	encl_page->vm_prot_bits = calc_vm_prot_bits(prot, 0);
 	ret = radix_tree_insert(&encl->page_tree, PFN_DOWN(encl_page->desc),
 				encl_page);
 	if (ret) {
@@ -517,7 +520,7 @@ static int __sgx_encl_add_page(struct sgx_encl *encl,
 
 static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
 			     void *data, struct sgx_secinfo *secinfo,
-			     unsigned int mrmask)
+			     unsigned int mrmask, unsigned long prot)
 {
 	u64 page_type = secinfo->flags & SGX_SECINFO_PAGE_TYPE_MASK;
 	struct sgx_encl_page *encl_page;
@@ -543,7 +546,7 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
 		goto out;
 	}
 
-	encl_page = sgx_encl_page_alloc(encl, addr);
+	encl_page = sgx_encl_page_alloc(encl, addr, prot);
 	if (IS_ERR(encl_page)) {
 		ret = PTR_ERR(encl_page);
 		goto out;
@@ -584,6 +587,7 @@ static long sgx_ioc_enclave_add_page(struct file *filep, void __user *arg)
 	struct sgx_enclave_add_page addp;
 	struct sgx_secinfo secinfo;
 	struct page *data_page;
+	unsigned long prot;
 	void *data;
 	int ret;
 
@@ -605,7 +609,10 @@ static long sgx_ioc_enclave_add_page(struct file *filep, void __user *arg)
 		goto out;
 	}
 
-	ret = sgx_encl_add_page(encl, addp.addr, data, &secinfo, addp.mrmask);
+	prot = addp.flags & (PROT_READ | PROT_WRITE | PROT_EXEC);
+
+	ret = sgx_encl_add_page(encl, addp.addr, data, &secinfo, addp.mrmask,
+				prot);
 	if (ret)
 		goto out;
 
diff --git a/arch/x86/kernel/cpu/sgx/driver/main.c b/arch/x86/kernel/cpu/sgx/driver/main.c
index d7de4d9aea87..cfc348b44ffb 100644
--- a/arch/x86/kernel/cpu/sgx/driver/main.c
+++ b/arch/x86/kernel/cpu/sgx/driver/main.c
@@ -79,17 +79,66 @@ static int sgx_encl_mm_add(struct sgx_encl *encl, struct mm_struct *mm)
 	return 0;
 }
 
+/*
+ * Returns the AND of VM_{READ,WRITE,EXEC} permissions across all pages
+ * covered by the specific VMA.  A non-existent (or yet to be added) enclave
+ * page is considered to have no RWX permissions, i.e. is inaccessible.
+ */
+static unsigned long sgx_allowed_rwx(struct sgx_encl *encl,
+				     struct vm_area_struct *vma)
+{
+	unsigned long allowed_rwx = VM_READ | VM_WRITE | VM_EXEC;
+	unsigned long idx, idx_start, idx_end;
+	struct sgx_encl_page *page;
+
+	idx_start = PFN_DOWN(vma->vm_start);
+	idx_end = PFN_DOWN(vma->vm_end - 1);
+
+	for (idx = idx_start; idx <= idx_end; ++idx) {
+		/*
+		 * No need to take encl->lock, vm_prot_bits is set prior to
+		 * insertion and never changes, and racing with adding pages is
+		 * a userspace bug.
+		 */
+		rcu_read_lock();
+		page = radix_tree_lookup(&encl->page_tree, idx);
+		rcu_read_unlock();
+
+		/* Do not allow R|W|X to a non-existent page. */
+		if (!page)
+			allowed_rwx = 0;
+		else
+			allowed_rwx &= page->vm_prot_bits;
+		if (!allowed_rwx)
+			break;
+	}
+
+	return allowed_rwx;
+}
+
 static int sgx_mmap(struct file *file, struct vm_area_struct *vma)
 {
 	struct sgx_encl *encl = file->private_data;
+	unsigned long allowed_rwx;
 	int ret;
 
+	allowed_rwx = sgx_allowed_rwx(encl, vma);
+	if (vma->vm_flags & (VM_READ | VM_WRITE | VM_EXEC) & ~allowed_rwx)
+		return -EACCES;
+
 	if (!sgx_encl_get_mm(encl, vma->vm_mm)) {
 		ret = sgx_encl_mm_add(encl, vma->vm_mm);
 		if (ret)
 			return ret;
 	}
 
+	if (!(allowed_rwx & VM_READ))
+		vma->vm_flags &= ~VM_MAYREAD;
+	if (!(allowed_rwx & VM_WRITE))
+		vma->vm_flags &= ~VM_MAYWRITE;
+	if (!(allowed_rwx & VM_EXEC))
+		vma->vm_flags &= ~VM_MAYEXEC;
+
 	vma->vm_ops = &sgx_vm_ops;
 	vma->vm_flags |= VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP | VM_IO;
 	vma->vm_private_data = encl;
diff --git a/arch/x86/kernel/cpu/sgx/encl.h b/arch/x86/kernel/cpu/sgx/encl.h
index 7b339334d875..5cb0f2653d4c 100644
--- a/arch/x86/kernel/cpu/sgx/encl.h
+++ b/arch/x86/kernel/cpu/sgx/encl.h
@@ -41,6 +41,7 @@ enum sgx_encl_page_desc {
 
 struct sgx_encl_page {
 	unsigned long desc;
+	unsigned long vm_prot_bits;
 	struct sgx_epc_page *epc_page;
 	struct sgx_va_page *va_page;
 	struct sgx_encl *encl;
diff --git a/tools/testing/selftests/x86/sgx/main.c b/tools/testing/selftests/x86/sgx/main.c
index e2265f841fb0..e14a34adc0e4 100644
--- a/tools/testing/selftests/x86/sgx/main.c
+++ b/tools/testing/selftests/x86/sgx/main.c
@@ -2,6 +2,7 @@
 // Copyright(c) 2016-18 Intel Corporation.
 
 #include <elf.h>
+#include <errno.h>
 #include <fcntl.h>
 #include <stdbool.h>
 #include <stdio.h>
@@ -18,6 +19,8 @@
 #include "../../../../../arch/x86/kernel/cpu/sgx/arch.h"
 #include "../../../../../arch/x86/include/uapi/asm/sgx.h"
 
+#define PAGE_SIZE  4096
+
 static const uint64_t MAGIC = 0x1122334455667788ULL;
 
 struct vdso_symtab {
@@ -135,8 +138,7 @@ static bool encl_create(int dev_fd, unsigned long bin_size,
 	for (secs->size = 4096; secs->size < bin_size; )
 		secs->size <<= 1;
 
-	base = mmap(NULL, secs->size, PROT_READ | PROT_WRITE | PROT_EXEC,
-		    MAP_SHARED, dev_fd, 0);
+	base = mmap(NULL, secs->size, PROT_NONE, MAP_SHARED, dev_fd, 0);
 	if (base == MAP_FAILED) {
 		perror("mmap");
 		return false;
@@ -147,7 +149,7 @@ static bool encl_create(int dev_fd, unsigned long bin_size,
 	ioc.src = (unsigned long)secs;
 	rc = ioctl(dev_fd, SGX_IOC_ENCLAVE_CREATE, &ioc);
 	if (rc) {
-		fprintf(stderr, "ECREATE failed rc=%d.\n", rc);
+		fprintf(stderr, "ECREATE failed rc=%d, err=%d.\n", rc, errno);
 		munmap(base, secs->size);
 		return false;
 	}
@@ -160,8 +162,14 @@ static bool encl_add_page(int dev_fd, unsigned long addr, void *data,
 {
 	struct sgx_enclave_add_page ioc;
 	struct sgx_secinfo secinfo;
+	unsigned long prot;
 	int rc;
 
+	if (flags == SGX_SECINFO_TCS)
+		prot = PROT_READ | PROT_WRITE;
+	else
+		prot = PROT_READ | PROT_WRITE | PROT_EXEC;
+
 	memset(&secinfo, 0, sizeof(secinfo));
 	secinfo.flags = flags;
 
@@ -169,6 +177,7 @@ static bool encl_add_page(int dev_fd, unsigned long addr, void *data,
 	ioc.mrmask = 0xFFFF;
 	ioc.addr = addr;
 	ioc.src = (uint64_t)data;
+	ioc.flags = prot;
 
 	rc = ioctl(dev_fd, SGX_IOC_ENCLAVE_ADD_PAGE, &ioc);
 	if (rc) {
@@ -184,6 +193,7 @@ static bool encl_load(struct sgx_secs *secs, unsigned long bin_size)
 	struct sgx_enclave_init ioc;
 	uint64_t offset;
 	uint64_t flags;
+	void *addr;
 	int dev_fd;
 	int rc;
 
@@ -215,6 +225,22 @@ static bool encl_load(struct sgx_secs *secs, unsigned long bin_size)
 		goto out_map;
 	}
 
+	addr = mmap((void *)secs->base, PAGE_SIZE, PROT_READ | PROT_WRITE,
+		    MAP_SHARED | MAP_FIXED, dev_fd, 0);
+	if (addr == MAP_FAILED) {
+		fprintf(stderr, "mmap() failed on TCS, errno=%d.\n", errno);
+		return false;
+	}
+
+	addr = mmap((void *)(secs->base + PAGE_SIZE), bin_size - PAGE_SIZE,
+		    PROT_READ | PROT_WRITE | PROT_EXEC,
+		    MAP_SHARED | MAP_FIXED, dev_fd, 0);
+	if (addr == MAP_FAILED) {
+		fprintf(stderr, "mmap() failed, errno=%d.\n", errno);
+		return false;
+	}
+
+
 	close(dev_fd);
 	return true;
 out_map:
-- 
2.21.0

[RFC PATCH v3 05/12] x86/sgx: Enforce noexec filesystem restriction for enclaves

[Sean Christopherson]

Do not allow an enclave page to be mapped with PROT_EXEC if the source
vma does not have VM_MAYEXEC.  This effectively enforces noexec as
do_mmap() clears VM_MAYEXEC if the vma is being loaded from a noexec
path, i.e. prevents executing a file by loading it into an enclave.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kernel/cpu/sgx/driver/ioctl.c | 42 +++++++++++++++++++++++---
 1 file changed, 37 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
index 8e95e45411f2..f239300e0fc2 100644
--- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
@@ -564,6 +564,39 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
 	return ret;
 }
 
+static int sgx_encl_page_copy(void *dst, unsigned long src, unsigned long prot)
+{
+	struct vm_area_struct *vma;
+	int ret;
+
+	/* Hold mmap_sem across copy_from_user() to avoid a TOCTOU race. */
+	down_read(&current->mm->mmap_sem);
+
+	/* Query vma's VM_MAYEXEC as an indirect path_noexec() check. */
+	if (prot & PROT_EXEC) {
+		vma = find_vma(current->mm, src);
+		if (!vma) {
+			ret = -EFAULT;
+			goto out;
+		}
+
+		if (!(vma->vm_flags & VM_MAYEXEC)) {
+			ret = -EACCES;
+			goto out;
+		}
+	}
+
+	if (copy_from_user(dst, (void __user *)src, PAGE_SIZE))
+		ret = -EFAULT;
+	else
+		ret = 0;
+
+out:
+	up_read(&current->mm->mmap_sem);
+
+	return ret;
+}
+
 /**
  * sgx_ioc_enclave_add_page - handler for %SGX_IOC_ENCLAVE_ADD_PAGE
  *
@@ -604,13 +637,12 @@ static long sgx_ioc_enclave_add_page(struct file *filep, void __user *arg)
 
 	data = kmap(data_page);
 
-	if (copy_from_user((void *)data, (void __user *)addp.src, PAGE_SIZE)) {
-		ret = -EFAULT;
-		goto out;
-	}
-
 	prot = addp.flags & (PROT_READ | PROT_WRITE | PROT_EXEC);
 
+	ret = sgx_encl_page_copy(data, addp.src, prot);
+	if (ret)
+		goto out;
+
 	ret = sgx_encl_add_page(encl, addp.addr, data, &secinfo, addp.mrmask,
 				prot);
 	if (ret)
-- 
2.21.0

[RFC PATCH v3 06/12] mm: Introduce vm_ops->may_mprotect()

[Sean Christopherson]

SGX will use ->may_mprotect() to invoke an SGX variant of the existing
file_mprotect() and mmap_file() LSM hooks.

The name may_mprotect() is intended to reflect the hook's purpose as a
way to restrict mprotect() as opposed to a wholesale replacement.

Due to the nature of SGX and its Enclave Page Cache (EPC), all enclave
VMAs are backed by a single file, i.e. /dev/sgx/enclave, that must be
MAP_SHARED.  Furthermore, all enclaves need read, write and execute
VMAs.  As a result, applying W^X restrictions on /dev/sgx/enclave using
existing LSM hooks is for all intents and purposes impossible, e.g.
denying either W or X would deny access to *any* enclave.

By hooking mprotect(), SGX can invoke an SGX specific LSM hook, which in
turn allows LSMs to enforce W^X policies.

Alternatively, SGX could provide a helper to identify enclaves given a
vma or file.  LSMs could then check if a mapping is for enclave and take
action according.

A second alternative would be to have SGX implement its own LSM hooks
for file_mprotect() and mmap_file(), using them to "forward" the call to
the SGX specific hook.

The major con to both alternatives is that they provide zero flexibility
for the SGX specific LSM hook.  The "is_sgx_enclave()" helper doesn't
allow SGX can't supply any additional information whatsoever, and the
mmap_file() hook is called before the final address is known, e.g. SGX
can't provide any information about the specific enclave being mapped.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 include/linux/mm.h |  2 ++
 mm/mprotect.c      | 15 +++++++++++----
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 0e8834ac32b7..b11ec420c8d7 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -458,6 +458,8 @@ struct vm_operations_struct {
 	void (*close)(struct vm_area_struct * area);
 	int (*split)(struct vm_area_struct * area, unsigned long addr);
 	int (*mremap)(struct vm_area_struct * area);
+	int (*may_mprotect)(struct vm_area_struct *vma, unsigned long start,
+			    unsigned long end, unsigned long prot);
 	vm_fault_t (*fault)(struct vm_fault *vmf);
 	vm_fault_t (*huge_fault)(struct vm_fault *vmf,
 			enum page_entry_size pe_size);
diff --git a/mm/mprotect.c b/mm/mprotect.c
index bf38dfbbb4b4..18732543b295 100644
--- a/mm/mprotect.c
+++ b/mm/mprotect.c
@@ -547,13 +547,20 @@ static int do_mprotect_pkey(unsigned long start, size_t len,
 			goto out;
 		}
 
-		error = security_file_mprotect(vma, reqprot, prot);
-		if (error)
-			goto out;
-
 		tmp = vma->vm_end;
 		if (tmp > end)
 			tmp = end;
+
+		if (vma->vm_ops && vma->vm_ops->may_mprotect) {
+			error = vma->vm_ops->may_mprotect(vma, nstart, tmp, prot);
+			if (error)
+				goto out;
+		}
+
+		error = security_file_mprotect(vma, reqprot, prot);
+		if (error)
+			goto out;
+
 		error = mprotect_fixup(vma, &prev, nstart, tmp, newflags);
 		if (error)
 			goto out;
-- 
2.21.0

[RFC PATCH v3 07/12] LSM: x86/sgx: Introduce ->enclave_map() hook for Intel SGX

[Sean Christopherson]

enclave_map() is an SGX specific variant of file_mprotect() and
mmap_file(), and is provided so that LSMs can apply W^X restrictions to
enclaves.

Due to the nature of SGX and its Enclave Page Cache (EPC), all enclave
VMAs are backed by a single file, i.e. /dev/sgx/enclave, that must be
MAP_SHARED.  Furthermore, all enclaves need read, write and execute
VMAs.  As a result, applying W^X restrictions on /dev/sgx/enclave using
existing LSM hooks is for all intents and purposes impossible, e.g.
denying either W or X would deny access to any enclave.

Note, extensive discussion yielded no sane alternative to some form of
SGX specific LSM hook[1].

[1] https://lkml.kernel.org/r/CALCETrXf8mSK45h7sTK5Wf+pXLVn=Bjsc_RLpgO-h-qdzBRo5Q@mail.gmail.com

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kernel/cpu/sgx/driver/main.c |  9 ++++++++-
 arch/x86/kernel/cpu/sgx/encl.c        | 12 ++++++++++++
 include/linux/lsm_hooks.h             | 12 ++++++++++++
 include/linux/security.h              | 11 +++++++++++
 security/security.c                   |  7 +++++++
 5 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/sgx/driver/main.c b/arch/x86/kernel/cpu/sgx/driver/main.c
index cfc348b44ffb..816f14194d75 100644
--- a/arch/x86/kernel/cpu/sgx/driver/main.c
+++ b/arch/x86/kernel/cpu/sgx/driver/main.c
@@ -119,13 +119,20 @@ static unsigned long sgx_allowed_rwx(struct sgx_encl *encl,
 static int sgx_mmap(struct file *file, struct vm_area_struct *vma)
 {
 	struct sgx_encl *encl = file->private_data;
-	unsigned long allowed_rwx;
+	unsigned long allowed_rwx, prot;
 	int ret;
 
 	allowed_rwx = sgx_allowed_rwx(encl, vma);
 	if (vma->vm_flags & (VM_READ | VM_WRITE | VM_EXEC) & ~allowed_rwx)
 		return -EACCES;
 
+	prot = _calc_vm_trans(vma->vm_flags, VM_READ, PROT_READ) |
+	       _calc_vm_trans(vma->vm_flags, VM_WRITE, PROT_WRITE) |
+	       _calc_vm_trans(vma->vm_flags, VM_EXEC, PROT_EXEC);
+	ret = security_enclave_map(prot);
+	if (ret)
+		return ret;
+
 	if (!sgx_encl_get_mm(encl, vma->vm_mm)) {
 		ret = sgx_encl_mm_add(encl, vma->vm_mm);
 		if (ret)
diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
index 6e9f3a41a40d..e061360f253c 100644
--- a/arch/x86/kernel/cpu/sgx/encl.c
+++ b/arch/x86/kernel/cpu/sgx/encl.c
@@ -2,6 +2,7 @@
 // Copyright(c) 2016-18 Intel Corporation.
 
 #include <linux/mm.h>
+#include <linux/security.h>
 #include <linux/shmem_fs.h>
 #include <linux/suspend.h>
 #include <linux/sched/mm.h>
@@ -344,11 +345,22 @@ static int sgx_vma_access(struct vm_area_struct *vma, unsigned long addr,
 	return ret < 0 ? ret : i;
 }
 
+#ifdef CONFIG_SECURITY
+static int sgx_vma_mprotect(struct vm_area_struct *vma, unsigned long start,
+			    unsigned long end, unsigned long prot)
+{
+	return security_enclave_map(prot);
+}
+#endif
+
 const struct vm_operations_struct sgx_vm_ops = {
 	.close = sgx_vma_close,
 	.open = sgx_vma_open,
 	.fault = sgx_vma_fault,
 	.access = sgx_vma_access,
+#ifdef CONFIG_SECURITY
+	.may_mprotect = sgx_vma_mprotect,
+#endif
 };
 
 /**
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 47f58cfb6a19..7c1357105e61 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1446,6 +1446,11 @@
  * @bpf_prog_free_security:
  *	Clean up the security information stored inside bpf prog.
  *
+ * Security hooks for Intel SGX enclaves.
+ *
+ * @enclave_map:
+ *	@prot contains the protection that will be applied by the kernel.
+ *	Return 0 if permission is granted.
  */
 union security_list_options {
 	int (*binder_set_context_mgr)(struct task_struct *mgr);
@@ -1807,6 +1812,10 @@ union security_list_options {
 	int (*bpf_prog_alloc_security)(struct bpf_prog_aux *aux);
 	void (*bpf_prog_free_security)(struct bpf_prog_aux *aux);
 #endif /* CONFIG_BPF_SYSCALL */
+
+#ifdef CONFIG_INTEL_SGX
+	int (*enclave_map)(unsigned long prot);
+#endif /* CONFIG_INTEL_SGX */
 };
 
 struct security_hook_heads {
@@ -2046,6 +2055,9 @@ struct security_hook_heads {
 	struct hlist_head bpf_prog_alloc_security;
 	struct hlist_head bpf_prog_free_security;
 #endif /* CONFIG_BPF_SYSCALL */
+#ifdef CONFIG_INTEL_SGX
+	struct hlist_head enclave_map;
+#endif /* CONFIG_INTEL_SGX */
 } __randomize_layout;
 
 /*
diff --git a/include/linux/security.h b/include/linux/security.h
index 659071c2e57c..6a1f54ba6794 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1829,5 +1829,16 @@ static inline void security_bpf_prog_free(struct bpf_prog_aux *aux)
 #endif /* CONFIG_SECURITY */
 #endif /* CONFIG_BPF_SYSCALL */
 
+#ifdef CONFIG_INTEL_SGX
+#ifdef CONFIG_SECURITY
+int security_enclave_map(unsigned long prot);
+#else
+static inline int security_enclave_map(unsigned long prot)
+{
+	return 0;
+}
+#endif /* CONFIG_SECURITY */
+#endif /* CONFIG_INTEL_SGX */
+
 #endif /* ! __LINUX_SECURITY_H */
 
diff --git a/security/security.c b/security/security.c
index 613a5c00e602..03951e08bdfc 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2359,3 +2359,10 @@ void security_bpf_prog_free(struct bpf_prog_aux *aux)
 	call_void_hook(bpf_prog_free_security, aux);
 }
 #endif /* CONFIG_BPF_SYSCALL */
+
+#ifdef CONFIG_INTEL_SGX
+int security_enclave_map(unsigned long prot)
+{
+	return call_int_hook(enclave_map, 0, prot);
+}
+#endif /* CONFIG_INTEL_SGX */
-- 
2.21.0

[RFC PATCH v3 08/12] security/selinux: Require SGX_EXECMEM to map enclave page WX

[Sean Christopherson]

Hook enclave_map() to require a new per-process capability, SGX_EXECMEM,
when mapping an enclave as simultaneously writable and executable.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 security/selinux/hooks.c            | 21 +++++++++++++++++++++
 security/selinux/include/classmap.h |  3 ++-
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 3ec702cf46ca..22e0f4a71333 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6726,6 +6726,23 @@ static void selinux_bpf_prog_free(struct bpf_prog_aux *aux)
 }
 #endif
 
+#ifdef CONFIG_INTEL_SGX
+static int selinux_enclave_map(unsigned long prot)
+{
+	const struct cred *cred = current_cred();
+	u32 sid = cred_sid(cred);
+
+	/* SGX is supported only in 64-bit kernels. */
+	WARN_ON_ONCE(!default_noexec);
+
+	if ((prot & PROT_EXEC) && (prot & PROT_WRITE))
+		return avc_has_perm(&selinux_state, sid, sid,
+				    SECCLASS_PROCESS2, PROCESS2__SGX_EXECMEM,
+				    NULL);
+	return 0;
+}
+#endif
+
 struct lsm_blob_sizes selinux_blob_sizes __lsm_ro_after_init = {
 	.lbs_cred = sizeof(struct task_security_struct),
 	.lbs_file = sizeof(struct file_security_struct),
@@ -6968,6 +6985,10 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(bpf_map_free_security, selinux_bpf_map_free),
 	LSM_HOOK_INIT(bpf_prog_free_security, selinux_bpf_prog_free),
 #endif
+
+#ifdef CONFIG_INTEL_SGX
+	LSM_HOOK_INIT(enclave_map, selinux_enclave_map),
+#endif
 };
 
 static __init int selinux_init(void)
diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
index 201f7e588a29..0f525f5b926f 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -51,7 +51,8 @@ struct security_class_mapping secclass_map[] = {
 	    "execmem", "execstack", "execheap", "setkeycreate",
 	    "setsockcreate", "getrlimit", NULL } },
 	{ "process2",
-	  { "nnp_transition", "nosuid_transition", NULL } },
+	  { "nnp_transition", "nosuid_transition",
+	    "sgx_execmem", NULL } },
 	{ "system",
 	  { "ipc_info", "syslog_read", "syslog_mod",
 	    "syslog_console", "module_request", "module_load", NULL } },
-- 
2.21.0

[RFC PATCH v3 09/12] LSM: x86/sgx: Introduce ->enclave_load() hook for Intel SGX

[Sean Christopherson]

enclave_load() is roughly analogous to the existing file_mprotect().

Due to the nature of SGX and its Enclave Page Cache (EPC), all enclave
VMAs are backed by a single file, i.e. /dev/sgx/enclave, that must be
MAP_SHARED.  Furthermore, all enclaves need read, write and execute
VMAs.  As a result, the existing/standard call to file_mprotect() does
not provide any meaningful security for enclaves since an LSM can only
deny/grant access to the EPC as a whole.

security_enclave_load() is called when SGX is first loading an enclave
page, i.e. copying a page from normal memory into the EPC.  Although
the prototype for enclave_load() is similar to file_mprotect(), e.g.
SGX could theoretically use file_mprotect() and set reqprot=prot, a
separate hook is desirable as the semantics of an enclave's protection
bits are different than those of vmas, e.g. an enclave page tracks the
maximal set of protections, whereas file_mprotect() operates on the
actual protections being provided.  Enclaves also have unique security
properties, e.g. measured code, that LSMs may want to consider.  In
other words, LSMs will likely want to implement different policies for
enclave page protections.

Note, extensive discussion yielded no sane alternative to some form of
SGX specific LSM hook[1].

[1] https://lkml.kernel.org/r/CALCETrXf8mSK45h7sTK5Wf+pXLVn=Bjsc_RLpgO-h-qdzBRo5Q@mail.gmail.com

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kernel/cpu/sgx/driver/ioctl.c | 32 ++++++++++++++------------
 include/linux/lsm_hooks.h              |  8 +++++++
 include/linux/security.h               |  7 ++++++
 security/security.c                    |  5 ++++
 4 files changed, 37 insertions(+), 15 deletions(-)

diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
index f239300e0fc2..9b554d698388 100644
--- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
@@ -9,6 +9,7 @@
 #include <linux/highmem.h>
 #include <linux/ratelimit.h>
 #include <linux/sched/signal.h>
+#include <linux/security.h>
 #include <linux/shmem_fs.h>
 #include <linux/slab.h>
 #include <linux/suspend.h>
@@ -564,7 +565,8 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
 	return ret;
 }
 
-static int sgx_encl_page_copy(void *dst, unsigned long src, unsigned long prot)
+static int sgx_encl_page_copy(void *dst, unsigned long src, unsigned long prot,
+			      u16 mrmask)
 {
 	struct vm_area_struct *vma;
 	int ret;
@@ -572,24 +574,24 @@ static int sgx_encl_page_copy(void *dst, unsigned long src, unsigned long prot)
 	/* Hold mmap_sem across copy_from_user() to avoid a TOCTOU race. */
 	down_read(&current->mm->mmap_sem);
 
+	vma = find_vma(current->mm, src);
+	if (!vma) {
+		ret = -EFAULT;
+		goto out;
+	}
+
 	/* Query vma's VM_MAYEXEC as an indirect path_noexec() check. */
-	if (prot & PROT_EXEC) {
-		vma = find_vma(current->mm, src);
-		if (!vma) {
-			ret = -EFAULT;
-			goto out;
-		}
-
-		if (!(vma->vm_flags & VM_MAYEXEC)) {
-			ret = -EACCES;
-			goto out;
-		}
+	if ((prot & PROT_EXEC) && !(vma->vm_flags & VM_MAYEXEC)) {
+		ret = -EACCES;
+		goto out;
 	}
 
+	ret = security_enclave_load(vma, prot, mrmask == 0xffff);
+	if (ret)
+		goto out;
+
 	if (copy_from_user(dst, (void __user *)src, PAGE_SIZE))
 		ret = -EFAULT;
-	else
-		ret = 0;
 
 out:
 	up_read(&current->mm->mmap_sem);
@@ -639,7 +641,7 @@ static long sgx_ioc_enclave_add_page(struct file *filep, void __user *arg)
 
 	prot = addp.flags & (PROT_READ | PROT_WRITE | PROT_EXEC);
 
-	ret = sgx_encl_page_copy(data, addp.src, prot);
+	ret = sgx_encl_page_copy(data, addp.src, prot, addp.mrmask);
 	if (ret)
 		goto out;
 
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 7c1357105e61..3bc92c65f287 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1451,6 +1451,11 @@
  * @enclave_map:
  *	@prot contains the protection that will be applied by the kernel.
  *	Return 0 if permission is granted.
+ *
+ * @enclave_load:
+ *	@vma: the source memory region of the enclave page being loaded.
+ *	@prot: the (maximal) protections of the enclave page.
+ *	Return 0 if permission is granted.
  */
 union security_list_options {
 	int (*binder_set_context_mgr)(struct task_struct *mgr);
@@ -1815,6 +1820,8 @@ union security_list_options {
 
 #ifdef CONFIG_INTEL_SGX
 	int (*enclave_map)(unsigned long prot);
+	int (*enclave_load)(struct vm_area_struct *vma, unsigned long prot,
+			    bool measured);
 #endif /* CONFIG_INTEL_SGX */
 };
 
@@ -2057,6 +2064,7 @@ struct security_hook_heads {
 #endif /* CONFIG_BPF_SYSCALL */
 #ifdef CONFIG_INTEL_SGX
 	struct hlist_head enclave_map;
+	struct hlist_head enclave_load;
 #endif /* CONFIG_INTEL_SGX */
 } __randomize_layout;
 
diff --git a/include/linux/security.h b/include/linux/security.h
index 6a1f54ba6794..572ddfc53039 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1832,11 +1832,18 @@ static inline void security_bpf_prog_free(struct bpf_prog_aux *aux)
 #ifdef CONFIG_INTEL_SGX
 #ifdef CONFIG_SECURITY
 int security_enclave_map(unsigned long prot);
+int security_enclave_load(struct vm_area_struct *vma, unsigned long prot,
+			  bool measured);
 #else
 static inline int security_enclave_map(unsigned long prot)
 {
 	return 0;
 }
+static inline int security_enclave_load(struct vm_area_struct *vma,
+					unsigned long prot, bool measured)
+{
+	return 0;
+}
 #endif /* CONFIG_SECURITY */
 #endif /* CONFIG_INTEL_SGX */
 
diff --git a/security/security.c b/security/security.c
index 03951e08bdfc..00f483beb1cc 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2365,4 +2365,9 @@ int security_enclave_map(unsigned long prot)
 {
 	return call_int_hook(enclave_map, 0, prot);
 }
+int security_enclave_load(struct vm_area_struct *vma, unsigned long prot,
+			  bool measured)
+{
+	return call_int_hook(enclave_load, 0, vma, prot, measured);
+}
 #endif /* CONFIG_INTEL_SGX */
-- 
2.21.0

[RFC PATCH v3 10/12] security/selinux: Add enclave_load() implementation

[Sean Christopherson]

The goal of selinux_enclave_load() is to provide a facsimile of the
existing selinux_file_mprotect() and file_map_prot_check() policies,
but tailored to the unique properties of SGX.

For example, an enclave page is technically backed by a MAP_SHARED file,
but the "file" is essentially shared memory that is never persisted
anywhere and also requires execute permissions (for some pages).

Enclaves are also less priveleged than normal user code, e.g. SYSCALL
instructions #UD if attempted in an enclave.  For this reason, add SGX
specific permissions instead of reusing existing permissions such as
FILE__EXECUTE so that policies can allow running code in an enclave, or
allow dynamically loading code in an enclave without having to grant the
same capability to normal user code outside of the enclave.

Intended use of each permission:

  - SGX_EXECMOD: dynamically load code within the enclave itself
  - SGX_EXECUNMR: load unmeasured code into the enclave, e.g. Graphene
  - SGX_EXECANON: load code from anonymous memory (likely Graphene)
  - SGX_EXECUTE: load an enclave from a file, i.e. normal behavior

Note, equivalents to FILE__READ and FILE__WRITE are intentionally never
required.  Writes to the enclave page are contained to the EPC, i.e.
never hit the original file, and read permissions have already been
vetted (or the VMA doesn't have PROT_READ, in which case loading the
page into the enclave will fail).

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 security/selinux/hooks.c            | 55 +++++++++++++++++++++++++++--
 security/selinux/include/classmap.h |  5 +--
 2 files changed, 55 insertions(+), 5 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 22e0f4a71333..ea452a416fe1 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6727,6 +6727,12 @@ static void selinux_bpf_prog_free(struct bpf_prog_aux *aux)
 #endif
 
 #ifdef CONFIG_INTEL_SGX
+static inline int sgx_has_perm(u32 sid, u32 requested)
+{
+	return avc_has_perm(&selinux_state, sid, sid,
+			    SECCLASS_PROCESS2, requested, NULL);
+}
+
 static int selinux_enclave_map(unsigned long prot)
 {
 	const struct cred *cred = current_cred();
@@ -6736,11 +6742,53 @@ static int selinux_enclave_map(unsigned long prot)
 	WARN_ON_ONCE(!default_noexec);
 
 	if ((prot & PROT_EXEC) && (prot & PROT_WRITE))
-		return avc_has_perm(&selinux_state, sid, sid,
-				    SECCLASS_PROCESS2, PROCESS2__SGX_EXECMEM,
-				    NULL);
+		return sgx_has_perm(sid, PROCESS2__SGX_EXECMEM);
+
 	return 0;
 }
+
+static int selinux_enclave_load(struct vm_area_struct *vma, unsigned long prot,
+				bool measured)
+{
+	const struct cred *cred = current_cred();
+	u32 sid = cred_sid(cred);
+	int ret;
+
+	/* SGX is supported only in 64-bit kernels. */
+	WARN_ON_ONCE(!default_noexec);
+
+	/* Only executable enclave pages are restricted in any way. */
+	if (!(prot & PROT_EXEC))
+		return 0;
+
+	/*
+	 * WX at load time only requires EXECMOD, e.g. to allow W->X.  Actual
+	 * WX mappings require EXECMEM (see selinux_enclave_map()).
+	 */
+	if (prot & PROT_WRITE) {
+		ret = sgx_has_perm(sid, PROCESS2__SGX_EXECMOD);
+		if (ret)
+			goto out;
+	}
+	if (!measured) {
+		ret = sgx_has_perm(sid, PROCESS2__SGX_EXECUNMR);
+		if (ret)
+			goto out;
+	}
+
+	if (!vma->vm_file || !IS_PRIVATE(file_inode(vma->vm_file)) ||
+	    vma->anon_vma)
+		/*
+		 * Loading enclave code from an anonymous mapping or from a
+		 * modified private file mapping.
+		 */
+		ret = sgx_has_perm(sid, PROCESS2__SGX_EXECANON);
+	else
+		/* Loading from a shared or unmodified private file mapping. */
+		ret = file_has_perm(cred, vma->vm_file, FILE__SGX_EXECUTE);
+out:
+	return ret;
+}
 #endif
 
 struct lsm_blob_sizes selinux_blob_sizes __lsm_ro_after_init = {
@@ -6988,6 +7036,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
 
 #ifdef CONFIG_INTEL_SGX
 	LSM_HOOK_INIT(enclave_map, selinux_enclave_map),
+	LSM_HOOK_INIT(enclave_load, selinux_enclave_load),
 #endif
 };
 
diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
index 0f525f5b926f..29a0a74268cd 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -52,7 +52,8 @@ struct security_class_mapping secclass_map[] = {
 	    "setsockcreate", "getrlimit", NULL } },
 	{ "process2",
 	  { "nnp_transition", "nosuid_transition",
-	    "sgx_execmem", NULL } },
+	    "sgx_execmem", "sgx_execmod", "sgx_execanon", "sgx_execunmr",
+	    NULL } },
 	{ "system",
 	  { "ipc_info", "syslog_read", "syslog_mod",
 	    "syslog_console", "module_request", "module_load", NULL } },
@@ -64,7 +65,7 @@ struct security_class_mapping secclass_map[] = {
 	    "quotaget", NULL } },
 	{ "file",
 	  { COMMON_FILE_PERMS,
-	    "execute_no_trans", "entrypoint", NULL } },
+	    "execute_no_trans", "entrypoint", "sgx_execute", NULL } },
 	{ "dir",
 	  { COMMON_FILE_PERMS, "add_name", "remove_name",
 	    "reparent", "search", "rmdir", NULL } },
-- 
2.21.0

[RFC PATCH v3 11/12] security/apparmor: Add enclave_load() implementation

[Sean Christopherson]

Require execute permissions when loading an enclave from a file.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 security/apparmor/include/audit.h |  2 ++
 security/apparmor/lsm.c           | 14 ++++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/security/apparmor/include/audit.h b/security/apparmor/include/audit.h
index ee559bc2acb8..84470483e04d 100644
--- a/security/apparmor/include/audit.h
+++ b/security/apparmor/include/audit.h
@@ -107,6 +107,8 @@ enum audit_type {
 #define OP_PROF_LOAD "profile_load"
 #define OP_PROF_RM "profile_remove"
 
+#define OP_ENCL_LOAD "enclave_load"
+
 
 struct apparmor_audit_data {
 	int error;
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 87500bde5a92..2ed1157e1f58 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -517,6 +517,17 @@ static int apparmor_file_mprotect(struct vm_area_struct *vma,
 			   !(vma->vm_flags & VM_SHARED) ? MAP_PRIVATE : 0);
 }
 
+#ifdef CONFIG_INTEL_SGX
+static int apparmor_enclave_load(struct vm_area_struct *vma, unsigned long prot,
+				bool measured)
+{
+	if (!(prot & PROT_EXEC))
+		return 0;
+
+	return common_file_perm(OP_ENCL_LOAD, vma->vm_file, AA_EXEC_MMAP);
+}
+#endif
+
 static int apparmor_sb_mount(const char *dev_name, const struct path *path,
 			     const char *type, unsigned long flags, void *data)
 {
@@ -1243,6 +1254,9 @@ static struct security_hook_list apparmor_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(secid_to_secctx, apparmor_secid_to_secctx),
 	LSM_HOOK_INIT(secctx_to_secid, apparmor_secctx_to_secid),
 	LSM_HOOK_INIT(release_secctx, apparmor_release_secctx),
+#ifdef CONFIG_INTEL_SGX
+	LSM_HOOK_INIT(enclave_load, apparmor_enclave_load),
+#endif
 };
 
 /*
-- 
2.21.0

[RFC PATCH v3 12/12] LSM: x86/sgx: Show line of sight to LSM support SGX2's EAUG

[Sean Christopherson]

Wire up a theoretical EAUG flag to show that the proposed LSM model is
extensible to SGX2, i.e. that SGX can communicate to LSMs that an EAUG'd
page is being mapped executable, as opposed to having to require
userspace to state that an EAUG'd page *may* be mapped executable in the
future.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kernel/cpu/sgx/driver/main.c | 10 +++++---
 arch/x86/kernel/cpu/sgx/encl.c        | 33 ++++++++++++++++++++++++++-
 arch/x86/kernel/cpu/sgx/encl.h        |  2 ++
 include/linux/lsm_hooks.h             |  2 +-
 include/linux/security.h              |  4 ++--
 security/security.c                   |  4 ++--
 security/selinux/hooks.c              |  4 +++-
 7 files changed, 49 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kernel/cpu/sgx/driver/main.c b/arch/x86/kernel/cpu/sgx/driver/main.c
index 816f14194d75..d471f965ede9 100644
--- a/arch/x86/kernel/cpu/sgx/driver/main.c
+++ b/arch/x86/kernel/cpu/sgx/driver/main.c
@@ -85,7 +85,8 @@ static int sgx_encl_mm_add(struct sgx_encl *encl, struct mm_struct *mm)
  * page is considered to have no RWX permissions, i.e. is inaccessible.
  */
 static unsigned long sgx_allowed_rwx(struct sgx_encl *encl,
-				     struct vm_area_struct *vma)
+				     struct vm_area_struct *vma,
+				     bool *eaug)
 {
 	unsigned long allowed_rwx = VM_READ | VM_WRITE | VM_EXEC;
 	unsigned long idx, idx_start, idx_end;
@@ -109,6 +110,8 @@ static unsigned long sgx_allowed_rwx(struct sgx_encl *encl,
 			allowed_rwx = 0;
 		else
 			allowed_rwx &= page->vm_prot_bits;
+		if (page->vm_prot_bits & SGX_VM_EAUG)
+			*eaug = true;
 		if (!allowed_rwx)
 			break;
 	}
@@ -120,16 +123,17 @@ static int sgx_mmap(struct file *file, struct vm_area_struct *vma)
 {
 	struct sgx_encl *encl = file->private_data;
 	unsigned long allowed_rwx, prot;
+	bool eaug = false;
 	int ret;
 
-	allowed_rwx = sgx_allowed_rwx(encl, vma);
+	allowed_rwx = sgx_allowed_rwx(encl, vma, &eaug);
 	if (vma->vm_flags & (VM_READ | VM_WRITE | VM_EXEC) & ~allowed_rwx)
 		return -EACCES;
 
 	prot = _calc_vm_trans(vma->vm_flags, VM_READ, PROT_READ) |
 	       _calc_vm_trans(vma->vm_flags, VM_WRITE, PROT_WRITE) |
 	       _calc_vm_trans(vma->vm_flags, VM_EXEC, PROT_EXEC);
-	ret = security_enclave_map(prot);
+	ret = security_enclave_map(prot, eaug);
 	if (ret)
 		return ret;
 
diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
index e061360f253c..8930d02afdc7 100644
--- a/arch/x86/kernel/cpu/sgx/encl.c
+++ b/arch/x86/kernel/cpu/sgx/encl.c
@@ -346,10 +346,41 @@ static int sgx_vma_access(struct vm_area_struct *vma, unsigned long addr,
 }
 
 #ifdef CONFIG_SECURITY
+static bool is_eaug_range(struct sgx_encl *encl, unsigned long start,
+			  unsigned long end)
+{
+	unsigned long idx, idx_start, idx_end;
+	struct sgx_encl_page *page;
+
+	/* Enclave is dead or inaccessible. */
+	if (!encl)
+		return false;
+
+	idx_start = PFN_DOWN(start);
+	idx_end = PFN_DOWN(end - 1);
+
+	for (idx = idx_start; idx <= idx_end; ++idx) {
+		/*
+		 * No need to take encl->lock, vm_prot_bits is set prior to
+		 * insertion and never changes, and racing with adding pages is
+		 * a userspace bug.
+		 */
+		rcu_read_lock();
+		page = radix_tree_lookup(&encl->page_tree, idx);
+		rcu_read_unlock();
+
+		/* Non-existent page can only be PROT_NONE, bail early. */
+		if (!page || page->vm_prot_bits & SGX_VM_EAUG)
+			return true;
+	}
+
+	return false;
+}
 static int sgx_vma_mprotect(struct vm_area_struct *vma, unsigned long start,
 			    unsigned long end, unsigned long prot)
 {
-	return security_enclave_map(prot);
+	return security_enclave_map(prot,
+		is_eaug_range(vma->vm_private_data, start, end));
 }
 #endif
 
diff --git a/arch/x86/kernel/cpu/sgx/encl.h b/arch/x86/kernel/cpu/sgx/encl.h
index 5cb0f2653d4c..1976866c096a 100644
--- a/arch/x86/kernel/cpu/sgx/encl.h
+++ b/arch/x86/kernel/cpu/sgx/encl.h
@@ -39,6 +39,8 @@ enum sgx_encl_page_desc {
 #define SGX_ENCL_PAGE_VA_OFFSET(encl_page) \
 	((encl_page)->desc & SGX_ENCL_PAGE_VA_OFFSET_MASK)
 
+#define SGX_VM_EAUG	BIT(3)
+
 struct sgx_encl_page {
 	unsigned long desc;
 	unsigned long vm_prot_bits;
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 3bc92c65f287..d7da732cf56e 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1819,7 +1819,7 @@ union security_list_options {
 #endif /* CONFIG_BPF_SYSCALL */
 
 #ifdef CONFIG_INTEL_SGX
-	int (*enclave_map)(unsigned long prot);
+	int (*enclave_map)(unsigned long prot, bool eaug);
 	int (*enclave_load)(struct vm_area_struct *vma, unsigned long prot,
 			    bool measured);
 #endif /* CONFIG_INTEL_SGX */
diff --git a/include/linux/security.h b/include/linux/security.h
index 572ddfc53039..c55e14d776c8 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1831,11 +1831,11 @@ static inline void security_bpf_prog_free(struct bpf_prog_aux *aux)
 
 #ifdef CONFIG_INTEL_SGX
 #ifdef CONFIG_SECURITY
-int security_enclave_map(unsigned long prot);
+int security_enclave_map(unsigned long prot, bool eaug);
 int security_enclave_load(struct vm_area_struct *vma, unsigned long prot,
 			  bool measured);
 #else
-static inline int security_enclave_map(unsigned long prot)
+static inline int security_enclave_map(unsigned long prot, bool eaug)
 {
 	return 0;
 }
diff --git a/security/security.c b/security/security.c
index 00f483beb1cc..f276f05341f2 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2361,9 +2361,9 @@ void security_bpf_prog_free(struct bpf_prog_aux *aux)
 #endif /* CONFIG_BPF_SYSCALL */
 
 #ifdef CONFIG_INTEL_SGX
-int security_enclave_map(unsigned long prot)
+int security_enclave_map(unsigned long prot, bool eaug)
 {
-	return call_int_hook(enclave_map, 0, prot);
+	return call_int_hook(enclave_map, 0, prot, eaug);
 }
 int security_enclave_load(struct vm_area_struct *vma, unsigned long prot,
 			  bool measured)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index ea452a416fe1..e3b431b2224a 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6733,7 +6733,7 @@ static inline int sgx_has_perm(u32 sid, u32 requested)
 			    SECCLASS_PROCESS2, requested, NULL);
 }
 
-static int selinux_enclave_map(unsigned long prot)
+static int selinux_enclave_map(unsigned long prot, bool eaug)
 {
 	const struct cred *cred = current_cred();
 	u32 sid = cred_sid(cred);
@@ -6743,6 +6743,8 @@ static int selinux_enclave_map(unsigned long prot)
 
 	if ((prot & PROT_EXEC) && (prot & PROT_WRITE))
 		return sgx_has_perm(sid, PROCESS2__SGX_EXECMEM);
+	else if (eaug && (prot & PROT_EXEC))
+		return sgx_has_perm(sid, PROCESS2__SGX_EXECMOD);
 
 	return 0;
 }
-- 
2.21.0

Re: [RFC PATCH v3 01/12] x86/sgx: Add mm to enclave at mmap()

[Dave Hansen]

On 6/17/19 3:24 PM, Sean Christopherson wrote:
> +static int sgx_encl_mm_add(struct sgx_encl *encl, struct mm_struct *mm)
> +{
> +	struct sgx_encl_mm *encl_mm;
> +
> +	encl_mm = kzalloc(sizeof(*encl_mm), GFP_KERNEL);
> +	if (!encl_mm)
> +		return -ENOMEM;
> +
> +	encl_mm->encl = encl;
> +	encl_mm->mm = mm;
> +	kref_init(&encl_mm->refcount);
> +
> +	spin_lock(&encl->mm_lock);
> +	list_add(&encl_mm->list, &encl->mm_list);
> +	spin_unlock(&encl->mm_lock);
> +
> +	return 0;
> +}

This probably need commenting and/or changelogging about the mm
refcounting or lack thereof.

Re: [RFC PATCH v3 01/12] x86/sgx: Add mm to enclave at mmap()

[Andy Lutomirski]

On Mon, Jun 17, 2019 at 3:24 PM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> The enclave mm tracking is currently broken:
>
>   - Adding current->mm during ECREATE is wrong as there is no guarantee
>     that the current process has mmap()'d the enclave, i.e. there may
>     never be an associated sgx_vma_close() to drop the encl_mm.
>
>   - Adding mm's at sgx_vma_open() is wrong as vm_ops->open is called
>     only when splitting or duplicating a vma.  If userspace performs a
>     single mmap() on the enclave then SGX will fail to track the mm.
>     This bug is partially hidden by tracking current->mm at ECREATE.
>
> Rework the tracking to get/add the mm at mmap().  A side effect of the
> bug fix is that sgx_vma_{open,close}() should never encounter a vma with
> an associated enclave and no associated encl_mm, i.e. WARN if an encl_mm
> cannot be found in either condition.
>

It would be nifty if you could also kill .vm_close, since then VMAs
could be merged properly.  Would this be straightforward?

--Andy

Re: [RFC PATCH v3 00/12] security: x86/sgx: SGX vs. LSM, round 3

[Stephen Smalley]

On 6/17/19 6:24 PM, Sean Christopherson wrote:
> My original plan was for my next RFC to be an implementation of Andy's
> proposed "dynamic tracking" model.  I actually finished the tracking
> portion, but was completely flummoxed by the auditing[1].  Since Cedric's
> RFC is essentially a variation of the dynamic tracking model, it too has
> the same auditing complexities.  End result, I ended back at the "make
> userspace state its intentions" approach.
> 
> Except for patch 12 (see below), the SGX changes have been fully tested,
> including updating the kernel's selftest as well as my own fork of (an old
> version of) Intel's SDK to use the new UAPI.  The LSM changes have been
> smoke tested, but I haven't actually configured AppArmor or SELinux to
> verify the permissions work as intended.

Was dropping linux-security-module and selinux lists intentional for 
this RFC? Not recommended.

Is the entire series aside from patch 12 available in a public tree 
somewhere?

Ultimately we'll want additions to the selinux-testsuite that exercise 
each of the new permissions, both a permission denied scenario and a 
permission allowed scenario.

> 
> Patches 1-3 are bug fixes that should go into v21 regardless of what we
> end up doing for LSM support.  They're included here as the actual LSM
> RFC patches are essentially untestable without them.
> 
> Patches 4-11 are the meat of the RFC.
> 
> Patch 12 is purely to show how we might implement SGX2 support.  It's not
> intended to be included in v21.
> 
> 
> This series is a delta to Jarkko's ongoing SGX series and applies on
> Jarkko's current master at https://github.com/jsakkine-intel/linux-sgx.git:
> 
>    4cc5d411db1b ("docs: x86/sgx: Document the enclave API")
> 
> The basic gist of the approach is to track an enclave's page protections
> separately from any vmas that map the page, and separate from the hardware
> enforced protections.  The SGX UAPI is modified to require userspace to
> explicitly define the protections for each enclave page, i.e. the ioctl
> to add pages to an enclave is extended to take PROT_{READ,WRITE,EXEC}
> flags.
> 
> An enclave page's protections are the maximal protections that userspace
> can use to map the page, e.g. mprotect() and mmap() are rejected if the
> protections for the vma would be more permissible than those of the
> associated enclave page.
> 
> Tracking protections for an enclave page (in additional to vmas) allows
> SGX to invoke LSM upcalls while the enclave is being built.  This is
> critical to enabling LSMs to implement policies for enclave pages that
> are functionally equivalent to existing policies for normal pages.
> 
> 
> [1] https://lkml.kernel.org/r/20190614003759.GE18385@linux.intel.com
> 
> v1: https://lkml.kernel.org/r/20190531233159.30992-1-sean.j.christopherson@intel.com
> 
> v2: https://lkml.kernel.org/r/20190606021145.12604-1-sean.j.christopherson@intel.com
> 
>    - Dropped the patch(es) to extend the SGX UAPI to allow adding multiple
>      enclave pages in a single syscall [Jarkko].
> 
>    - Reject ioctl() immediately on LSM denial [Stephen].
> 
>    - Rework SELinux code to avoid checking EXEMEM multiple times [Stephen].
> 
>    - Adding missing equivalents to existing selinux_file_protect() checks
>      [Stephen].
> 
>    - Hold mmap_sem across copy_to_user() to prevent a TOCTOU race when
>      checking the source vma [Stephen].
> 
>    - Stubify security_enclave_load() if !CONFIG_SECURITY [Stephen].
> 
>    - Make flags a 32-bit field [Andy].
> 
>    - Don't validate the SECINFO protection flags against the enclave
>      page's protection flags [Andy].
> 
>    - Rename mprotect() hook to may_mprotect() [Andy].
> 
>    - Test 'vma->vm_flags & VM_MAYEXEC' instead of manually checking for
>      a noexec path [Jarkko].
> 
>    - Drop the SGX defined flags (use PROT_*) [Jarkko].
> 
>    - Improve comments and changelogs [Jarkko].
> 
> v3:
>    - Clear VM_MAY* flags instead of using .may_mprotect() to enforce
>      maximal enclave page protections.
> 
>    - Update the SGX selftest to work with the new API.
> 
>    - Rewrite SELinux code to use SGX specific permissions, with the goal
>      of addressing Andy's feedback regarding what people will actually
>      care about when it comes to SGX, e.g. add permissions for restricing
>      unmeasured code and stop trying to infer permissions from the source
>      of each enclave page.
> 
>    - Add a (very minimal) AppArmor patch.
> 
>    - Show line of sight to SGX2 support.
> 
>    - Rebased to Jarkko's latest code base.
> 
> Sean Christopherson (12):
>    x86/sgx: Add mm to enclave at mmap()
>    x86/sgx: Do not naturally align MAP_FIXED address
>    selftests: x86/sgx: Mark the enclave loader as not needing an exec
>      stack
>    x86/sgx: Require userspace to define enclave pages' protection bits
>    x86/sgx: Enforce noexec filesystem restriction for enclaves
>    mm: Introduce vm_ops->may_mprotect()
>    LSM: x86/sgx: Introduce ->enclave_map() hook for Intel SGX
>    security/selinux: Require SGX_EXECMEM to map enclave page WX
>    LSM: x86/sgx: Introduce ->enclave_load() hook for Intel SGX
>    security/selinux: Add enclave_load() implementation
>    security/apparmor: Add enclave_load() implementation
>    LSM: x86/sgx: Show line of sight to LSM support SGX2's EAUG
> 
>   arch/x86/include/uapi/asm/sgx.h          |  5 +-
>   arch/x86/kernel/cpu/sgx/driver/ioctl.c   | 69 +++++++++++------
>   arch/x86/kernel/cpu/sgx/driver/main.c    | 94 +++++++++++++++++++++++-
>   arch/x86/kernel/cpu/sgx/encl.c           | 81 ++++++++++++--------
>   arch/x86/kernel/cpu/sgx/encl.h           |  7 +-
>   include/linux/lsm_hooks.h                | 20 +++++
>   include/linux/mm.h                       |  2 +
>   include/linux/security.h                 | 18 +++++
>   mm/mprotect.c                            | 15 +++-
>   security/apparmor/include/audit.h        |  2 +
>   security/apparmor/lsm.c                  | 14 ++++
>   security/security.c                      | 12 +++
>   security/selinux/hooks.c                 | 72 ++++++++++++++++++
>   security/selinux/include/classmap.h      |  6 +-
>   tools/testing/selftests/x86/sgx/Makefile |  2 +-
>   tools/testing/selftests/x86/sgx/main.c   | 32 +++++++-
>   16 files changed, 384 insertions(+), 67 deletions(-)
> 

Re: [RFC PATCH v3 00/12] security: x86/sgx: SGX vs. LSM, round 3

[Sean Christopherson]

On Tue, Jun 18, 2019 at 09:38:19AM -0400, Stephen Smalley wrote:
> On 6/17/19 6:24 PM, Sean Christopherson wrote:
> >My original plan was for my next RFC to be an implementation of Andy's
> >proposed "dynamic tracking" model.  I actually finished the tracking
> >portion, but was completely flummoxed by the auditing[1].  Since Cedric's
> >RFC is essentially a variation of the dynamic tracking model, it too has
> >the same auditing complexities.  End result, I ended back at the "make
> >userspace state its intentions" approach.
> >
> >Except for patch 12 (see below), the SGX changes have been fully tested,
> >including updating the kernel's selftest as well as my own fork of (an old
> >version of) Intel's SDK to use the new UAPI.  The LSM changes have been
> >smoke tested, but I haven't actually configured AppArmor or SELinux to
> >verify the permissions work as intended.
> 
> Was dropping linux-security-module and selinux lists intentional for this
> RFC? Not recommended.

Yes, my thought was to keep the noise to the sgx list until we at least
agree on a direction for the SGX UAPI.  I am fully expecting that whatever
LSM and SELinux patches we end up with will go through a lot more scrutiny
when Jarkko sends them with his SGX series.

Anyways, would you like me to resend the series to Cc the aforementioned
lists?

> Is the entire series aside from patch 12 available in a public tree
> somewhere?

I pushed tag 'sgx-lsm-v3' to https://github.com/sean-jc/linux.git.

> Ultimately we'll want additions to the selinux-testsuite that exercise each
> of the new permissions, both a permission denied scenario and a permission
> allowed scenario.

Re: [RFC PATCH v3 01/12] x86/sgx: Add mm to enclave at mmap()

[Sean Christopherson]

On Mon, Jun 17, 2019 at 04:42:59PM -0700, Andy Lutomirski wrote:
> On Mon, Jun 17, 2019 at 3:24 PM Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
> >
> > The enclave mm tracking is currently broken:
> >
> >   - Adding current->mm during ECREATE is wrong as there is no guarantee
> >     that the current process has mmap()'d the enclave, i.e. there may
> >     never be an associated sgx_vma_close() to drop the encl_mm.
> >
> >   - Adding mm's at sgx_vma_open() is wrong as vm_ops->open is called
> >     only when splitting or duplicating a vma.  If userspace performs a
> >     single mmap() on the enclave then SGX will fail to track the mm.
> >     This bug is partially hidden by tracking current->mm at ECREATE.
> >
> > Rework the tracking to get/add the mm at mmap().  A side effect of the
> > bug fix is that sgx_vma_{open,close}() should never encounter a vma with
> > an associated enclave and no associated encl_mm, i.e. WARN if an encl_mm
> > cannot be found in either condition.
> >
> 
> It would be nifty if you could also kill .vm_close, since then VMAs
> could be merged properly.  Would this be straightforward?

Hmm, we probably could move the mm tracking to f_op->{open,release}.  The
downside to that approach is that EPC reclaim would unnecessarily walk the
vmas for processes that have opened the enclave but not mapped any EPC
pages.  In the grand scheme, that's a minor issue and probably worth the
tradeoff of vma merging.

On the plus side, in addition to zapping ->close, I think it would allow
for a simpler vma walking scheme.  Maybe.

Re: [RFC PATCH v3 10/12] security/selinux: Add enclave_load() implementation

[Stephen Smalley]

On 6/17/19 6:24 PM, Sean Christopherson wrote:
> The goal of selinux_enclave_load() is to provide a facsimile of the
> existing selinux_file_mprotect() and file_map_prot_check() policies,
> but tailored to the unique properties of SGX.
> 
> For example, an enclave page is technically backed by a MAP_SHARED file,
> but the "file" is essentially shared memory that is never persisted
> anywhere and also requires execute permissions (for some pages).
> 
> Enclaves are also less priveleged than normal user code, e.g. SYSCALL
> instructions #UD if attempted in an enclave.  For this reason, add SGX
> specific permissions instead of reusing existing permissions such as
> FILE__EXECUTE so that policies can allow running code in an enclave, or
> allow dynamically loading code in an enclave without having to grant the
> same capability to normal user code outside of the enclave.
> 
> Intended use of each permission:
> 
>    - SGX_EXECMOD: dynamically load code within the enclave itself
>    - SGX_EXECUNMR: load unmeasured code into the enclave, e.g. Graphene
>    - SGX_EXECANON: load code from anonymous memory (likely Graphene)
>    - SGX_EXECUTE: load an enclave from a file, i.e. normal behavior
> 
> Note, equivalents to FILE__READ and FILE__WRITE are intentionally never
> required.  Writes to the enclave page are contained to the EPC, i.e.
> never hit the original file, and read permissions have already been
> vetted (or the VMA doesn't have PROT_READ, in which case loading the
> page into the enclave will fail).
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>   security/selinux/hooks.c            | 55 +++++++++++++++++++++++++++--
>   security/selinux/include/classmap.h |  5 +--
>   2 files changed, 55 insertions(+), 5 deletions(-)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 22e0f4a71333..ea452a416fe1 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -6727,6 +6727,12 @@ static void selinux_bpf_prog_free(struct bpf_prog_aux *aux)
>   #endif
>   
>   #ifdef CONFIG_INTEL_SGX
> +static inline int sgx_has_perm(u32 sid, u32 requested)
> +{
> +	return avc_has_perm(&selinux_state, sid, sid,
> +			    SECCLASS_PROCESS2, requested, NULL);
> +}
> +
>   static int selinux_enclave_map(unsigned long prot)
>   {
>   	const struct cred *cred = current_cred();
> @@ -6736,11 +6742,53 @@ static int selinux_enclave_map(unsigned long prot)
>   	WARN_ON_ONCE(!default_noexec);
>   
>   	if ((prot & PROT_EXEC) && (prot & PROT_WRITE))
> -		return avc_has_perm(&selinux_state, sid, sid,
> -				    SECCLASS_PROCESS2, PROCESS2__SGX_EXECMEM,
> -				    NULL);
> +		return sgx_has_perm(sid, PROCESS2__SGX_EXECMEM);
> +
>   	return 0;
>   }
> +
> +static int selinux_enclave_load(struct vm_area_struct *vma, unsigned long prot,
> +				bool measured)
> +{
> +	const struct cred *cred = current_cred();
> +	u32 sid = cred_sid(cred);
> +	int ret;
> +
> +	/* SGX is supported only in 64-bit kernels. */
> +	WARN_ON_ONCE(!default_noexec);
> +
> +	/* Only executable enclave pages are restricted in any way. */
> +	if (!(prot & PROT_EXEC))
> +		return 0;
> +
> +	/*
> +	 * WX at load time only requires EXECMOD, e.g. to allow W->X.  Actual
> +	 * WX mappings require EXECMEM (see selinux_enclave_map()).
> +	 */
> +	if (prot & PROT_WRITE) {
> +		ret = sgx_has_perm(sid, PROCESS2__SGX_EXECMOD);

So, security_enclave_load() can be called with PROT_WRITE|PROT_EXEC, but 
the subsequent calls to security_enclave_map() won't have both set 
unless a mapping is actually simultaneously WX?  Is that correct? 
Trying to make sure that every PROCESS2__SGX_EXECMOD check here won't be 
followed immediately by a PROCESS2__SGX_EXECMEM check from 
security_enclave_map(), thereby rendering any distinction between them moot.

> +		if (ret)
> +			goto out;
> +	}
> +	if (!measured) {
> +		ret = sgx_has_perm(sid, PROCESS2__SGX_EXECUNMR);

I'm unclear on the concept of loading unmeasured code into an enclave; I 
thought that wasn't supposed to happen apart from SGX2.  How does that 
occur?

> +		if (ret)
> +			goto out;
> +	}
> +
> +	if (!vma->vm_file || !IS_PRIVATE(file_inode(vma->vm_file)) ||

I think this should be IS_PRIVATE(), not !IS_PRIVATE()?

> +	    vma->anon_vma)
> +		/*
> +		 * Loading enclave code from an anonymous mapping or from a
> +		 * modified private file mapping.
> +		 */
> +		ret = sgx_has_perm(sid, PROCESS2__SGX_EXECANON);

I might have expected this to be EXECMEM.  It is actually a blend of 
EXECMEM and EXECMOD, so I'm ok with the new name. However, I'm wondering 
if you should use an entirely different set of permission names than 
EXECMEM or EXECMOD for the other checks too since none of them quite 
align with the existing usage; your SGX__EXECMEM is effectively MAPWX 
and your SGX__EXECMOD is effectively MAPXAFTERW.  Or something like that.

> +	else
> +		/* Loading from a shared or unmodified private file mapping. */
> +		ret = file_has_perm(cred, vma->vm_file, FILE__SGX_EXECUTE);
> +out:
> +	return ret;
> +}
>   #endif
>   
>   struct lsm_blob_sizes selinux_blob_sizes __lsm_ro_after_init = {
> @@ -6988,6 +7036,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
>   
>   #ifdef CONFIG_INTEL_SGX
>   	LSM_HOOK_INIT(enclave_map, selinux_enclave_map),
> +	LSM_HOOK_INIT(enclave_load, selinux_enclave_load),
>   #endif
>   };
>   
> diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
> index 0f525f5b926f..29a0a74268cd 100644
> --- a/security/selinux/include/classmap.h
> +++ b/security/selinux/include/classmap.h
> @@ -52,7 +52,8 @@ struct security_class_mapping secclass_map[] = {
>   	    "setsockcreate", "getrlimit", NULL } },
>   	{ "process2",
>   	  { "nnp_transition", "nosuid_transition",
> -	    "sgx_execmem", NULL } },
> +	    "sgx_execmem", "sgx_execmod", "sgx_execanon", "sgx_execunmr",
> +	    NULL } },
>   	{ "system",
>   	  { "ipc_info", "syslog_read", "syslog_mod",
>   	    "syslog_console", "module_request", "module_load", NULL } },
> @@ -64,7 +65,7 @@ struct security_class_mapping secclass_map[] = {
>   	    "quotaget", NULL } },
>   	{ "file",
>   	  { COMMON_FILE_PERMS,
> -	    "execute_no_trans", "entrypoint", NULL } },
> +	    "execute_no_trans", "entrypoint", "sgx_execute", NULL } },

If there is any possibility of a mapping of a non-regular file reaching 
the point of the permission check, then the permission needs to either 
be added to COMMON_FILE_PERMS or be added to each of the *_file classes 
for which it can legitimately be checked.  That's why execmod is in 
COMMON_FILE_PERMS; it can show up on e.g. a modified private file 
mapping of a device file in addition to regular files.

>   	{ "dir",
>   	  { COMMON_FILE_PERMS, "add_name", "remove_name",
>   	    "reparent", "search", "rmdir", NULL } },
> 

Re: [RFC PATCH v3 00/12] security: x86/sgx: SGX vs. LSM, round 3

[Stephen Smalley]

On 6/18/19 9:55 AM, Sean Christopherson wrote:
> On Tue, Jun 18, 2019 at 09:38:19AM -0400, Stephen Smalley wrote:
>> On 6/17/19 6:24 PM, Sean Christopherson wrote:
>>> My original plan was for my next RFC to be an implementation of Andy's
>>> proposed "dynamic tracking" model.  I actually finished the tracking
>>> portion, but was completely flummoxed by the auditing[1].  Since Cedric's
>>> RFC is essentially a variation of the dynamic tracking model, it too has
>>> the same auditing complexities.  End result, I ended back at the "make
>>> userspace state its intentions" approach.
>>>
>>> Except for patch 12 (see below), the SGX changes have been fully tested,
>>> including updating the kernel's selftest as well as my own fork of (an old
>>> version of) Intel's SDK to use the new UAPI.  The LSM changes have been
>>> smoke tested, but I haven't actually configured AppArmor or SELinux to
>>> verify the permissions work as intended.
>>
>> Was dropping linux-security-module and selinux lists intentional for this
>> RFC? Not recommended.
> 
> Yes, my thought was to keep the noise to the sgx list until we at least
> agree on a direction for the SGX UAPI.  I am fully expecting that whatever
> LSM and SELinux patches we end up with will go through a lot more scrutiny
> when Jarkko sends them with his SGX series.
> 
> Anyways, would you like me to resend the series to Cc the aforementioned
> lists?

I guess it depends on how soon you plan to spin another version. If 
soon, then you can wait on the next round.  But I wouldn't wait until 
everything else is fully baked because the LSM discussion might chase 
out issues that require changes elsewhere.

> 
>> Is the entire series aside from patch 12 available in a public tree
>> somewhere?
> 
> I pushed tag 'sgx-lsm-v3' to https://github.com/sean-jc/linux.git.
> 
>> Ultimately we'll want additions to the selinux-testsuite that exercise each
>> of the new permissions, both a permission denied scenario and a permission
>> allowed scenario.

Re: [RFC PATCH v3 01/12] x86/sgx: Add mm to enclave at mmap()

[Sean Christopherson]

On Tue, Jun 18, 2019 at 07:11:47AM -0700, Sean Christopherson wrote:
> On Mon, Jun 17, 2019 at 04:42:59PM -0700, Andy Lutomirski wrote:
> > On Mon, Jun 17, 2019 at 3:24 PM Sean Christopherson
> > <sean.j.christopherson@intel.com> wrote:
> > >
> > > The enclave mm tracking is currently broken:
> > >
> > >   - Adding current->mm during ECREATE is wrong as there is no guarantee
> > >     that the current process has mmap()'d the enclave, i.e. there may
> > >     never be an associated sgx_vma_close() to drop the encl_mm.
> > >
> > >   - Adding mm's at sgx_vma_open() is wrong as vm_ops->open is called
> > >     only when splitting or duplicating a vma.  If userspace performs a
> > >     single mmap() on the enclave then SGX will fail to track the mm.
> > >     This bug is partially hidden by tracking current->mm at ECREATE.
> > >
> > > Rework the tracking to get/add the mm at mmap().  A side effect of the
> > > bug fix is that sgx_vma_{open,close}() should never encounter a vma with
> > > an associated enclave and no associated encl_mm, i.e. WARN if an encl_mm
> > > cannot be found in either condition.
> > >
> > 
> > It would be nifty if you could also kill .vm_close, since then VMAs
> > could be merged properly.  Would this be straightforward?
> 
> Hmm, we probably could move the mm tracking to f_op->{open,release}.  The

Scratch that thought, pre-coffee brain was thinking there was a magical
file op that was called whenever a new reference to the file was acquired.

The only idea I can think of to avoid .vm_close() would be to not refcount
the mm at all, and instead register a mmu notifier at .mmap() and use
mmu_notifier.release() to remove the mm.  The downside is that the mm
tracking would be sticky, i.e. once a process mmap()'d an enclave it would
forever be tracked by the enclave.  Practically speaking, in the worst
case this would add a small amount of overhead to reclaiming from an
enclave that was temporarily mapped by multiple processes.  So it might
be a viable option?

> downside to that approach is that EPC reclaim would unnecessarily walk the
> vmas for processes that have opened the enclave but not mapped any EPC
> pages.  In the grand scheme, that's a minor issue and probably worth the
> tradeoff of vma merging.
> 
> On the plus side, in addition to zapping ->close, I think it would allow
> for a simpler vma walking scheme.  Maybe.

Re: [RFC PATCH v3 01/12] x86/sgx: Add mm to enclave at mmap()

[Jarkko Sakkinen]

On Mon, 2019-06-17 at 15:24 -0700, Sean Christopherson wrote:
> The enclave mm tracking is currently broken:
> 
>   - Adding current->mm during ECREATE is wrong as there is no guarantee
>     that the current process has mmap()'d the enclave, i.e. there may
>     never be an associated sgx_vma_close() to drop the encl_mm.
> 
>   - Adding mm's at sgx_vma_open() is wrong as vm_ops->open is called
>     only when splitting or duplicating a vma.  If userspace performs a
>     single mmap() on the enclave then SGX will fail to track the mm.
>     This bug is partially hidden by tracking current->mm at ECREATE.
> 
> Rework the tracking to get/add the mm at mmap().  A side effect of the
> bug fix is that sgx_vma_{open,close}() should never encounter a vma with
> an associated enclave and no associated encl_mm, i.e. WARN if an encl_mm
> cannot be found in either condition.
> 
> Change the WARN() on a non-empty mm_list to a WARN_ONCE().  The warning
> will fire over and over (and over) if the mm tracking is broken, which
> hampers debug/triage far more than it helps.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/kernel/cpu/sgx/driver/ioctl.c | 14 ----------
>  arch/x86/kernel/cpu/sgx/driver/main.c  | 26 ++++++++++++++++++
>  arch/x86/kernel/cpu/sgx/encl.c         | 38 +++++---------------------
>  arch/x86/kernel/cpu/sgx/encl.h         |  4 +--
>  4 files changed, 35 insertions(+), 47 deletions(-)

BTW, sgx_encl_mm is a bit confusing name (which I made) to begin with. 
It clues as it was a some kind of 1:1 association with the process,
which it isn't.

Could you update the patch and rename it as sgx_encl_mapping? After that
I'm happy to merge.

/Jarkko

Re: [RFC PATCH v3 01/12] x86/sgx: Add mm to enclave at mmap()

[Jarkko Sakkinen]

On Wed, 2019-06-19 at 15:56 +0300, Jarkko Sakkinen wrote:
> On Mon, 2019-06-17 at 15:24 -0700, Sean Christopherson wrote:
> > The enclave mm tracking is currently broken:
> > 
> >   - Adding current->mm during ECREATE is wrong as there is no guarantee
> >     that the current process has mmap()'d the enclave, i.e. there may
> >     never be an associated sgx_vma_close() to drop the encl_mm.
> > 
> >   - Adding mm's at sgx_vma_open() is wrong as vm_ops->open is called
> >     only when splitting or duplicating a vma.  If userspace performs a
> >     single mmap() on the enclave then SGX will fail to track the mm.
> >     This bug is partially hidden by tracking current->mm at ECREATE.
> > 
> > Rework the tracking to get/add the mm at mmap().  A side effect of the
> > bug fix is that sgx_vma_{open,close}() should never encounter a vma with
> > an associated enclave and no associated encl_mm, i.e. WARN if an encl_mm
> > cannot be found in either condition.
> > 
> > Change the WARN() on a non-empty mm_list to a WARN_ONCE().  The warning
> > will fire over and over (and over) if the mm tracking is broken, which
> > hampers debug/triage far more than it helps.
> > 
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > ---
> >  arch/x86/kernel/cpu/sgx/driver/ioctl.c | 14 ----------
> >  arch/x86/kernel/cpu/sgx/driver/main.c  | 26 ++++++++++++++++++
> >  arch/x86/kernel/cpu/sgx/encl.c         | 38 +++++---------------------
> >  arch/x86/kernel/cpu/sgx/encl.h         |  4 +--
> >  4 files changed, 35 insertions(+), 47 deletions(-)
> 
> BTW, sgx_encl_mm is a bit confusing name (which I made) to begin with. 
> It clues as it was a some kind of 1:1 association with the process,
> which it isn't.
> 
> Could you update the patch and rename it as sgx_encl_mapping? After that
> I'm happy to merge.

And send it as a separate patch. Can merge it today/tomorrow after I've
finished my reaper change.

/Jarkko

Re: [RFC PATCH v3 02/12] x86/sgx: Do not naturally align MAP_FIXED address

[Jarkko Sakkinen]

On Mon, 2019-06-17 at 15:24 -0700, Sean Christopherson wrote:
>  {
> -	if (len < 2 * PAGE_SIZE || len & (len - 1) || flags & MAP_PRIVATE)
> +	if (flags & MAP_PRIVATE)
> +		return -EINVAL;
> +
> +	if (flags & MAP_FIXED)
> +		return addr;
> +
> +	if (len < 2 * PAGE_SIZE || len & (len - 1))
>  		return -EINVAL;
>
>  	addr = current->mm->get_unmapped_area(file, addr, 2 * len, pgoff,

Just sanity checking that for MAP_FIXED case the mm checks that the area is
unmapped before calling this?

I don't think we need to check any alignment constraints here anymore.

The summarize end result would be:

static unsigned long sgx_get_unmapped_area(struct file *file,
					   unsigned long addr,
					   unsigned long len,
					   unsigned long pgoff,
					   unsigned long flags)
{
	if (flags & MAP_PRIVATE)
		return -EINVAL;

	if (flags & MAP_FIXED)
		return addr;

	return current->mm->get_unmapped_area(file, addr, 2 * len, pgoff,
					      flags);
}

/Jarkko

Re: [RFC PATCH v3 02/12] x86/sgx: Do not naturally align MAP_FIXED address

[Sean Christopherson]

On Wed, Jun 19, 2019 at 04:24:05PM +0300, Jarkko Sakkinen wrote:
> On Mon, 2019-06-17 at 15:24 -0700, Sean Christopherson wrote:
> >  {
> > -	if (len < 2 * PAGE_SIZE || len & (len - 1) || flags & MAP_PRIVATE)
> > +	if (flags & MAP_PRIVATE)
> > +		return -EINVAL;
> > +
> > +	if (flags & MAP_FIXED)
> > +		return addr;
> > +
> > +	if (len < 2 * PAGE_SIZE || len & (len - 1))
> >  		return -EINVAL;
> >
> >  	addr = current->mm->get_unmapped_area(file, addr, 2 * len, pgoff,
> 
> Just sanity checking that for MAP_FIXED case the mm checks that the area is
> unmapped before calling this?

No, straight MAP_FIXED unmaps any existing mappings.  The NOREPLACE variant
fails with -EEXIST if there are existing mappings.

The MAP_FIXED behavior is actually useful, bordering on mandatory, for the
new flow.  It allows the loader to keep its initial mmap(PROT_NONE) of
ELRANGE while (re)mapping the individual enclave sections, e.g. to prevent
a different aspect of the process from mapping the require ELRANGE.

> 
> I don't think we need to check any alignment constraints here anymore.
> 
> The summarize end result would be:
> 
> static unsigned long sgx_get_unmapped_area(struct file *file,
> 					   unsigned long addr,
> 					   unsigned long len,
> 					   unsigned long pgoff,
> 					   unsigned long flags)
> {
> 	if (flags & MAP_PRIVATE)
> 		return -EINVAL;
> 
> 	if (flags & MAP_FIXED)
> 		return addr;
> 
> 	return current->mm->get_unmapped_area(file, addr, 2 * len, pgoff,
> 					      flags);
> }
> 
> /Jarkko
> 

Re: [RFC PATCH v3 04/12] x86/sgx: Require userspace to define enclave pages' protection bits

[Jarkko Sakkinen]

On Mon, 2019-06-17 at 15:24 -0700, Sean Christopherson wrote:
> +	__u32	flags;

This should be changed to secinfo_flags_mask containing a mask of the
allowed bits for the secinfo flags because of two obvious reasons:

1. Protection flags are used mainly with syscalls and contain also other
   things than just the permissions that do not apply in this context.
2. Having a mask for all secinfo flags is more future proof.

With the protection flags you end up reserving bits forever for things
that we will never have any use for (e.g. PROT_SEM).

Looking the change you convert 'flags' (wondering why it isn't called
'prot') to VM flags, which means that you essentially gain absolutely
nothing and loose some potential versatility as a side-effect by doing
that.

/Jarkko

Re: [RFC PATCH v3 05/12] x86/sgx: Enforce noexec filesystem restriction for enclaves

[Jarkko Sakkinen]

On Mon, 2019-06-17 at 15:24 -0700, Sean Christopherson wrote:
> Do not allow an enclave page to be mapped with PROT_EXEC if the source
> vma does not have VM_MAYEXEC.  This effectively enforces noexec as
> do_mmap() clears VM_MAYEXEC if the vma is being loaded from a noexec
> path, i.e. prevents executing a file by loading it into an enclave.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

Andy, I recall you questioning this earlier. What was your argument
and what are your thoughts ATM?

/Jarkko

Re: [RFC PATCH v3 09/12] LSM: x86/sgx: Introduce ->enclave_load() hook for Intel SGX

[Jarkko Sakkinen]

On Mon, 2019-06-17 at 15:24 -0700, Sean Christopherson wrote:
> enclave_load() is roughly analogous to the existing file_mprotect().
> 
> Due to the nature of SGX and its Enclave Page Cache (EPC), all enclave
> VMAs are backed by a single file, i.e. /dev/sgx/enclave, that must be
> MAP_SHARED.  Furthermore, all enclaves need read, write and execute
> VMAs.  As a result, the existing/standard call to file_mprotect() does
> not provide any meaningful security for enclaves since an LSM can only
> deny/grant access to the EPC as a whole.
> 
> security_enclave_load() is called when SGX is first loading an enclave
> page, i.e. copying a page from normal memory into the EPC.  Although
> the prototype for enclave_load() is similar to file_mprotect(), e.g.
> SGX could theoretically use file_mprotect() and set reqprot=prot, a
> separate hook is desirable as the semantics of an enclave's protection
> bits are different than those of vmas, e.g. an enclave page tracks the
> maximal set of protections, whereas file_mprotect() operates on the
> actual protections being provided.  Enclaves also have unique security
> properties, e.g. measured code, that LSMs may want to consider.  In
> other words, LSMs will likely want to implement different policies for
> enclave page protections.
> 
> Note, extensive discussion yielded no sane alternative to some form of
> SGX specific LSM hook[1].
> 
> [1] 
> 
https://lkml.kernel.org/r/CALCETrXf8mSK45h7sTK5Wf+pXLVn=Bjsc_RLpgO-h-qdzBRo5Q@mail.gmail.com
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

Can LSM callbacks ever non-generic when it comes to hardware? This is
the very first time I ever see such callbacks being introduced.

I suspect that from maintainers perspective, accepting such changes for
Intel hardware, could open a pandoras box.

/Jarkko

Re: [RFC PATCH v3 04/12] x86/sgx: Require userspace to define enclave pages' protection bits

[Sean Christopherson]

On Wed, Jun 19, 2019 at 05:43:11PM +0300, Jarkko Sakkinen wrote:
> On Mon, 2019-06-17 at 15:24 -0700, Sean Christopherson wrote:
> > +	__u32	flags;
> 
> This should be changed to secinfo_flags_mask containing a mask of the
> allowed bits for the secinfo flags because of two obvious reasons:
> 
> 1. Protection flags are used mainly with syscalls and contain also other
>    things than just the permissions that do not apply in this context.
> 2. Having a mask for all secinfo flags is more future proof.
> 
> With the protection flags you end up reserving bits forever for things
> that we will never have any use for (e.g. PROT_SEM).
> 
> Looking the change you convert 'flags' (wondering why it isn't called
> 'prot') to VM flags, which means that you essentially gain absolutely
> nothing and loose some potential versatility as a side-effect by doing
> that.

Ah, I see where you're coming from.  My intent was that supported flags
would be SGX specific, not generic PROT_* flags.  I.e. bits 2:0 are used
for PROT_{READ,WRITE,EXEC}, bit 7 can be used for SGX_ZERO_PAGE, etc...

I have two objections to 'secinfo_flags_mask':

  - A full SECINFO mask is problematic for literally every other bit/field
    currently defined in SECINFO.FLAGS, e.g. masking PAGE_TYPE, PENDING
    and MODIFIED adds no value that I can think of, but would require the
    kernel do to weird things like reject page types and EMODPR requests
    (due to their PENDING/MODIFIED interaction).

  - The kernel doesn't actually restrict SECINFO based on the param, it's
    restricting VM_MAY* flags in the vmas.  'secinfo_flags_mask' implies
    the kernel is somehow masking SECINFO.

What about something like this?

/**
 * struct sgx_enclave_add_page - parameter structure for the
 *                               %SGX_IOC_ENCLAVE_ADD_PAGE ioctl
 * @addr:	address within the ELRANGE
 * @src:	address for the page data
 * @secinfo:	address for the SECINFO data
 * @mrmask:	bitmask for the measured 256 byte chunks
 * @prot:	maximal PROT_{READ,WRITE,EXEC} permissions for the page
 */
struct sgx_enclave_add_page {
	__u64		addr;
	__u64		src;
	__u64		secinfo;
	__u16		mrmask;
	__u8		prot;
	__u8		pad;
	__u64[2]	reserved;
};

Re: [RFC PATCH v3 10/12] security/selinux: Add enclave_load() implementation

[Sean Christopherson]

On Tue, Jun 18, 2019 at 10:49:55AM -0400, Stephen Smalley wrote:
> On 6/17/19 6:24 PM, Sean Christopherson wrote:
> >diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> >index 22e0f4a71333..ea452a416fe1 100644
> >--- a/security/selinux/hooks.c
> >+++ b/security/selinux/hooks.c
> >@@ -6727,6 +6727,12 @@ static void selinux_bpf_prog_free(struct bpf_prog_aux *aux)
> >  #endif
> >  #ifdef CONFIG_INTEL_SGX
> >+static inline int sgx_has_perm(u32 sid, u32 requested)
> >+{
> >+	return avc_has_perm(&selinux_state, sid, sid,
> >+			    SECCLASS_PROCESS2, requested, NULL);
> >+}
> >+
> >  static int selinux_enclave_map(unsigned long prot)
> >  {
> >  	const struct cred *cred = current_cred();
> >@@ -6736,11 +6742,53 @@ static int selinux_enclave_map(unsigned long prot)
> >  	WARN_ON_ONCE(!default_noexec);
> >  	if ((prot & PROT_EXEC) && (prot & PROT_WRITE))
> >-		return avc_has_perm(&selinux_state, sid, sid,
> >-				    SECCLASS_PROCESS2, PROCESS2__SGX_EXECMEM,
> >-				    NULL);
> >+		return sgx_has_perm(sid, PROCESS2__SGX_EXECMEM);
> >+
> >  	return 0;
> >  }
> >+
> >+static int selinux_enclave_load(struct vm_area_struct *vma, unsigned long prot,
> >+				bool measured)
> >+{
> >+	const struct cred *cred = current_cred();
> >+	u32 sid = cred_sid(cred);
> >+	int ret;
> >+
> >+	/* SGX is supported only in 64-bit kernels. */
> >+	WARN_ON_ONCE(!default_noexec);
> >+
> >+	/* Only executable enclave pages are restricted in any way. */
> >+	if (!(prot & PROT_EXEC))
> >+		return 0;
> >+
> >+	/*
> >+	 * WX at load time only requires EXECMOD, e.g. to allow W->X.  Actual
> >+	 * WX mappings require EXECMEM (see selinux_enclave_map()).
> >+	 */
> >+	if (prot & PROT_WRITE) {
> >+		ret = sgx_has_perm(sid, PROCESS2__SGX_EXECMOD);
> 
> So, security_enclave_load() can be called with PROT_WRITE|PROT_EXEC, but the
> subsequent calls to security_enclave_map() won't have both set unless a
> mapping is actually simultaneously WX?  Is that correct? Trying to make sure
> that every PROCESS2__SGX_EXECMOD check here won't be followed immediately by
> a PROCESS2__SGX_EXECMEM check from security_enclave_map(), thereby rendering
> any distinction between them moot.

Yes, @prot passed to security_enclave_map() will be the actual protection
bits being set by mmap() or mprotect().

Note, I intentionally omitted @reqprot from .enclave_map() as SGX doesn't
have that information when it calls security_enclave_map() in its mmap()
hook.  Given that checking @reqprot reduces security, I assumed this is a
non-issue.

> >+		if (ret)
> >+			goto out;
> >+	}
> >+	if (!measured) {
> >+		ret = sgx_has_perm(sid, PROCESS2__SGX_EXECUNMR);
> 
> I'm unclear on the concept of loading unmeasured code into an enclave; I
> thought that wasn't supposed to happen apart from SGX2.  How does that
> occur?

Adding a page, i.e. EADD, updates enclave's measurement with the offset and
protection bits of the page, but not the page contents.  The contents of
the page are included in the enclave measurement by executing EEXTEND.
Whether or not a page (technically 256 byte chunks) is measured is purely
controlled by userspace (via the @mrmask param in the add page ioctl()).

Put differently, SGX ISA does not require code pages to be measured, nor
does the SGX driver.  The SGX driver also does *not* zero out unmeasured
pages.  This means an enclave can load unmeasured RX or X pages.

The expected use case is Graphene and the like, which is essentially a two
step load.  The measured enclave code is a shim that wraps an unmodified
non-SGX application, e.g. redis.  Before running the unmeasured code, the
measured portion of the enclave verifies the unmeasured code.

> >+		if (ret)
> >+			goto out;
> >+	}
> >+
> >+	if (!vma->vm_file || !IS_PRIVATE(file_inode(vma->vm_file)) ||
> 
> I think this should be IS_PRIVATE(), not !IS_PRIVATE()?

Argh, thanks!

> >+	    vma->anon_vma)
> >+		/*
> >+		 * Loading enclave code from an anonymous mapping or from a
> >+		 * modified private file mapping.
> >+		 */
> >+		ret = sgx_has_perm(sid, PROCESS2__SGX_EXECANON);
> 
> I might have expected this to be EXECMEM.  It is actually a blend of EXECMEM
> and EXECMOD, so I'm ok with the new name.

Ya, my thought was that we'd want to distinguish between building an
enclave in memory (EXECANON) and the enclave itself modifying its code
(EXECMOD).

> However, I'm wondering if you
> should use an entirely different set of permission names than EXECMEM or
> EXECMOD for the other checks too since none of them quite align with the
> existing usage; your SGX__EXECMEM is effectively MAPWX and your SGX__EXECMOD
> is effectively MAPXAFTERW.  Or something like that.

Hmm, I was thinking that reusing the names would aid in understanding the
SGX specific concepts, but it's definitely a double edge sword.

How about SGX__MAPWX (was EXECMEM) and SGX__EXECDIRTY (was EXECMOD)?

MAPXAFTERW isn't technically accurate for the SGX1 case of loading a WX
page, e.g. the permission is required even if the page is never mapped.

> >+	else
> >+		/* Loading from a shared or unmodified private file mapping. */
> >+		ret = file_has_perm(cred, vma->vm_file, FILE__SGX_EXECUTE);
> >+out:
> >+	return ret;
> >+}
> >  #endif
> >  struct lsm_blob_sizes selinux_blob_sizes __lsm_ro_after_init = {
> >@@ -6988,6 +7036,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
> >  #ifdef CONFIG_INTEL_SGX
> >  	LSM_HOOK_INIT(enclave_map, selinux_enclave_map),
> >+	LSM_HOOK_INIT(enclave_load, selinux_enclave_load),
> >  #endif
> >  };
> >diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
> >index 0f525f5b926f..29a0a74268cd 100644
> >--- a/security/selinux/include/classmap.h
> >+++ b/security/selinux/include/classmap.h
> >@@ -52,7 +52,8 @@ struct security_class_mapping secclass_map[] = {
> >  	    "setsockcreate", "getrlimit", NULL } },
> >  	{ "process2",
> >  	  { "nnp_transition", "nosuid_transition",
> >-	    "sgx_execmem", NULL } },
> >+	    "sgx_execmem", "sgx_execmod", "sgx_execanon", "sgx_execunmr",
> >+	    NULL } },
> >  	{ "system",
> >  	  { "ipc_info", "syslog_read", "syslog_mod",
> >  	    "syslog_console", "module_request", "module_load", NULL } },
> >@@ -64,7 +65,7 @@ struct security_class_mapping secclass_map[] = {
> >  	    "quotaget", NULL } },
> >  	{ "file",
> >  	  { COMMON_FILE_PERMS,
> >-	    "execute_no_trans", "entrypoint", NULL } },
> >+	    "execute_no_trans", "entrypoint", "sgx_execute", NULL } },
> 
> If there is any possibility of a mapping of a non-regular file reaching the
> point of the permission check, then the permission needs to either be added
> to COMMON_FILE_PERMS or be added to each of the *_file classes for which it
> can legitimately be checked.  That's why execmod is in COMMON_FILE_PERMS; it
> can show up on e.g. a modified private file mapping of a device file in
> addition to regular files.

Ah, yes.  Adding it to COMMON_FILE_PERMS seems like the safest bet.

> >  	{ "dir",
> >  	  { COMMON_FILE_PERMS, "add_name", "remove_name",
> >  	    "reparent", "search", "rmdir", NULL } },
> >
> 

Re: [RFC PATCH v3 09/12] LSM: x86/sgx: Introduce ->enclave_load() hook for Intel SGX

[James Morris]

On Wed, 19 Jun 2019, Jarkko Sakkinen wrote:

> Can LSM callbacks ever non-generic when it comes to hardware? This is
> the very first time I ever see such callbacks being introduced.
> 
> I suspect that from maintainers perspective, accepting such changes for
> Intel hardware, could open a pandoras box.

If there's a major distro/userbase committing to ship with these hooks 
enabled via a supported in-tree LSM, the case for inclusion is clear.

If the hooks could be generalized beyond just SGX, that would be ideal, 
but it's not clear if that's feasible.

-- 
James Morris
<jmorris@namei.org>

Re: [RFC PATCH v3 09/12] LSM: x86/sgx: Introduce ->enclave_load() hook for Intel SGX

[Dr. Greg]

On Thu, Jun 20, 2019 at 07:13:50AM +1000, James Morris wrote:

Good morning, I hope the week is going well for everyone.

> On Wed, 19 Jun 2019, Jarkko Sakkinen wrote:
> 
> > Can LSM callbacks ever non-generic when it comes to hardware? This is
> > the very first time I ever see such callbacks being introduced.
> > 
> > I suspect that from maintainers perspective, accepting such changes for
> > Intel hardware, could open a pandoras box.

> If there's a major distro/userbase committing to ship with these
> hooks enabled via a supported in-tree LSM, the case for inclusion is
> clear.

We've been waiting for this concern over SGX specific LSM
functionality to eventuate for the last month or so of this
discussion.

It would seem that mainstream acceptance of SGX specific LSM
modifications is complicated by the fact, as we noted in a previous
e-mail, that a 1400+ machine SAAS implementation we have experience
with will only ever be run with selinux=0.

Hence our concerns and continued comments regarding the need to strike
the proper balance between implementation complexity and the actual
effective security guarantees that are being achieved.

> If the hooks could be generalized beyond just SGX, that would be
> ideal, but it's not clear if that's feasible.

We have been working to develop some thoughts on this issue.

We will forward those thoughts along after I get somewhere different
from where I am right now.

> James Morris

Have a good day.

Dr. Greg

As always,
Dr. Greg Wettstein, Ph.D, Worker
IDfusion, LLC
4206 N. 19th Ave.           Implementing measured information privacy
Fargo, ND  58102            and integrity architectures.
PH: 701-281-1686
FAX: 701-281-3949           EMAIL: gw@idfusion.org
------------------------------------------------------------------------------
"Can't they?

 A 64bit number incremented every millisecond can grow for half a
 billion years.  As far as I'm concerned, that is forever."
                                -- Neil Brown
                                   linux-raid

Re: [RFC PATCH v3 01/12] x86/sgx: Add mm to enclave at mmap()

[Jarkko Sakkinen]

On Wed, Jun 19, 2019 at 04:00:14PM +0300, Jarkko Sakkinen wrote:
> On Wed, 2019-06-19 at 15:56 +0300, Jarkko Sakkinen wrote:
> > On Mon, 2019-06-17 at 15:24 -0700, Sean Christopherson wrote:
> > > The enclave mm tracking is currently broken:
> > > 
> > >   - Adding current->mm during ECREATE is wrong as there is no guarantee
> > >     that the current process has mmap()'d the enclave, i.e. there may
> > >     never be an associated sgx_vma_close() to drop the encl_mm.
> > > 
> > >   - Adding mm's at sgx_vma_open() is wrong as vm_ops->open is called
> > >     only when splitting or duplicating a vma.  If userspace performs a
> > >     single mmap() on the enclave then SGX will fail to track the mm.
> > >     This bug is partially hidden by tracking current->mm at ECREATE.
> > > 
> > > Rework the tracking to get/add the mm at mmap().  A side effect of the
> > > bug fix is that sgx_vma_{open,close}() should never encounter a vma with
> > > an associated enclave and no associated encl_mm, i.e. WARN if an encl_mm
> > > cannot be found in either condition.
> > > 
> > > Change the WARN() on a non-empty mm_list to a WARN_ONCE().  The warning
> > > will fire over and over (and over) if the mm tracking is broken, which
> > > hampers debug/triage far more than it helps.
> > > 
> > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > > ---
> > >  arch/x86/kernel/cpu/sgx/driver/ioctl.c | 14 ----------
> > >  arch/x86/kernel/cpu/sgx/driver/main.c  | 26 ++++++++++++++++++
> > >  arch/x86/kernel/cpu/sgx/encl.c         | 38 +++++---------------------
> > >  arch/x86/kernel/cpu/sgx/encl.h         |  4 +--
> > >  4 files changed, 35 insertions(+), 47 deletions(-)
> > 
> > BTW, sgx_encl_mm is a bit confusing name (which I made) to begin with. 
> > It clues as it was a some kind of 1:1 association with the process,
> > which it isn't.
> > 
> > Could you update the patch and rename it as sgx_encl_mapping? After that
> > I'm happy to merge.
> 
> And send it as a separate patch. Can merge it today/tomorrow after I've
> finished my reaper change.

Still working/testing reaper stuff but this one is now applied. Lets
not do any renames for the moment because they only slow down. That
was a bad suggestion from my part.

The reason I squashed this change is that it is a pure bug fix.

If there is a bug, lets try to just fix that with absolutely minimal
intrusion. Only after that consider a "better way".

/Jarkko

Re: [RFC PATCH v3 02/12] x86/sgx: Do not naturally align MAP_FIXED address

[Jarkko Sakkinen]

On Wed, Jun 19, 2019 at 07:08:53AM -0700, Sean Christopherson wrote:
> On Wed, Jun 19, 2019 at 04:24:05PM +0300, Jarkko Sakkinen wrote:
> > On Mon, 2019-06-17 at 15:24 -0700, Sean Christopherson wrote:
> > >  {
> > > -	if (len < 2 * PAGE_SIZE || len & (len - 1) || flags & MAP_PRIVATE)
> > > +	if (flags & MAP_PRIVATE)
> > > +		return -EINVAL;
> > > +
> > > +	if (flags & MAP_FIXED)
> > > +		return addr;
> > > +
> > > +	if (len < 2 * PAGE_SIZE || len & (len - 1))
> > >  		return -EINVAL;
> > >
> > >  	addr = current->mm->get_unmapped_area(file, addr, 2 * len, pgoff,
> > 
> > Just sanity checking that for MAP_FIXED case the mm checks that the area is
> > unmapped before calling this?
> 
> No, straight MAP_FIXED unmaps any existing mappings.  The NOREPLACE variant
> fails with -EEXIST if there are existing mappings.

Ah so it was [1]!

> The MAP_FIXED behavior is actually useful, bordering on mandatory, for the
> new flow.  It allows the loader to keep its initial mmap(PROT_NONE) of
> ELRANGE while (re)mapping the individual enclave sections, e.g. to prevent
> a different aspect of the process from mapping the require ELRANGE.

Yeah, totally agree.

[1] http://man7.org/linux/man-pages/man2/mmap.2.html

/Jarkko

Re: [RFC PATCH v3 04/12] x86/sgx: Require userspace to define enclave pages' protection bits

[Jarkko Sakkinen]

On Wed, Jun 19, 2019 at 08:20:18AM -0700, Sean Christopherson wrote:
> On Wed, Jun 19, 2019 at 05:43:11PM +0300, Jarkko Sakkinen wrote:
> > On Mon, 2019-06-17 at 15:24 -0700, Sean Christopherson wrote:
> > > +	__u32	flags;
> > 
> > This should be changed to secinfo_flags_mask containing a mask of the
> > allowed bits for the secinfo flags because of two obvious reasons:
> > 
> > 1. Protection flags are used mainly with syscalls and contain also other
> >    things than just the permissions that do not apply in this context.
> > 2. Having a mask for all secinfo flags is more future proof.
> > 
> > With the protection flags you end up reserving bits forever for things
> > that we will never have any use for (e.g. PROT_SEM).
> > 
> > Looking the change you convert 'flags' (wondering why it isn't called
> > 'prot') to VM flags, which means that you essentially gain absolutely
> > nothing and loose some potential versatility as a side-effect by doing
> > that.
> 
> Ah, I see where you're coming from.  My intent was that supported flags
> would be SGX specific, not generic PROT_* flags.  I.e. bits 2:0 are used
> for PROT_{READ,WRITE,EXEC}, bit 7 can be used for SGX_ZERO_PAGE, etc...
> 
> I have two objections to 'secinfo_flags_mask':
> 
>   - A full SECINFO mask is problematic for literally every other bit/field
>     currently defined in SECINFO.FLAGS, e.g. masking PAGE_TYPE, PENDING
>     and MODIFIED adds no value that I can think of, but would require the
>     kernel do to weird things like reject page types and EMODPR requests
>     (due to their PENDING/MODIFIED interaction).

You're probably right that in practice it would hard to do much with
EMODT.

>   - The kernel doesn't actually restrict SECINFO based on the param, it's
>     restricting VM_MAY* flags in the vmas.  'secinfo_flags_mask' implies
>     the kernel is somehow masking SECINFO.
>
> What about something like this?
> 
> /**
>  * struct sgx_enclave_add_page - parameter structure for the
>  *                               %SGX_IOC_ENCLAVE_ADD_PAGE ioctl
>  * @addr:	address within the ELRANGE
>  * @src:	address for the page data
>  * @secinfo:	address for the SECINFO data
>  * @mrmask:	bitmask for the measured 256 byte chunks
>  * @prot:	maximal PROT_{READ,WRITE,EXEC} permissions for the page
>  */
> struct sgx_enclave_add_page {
> 	__u64		addr;
> 	__u64		src;
> 	__u64		secinfo;
> 	__u16		mrmask;
> 	__u8		prot;
> 	__u8		pad;
> 	__u64[2]	reserved;
> };

LGTM but why it isn't like:

__u16 mrmask;
__u8 prot;
__u8 reserved[5];

/Jarkko

Re: [RFC PATCH v3 09/12] LSM: x86/sgx: Introduce ->enclave_load() hook for Intel SGX

[Jarkko Sakkinen]

On Thu, Jun 20, 2019 at 07:13:50AM +1000, James Morris wrote:
> On Wed, 19 Jun 2019, Jarkko Sakkinen wrote:
> 
> > Can LSM callbacks ever non-generic when it comes to hardware? This is
> > the very first time I ever see such callbacks being introduced.
> > 
> > I suspect that from maintainers perspective, accepting such changes for
> > Intel hardware, could open a pandoras box.
> 
> If there's a major distro/userbase committing to ship with these hooks 
> enabled via a supported in-tree LSM, the case for inclusion is clear.

I think there is.

> If the hooks could be generalized beyond just SGX, that would be ideal, 
> but it's not clear if that's feasible.

OK, thanks for responding. This was really important to know what to
focus on (and what not).

/Jarkko

Re: [RFC PATCH v3 09/12] LSM: x86/sgx: Introduce ->enclave_load() hook for Intel SGX

[Dr. Greg]

On Thu, Jun 20, 2019 at 07:13:50AM +1000, James Morris wrote:

Good morning, I hope the weekend has been going well for everyone.

> On Wed, 19 Jun 2019, Jarkko Sakkinen wrote:
> 
> > Can LSM callbacks ever non-generic when it comes to hardware? This is
> > the very first time I ever see such callbacks being introduced.
> > 
> > I suspect that from maintainers perspective, accepting such changes for
> > Intel hardware, could open a pandoras box.

> If there's a major distro/userbase committing to ship with these
> hooks enabled via a supported in-tree LSM, the case for inclusion is
> clear.

I see that Jarkko responded down thread that there may be a major
distribution already committed to SGX specific LSM hooks.  My
apologies for providing these reflections if that is the case and
there is some type of 'deal' in place with respect to all of this.

> If the hooks could be generalized beyond just SGX, that would be
> ideal, but it's not clear if that's feasible.

We believe there is some degree of commonality that can be addressed
with respect to implementing LSM enforcement over SGX enclaves.

However, big picture, here is the challenge that we see with respect
to these conversations surrounding the integration of the SGX driver
with the LSM:

As a technology, SGX was designed to enable software to protect itself
from an adversarial operating system and hardware platform.  Given
that, if we are intellectually honest, how effective can the LSM/OS be
with respect to controlling the actions of an enclave?

Without question, being able to regulate and control which identities
can intersect to load executable content into an enclave is important.
All of the infrastructure appears to be already there to accomplish
that, given the default model of a shared library implementation of an
enclave and requiring the loader to mmap file backed TEXT pages RX.

The most relevant and important control with respect to whether or not
an enclave should be allowed to execute is evaluation of the
SIGSTRUCT.  Given the trajectory that platform security is on, SGX is
not going to be the last technology of its type nor the only
technology that makes use of cryptographically based code provenance.

As a result, if we are content with handing an opaque pointer of a
descriptive struture to an LSM routine, a generic hook that is tasked
with verifying code or execution environment provenance doesn't seem
like it would need to be technology specific nor controversial.

That leaves as the last thorny issue the question of dynamic
allocation of memory for executable content.  As we have stated
before, and at the outset of this note, from a security perspective
this is only, effectively, a binary question for the platform owner as
to whether or not the concept should be allowed.

A generic LSM hook, appropriately named, could execute that decision
without being SGX specific.  Arguably, the hook should be named to
indicate that it is seeking approval for allocating memory to be used
for anonymous executable content, since that is what it would be
effectively requesting approval for, in the case of SGX.

For completeness a third generic hook may be useful.  The purpose of
that hook would be to verify a block of memory as being
measured or signed for consideration as executable content.  Arguably
that will have utility far beyond SGX.

In the case of SGX it would address the issue as to whether or not a
block of executable content in untrusted space is eligible for
anonymous execution.  That may be a useful security measure in order
to provide some control over an enclave being used as a random
execution oracle.

It obviously has no security utility against the enclave author since,
as we have noted before, it is possible for the enclave author to
simply pull whatever code is desired over an encrypted network
connection.

> James Morris

Hopefully these comments are a useful basis for further discussion.

Best wishes for a productive week to everyone.

Dr. Greg

As always,
Dr. Greg Wettstein, Ph.D, Worker
IDfusion, LLC
4206 N. 19th Ave.           Implementing measured information privacy
Fargo, ND  58102            and integrity architectures.
PH: 701-281-1686
FAX: 701-281-3949           EMAIL: gw@idfusion.org
------------------------------------------------------------------------------
"My thoughts on trusting Open-Source?  A quote I once saw said it
 best: 'Remember, Amateurs built the ark.  Professionals built the
 Titanic.'  Perhaps most significantly the ark was one guy, there were
 no doubt committees involved with the Titanic project."
                                -- Dr. G.W. Wettstein
                                   Resurrection

Re: [RFC PATCH v3 00/12] security: x86/sgx: SGX vs. LSM, round 3

[Jarkko Sakkinen]

On Mon, Jun 17, 2019 at 03:24:26PM -0700, Sean Christopherson wrote:

Since you are gone up until end of next week, I'm going to do some
proactive work and for those patches that I only had some feedback
concerning how the code is written etc. I'll just fix those myself and
squash. You can check when you're back and send me follow ups if you
disagree on something I made.

I'll also try to extract *only* the SRCU part from [1] and squash that
to my tree. The main reason is that I want to do my RFC for enclave
life-cycle handling on the same codebase so that we can compare and
make the best possible decision on that.

[1] https://patchwork.kernel.org/patch/11005495/

/Jarkko

Re: [RFC PATCH v3 09/12] LSM: x86/sgx: Introduce ->enclave_load() hook for Intel SGX

[James Morris]

On Sun, 23 Jun 2019, Dr. Greg wrote:

> The most relevant and important control with respect to whether or not
> an enclave should be allowed to execute is evaluation of the
> SIGSTRUCT.  Given the trajectory that platform security is on, SGX is
> not going to be the last technology of its type nor the only
> technology that makes use of cryptographically based code provenance.
> 
> As a result, if we are content with handing an opaque pointer of a
> descriptive struture to an LSM routine, a generic hook that is tasked
> with verifying code or execution environment provenance doesn't seem
> like it would need to be technology specific nor controversial.
> 
> That leaves as the last thorny issue the question of dynamic
> allocation of memory for executable content.  As we have stated
> before, and at the outset of this note, from a security perspective
> this is only, effectively, a binary question for the platform owner as
> to whether or not the concept should be allowed.
> 
> A generic LSM hook, appropriately named, could execute that decision
> without being SGX specific.  Arguably, the hook should be named to
> indicate that it is seeking approval for allocating memory to be used
> for anonymous executable content, since that is what it would be
> effectively requesting approval for, in the case of SGX.
> 
> For completeness a third generic hook may be useful.  The purpose of
> that hook would be to verify a block of memory as being
> measured or signed for consideration as executable content.  Arguably
> that will have utility far beyond SGX.
> 
> In the case of SGX it would address the issue as to whether or not a
> block of executable content in untrusted space is eligible for
> anonymous execution.  That may be a useful security measure in order
> to provide some control over an enclave being used as a random
> execution oracle.
> 
> It obviously has no security utility against the enclave author since,
> as we have noted before, it is possible for the enclave author to
> simply pull whatever code is desired over an encrypted network
> connection.
> 
> > James Morris
> 
> Hopefully these comments are a useful basis for further discussion.

Thanks, this is helpful.

-- 
James Morris
<jmorris@namei.org>

Re: [RFC PATCH v3 04/12] x86/sgx: Require userspace to define enclave pages' protection bits

[Sean Christopherson]

On Fri, Jun 21, 2019 at 01:17:02AM +0300, Jarkko Sakkinen wrote:
> On Wed, Jun 19, 2019 at 08:20:18AM -0700, Sean Christopherson wrote:
> > On Wed, Jun 19, 2019 at 05:43:11PM +0300, Jarkko Sakkinen wrote:
> > > On Mon, 2019-06-17 at 15:24 -0700, Sean Christopherson wrote:
> > > > +	__u32	flags;
> > > 
> > > This should be changed to secinfo_flags_mask containing a mask of the
> > > allowed bits for the secinfo flags because of two obvious reasons:
> > > 
> > > 1. Protection flags are used mainly with syscalls and contain also other
> > >    things than just the permissions that do not apply in this context.
> > > 2. Having a mask for all secinfo flags is more future proof.
> > > 
> > > With the protection flags you end up reserving bits forever for things
> > > that we will never have any use for (e.g. PROT_SEM).
> > > 
> > > Looking the change you convert 'flags' (wondering why it isn't called
> > > 'prot') to VM flags, which means that you essentially gain absolutely
> > > nothing and loose some potential versatility as a side-effect by doing
> > > that.
> > 
> > Ah, I see where you're coming from.  My intent was that supported flags
> > would be SGX specific, not generic PROT_* flags.  I.e. bits 2:0 are used
> > for PROT_{READ,WRITE,EXEC}, bit 7 can be used for SGX_ZERO_PAGE, etc...
> > 
> > I have two objections to 'secinfo_flags_mask':
> > 
> >   - A full SECINFO mask is problematic for literally every other bit/field
> >     currently defined in SECINFO.FLAGS, e.g. masking PAGE_TYPE, PENDING
> >     and MODIFIED adds no value that I can think of, but would require the
> >     kernel do to weird things like reject page types and EMODPR requests
> >     (due to their PENDING/MODIFIED interaction).
> 
> You're probably right that in practice it would hard to do much with
> EMODT.
> 
> >   - The kernel doesn't actually restrict SECINFO based on the param, it's
> >     restricting VM_MAY* flags in the vmas.  'secinfo_flags_mask' implies
> >     the kernel is somehow masking SECINFO.
> >
> > What about something like this?
> > 
> > /**
> >  * struct sgx_enclave_add_page - parameter structure for the
> >  *                               %SGX_IOC_ENCLAVE_ADD_PAGE ioctl
> >  * @addr:	address within the ELRANGE
> >  * @src:	address for the page data
> >  * @secinfo:	address for the SECINFO data
> >  * @mrmask:	bitmask for the measured 256 byte chunks
> >  * @prot:	maximal PROT_{READ,WRITE,EXEC} permissions for the page
> >  */
> > struct sgx_enclave_add_page {
> > 	__u64		addr;
> > 	__u64		src;
> > 	__u64		secinfo;
> > 	__u16		mrmask;
> > 	__u8		prot;
> > 	__u8		pad;
> > 	__u64[2]	reserved;
> > };
> 
> LGTM but why it isn't like:
> 
> __u16 mrmask;
> __u8 prot;
> __u8 reserved[5];

Because math is hard :-)  Though I think we'd want

  __u16 mrmask
  __u8  prot
  __u8  pad[5];

with an optional

  __u64 reserved[?];

"pad" can be ignored, e.g. doesn't need to be explicitly zeroed by
userspace, whereas "reserved" requires explicit zeroing and probably an
associated kernel check.

Re: [RFC PATCH v3 04/12] x86/sgx: Require userspace to define enclave pages' protection bits

[Jarkko Sakkinen]

On Sun, 2019-07-07 at 12:08 -0700, Sean Christopherson wrote:
> LGTM but why it isn't like:
> > 
> > __u16 mrmask;
> > __u8 prot;
> > __u8 reserved[5];
> 
> Because math is hard :-)  Though I think we'd want
> 
>   __u16 mrmask
>   __u8  prot
>   __u8  pad[5];
> 
> with an optional
> 
>   __u64 reserved[?];
> 
> "pad" can be ignored, e.g. doesn't need to be explicitly zeroed by
> userspace, whereas "reserved" requires explicit zeroing and probably an
> associated kernel check.

OK, cool with the change itself. Still need to get a better idea
how these make sense in architectural sense.

Things that would help with overall picture:

1. We have to figure out how this can be useful when LSM's are not used.
That gives at least some evidence that the security model is overally
good if it makes sense with and without LSM. Right now this looks like
dead functionality if not coupled with an LSM.
2. Probably some "user story" type of examples would help with the
discussion overall [1] i.e. how one would use this for
her own good.

[1] Probably many of the folks who work x86 tree have ignored major
    part of the discussion. Somehow these should be brought to
    nutshell so that anyone can get whatever the model is. Anyone
    should get it basically.

/Jarkko

Re: [RFC PATCH v3 04/12] x86/sgx: Require userspace to define enclave pages' protection bits

[Sean Christopherson]

On Mon, Jul 08, 2019 at 06:23:46PM +0300, Jarkko Sakkinen wrote:
> On Sun, 2019-07-07 at 12:08 -0700, Sean Christopherson wrote:
> > LGTM but why it isn't like:
> > > 
> > > __u16 mrmask;
> > > __u8 prot;
> > > __u8 reserved[5];
> > 
> > Because math is hard :-)  Though I think we'd want
> > 
> >   __u16 mrmask
> >   __u8  prot
> >   __u8  pad[5];
> > 
> > with an optional
> > 
> >   __u64 reserved[?];
> > 
> > "pad" can be ignored, e.g. doesn't need to be explicitly zeroed by
> > userspace, whereas "reserved" requires explicit zeroing and probably an
> > associated kernel check.
> 
> OK, cool with the change itself. Still need to get a better idea
> how these make sense in architectural sense.
> 
> Things that would help with overall picture:
> 
> 1. We have to figure out how this can be useful when LSM's are not used.

Declaring PROT_EXEC is useful to enforce noexec filesystems.  Beyond that,
I am not aware of any meaningful use case.

> That gives at least some evidence that the security model is overally
> good if it makes sense with and without LSM. Right now this looks like
> dead functionality if not coupled with an LSM.

I agree that it's effectively dead functionality without LSMs, but keep
in mind that this LSM rat hole was opened specifically because SGX could
be used to circumvent existing LSM security policies.  In other words,
the purpose of the UAPI extension is to achieve LSM compatibility without
incurring significant complexity in the LSM subsystem.

> 2. Probably some "user story" type of examples would help with the
> discussion overall [1] i.e. how one would use this for
> her own good.

The compelling story is Andy's original concern that userspace could
circumvent existing security policies by running code in an enclave.

AIUI, closing the LSM loophole is the minimal requirement to get SGX
upstreamed.  The extensive discussion has largely been focused on
ensuring that whatever mechanism is used to close the loophole will
play nice with future SGX functionality and/or LSM security policies.

> [1] Probably many of the folks who work x86 tree have ignored major
>     part of the discussion. Somehow these should be brought to
>     nutshell so that anyone can get whatever the model is. Anyone
>     should get it basically.
> 
> /Jarkko
> 

Re: [RFC PATCH v3 04/12] x86/sgx: Require userspace to define enclave pages' protection bits

[Jarkko Sakkinen]

On Mon, Jul 08, 2019 at 09:19:32AM -0700, Sean Christopherson wrote:
> > 2. Probably some "user story" type of examples would help with the
> > discussion overall [1] i.e. how one would use this for
> > her own good.
> 
> The compelling story is Andy's original concern that userspace could
> circumvent existing security policies by running code in an enclave.
> 
> AIUI, closing the LSM loophole is the minimal requirement to get SGX
> upstreamed.  The extensive discussion has largely been focused on
> ensuring that whatever mechanism is used to close the loophole will
> play nice with future SGX functionality and/or LSM security policies.

OK, might be getting here where I fall out of the wagon so:

Doesn't Andy's example anyway require a process that has privileges to
make pages executable i.e. it could run arbitrary code even without an
enclave?

/Jarkko

Re: [RFC PATCH v3 04/12] x86/sgx: Require userspace to define enclave pages' protection bits

[Sean Christopherson]

On Tue, Jul 09, 2019 at 07:06:34PM +0300, Jarkko Sakkinen wrote:
> On Mon, Jul 08, 2019 at 09:19:32AM -0700, Sean Christopherson wrote:
> > > 2. Probably some "user story" type of examples would help with the
> > > discussion overall [1] i.e. how one would use this for
> > > her own good.
> > 
> > The compelling story is Andy's original concern that userspace could
> > circumvent existing security policies by running code in an enclave.
> > 
> > AIUI, closing the LSM loophole is the minimal requirement to get SGX
> > upstreamed.  The extensive discussion has largely been focused on
> > ensuring that whatever mechanism is used to close the loophole will
> > play nice with future SGX functionality and/or LSM security policies.
> 
> OK, might be getting here where I fall out of the wagon so:
> 
> Doesn't Andy's example anyway require a process that has privileges to
> make pages executable i.e. it could run arbitrary code even without an
> enclave?

Ah, no.  He did raise that concern, but it'd only be an issue if the
enclave fd were backed by an anon inode, in which case all enclaves would
need EXECMEM in order to gain PROT_EXEC on EPC.  Because the fd is backed
/dev/sgx/enclave, userspace just needs FILE__EXECUTE on /dev/sgx/enclave.

Re: [RFC PATCH v3 04/12] x86/sgx: Require userspace to define enclave pages' protection bits

[Andy Lutomirski]

On Wed, Jul 10, 2019 at 10:25 AM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> On Tue, Jul 09, 2019 at 07:06:34PM +0300, Jarkko Sakkinen wrote:
> > On Mon, Jul 08, 2019 at 09:19:32AM -0700, Sean Christopherson wrote:
> > > > 2. Probably some "user story" type of examples would help with the
> > > > discussion overall [1] i.e. how one would use this for
> > > > her own good.
> > >
> > > The compelling story is Andy's original concern that userspace could
> > > circumvent existing security policies by running code in an enclave.
> > >
> > > AIUI, closing the LSM loophole is the minimal requirement to get SGX
> > > upstreamed.  The extensive discussion has largely been focused on
> > > ensuring that whatever mechanism is used to close the loophole will
> > > play nice with future SGX functionality and/or LSM security policies.
> >
> > OK, might be getting here where I fall out of the wagon so:
> >
> > Doesn't Andy's example anyway require a process that has privileges to
> > make pages executable i.e. it could run arbitrary code even without an
> > enclave?
>
> Ah, no.  He did raise that concern, but it'd only be an issue if the
> enclave fd were backed by an anon inode, in which case all enclaves would
> need EXECMEM in order to gain PROT_EXEC on EPC.  Because the fd is backed
> /dev/sgx/enclave, userspace just needs FILE__EXECUTE on /dev/sgx/enclave.

I would say it differently: regardless of exactly how /dev/sgx/enclave
is wired up under the hood, we want a way that a process can be
granted permission to usefully run enclaves without being granted
permission to execute whatever bytes of code it wants.  Preferably
without requiring LSMs to maintain some form of enclave signature
whitelist.

This is pretty much the only hard requirement I see.  We really could
achieve this, in a somewhat limited form, by saying that LSMs can
approve or reject the SIGSTRUCT.  But doing it that way is a bit nasty
as we've noticed, for a few reasons.  Several of you have raised
objections to requiring SIGSTRUCT to come from a .sigstruct file.  We
also need to worry about a SIGSTRUCT that refers to an enclave that
forgot to measure its text.  And we need to worry about SGX2.

So this whole messy exercise boils down to: a bunch of security policy
authors think that EXECMEM and similar are not to be given out
lightly.  How to we allow policies like that to be compatible with
SGX?

Re: [RFC PATCH v3 04/12] x86/sgx: Require userspace to define enclave pages' protection bits

[Jarkko Sakkinen]

On Mon, Jul 15, 2019 at 03:29:23PM -0700, Andy Lutomirski wrote:
> I would say it differently: regardless of exactly how /dev/sgx/enclave
> is wired up under the hood, we want a way that a process can be
> granted permission to usefully run enclaves without being granted
> permission to execute whatever bytes of code it wants.  Preferably
> without requiring LSMs to maintain some form of enclave signature
> whitelist.

Would it be better to have a signer whitelist instead or some
combination? E.g. you could whiteliste either by signer or
enclave signature.

/Jarkko

Re: [RFC PATCH v3 04/12] x86/sgx: Require userspace to define enclave pages' protection bits

[Andy Lutomirski]

On Thu, Aug 1, 2019 at 9:38 AM Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
>
> On Mon, Jul 15, 2019 at 03:29:23PM -0700, Andy Lutomirski wrote:
> > I would say it differently: regardless of exactly how /dev/sgx/enclave
> > is wired up under the hood, we want a way that a process can be
> > granted permission to usefully run enclaves without being granted
> > permission to execute whatever bytes of code it wants.  Preferably
> > without requiring LSMs to maintain some form of enclave signature
> > whitelist.
>
> Would it be better to have a signer whitelist instead or some
> combination? E.g. you could whiteliste either by signer or
> enclave signature.
>

I'm not sure, and also don't really think we need to commit to an
answer right now.  I do think that the eventual solution should be
more flexible than just whitelisting the signers.  In particular, it
should be possible to make secure enclaves, open-source or otherwise,
that are reproducibly buildable.  This more or less requires that the
signing private key not be a secret, which means that no one would
want to whitelist the signing key.  The enclave would be trusted, and
would seal data, on the basis of its MRENCLAVE, and the policy, if
any, would want to whitelist the MRENCLAVE or perhaps the whole
SIGSTRUCT.

But my overall point is that it should be possible to have a conherent
policy that allows any enclave whatsoever to run but that still
respects EXECMEM and such.

Re: [RFC PATCH v3 04/12] x86/sgx: Require userspace to define enclave pages' protection bits

[Jarkko Sakkinen]

On Sun, Aug 04, 2019 at 03:20:24PM -0700, Andy Lutomirski wrote:
> On Thu, Aug 1, 2019 at 9:38 AM Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> >
> > On Mon, Jul 15, 2019 at 03:29:23PM -0700, Andy Lutomirski wrote:
> > > I would say it differently: regardless of exactly how /dev/sgx/enclave
> > > is wired up under the hood, we want a way that a process can be
> > > granted permission to usefully run enclaves without being granted
> > > permission to execute whatever bytes of code it wants.  Preferably
> > > without requiring LSMs to maintain some form of enclave signature
> > > whitelist.
> >
> > Would it be better to have a signer whitelist instead or some
> > combination? E.g. you could whiteliste either by signer or
> > enclave signature.
> >
> 
> I'm not sure, and also don't really think we need to commit to an
> answer right now.  I do think that the eventual solution should be
> more flexible than just whitelisting the signers.  In particular, it
> should be possible to make secure enclaves, open-source or otherwise,
> that are reproducibly buildable.  This more or less requires that the
> signing private key not be a secret, which means that no one would
> want to whitelist the signing key.  The enclave would be trusted, and
> would seal data, on the basis of its MRENCLAVE, and the policy, if
> any, would want to whitelist the MRENCLAVE or perhaps the whole
> SIGSTRUCT.
> 
> But my overall point is that it should be possible to have a conherent
> policy that allows any enclave whatsoever to run but that still
> respects EXECMEM and such.

So could kernel embed a fixed signing key that would be made available
through sysfs for signing? Already have one for my selftest.

/Jarkko

Re: [RFC PATCH v3 04/12] x86/sgx: Require userspace to define enclave pages' protection bits

[Andy Lutomirski]

On Mon, Aug 5, 2019 at 1:51 PM Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
>
> On Sun, Aug 04, 2019 at 03:20:24PM -0700, Andy Lutomirski wrote:
> > On Thu, Aug 1, 2019 at 9:38 AM Jarkko Sakkinen
> > <jarkko.sakkinen@linux.intel.com> wrote:
> > >
> > > On Mon, Jul 15, 2019 at 03:29:23PM -0700, Andy Lutomirski wrote:
> > > > I would say it differently: regardless of exactly how /dev/sgx/enclave
> > > > is wired up under the hood, we want a way that a process can be
> > > > granted permission to usefully run enclaves without being granted
> > > > permission to execute whatever bytes of code it wants.  Preferably
> > > > without requiring LSMs to maintain some form of enclave signature
> > > > whitelist.
> > >
> > > Would it be better to have a signer whitelist instead or some
> > > combination? E.g. you could whiteliste either by signer or
> > > enclave signature.
> > >
> >
> > I'm not sure, and also don't really think we need to commit to an
> > answer right now.  I do think that the eventual solution should be
> > more flexible than just whitelisting the signers.  In particular, it
> > should be possible to make secure enclaves, open-source or otherwise,
> > that are reproducibly buildable.  This more or less requires that the
> > signing private key not be a secret, which means that no one would
> > want to whitelist the signing key.  The enclave would be trusted, and
> > would seal data, on the basis of its MRENCLAVE, and the policy, if
> > any, would want to whitelist the MRENCLAVE or perhaps the whole
> > SIGSTRUCT.
> >
> > But my overall point is that it should be possible to have a conherent
> > policy that allows any enclave whatsoever to run but that still
> > respects EXECMEM and such.
>
> So could kernel embed a fixed signing key that would be made available
> through sysfs for signing? Already have one for my selftest.
>

Do you mean a public and private key?  I was imagining that someone
would just create a key pair and publish it for the case of SGX
programs that don't depend on MRSIGNER.  There doesn't have to be just
one.

But I may be misunderstanding you.

Re: [RFC PATCH v3 04/12] x86/sgx: Require userspace to define enclave pages' protection bits

[Jarkko Sakkinen]

On Mon, Aug 05, 2019 at 02:30:22PM -0700, Andy Lutomirski wrote:
> On Mon, Aug 5, 2019 at 1:51 PM Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> >
> > On Sun, Aug 04, 2019 at 03:20:24PM -0700, Andy Lutomirski wrote:
> > > On Thu, Aug 1, 2019 at 9:38 AM Jarkko Sakkinen
> > > <jarkko.sakkinen@linux.intel.com> wrote:
> > > >
> > > > On Mon, Jul 15, 2019 at 03:29:23PM -0700, Andy Lutomirski wrote:
> > > > > I would say it differently: regardless of exactly how /dev/sgx/enclave
> > > > > is wired up under the hood, we want a way that a process can be
> > > > > granted permission to usefully run enclaves without being granted
> > > > > permission to execute whatever bytes of code it wants.  Preferably
> > > > > without requiring LSMs to maintain some form of enclave signature
> > > > > whitelist.
> > > >
> > > > Would it be better to have a signer whitelist instead or some
> > > > combination? E.g. you could whiteliste either by signer or
> > > > enclave signature.
> > > >
> > >
> > > I'm not sure, and also don't really think we need to commit to an
> > > answer right now.  I do think that the eventual solution should be
> > > more flexible than just whitelisting the signers.  In particular, it
> > > should be possible to make secure enclaves, open-source or otherwise,
> > > that are reproducibly buildable.  This more or less requires that the
> > > signing private key not be a secret, which means that no one would
> > > want to whitelist the signing key.  The enclave would be trusted, and
> > > would seal data, on the basis of its MRENCLAVE, and the policy, if
> > > any, would want to whitelist the MRENCLAVE or perhaps the whole
> > > SIGSTRUCT.
> > >
> > > But my overall point is that it should be possible to have a conherent
> > > policy that allows any enclave whatsoever to run but that still
> > > respects EXECMEM and such.
> >
> > So could kernel embed a fixed signing key that would be made available
> > through sysfs for signing? Already have one for my selftest.
> >
> 
> Do you mean a public and private key?  I was imagining that someone
> would just create a key pair and publish it for the case of SGX
> programs that don't depend on MRSIGNER.  There doesn't have to be just
> one.
> 
> But I may be misunderstanding you.

Aa, OK, got you. I actually misunderstood you.

/Jarkko