[RFC PATCH 0/9] security: x86/sgx: SGX vs. LSM

[RFC PATCH 0/9] security: x86/sgx: SGX vs. LSM

[Sean Christopherson]

This series is the result of a rather absurd amount of discussion over
how to get SGX to play nice with LSM policies, without having to resort
to evil shenanigans or put undue burden on userspace.  The discussion
definitely wandered into completely insane territory at times, but I
think/hope we ended up with something reasonable.

The basic gist of the approach is to require userspace to declare what
protections are maximally allowed for any given page, e.g. add a flags
field for loading enclave pages that takes ALLOW_{READ,WRITE,EXEC}.  LSMs
can then adjust the allowed protections, e.g. clear ALLOW_EXEC to prevent
ever mapping the page with PROT_EXEC.  SGX enforces the allowed perms
via a new mprotect() vm_ops hook, e.g. like regular mprotect() uses
MAY_{READ,WRITE,EXEC}.

ALLOW_EXEC is used to deny hings like loading an enclave from a noexec
file system or from a file without EXECUTE permissions, e.g. without
the ALLOW_EXEC concept, on SGX2 hardware (regardless of kernel support)
userspace could EADD from a noexec file using read-only permissions,
and later use mprotect() and ENCLU[EMODPE] to gain execute permissions.

ALLOW_WRITE is used in conjuction with ALLOW_EXEC to enforce SELinux's
EXECMOD (or EXECMEM).

This is very much an RFC series.  It's only compile tested, likely has
obvious bugs, the SELinux patch could be completely harebrained, etc...
My goal at this point is to get feedback at a macro level, e.g. is the
core concept viable/acceptable, are there objection to hooking
mprotect(), etc...

Andy and Cedric, hopefully this aligns with your general expectations
based on our last discussion.

Lastly, I added a patch to allow userspace to add multiple pages in a
single ioctl().  It's obviously not directly related to the security
stuff, but the idea tangentially came up during earlier discussions and
it's something I think the UAPI should provide (it's a tiny change).
Since I was modifying the UAPI anyways, I threw it in.

Sean Christopherson (9):
  x86/sgx: Remove unused local variable in sgx_encl_release()
  x86/sgx: Do not naturally align MAP_FIXED address
  x86/sgx: Allow userspace to add multiple pages in single ioctl()
  mm: Introduce vm_ops->mprotect()
  x86/sgx: Restrict mapping without an enclave page to PROT_NONE
  x86/sgx: Require userspace to provide allowed prots to ADD_PAGES
  x86/sgx: Enforce noexec filesystem restriction for enclaves
  LSM: x86/sgx: Introduce ->enclave_load() hook for Intel SGX
  security/selinux: Add enclave_load() implementation

 arch/x86/include/uapi/asm/sgx.h        |  30 ++++--
 arch/x86/kernel/cpu/sgx/driver/ioctl.c | 143 +++++++++++++++++--------
 arch/x86/kernel/cpu/sgx/driver/main.c  |  13 ++-
 arch/x86/kernel/cpu/sgx/encl.c         |  31 +++++-
 arch/x86/kernel/cpu/sgx/encl.h         |   4 +
 include/linux/lsm_hooks.h              |  16 +++
 include/linux/mm.h                     |   2 +
 include/linux/security.h               |   2 +
 mm/mprotect.c                          |  15 ++-
 security/security.c                    |   8 ++
 security/selinux/hooks.c               |  85 +++++++++++++++
 11 files changed, 290 insertions(+), 59 deletions(-)

-- 
2.21.0

[RFC PATCH 1/9] x86/sgx: Remove unused local variable in sgx_encl_release()

[Sean Christopherson]

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

diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
index 7216bdf07bd0..f23ea0fbaa47 100644
--- a/arch/x86/kernel/cpu/sgx/encl.c
+++ b/arch/x86/kernel/cpu/sgx/encl.c
@@ -463,7 +463,6 @@ EXPORT_SYMBOL_GPL(sgx_encl_destroy);
 void sgx_encl_release(struct kref *ref)
 {
 	struct sgx_encl *encl = container_of(ref, struct sgx_encl, refcount);
-	struct sgx_encl_mm *encl_mm;
 
 	if (encl->pm_notifier.notifier_call)
 		unregister_pm_notifier(&encl->pm_notifier);
-- 
2.21.0

[RFC PATCH 2/9] 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 afe844aa81d6..129d356aff30 100644
--- a/arch/x86/kernel/cpu/sgx/driver/main.c
+++ b/arch/x86/kernel/cpu/sgx/driver/main.c
@@ -79,7 +79,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 3/9] x86/sgx: Allow userspace to add multiple pages in single ioctl()

[Sean Christopherson]

...to improve performance when building enclaves by reducing the number
of user<->system transitions.  Rather than provide arbitrary batching,
e.g. with per-page SECINFO and mrmask, take advantage of the fact that
any sane enclave will have large swaths of pages with identical
properties, e.g. code vs. data sections.

For simplicity and stability in the initial implementation, loop over
the existing add page flow instead of taking a more agressive approach,
which would require tracking transitions between VMAs and holding
mmap_sem for an extended duration.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/include/uapi/asm/sgx.h        |  21 ++---
 arch/x86/kernel/cpu/sgx/driver/ioctl.c | 104 +++++++++++++++----------
 2 files changed, 74 insertions(+), 51 deletions(-)

diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
index 9ed690a38c70..4a12d6abbcb7 100644
--- a/arch/x86/include/uapi/asm/sgx.h
+++ b/arch/x86/include/uapi/asm/sgx.h
@@ -12,8 +12,8 @@
 
 #define SGX_IOC_ENCLAVE_CREATE \
 	_IOW(SGX_MAGIC, 0x00, struct sgx_enclave_create)
-#define SGX_IOC_ENCLAVE_ADD_PAGE \
-	_IOW(SGX_MAGIC, 0x01, struct sgx_enclave_add_page)
+#define SGX_IOC_ENCLAVE_ADD_PAGES \
+	_IOW(SGX_MAGIC, 0x01, struct sgx_enclave_add_pages)
 #define SGX_IOC_ENCLAVE_INIT \
 	_IOW(SGX_MAGIC, 0x02, struct sgx_enclave_init)
 #define SGX_IOC_ENCLAVE_SET_ATTRIBUTE \
@@ -32,21 +32,22 @@ struct sgx_enclave_create  {
 };
 
 /**
- * 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
+ * struct sgx_enclave_add_pages - parameter structure for the
+ *                                %SGX_IOC_ENCLAVE_ADD_PAGES ioctl
+ * @addr:	start address within the ELRANGE
+ * @src:	start address for the pages' data
+ * @secinfo:	address for the SECINFO data (common to all pages)
+ * @nr_pages:	number of pages (must be virtually contiguous)
+ * @mrmask:	bitmask for the measured 256 byte chunks (common to all pages)
  */
-struct sgx_enclave_add_page {
+struct sgx_enclave_add_pages {
 	__u64	addr;
 	__u64	src;
 	__u64	secinfo;
+	__u32	nr_pages;
 	__u16	mrmask;
 } __attribute__((__packed__));
 
-
 /**
  * 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 a27ec26a9350..6acfcbdeca9a 100644
--- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
@@ -487,10 +487,9 @@ static int sgx_validate_tcs(struct sgx_encl *encl, struct sgx_tcs *tcs)
 	return 0;
 }
 
-static int __sgx_encl_add_page(struct sgx_encl *encl,
+static int sgx_encl_queue_page(struct sgx_encl *encl,
 			       struct sgx_encl_page *encl_page,
-			       void *data,
-			       struct sgx_secinfo *secinfo,
+			       void *data, struct sgx_secinfo *secinfo,
 			       unsigned int mrmask)
 {
 	unsigned long page_index = sgx_encl_get_index(encl, encl_page);
@@ -529,9 +528,9 @@ static int __sgx_encl_add_page(struct sgx_encl *encl,
 	return 0;
 }
 
-static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
-			     void *data, struct sgx_secinfo *secinfo,
-			     unsigned int mrmask)
+static int __sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
+			       void *data, struct sgx_secinfo *secinfo,
+			       unsigned int mrmask)
 {
 	u64 page_type = secinfo->flags & SGX_SECINFO_PAGE_TYPE_MASK;
 	struct sgx_encl_page *encl_page;
@@ -563,7 +562,7 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
 		goto out;
 	}
 
-	ret = __sgx_encl_add_page(encl, encl_page, data, secinfo, mrmask);
+	ret = sgx_encl_queue_page(encl, encl_page, data, secinfo, mrmask);
 	if (ret) {
 		radix_tree_delete(&encl_page->encl->page_tree,
 				  PFN_DOWN(encl_page->desc));
@@ -575,56 +574,79 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
 	return ret;
 }
 
-/**
- * sgx_ioc_enclave_add_page - handler for %SGX_IOC_ENCLAVE_ADD_PAGE
- *
- * @filep:	open file to /dev/sgx
- * @cmd:	the command value
- * @arg:	pointer to an &sgx_enclave_add_page instance
- *
- * Add a page to an uninitialized enclave (EADD), and optionally extend the
- * enclave's measurement with the contents of the page (EEXTEND).  EADD and
- * EEXTEND are done asynchronously via worker threads.  A successful
- * sgx_ioc_enclave_add_page() only indicates the page has been added to the
- * work queue, it does not guarantee adding the page to the enclave will
- * succeed.
- *
- * Return:
- *   0 on success,
- *   -errno otherwise
- */
-static long sgx_ioc_enclave_add_page(struct file *filep, unsigned int cmd,
-				     unsigned long arg)
+static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
+			     unsigned long src, struct sgx_secinfo *secinfo,
+			     unsigned int mrmask)
 {
-	struct sgx_enclave_add_page *addp = (void *)arg;
-	struct sgx_encl *encl = filep->private_data;
-	struct sgx_secinfo secinfo;
 	struct page *data_page;
 	void *data;
 	int ret;
 
-	if (copy_from_user(&secinfo, (void __user *)addp->secinfo,
-			   sizeof(secinfo)))
-		return -EFAULT;
-
 	data_page = alloc_page(GFP_HIGHUSER);
 	if (!data_page)
 		return -ENOMEM;
 
 	data = kmap(data_page);
 
-	if (copy_from_user((void *)data, (void __user *)addp->src, PAGE_SIZE)) {
+	if (copy_from_user((void *)data, (void __user *)src, PAGE_SIZE)) {
 		ret = -EFAULT;
 		goto out;
 	}
 
-	ret = sgx_encl_add_page(encl, addp->addr, data, &secinfo, addp->mrmask);
-	if (ret)
-		goto out;
-
+	ret = __sgx_encl_add_page(encl, addr, data, secinfo, mrmask);
 out:
 	kunmap(data_page);
 	__free_page(data_page);
+
+	return ret;
+}
+
+/**
+ * sgx_ioc_enclave_add_pages - handler for %SGX_IOC_ENCLAVE_ADD_PAGES
+ *
+ * @filep:	open file to /dev/sgx
+ * @cmd:	the command value
+ * @arg:	pointer to an &sgx_enclave_add_page instance
+ *
+ * Add a range of pages to an uninitialized enclave (EADD), and optionally
+ * extend the enclave's measurement with the contents of the page (EEXTEND).
+ * The range of pages must be virtually contiguous.  The SECINFO and
+ * measurement maskare applied to all pages, i.e. pages with different
+ * properties must be added in separate calls.
+ *
+ * EADD and EEXTEND are done asynchronously via worker threads.  A successful
+ * sgx_ioc_enclave_add_page() only indicates the pages have been added to the
+ * work queue, it does not guarantee adding the pages to the enclave will
+ * succeed.
+ *
+ * Return:
+ *   0 on success,
+ *   -errno otherwise
+ */
+static long sgx_ioc_enclave_add_pages(struct file *filep, unsigned int cmd,
+				      unsigned long arg)
+{
+	struct sgx_enclave_add_pages *addp = (void *)arg;
+	struct sgx_encl *encl = filep->private_data;
+	struct sgx_secinfo secinfo;
+	unsigned int i;
+	int ret;
+
+	if (copy_from_user(&secinfo, (void __user *)addp->secinfo,
+			   sizeof(secinfo)))
+		return -EFAULT;
+
+	for (i = 0, ret = 0; i < addp->nr_pages && !ret; i++) {
+		if (signal_pending(current))
+			return -ERESTARTSYS;
+
+		if (need_resched())
+			cond_resched();
+
+		ret = sgx_encl_add_page(encl, addp->addr + i*PAGE_SIZE,
+					addp->src + i*PAGE_SIZE,
+					&secinfo, addp->mrmask);
+	}
 	return ret;
 }
 
@@ -823,8 +845,8 @@ long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
 	case SGX_IOC_ENCLAVE_CREATE:
 		handler = sgx_ioc_enclave_create;
 		break;
-	case SGX_IOC_ENCLAVE_ADD_PAGE:
-		handler = sgx_ioc_enclave_add_page;
+	case SGX_IOC_ENCLAVE_ADD_PAGES:
+		handler = sgx_ioc_enclave_add_pages;
 		break;
 	case SGX_IOC_ENCLAVE_INIT:
 		handler = sgx_ioc_enclave_init;
-- 
2.21.0

[RFC PATCH 4/9] mm: Introduce vm_ops->mprotect()

[Sean Christopherson]

SGX will use the mprotect() hook to prevent userspace from circumventing
various security checks, i.e. Linux Security Modules.

Enclaves are built by copying data from normal memory into the Enclave
Page Cache (EPC).  Due to the nature of SGX, the EPC is represented by a
single file that must be MAP_SHARED, i.e. mprotect() only ever sees a
single MAP_SHARED vm_file.  Furthermore, all enclaves will need read,
write and execute pages in the EPC.

As a result, LSM policies cannot be meaningfully applied, e.g. an LSM
can deny access to the EPC as a whole, but can't deny PROT_EXEC on page
that originated in a non-EXECUTE file (which is long gone by the time
mprotect() is called).

By hooking mprotect(), SGX can make explicit LSM upcalls while an
enclave is being built, i.e. when the kernel has a handle to origin of
each enclave page, and enforce the result of the LSM policy whenever
userspace maps the enclave page in the future.

Alternatively, SGX could play games with MAY_{READ,WRITE,EXEC}, but
that approach is quite ugly, e.g. would require userspace to call an
SGX ioctl() prior to using mprotect() to extend a page's protections.

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..50a42364a885 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 (*mprotect)(struct vm_area_struct * area, 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..e466ca5e4fe0 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->mprotect) {
+			error = vma->vm_ops->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 5/9] x86/sgx: Restrict mapping without an enclave page to PROT_NONE

[Sean Christopherson]

To support LSM integration, SGX will require userspace to explicitly
specify the allowed protections for each page.  The allowed protections
will be supplied to and modified by LSMs (based on their policies).
To prevent userspace from circumventing the allowed protections, do not
allow PROT_{READ,WRITE,EXEC} mappings to an enclave without an
associated enclave page (which will track the allowed protections).

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kernel/cpu/sgx/driver/main.c |  5 +++++
 arch/x86/kernel/cpu/sgx/encl.c        | 30 +++++++++++++++++++++++++++
 arch/x86/kernel/cpu/sgx/encl.h        |  3 +++
 3 files changed, 38 insertions(+)

diff --git a/arch/x86/kernel/cpu/sgx/driver/main.c b/arch/x86/kernel/cpu/sgx/driver/main.c
index 129d356aff30..65a87c2fdf02 100644
--- a/arch/x86/kernel/cpu/sgx/driver/main.c
+++ b/arch/x86/kernel/cpu/sgx/driver/main.c
@@ -63,6 +63,11 @@ static long sgx_compat_ioctl(struct file *filep, unsigned int cmd,
 static int sgx_mmap(struct file *file, struct vm_area_struct *vma)
 {
 	struct sgx_encl *encl = file->private_data;
+	int ret;
+
+	ret = sgx_map_allowed(encl, vma->vm_start, vma->vm_end, vma->vm_flags);
+	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 f23ea0fbaa47..955d4f430adc 100644
--- a/arch/x86/kernel/cpu/sgx/encl.c
+++ b/arch/x86/kernel/cpu/sgx/encl.c
@@ -235,6 +235,35 @@ static void sgx_vma_close(struct vm_area_struct *vma)
 	kref_put(&encl->refcount, sgx_encl_release);
 }
 
+int sgx_map_allowed(struct sgx_encl *encl, unsigned long start,
+		    unsigned long end, unsigned long prot)
+{
+	struct sgx_encl_page *page;
+	unsigned long addr;
+
+	prot &= (VM_READ | VM_WRITE | VM_EXEC);
+	if (!prot || !encl)
+		return 0;
+
+	mutex_lock(&encl->lock);
+
+	for (addr = start; addr < end; addr += PAGE_SIZE) {
+		page = radix_tree_lookup(&encl->page_tree, addr >> PAGE_SHIFT);
+		if (!page)
+			return -EACCES;
+	}
+
+	mutex_unlock(&encl->lock);
+
+	return 0;
+}
+
+static int sgx_vma_mprotect(struct vm_area_struct *vma, unsigned long start,
+			    unsigned long end, unsigned long prot)
+{
+	return sgx_map_allowed(vma->vm_private_data, start, end, prot);
+}
+
 static unsigned int sgx_vma_fault(struct vm_fault *vmf)
 {
 	unsigned long addr = (unsigned long)vmf->address;
@@ -372,6 +401,7 @@ static int sgx_vma_access(struct vm_area_struct *vma, unsigned long addr,
 const struct vm_operations_struct sgx_vm_ops = {
 	.close = sgx_vma_close,
 	.open = sgx_vma_open,
+	.mprotect = sgx_vma_mprotect,
 	.fault = sgx_vma_fault,
 	.access = sgx_vma_access,
 };
diff --git a/arch/x86/kernel/cpu/sgx/encl.h b/arch/x86/kernel/cpu/sgx/encl.h
index c557f0374d74..6e310e3b3fff 100644
--- a/arch/x86/kernel/cpu/sgx/encl.h
+++ b/arch/x86/kernel/cpu/sgx/encl.h
@@ -106,6 +106,9 @@ static inline unsigned long sgx_pcmd_offset(pgoff_t page_index)
 	       sizeof(struct sgx_pcmd);
 }
 
+int sgx_map_allowed(struct sgx_encl *encl, unsigned long start,
+		    unsigned long end, unsigned long prot);
+
 enum sgx_encl_mm_iter {
 	SGX_ENCL_MM_ITER_DONE		= 0,
 	SGX_ENCL_MM_ITER_NEXT		= 1,
-- 
2.21.0

[RFC PATCH 6/9] x86/sgx: Require userspace to provide allowed prots to ADD_PAGES

[Sean Christopherson]

...to support (the equivalent) of existing Linux Security Module
functionality.

Because SGX manually manages EPC memory, all enclave VMAs are backed by
the same vm_file, i.e. /dev/sgx/enclave, so that SGX can implement the
necessary hooks to move pages in/out of the EPC.  And because EPC pages
for any given enclave are fundamentally shared between processes, i.e.
CoW semantics are not possible with EPC pages, /dev/sgx/enclave must
always be MAP_SHARED.  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 any reasonable granularity, e.g. an LSM can
deny access to EPC altogether, but can't deny potentially dangerous
behavior such as mapping pages RW->RW or RWX.

To give LSMs enough information to implement their policies without
having to resort to ugly things, e.g. holding a reference to the vm_file
of each enclave page, require userspace to explicitly state the allowed
protections for each page (region), i.e. take ALLOW_{READ,WRITE,EXEC}
in the ADD_PAGES ioctl.

The ALLOW_* flags will be passed to LSMs so that they can make informed
decisions when the enclave is being built, i.e. when the source vm_file
is available.  For example, SELinux's EXECMOD permission can be
required if an enclave is requesting both ALLOW_WRITE and ALLOW_EXEC.

Update the mmap()/mprotect() hooks to enforce the ALLOW_* protections,
a la the standard VM_MAY{READ,WRITE,EXEC} flags.

The ALLOW_EXEC flag also has a second (important) 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 from a noexec path using read-only permissions and later mprotect()
and ENCLU[EMODPE] the page to gain execute permissions.  By requiring
ALLOW_EXEC up front, SGX will be able to enforce noexec paths when
building the enclave.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/include/uapi/asm/sgx.h        |  9 ++++++++-
 arch/x86/kernel/cpu/sgx/driver/ioctl.c | 23 +++++++++++++++++------
 arch/x86/kernel/cpu/sgx/encl.c         |  2 +-
 arch/x86/kernel/cpu/sgx/encl.h         |  1 +
 4 files changed, 27 insertions(+), 8 deletions(-)

diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
index 4a12d6abbcb7..4489e92fa0dc 100644
--- a/arch/x86/include/uapi/asm/sgx.h
+++ b/arch/x86/include/uapi/asm/sgx.h
@@ -31,6 +31,11 @@ struct sgx_enclave_create  {
 	__u64	src;
 };
 
+/* Supported flags for struct sgx_enclave_add_pages. */
+#define SGX_ALLOW_READ	VM_READ
+#define SGX_ALLOW_WRITE	VM_WRITE
+#define SGX_ALLOW_EXEC	VM_EXEC
+
 /**
  * struct sgx_enclave_add_pages - parameter structure for the
  *                                %SGX_IOC_ENCLAVE_ADD_PAGES ioctl
@@ -39,6 +44,7 @@ struct sgx_enclave_create  {
  * @secinfo:	address for the SECINFO data (common to all pages)
  * @nr_pages:	number of pages (must be virtually contiguous)
  * @mrmask:	bitmask for the measured 256 byte chunks (common to all pages)
+ * @flags:	flags, e.g. SGX_ALLOW_{READ,WRITE,EXEC} (common to all pages)
  */
 struct sgx_enclave_add_pages {
 	__u64	addr;
@@ -46,7 +52,8 @@ struct sgx_enclave_add_pages {
 	__u64	secinfo;
 	__u32	nr_pages;
 	__u16	mrmask;
-} __attribute__((__packed__));
+	__u16	flags;
+};
 
 /**
  * struct sgx_enclave_init - parameter structure for the
diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
index 6acfcbdeca9a..c30acd3fbbdd 100644
--- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
@@ -235,7 +235,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 allowed_prot)
 {
 	struct sgx_encl_page *encl_page;
 	int ret;
@@ -247,6 +248,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->allowed_prot = allowed_prot;
 	ret = radix_tree_insert(&encl->page_tree, PFN_DOWN(encl_page->desc),
 				encl_page);
 	if (ret) {
@@ -530,7 +532,7 @@ static int sgx_encl_queue_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 allowed_prot)
 {
 	u64 page_type = secinfo->flags & SGX_SECINFO_PAGE_TYPE_MASK;
 	struct sgx_encl_page *encl_page;
@@ -556,7 +558,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, allowed_prot);
 	if (IS_ERR(encl_page)) {
 		ret = PTR_ERR(encl_page);
 		goto out;
@@ -576,12 +578,20 @@ static int __sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
 
 static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
 			     unsigned long src, struct sgx_secinfo *secinfo,
-			     unsigned int mrmask)
+			     unsigned int mrmask, unsigned int flags)
 {
+	unsigned long prot = secinfo->flags & (VM_READ | VM_WRITE | VM_EXEC);
+	unsigned long allowed_prot = flags & (VM_READ | VM_WRITE | VM_EXEC);
 	struct page *data_page;
 	void *data;
 	int ret;
 
+	BUILD_BUG_ON(SGX_SECINFO_R != VM_READ || SGX_SECINFO_W != VM_WRITE ||
+		     SGX_SECINFO_X != VM_EXEC);
+
+	if (prot & ~allowed_prot)
+		return -EACCES;
+
 	data_page = alloc_page(GFP_HIGHUSER);
 	if (!data_page)
 		return -ENOMEM;
@@ -593,7 +603,8 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
 		goto out;
 	}
 
-	ret = __sgx_encl_add_page(encl, addr, data, secinfo, mrmask);
+	ret = __sgx_encl_add_page(encl, addr, data, secinfo, mrmask,
+				  allowed_prot);
 out:
 	kunmap(data_page);
 	__free_page(data_page);
@@ -645,7 +656,7 @@ static long sgx_ioc_enclave_add_pages(struct file *filep, unsigned int cmd,
 
 		ret = sgx_encl_add_page(encl, addp->addr + i*PAGE_SIZE,
 					addp->src + i*PAGE_SIZE,
-					&secinfo, addp->mrmask);
+					&secinfo, addp->mrmask, addp->flags);
 	}
 	return ret;
 }
diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
index 955d4f430adc..e5847571a265 100644
--- a/arch/x86/kernel/cpu/sgx/encl.c
+++ b/arch/x86/kernel/cpu/sgx/encl.c
@@ -249,7 +249,7 @@ int sgx_map_allowed(struct sgx_encl *encl, unsigned long start,
 
 	for (addr = start; addr < end; addr += PAGE_SIZE) {
 		page = radix_tree_lookup(&encl->page_tree, addr >> PAGE_SHIFT);
-		if (!page)
+		if (!page || (prot & ~page->allowed_prot))
 			return -EACCES;
 	}
 
diff --git a/arch/x86/kernel/cpu/sgx/encl.h b/arch/x86/kernel/cpu/sgx/encl.h
index 6e310e3b3fff..7cca076a4987 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 allowed_prot;
 	struct sgx_epc_page *epc_page;
 	struct sgx_va_page *va_page;
 	struct sgx_encl *encl;
-- 
2.21.0

[RFC PATCH 7/9] 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
page is backed by a file on a noexec file system.

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

diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
index c30acd3fbbdd..5f71be7cbb01 100644
--- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
@@ -576,6 +576,27 @@ static int __sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
 	return ret;
 }
 
+static int sgx_encl_page_protect(unsigned long src, unsigned long prot,
+				 unsigned long *allowed_prot)
+{
+	struct vm_area_struct *vma;
+
+	if (!(*allowed_prot & VM_EXEC))
+		goto do_check;
+
+	down_read(&current->mm->mmap_sem);
+	vma = find_vma(current->mm, src);
+	if (!vma || (vma->vm_file && path_noexec(&vma->vm_file->f_path)))
+		*allowed_prot &= ~VM_EXEC;
+	up_read(&current->mm->mmap_sem);
+
+do_check:
+	if (prot & ~*allowed_prot)
+		return -EACCES;
+
+	return 0;
+}
+
 static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
 			     unsigned long src, struct sgx_secinfo *secinfo,
 			     unsigned int mrmask, unsigned int flags)
@@ -589,8 +610,9 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
 	BUILD_BUG_ON(SGX_SECINFO_R != VM_READ || SGX_SECINFO_W != VM_WRITE ||
 		     SGX_SECINFO_X != VM_EXEC);
 
-	if (prot & ~allowed_prot)
-		return -EACCES;
+	ret = sgx_encl_page_protect(src, prot, &allowed_prot);
+	if (ret)
+		return ret;
 
 	data_page = alloc_page(GFP_HIGHUSER);
 	if (!data_page)
-- 
2.21.0

[RFC PATCH 8/9] 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, 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.  The notable
difference from file_mprotect() is the allowed_prot parameter, which
is essentially an SGX-specific version of a VMA's MAY_{READ,WRITE,EXEC}
flags.  The purpose of allowed_prot is to enable checks such as
SELinux's FILE__EXECMOD permission without having to track and update
VMAs across multiple mm structs, i.e. SGX can ensure userspace doesn't
overstep its bounds simply by restricting an enclave VMA's protections
by vetting what is maximally allowed during build time.

An alternative to the allowed_prot approach would be to use an enclave's
SIGSTRUCT (a smallish structure that can uniquely identify an enclave)
as a proxy for the enclave.  For example, SGX could take and hold a
reference to the file containing the SIGSTRUCT (if it's in a file) and
call security_enclave_load() during mprotect().  While the SIGSTRUCT
approach would provide better precision, the actual value added was
deemed to be negligible.  On the other hand, pinning a file for the
lifetime of the enclave is ugly, and essentially caching LSM policies
in each page's allowed_prot avoids having to make an extra LSM upcall
during mprotect().

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 | 14 +++++++++-----
 include/linux/lsm_hooks.h              | 16 ++++++++++++++++
 include/linux/security.h               |  2 ++
 security/security.c                    |  8 ++++++++
 4 files changed, 35 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
index 5f71be7cbb01..260417ecbcff 100644
--- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
+++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
@@ -8,6 +8,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>
@@ -580,21 +581,24 @@ static int sgx_encl_page_protect(unsigned long src, unsigned long prot,
 				 unsigned long *allowed_prot)
 {
 	struct vm_area_struct *vma;
+	int ret = 0;
 
-	if (!(*allowed_prot & VM_EXEC))
+	if (!(*allowed_prot & VM_EXEC) && !IS_ENABLED(CONFIG_SECURITY))
 		goto do_check;
 
 	down_read(&current->mm->mmap_sem);
 	vma = find_vma(current->mm, src);
 	if (!vma || (vma->vm_file && path_noexec(&vma->vm_file->f_path)))
 		*allowed_prot &= ~VM_EXEC;
+#ifdef CONFIG_SECURITY
+	ret = security_enclave_load(vma, prot, allowed_prot);
+#endif
 	up_read(&current->mm->mmap_sem);
 
 do_check:
-	if (prot & ~*allowed_prot)
-		return -EACCES;
-
-	return 0;
+	if (!ret && (prot & ~*allowed_prot))
+		ret = -EACCES;
+	return ret;
 }
 
 static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 47f58cfb6a19..0562775424a0 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1446,6 +1446,14 @@
  * @bpf_prog_free_security:
  *	Clean up the security information stored inside bpf prog.
  *
+ * Security hooks for Intel SGX enclaves.
+ *
+ * @enclave_load:
+ *	On success, returns 0 and optionally adjusts @allowed_prot
+ *	@vma: the source memory region of the enclave page being loaded.
+ *	@prot: the initial protection of the enclave page.
+ *	@allowed_prot: the maximum protections of the enclave page.
+ *	Return 0 if permission is granted.
  */
 union security_list_options {
 	int (*binder_set_context_mgr)(struct task_struct *mgr);
@@ -1807,6 +1815,11 @@ 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_load)(struct vm_area_struct *vma, unsigned long prot,
+			    unsigned long *allowed_prot);
+#endif /* CONFIG_INTEL_SGX */
 };
 
 struct security_hook_heads {
@@ -2046,6 +2059,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_load;
+#endif /* CONFIG_INTEL_SGX */
 } __randomize_layout;
 
 /*
diff --git a/include/linux/security.h b/include/linux/security.h
index 659071c2e57c..2f7925eeef7e 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -392,6 +392,8 @@ void security_inode_invalidate_secctx(struct inode *inode);
 int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
 int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen);
 int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen);
+int security_enclave_load(struct vm_area_struct *vma, unsigned long prot,
+			  unsigned long *allowed_prot);
 #else /* CONFIG_SECURITY */
 
 static inline int call_lsm_notifier(enum lsm_event event, void *data)
diff --git a/security/security.c b/security/security.c
index 613a5c00e602..07ed6763571e 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2359,3 +2359,11 @@ 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_load(struct vm_area_struct *vma, unsigned long prot,
+			  unsigned long *allowed_prot)
+{
+	return call_int_hook(enclave_load, 0, vma, prot, allowed_prot);
+}
+#endif /* CONFIG_INTEL_SGX */
-- 
2.21.0

[RFC PATCH 9/9] 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).

The basic concept is to require appropriate execute permissions on the
source of the enclave for pages that are requesting PROT_EXEC, e.g. if
an enclave page is being loaded from a regular file, require
FILE__EXECUTE and/or FILE__EXECMOND, and if it's coming from an
anonymous/private mapping, require PROCESS__EXECMEM since the process
is essentially executing from the mapping, albeit in a roundabout way.

Note, FILE__READ and FILE__WRITE are intentionally not required even if
the source page is backed by a regular file.  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 | 85 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 85 insertions(+)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 3ec702cf46ca..f436a055dda7 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6726,6 +6726,87 @@ static void selinux_bpf_prog_free(struct bpf_prog_aux *aux)
 }
 #endif
 
+#ifdef CONFIG_INTEL_SGX
+int selinux_enclave_load(struct vm_area_struct *vma, unsigned long prot,
+			 unsigned long *allowed_prot)
+{
+	const struct cred *cred = current_cred();
+	u32 sid = cred_sid(cred);
+	int rc;
+
+	/* SGX is supported only in 64-bit kernels. */
+	WARN_ON_ONCE(!default_noexec);
+
+	/*
+	 * SGX is responsible for checking @prot vs @allowed_prot, and SELinux
+	 * only cares about execute related permissions for enclaves.
+	 */
+	if (!(*allowed_prot & PROT_EXEC))
+		return 0;
+
+	/*
+	 * Loading an executable enclave page from a VMA that is not executable
+	 * itself requires EXECUTE permissions on the source file, or if there
+	 * is no regular source file, EXECMEM since the page is being loaded
+	 * from a non-executable anonymous mapping.
+	 */
+	if (!(vma->vm_flags & VM_EXEC)) {
+		if (vma->vm_file && !IS_PRIVATE(file_inode(vma->vm_file)))
+			rc = file_has_perm(cred, vma->vm_file, FILE__EXECUTE);
+		else
+			rc = avc_has_perm(&selinux_state,
+					  sid, sid, SECCLASS_PROCESS,
+					  PROCESS__EXECMEM, NULL);
+
+		/*
+		 * Reject the load if the enclave *needs* the page to be
+		 * executable, otherwise prevent it from becoming executable.
+		 */
+		if (rc) {
+			if (prot & PROT_EXEC)
+				return rc;
+
+			*allowed_prot &= ~PROT_EXEC;
+		}
+	}
+
+	/*
+	 * An enclave page that may do RW->RX or W+X requires EXECMOD (backed
+	 * by a regular file) or EXECMEM (loaded from an anonymous mapping).
+	 * Note, this hybrid EXECMOD and EXECMEM behavior is intentional and
+	 * reflects the nature of enclaves and the EPC, e.g. EPC is effectively
+	 * a non-persistent shared file, but each enclave is a private domain
+	 * within that shared file, so delegate to the source of the enclave.
+	 */
+	if ((*allowed_prot & PROT_EXEC) && (*allowed_prot & PROT_WRITE)) {
+		if (vma->vm_file && !IS_PRIVATE(file_inode(vma->vm_file)))
+			rc = file_has_perm(cred, vma->vm_file, FILE__EXECMOD);
+		else
+			rc = avc_has_perm(&selinux_state,
+					  sid, sid, SECCLASS_PROCESS,
+					  PROCESS__EXECMEM, NULL);
+		/*
+		 * Clear ALLOW_EXEC instead of ALLOWED_WRITE if permissions are
+		 * lacking and @prot has neither PROT_WRITE or PROT_EXEC.  If
+		 * userspace wanted RX they would have requested RX, and due to
+		 * lack of permissions they can never get RW->RX, i.e. the only
+		 * useful transition is R->RW.
+		 */
+		if (rc) {
+			if ((prot & PROT_EXEC) && (prot & PROT_WRITE))
+				return rc;
+
+			if (prot & PROT_EXEC)
+				*allowed_prot &= ~PROT_WRITE;
+			else
+				*allowed_prot &= ~PROT_EXEC;
+		}
+	}
+
+	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 +7049,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_load, selinux_enclave_load),
+#endif
 };
 
 static __init int selinux_init(void)
-- 
2.21.0

RE: [RFC PATCH 0/9] security: x86/sgx: SGX vs. LSM

[Xing, Cedric]

Hi Sean,

> From: Christopherson, Sean J
> Sent: Friday, May 31, 2019 4:32 PM
> 
> This series is the result of a rather absurd amount of discussion over how to get SGX to play
> nice with LSM policies, without having to resort to evil shenanigans or put undue burden on
> userspace.  The discussion definitely wandered into completely insane territory at times, but
> I think/hope we ended up with something reasonable.
> 
> The basic gist of the approach is to require userspace to declare what protections are
> maximally allowed for any given page, e.g. add a flags field for loading enclave pages that
> takes ALLOW_{READ,WRITE,EXEC}.  LSMs can then adjust the allowed protections, e.g. clear
> ALLOW_EXEC to prevent ever mapping the page with PROT_EXEC.  SGX enforces the allowed perms
> via a new mprotect() vm_ops hook, e.g. like regular mprotect() uses MAY_{READ,WRITE,EXEC}.
> 
> ALLOW_EXEC is used to deny hings like loading an enclave from a noexec file system or from a
> file without EXECUTE permissions, e.g. without the ALLOW_EXEC concept, on SGX2 hardware
> (regardless of kernel support) userspace could EADD from a noexec file using read-only
> permissions, and later use mprotect() and ENCLU[EMODPE] to gain execute permissions.
> 
> ALLOW_WRITE is used in conjuction with ALLOW_EXEC to enforce SELinux's EXECMOD (or EXECMEM).
> 
> This is very much an RFC series.  It's only compile tested, likely has obvious bugs, the
> SELinux patch could be completely harebrained, etc...
> My goal at this point is to get feedback at a macro level, e.g. is the core concept
> viable/acceptable, are there objection to hooking mprotect(), etc...
> 
> Andy and Cedric, hopefully this aligns with your general expectations based on our last
> discussion.

I couldn't understand the real intentions of ALLOW_* flags until I saw them in code. I have to say C is more expressive than English in that regard :)

Generally I agree with your direction but think ALLOW_* flags are completely internal to LSM because they can be both produced and consumed inside an LSM module. So spilling them into SGX driver and also user mode code makes the solution ugly and in some cases impractical because not every enclave host process has a priori knowledge on whether or not an enclave page would be EMODPE'd at runtime.

Theoretically speaking, what you really need is a per page flag (let's name it WRITTEN?) indicating whether a page has ever been written to (or more precisely, granted PROT_WRITE), which will be used to decide whether to grant PROT_EXEC when requested in future. Given the fact that all mprotect() goes through LSM and mmap() is limited to PROT_NONE, it's easy for LSM to capture that flag by itself instead of asking user mode code to provide it.

That said, here is the summary of what I think is a better approach.
* In hook security_file_alloc(), if @file is an enclave, allocate some data structure to store for every page, the WRITTEN flag as described above. WRITTEN is cleared initially for all pages.
  Open: Given a file of type struct file *, how to tell if it is an enclave (i.e. /dev/sgx/enclave)?
* In hook security_mmap_file(), if @file is an enclave, make sure @prot can only be PROT_NONE. This is to force all protection changes to go through security_file_mprotect().
* In the newly introduced hook security_enclave_load(), set WRITTEN for pages that are requested PROT_WRITE.
* In hook security_file_mprotect(), if @vma->vm_file is an enclave, look up and use WRITTEN flags for all pages within @vma, along with other global flags (e.g. PROCESS__EXECMEM/FILE__EXECMOD in the case of SELinux) to decide on allowing/rejecting @prot.
* In hook security_file_free(), if @file is an enclave, free storage allocated for WRITTEN flags. 

I'll try to make more detailed comments in my replies to individual patches sometime tomorrow.

> 
> Lastly, I added a patch to allow userspace to add multiple pages in a single ioctl().  It's
> obviously not directly related to the security stuff, but the idea tangentially came up during
> earlier discussions and it's something I think the UAPI should provide (it's a tiny change).
> Since I was modifying the UAPI anyways, I threw it in.
> 
> Sean Christopherson (9):
>   x86/sgx: Remove unused local variable in sgx_encl_release()
>   x86/sgx: Do not naturally align MAP_FIXED address
>   x86/sgx: Allow userspace to add multiple pages in single ioctl()
>   mm: Introduce vm_ops->mprotect()
>   x86/sgx: Restrict mapping without an enclave page to PROT_NONE
>   x86/sgx: Require userspace to provide allowed prots to ADD_PAGES
>   x86/sgx: Enforce noexec filesystem restriction for enclaves
>   LSM: x86/sgx: Introduce ->enclave_load() hook for Intel SGX
>   security/selinux: Add enclave_load() implementation
> 
>  arch/x86/include/uapi/asm/sgx.h        |  30 ++++--
>  arch/x86/kernel/cpu/sgx/driver/ioctl.c | 143 +++++++++++++++++--------
> arch/x86/kernel/cpu/sgx/driver/main.c  |  13 ++-
>  arch/x86/kernel/cpu/sgx/encl.c         |  31 +++++-
>  arch/x86/kernel/cpu/sgx/encl.h         |   4 +
>  include/linux/lsm_hooks.h              |  16 +++
>  include/linux/mm.h                     |   2 +
>  include/linux/security.h               |   2 +
>  mm/mprotect.c                          |  15 ++-
>  security/security.c                    |   8 ++
>  security/selinux/hooks.c               |  85 +++++++++++++++
>  11 files changed, 290 insertions(+), 59 deletions(-)
> 
> --
> 2.21.0

-Cedric

RE: [RFC PATCH 3/9] x86/sgx: Allow userspace to add multiple pages in single ioctl()

[Xing, Cedric]

> From: Christopherson, Sean J
> Sent: Friday, May 31, 2019 4:32 PM
> 
> +/**
> + * sgx_ioc_enclave_add_pages - handler for %SGX_IOC_ENCLAVE_ADD_PAGES
> + *
> + * @filep:	open file to /dev/sgx
> + * @cmd:	the command value
> + * @arg:	pointer to an &sgx_enclave_add_page instance
> + *
> + * Add a range of pages to an uninitialized enclave (EADD), and
> +optionally
> + * extend the enclave's measurement with the contents of the page (EEXTEND).
> + * The range of pages must be virtually contiguous.  The SECINFO and
> + * measurement maskare applied to all pages, i.e. pages with different
> + * properties must be added in separate calls.
> + *
> + * EADD and EEXTEND are done asynchronously via worker threads.  A
> +successful
> + * sgx_ioc_enclave_add_page() only indicates the pages have been added
> +to the
> + * work queue, it does not guarantee adding the pages to the enclave
> +will
> + * succeed.
> + *
> + * Return:
> + *   0 on success,
> + *   -errno otherwise
> + */
> +static long sgx_ioc_enclave_add_pages(struct file *filep, unsigned int cmd,
> +				      unsigned long arg)
> +{
> +	struct sgx_enclave_add_pages *addp = (void *)arg;
> +	struct sgx_encl *encl = filep->private_data;
> +	struct sgx_secinfo secinfo;
> +	unsigned int i;
> +	int ret;
> +
> +	if (copy_from_user(&secinfo, (void __user *)addp->secinfo,
> +			   sizeof(secinfo)))
> +		return -EFAULT;
> +
> +	for (i = 0, ret = 0; i < addp->nr_pages && !ret; i++) {
> +		if (signal_pending(current))
> +			return -ERESTARTSYS;

If interrupted, how would user mode code know how many pages have been EADD'ed?

> +
> +		if (need_resched())
> +			cond_resched();
> +
> +		ret = sgx_encl_add_page(encl, addp->addr + i*PAGE_SIZE,
> +					addp->src + i*PAGE_SIZE,
> +					&secinfo, addp->mrmask);
> +	}
>  	return ret;
>  }
> 
> @@ -823,8 +845,8 @@ long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
>  	case SGX_IOC_ENCLAVE_CREATE:
>  		handler = sgx_ioc_enclave_create;
>  		break;
> -	case SGX_IOC_ENCLAVE_ADD_PAGE:
> -		handler = sgx_ioc_enclave_add_page;
> +	case SGX_IOC_ENCLAVE_ADD_PAGES:
> +		handler = sgx_ioc_enclave_add_pages;
>  		break;
>  	case SGX_IOC_ENCLAVE_INIT:
>  		handler = sgx_ioc_enclave_init;
> --
> 2.21.0

RE: [RFC PATCH 4/9] mm: Introduce vm_ops->mprotect()

[Xing, Cedric]

> From: Christopherson, Sean J
> Sent: Friday, May 31, 2019 4:32 PM
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h index 0e8834ac32b7..50a42364a885
> 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 (*mprotect)(struct vm_area_struct * area, unsigned long start,
> +			unsigned long end, unsigned long prot);

As I commented in my reply to the cover letter, SGX driver doesn't need to intercept mprotect() if ALLOW_* flags are not spilled into it.

>  	vm_fault_t (*fault)(struct vm_fault *vmf);
>  	vm_fault_t (*huge_fault)(struct vm_fault *vmf,
>  			enum page_entry_size pe_size);

RE: [RFC PATCH 8/9] LSM: x86/sgx: Introduce ->enclave_load() hook for Intel SGX

[Xing, Cedric]

> From: Christopherson, Sean J
> Sent: Friday, May 31, 2019 4:32 PM
> 
> diff --git a/include/linux/security.h b/include/linux/security.h index
> 659071c2e57c..2f7925eeef7e 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -392,6 +392,8 @@ void security_inode_invalidate_secctx(struct inode *inode);  int
> security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);  int
> security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen);  int
> security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen);
> +int security_enclave_load(struct vm_area_struct *vma, unsigned long prot,
> +			  unsigned long *allowed_prot);

Per my comments to the cover letter, security_enclave_load() should have a signature similar to the following:

int security_enclave_load(struct file *enclave_fd, unsigned long linear_address, unsigned long nr_pages, int prot, struct vm_area_struct *source_vma);

@enclave_fd identifies the enclave to which new pages are being added.
@linear_address/@nr_pages specifies the linear range of pages being added.
@prot specifies the initial protection of those newly added pages. It is taken from the vma covering the target range.
@source_vma covers the source pages in the case of EADD. An LSM is expected to make sure security_file_mprotect(source_vma, prot, prot) would succeed before checking anything else, unless @source_vma is NULL, indicating pages are being EAUG'ed. In all cases, LSM is expected to "remember" @prot for all those pages to be checked in future security_file_mprotect() invocations.

>  #else /* CONFIG_SECURITY */

RE: [RFC PATCH 5/9] x86/sgx: Restrict mapping without an enclave page to PROT_NONE

[Xing, Cedric]

> From: Christopherson, Sean J
> Sent: Friday, May 31, 2019 4:32 PM
> 
> To support LSM integration, SGX will require userspace to explicitly specify the allowed
> protections for each page.  The allowed protections will be supplied to and modified by
> LSMs (based on their policies).
> To prevent userspace from circumventing the allowed protections, do not allow
> PROT_{READ,WRITE,EXEC} mappings to an enclave without an associated enclave page (which
> will track the allowed protections).

This is unnecessary. 

For mprotect(), LSM shall validate @prot against existing pages with applicable global flags (e.g. FILE__EXECMOD/PROCESS__EXECMEM in the case of SELinux). 

For mmap(), SGX driver could invoke security_file_mprotect() explicitly to have LSM validate requested protection.

In the case where there's no page associated with an VMA, security_file_mprotect() shall still dictate whether to allow/deny the request. LSM internally is able to track existence/nonexistence of enclave pages. If there's no page, there's no conflict so the decision shall only depend on global flags (if any). Afterwards, #PF may trigger SGX driver to EAUG, in which case security_enclave_load() will be invoked and LSM could decide whether to approve/decline EAUG request. 

RE: [RFC PATCH 6/9] x86/sgx: Require userspace to provide allowed prots to ADD_PAGES

[Xing, Cedric]

> From: Christopherson, Sean J
> Sent: Friday, May 31, 2019 4:32 PM
> 
> ...to support (the equivalent) of existing Linux Security Module functionality.
> 
> Because SGX manually manages EPC memory, all enclave VMAs are backed by the same vm_file,
> i.e. /dev/sgx/enclave, so that SGX can implement the necessary hooks to move pages in/out
> of the EPC.  And because EPC pages for any given enclave are fundamentally shared between
> processes, i.e.
> CoW semantics are not possible with EPC pages, /dev/sgx/enclave must always be MAP_SHARED.
> 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 any reasonable granularity, e.g. an LSM can deny access to
> EPC altogether, but can't deny potentially dangerous behavior such as mapping pages RW->RW
> or RWX.
> 
> To give LSMs enough information to implement their policies without having to resort to
> ugly things, e.g. holding a reference to the vm_file of each enclave page, require
> userspace to explicitly state the allowed protections for each page (region), i.e. take
> ALLOW_{READ,WRITE,EXEC} in the ADD_PAGES ioctl.
> 
> The ALLOW_* flags will be passed to LSMs so that they can make informed decisions when the
> enclave is being built, i.e. when the source vm_file is available.  For example, SELinux's
> EXECMOD permission can be required if an enclave is requesting both ALLOW_WRITE and
> ALLOW_EXEC.
> 
> Update the mmap()/mprotect() hooks to enforce the ALLOW_* protections, a la the standard
> VM_MAY{READ,WRITE,EXEC} flags.
> 
> The ALLOW_EXEC flag also has a second (important) 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 from a noexec
> path using read-only permissions and later mprotect() and ENCLU[EMODPE] the page to gain
> execute permissions.  By requiring ALLOW_EXEC up front, SGX will be able to enforce noexec
> paths when building the enclave.

ALLOW_* flags shall be kept internal to LSM.

This patch is completely unnecessary.

RE: [RFC PATCH 7/9] x86/sgx: Enforce noexec filesystem restriction for enclaves

[Xing, Cedric]

> From: Christopherson, Sean J
> Sent: Friday, May 31, 2019 4:32 PM
> 
> Do not allow an enclave page to be mapped with PROT_EXEC if the source page is backed by a
> file on a noexec file system.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/kernel/cpu/sgx/driver/ioctl.c | 26 ++++++++++++++++++++++++--
>  1 file changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> index c30acd3fbbdd..5f71be7cbb01 100644
> --- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> +++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> @@ -576,6 +576,27 @@ static int __sgx_encl_add_page(struct sgx_encl *encl, unsigned long
> addr,
>  	return ret;
>  }
> 
> +static int sgx_encl_page_protect(unsigned long src, unsigned long prot,
> +				 unsigned long *allowed_prot)
> +{
> +	struct vm_area_struct *vma;
> +
> +	if (!(*allowed_prot & VM_EXEC))
> +		goto do_check;
> +
> +	down_read(&current->mm->mmap_sem);
> +	vma = find_vma(current->mm, src);
> +	if (!vma || (vma->vm_file && path_noexec(&vma->vm_file->f_path)))
> +		*allowed_prot &= ~VM_EXEC;

Testing (vma->vm_flags & VM_MAYEXEC) == 0 should be a better approach.

Moreover, it looks like the check is done per page, so say 100 pages would cause this test to run 100 times even if they are within the same VMA. Wouldn't that be a bit inefficient? 
 
> +	up_read(&current->mm->mmap_sem);
> +
> +do_check:
> +	if (prot & ~*allowed_prot)
> +		return -EACCES;
> +
> +	return 0;
> +}

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

[Stephen Smalley]

On 5/31/19 7:31 PM, 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, 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.  The notable
> difference from file_mprotect() is the allowed_prot parameter, which
> is essentially an SGX-specific version of a VMA's MAY_{READ,WRITE,EXEC}
> flags.  The purpose of allowed_prot is to enable checks such as
> SELinux's FILE__EXECMOD permission without having to track and update
> VMAs across multiple mm structs, i.e. SGX can ensure userspace doesn't
> overstep its bounds simply by restricting an enclave VMA's protections
> by vetting what is maximally allowed during build time.
> 
> An alternative to the allowed_prot approach would be to use an enclave's
> SIGSTRUCT (a smallish structure that can uniquely identify an enclave)
> as a proxy for the enclave.  For example, SGX could take and hold a
> reference to the file containing the SIGSTRUCT (if it's in a file) and
> call security_enclave_load() during mprotect().  While the SIGSTRUCT
> approach would provide better precision, the actual value added was
> deemed to be negligible.  On the other hand, pinning a file for the
> lifetime of the enclave is ugly, and essentially caching LSM policies
> in each page's allowed_prot avoids having to make an extra LSM upcall
> during mprotect().
> 
> 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 | 14 +++++++++-----
>   include/linux/lsm_hooks.h              | 16 ++++++++++++++++
>   include/linux/security.h               |  2 ++
>   security/security.c                    |  8 ++++++++
>   4 files changed, 35 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> index 5f71be7cbb01..260417ecbcff 100644
> --- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> +++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> @@ -8,6 +8,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>
> @@ -580,21 +581,24 @@ static int sgx_encl_page_protect(unsigned long src, unsigned long prot,
>   				 unsigned long *allowed_prot)
>   {
>   	struct vm_area_struct *vma;
> +	int ret = 0;
>   
> -	if (!(*allowed_prot & VM_EXEC))
> +	if (!(*allowed_prot & VM_EXEC) && !IS_ENABLED(CONFIG_SECURITY))
>   		goto do_check;
>   
>   	down_read(&current->mm->mmap_sem);
>   	vma = find_vma(current->mm, src);
>   	if (!vma || (vma->vm_file && path_noexec(&vma->vm_file->f_path)))
>   		*allowed_prot &= ~VM_EXEC;
> +#ifdef CONFIG_SECURITY
> +	ret = security_enclave_load(vma, prot, allowed_prot);
> +#endif

Normally you'd define a static inline stub for the hook in the #else 
clause for CONFIG_SECURITY in include/linux/security.h and avoid any 
ifdef here.

What ensures that the mapping referenced by src can't be changed to an 
entirely different one (with a different vm_file) between the time of 
check (here) and the time of use?

>   	up_read(&current->mm->mmap_sem);
>   
>   do_check:
> -	if (prot & ~*allowed_prot)
> -		return -EACCES;
> -
> -	return 0;
> +	if (!ret && (prot & ~*allowed_prot))
> +		ret = -EACCES;
> +	return ret;
>   }
>   
>   static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 47f58cfb6a19..0562775424a0 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -1446,6 +1446,14 @@
>    * @bpf_prog_free_security:
>    *	Clean up the security information stored inside bpf prog.
>    *
> + * Security hooks for Intel SGX enclaves.
> + *
> + * @enclave_load:
> + *	On success, returns 0 and optionally adjusts @allowed_prot
> + *	@vma: the source memory region of the enclave page being loaded.
> + *	@prot: the initial protection of the enclave page.
> + *	@allowed_prot: the maximum protections of the enclave page.
> + *	Return 0 if permission is granted.
>    */
>   union security_list_options {
>   	int (*binder_set_context_mgr)(struct task_struct *mgr);
> @@ -1807,6 +1815,11 @@ 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_load)(struct vm_area_struct *vma, unsigned long prot,
> +			    unsigned long *allowed_prot);
> +#endif /* CONFIG_INTEL_SGX */
>   };
>   
>   struct security_hook_heads {
> @@ -2046,6 +2059,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_load;
> +#endif /* CONFIG_INTEL_SGX */
>   } __randomize_layout;
>   
>   /*
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 659071c2e57c..2f7925eeef7e 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -392,6 +392,8 @@ void security_inode_invalidate_secctx(struct inode *inode);
>   int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
>   int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen);
>   int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen);
> +int security_enclave_load(struct vm_area_struct *vma, unsigned long prot,
> +			  unsigned long *allowed_prot);
>   #else /* CONFIG_SECURITY */
>   
>   static inline int call_lsm_notifier(enum lsm_event event, void *data)
> diff --git a/security/security.c b/security/security.c
> index 613a5c00e602..07ed6763571e 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2359,3 +2359,11 @@ 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_load(struct vm_area_struct *vma, unsigned long prot,
> +			  unsigned long *allowed_prot)
> +{
> +	return call_int_hook(enclave_load, 0, vma, prot, allowed_prot);
> +}
> +#endif /* CONFIG_INTEL_SGX */
> 

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

[Sean Christopherson]

On Mon, Jun 03, 2019 at 10:19:18AM -0400, Stephen Smalley wrote:
> On 5/31/19 7:31 PM, 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, 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.  The notable
> >difference from file_mprotect() is the allowed_prot parameter, which
> >is essentially an SGX-specific version of a VMA's MAY_{READ,WRITE,EXEC}
> >flags.  The purpose of allowed_prot is to enable checks such as
> >SELinux's FILE__EXECMOD permission without having to track and update
> >VMAs across multiple mm structs, i.e. SGX can ensure userspace doesn't
> >overstep its bounds simply by restricting an enclave VMA's protections
> >by vetting what is maximally allowed during build time.
> >
> >An alternative to the allowed_prot approach would be to use an enclave's
> >SIGSTRUCT (a smallish structure that can uniquely identify an enclave)
> >as a proxy for the enclave.  For example, SGX could take and hold a
> >reference to the file containing the SIGSTRUCT (if it's in a file) and
> >call security_enclave_load() during mprotect().  While the SIGSTRUCT
> >approach would provide better precision, the actual value added was
> >deemed to be negligible.  On the other hand, pinning a file for the
> >lifetime of the enclave is ugly, and essentially caching LSM policies
> >in each page's allowed_prot avoids having to make an extra LSM upcall
> >during mprotect().
> >
> >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 | 14 +++++++++-----
> >  include/linux/lsm_hooks.h              | 16 ++++++++++++++++
> >  include/linux/security.h               |  2 ++
> >  security/security.c                    |  8 ++++++++
> >  4 files changed, 35 insertions(+), 5 deletions(-)
> >
> >diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> >index 5f71be7cbb01..260417ecbcff 100644
> >--- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> >+++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> >@@ -8,6 +8,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>
> >@@ -580,21 +581,24 @@ static int sgx_encl_page_protect(unsigned long src, unsigned long prot,
> >  				 unsigned long *allowed_prot)
> >  {
> >  	struct vm_area_struct *vma;
> >+	int ret = 0;
> >-	if (!(*allowed_prot & VM_EXEC))
> >+	if (!(*allowed_prot & VM_EXEC) && !IS_ENABLED(CONFIG_SECURITY))
> >  		goto do_check;
> >  	down_read(&current->mm->mmap_sem);
> >  	vma = find_vma(current->mm, src);
> >  	if (!vma || (vma->vm_file && path_noexec(&vma->vm_file->f_path)))
> >  		*allowed_prot &= ~VM_EXEC;
> >+#ifdef CONFIG_SECURITY
> >+	ret = security_enclave_load(vma, prot, allowed_prot);
> >+#endif
> 
> Normally you'd define a static inline stub for the hook in the #else clause
> for CONFIG_SECURITY in include/linux/security.h and avoid any ifdef here.

Ah, right.
 
> What ensures that the mapping referenced by src can't be changed to an
> entirely different one (with a different vm_file) between the time of check
> (here) and the time of use?

Nothing.  Holding mmap_sem across copy_from_user() would suffice, correct?

> >  	up_read(&current->mm->mmap_sem);
> >  do_check:
> >-	if (prot & ~*allowed_prot)
> >-		return -EACCES;
> >-
> >-	return 0;
> >+	if (!ret && (prot & ~*allowed_prot))
> >+		ret = -EACCES;
> >+	return ret;
> >  }
> >  static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,

Re: [RFC PATCH 9/9] security/selinux: Add enclave_load() implementation

[Stephen Smalley]

On 5/31/19 7:31 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).
> 
> The basic concept is to require appropriate execute permissions on the
> source of the enclave for pages that are requesting PROT_EXEC, e.g. if
> an enclave page is being loaded from a regular file, require
> FILE__EXECUTE and/or FILE__EXECMOND, and if it's coming from an
> anonymous/private mapping, require PROCESS__EXECMEM since the process
> is essentially executing from the mapping, albeit in a roundabout way.
> 
> Note, FILE__READ and FILE__WRITE are intentionally not required even if
> the source page is backed by a regular file.  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 | 85 ++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 85 insertions(+)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 3ec702cf46ca..f436a055dda7 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -6726,6 +6726,87 @@ static void selinux_bpf_prog_free(struct bpf_prog_aux *aux)
>   }
>   #endif
>   
> +#ifdef CONFIG_INTEL_SGX
> +int selinux_enclave_load(struct vm_area_struct *vma, unsigned long prot,
> +			 unsigned long *allowed_prot)
> +{
> +	const struct cred *cred = current_cred();
> +	u32 sid = cred_sid(cred);
> +	int rc;
> +
> +	/* SGX is supported only in 64-bit kernels. */
> +	WARN_ON_ONCE(!default_noexec);
> +
> +	/*
> +	 * SGX is responsible for checking @prot vs @allowed_prot, and SELinux
> +	 * only cares about execute related permissions for enclaves.
> +	 */
> +	if (!(*allowed_prot & PROT_EXEC))
> +		return 0;
> +
> +	/*
> +	 * Loading an executable enclave page from a VMA that is not executable
> +	 * itself requires EXECUTE permissions on the source file, or if there
> +	 * is no regular source file, EXECMEM since the page is being loaded
> +	 * from a non-executable anonymous mapping.
> +	 */
> +	if (!(vma->vm_flags & VM_EXEC)) {
> +		if (vma->vm_file && !IS_PRIVATE(file_inode(vma->vm_file)))
> +			rc = file_has_perm(cred, vma->vm_file, FILE__EXECUTE);

We might need an EXECMOD check here as well if (vma->vm_file && 
vma->anon_vma).  The scenario would be that the host application mapped 
the file with PROT_WRITE, modified it, but haven't mapped it PROT_EXEC. 
Now the enclave loader requests PROT_EXEC without PROT_WRITE or allows 
it.  FILE__EXECUTE is insufficient for this case.

> +		else
> +			rc = avc_has_perm(&selinux_state,
> +					  sid, sid, SECCLASS_PROCESS,
> +					  PROCESS__EXECMEM, NULL);

These calls will audit FILE__EXECUTE or PROCESS__EXECMEM denials even 
when userspace never asked for PROT_EXEC. Possibly we should use 
avc_has_perm_noaudit() and only call avc_audit() if (prot & PROT_EXEC)? 
And similarly introduce file_has_perm_noaudit() -> 
inode_has_perm_noaudit() -> avc_has_perm_noaudit() or inline here and 
switch to avc_has_perm_noaudit() throughout?

> +
> +		/*
> +		 * Reject the load if the enclave *needs* the page to be
> +		 * executable, otherwise prevent it from becoming executable.
> +		 */
> +		if (rc) {
> +			if (prot & PROT_EXEC)
> +				return rc;
> +
> +			*allowed_prot &= ~PROT_EXEC;
> +		}
> +	}
> +
> +	/*
> +	 * An enclave page that may do RW->RX or W+X requires EXECMOD (backed
> +	 * by a regular file) or EXECMEM (loaded from an anonymous mapping).

At present EXECMEM is also triggered for W+X private file mappings, to 
allow denying W+X while permitting exceptions for W->X for text relocations.

> +	 * Note, this hybrid EXECMOD and EXECMEM behavior is intentional and
> +	 * reflects the nature of enclaves and the EPC, e.g. EPC is effectively
> +	 * a non-persistent shared file, but each enclave is a private domain
> +	 * within that shared file, so delegate to the source of the enclave.
> +	 */
> +	if ((*allowed_prot & PROT_EXEC) && (*allowed_prot & PROT_WRITE)) {
> +		if (vma->vm_file && !IS_PRIVATE(file_inode(vma->vm_file)))
> +			rc = file_has_perm(cred, vma->vm_file, FILE__EXECMOD);
> +		else
> +			rc = avc_has_perm(&selinux_state,
> +					  sid, sid, SECCLASS_PROCESS,
> +					  PROCESS__EXECMEM, NULL);

Same issue wrt auditing here.  Could also potentially skip the EXECMEM 
check this time if we performed it above (if so, then we must have 
passed it because *allowed_prot still had PROT_EXEC set).

> +		/*
> +		 * Clear ALLOW_EXEC instead of ALLOWED_WRITE if permissions are
> +		 * lacking and @prot has neither PROT_WRITE or PROT_EXEC.  If
> +		 * userspace wanted RX they would have requested RX, and due to
> +		 * lack of permissions they can never get RW->RX, i.e. the only
> +		 * useful transition is R->RW.
> +		 */
> +		if (rc) {
> +			if ((prot & PROT_EXEC) && (prot & PROT_WRITE))
> +				return rc;
> +
> +			if (prot & PROT_EXEC)
> +				*allowed_prot &= ~PROT_WRITE;
> +			else
> +				*allowed_prot &= ~PROT_EXEC;
> +		}
> +	}
> +
> +	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 +7049,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_load, selinux_enclave_load),
> +#endif
>   };
>   
>   static __init int selinux_init(void)
> 

Re: [RFC PATCH 9/9] security/selinux: Add enclave_load() implementation

[Sean Christopherson]

On Mon, Jun 03, 2019 at 11:01:32AM -0400, Stephen Smalley wrote:
> On 5/31/19 7:31 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).
> >
> >The basic concept is to require appropriate execute permissions on the
> >source of the enclave for pages that are requesting PROT_EXEC, e.g. if
> >an enclave page is being loaded from a regular file, require
> >FILE__EXECUTE and/or FILE__EXECMOND, and if it's coming from an
> >anonymous/private mapping, require PROCESS__EXECMEM since the process
> >is essentially executing from the mapping, albeit in a roundabout way.
> >
> >Note, FILE__READ and FILE__WRITE are intentionally not required even if
> >the source page is backed by a regular file.  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 | 85 ++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 85 insertions(+)
> >
> >diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> >index 3ec702cf46ca..f436a055dda7 100644
> >--- a/security/selinux/hooks.c
> >+++ b/security/selinux/hooks.c
> >@@ -6726,6 +6726,87 @@ static void selinux_bpf_prog_free(struct bpf_prog_aux *aux)
> >  }
> >  #endif
> >+#ifdef CONFIG_INTEL_SGX
> >+int selinux_enclave_load(struct vm_area_struct *vma, unsigned long prot,
> >+			 unsigned long *allowed_prot)
> >+{
> >+	const struct cred *cred = current_cred();
> >+	u32 sid = cred_sid(cred);
> >+	int rc;
> >+
> >+	/* SGX is supported only in 64-bit kernels. */
> >+	WARN_ON_ONCE(!default_noexec);
> >+
> >+	/*
> >+	 * SGX is responsible for checking @prot vs @allowed_prot, and SELinux
> >+	 * only cares about execute related permissions for enclaves.
> >+	 */
> >+	if (!(*allowed_prot & PROT_EXEC))
> >+		return 0;
> >+
> >+	/*
> >+	 * Loading an executable enclave page from a VMA that is not executable
> >+	 * itself requires EXECUTE permissions on the source file, or if there
> >+	 * is no regular source file, EXECMEM since the page is being loaded
> >+	 * from a non-executable anonymous mapping.
> >+	 */
> >+	if (!(vma->vm_flags & VM_EXEC)) {
> >+		if (vma->vm_file && !IS_PRIVATE(file_inode(vma->vm_file)))
> >+			rc = file_has_perm(cred, vma->vm_file, FILE__EXECUTE);
> 
> We might need an EXECMOD check here as well if (vma->vm_file &&
> vma->anon_vma).  The scenario would be that the host application mapped the
> file with PROT_WRITE, modified it, but haven't mapped it PROT_EXEC. Now the
> enclave loader requests PROT_EXEC without PROT_WRITE or allows it.
> FILE__EXECUTE is insufficient for this case.

Ya, agreed.

> >+		else
> >+			rc = avc_has_perm(&selinux_state,
> >+					  sid, sid, SECCLASS_PROCESS,
> >+					  PROCESS__EXECMEM, NULL);
> 
> These calls will audit FILE__EXECUTE or PROCESS__EXECMEM denials even when
> userspace never asked for PROT_EXEC. Possibly we should use
> avc_has_perm_noaudit() and only call avc_audit() if (prot & PROT_EXEC)? And
> similarly introduce file_has_perm_noaudit() -> inode_has_perm_noaudit() ->
> avc_has_perm_noaudit() or inline here and switch to avc_has_perm_noaudit()
> throughout?

Hmm, I think we want to audit the denials even if userspace technically
hasn't requested PROT_EXEC, yet...  The idea of @allowed_prot is for
userspace to declare its intent, i.e. for all intents and purposes,
userspace is asking for PROT_EXEC by declaring PROT_EXEC in @allowed_prot.

For SGX1 enclaves, where permissions are fixed at enclave build time,
declaring PROT_EXEC but not actually mapping the page as PROT_EXEC is a
userspace bug, and a fairly egregious one at that.

For SGX2 enclaves, where enclaves can convert RW->RX (among others), not
auditing would make it difficult for userspace to debug failures due to
the denial.  E.g. a properly written application that declared PROT_EXEC
on a RW page with the intent of converting it to RX would eventually fail
at mprotect() due to PROT_EXEC being cleared.  Without the audit, there
wouldn't be any indication that mprotect() failed with -EACCES due to LSM
restrictions.

> >+
> >+		/*
> >+		 * Reject the load if the enclave *needs* the page to be
> >+		 * executable, otherwise prevent it from becoming executable.
> >+		 */
> >+		if (rc) {
> >+			if (prot & PROT_EXEC)
> >+				return rc;
> >+
> >+			*allowed_prot &= ~PROT_EXEC;
> >+		}
> >+	}
> >+
> >+	/*
> >+	 * An enclave page that may do RW->RX or W+X requires EXECMOD (backed
> >+	 * by a regular file) or EXECMEM (loaded from an anonymous mapping).
> 
> At present EXECMEM is also triggered for W+X private file mappings, to allow
> denying W+X while permitting exceptions for W->X for text relocations.

Doh, missed that one.  Thanks!

> >+	 * Note, this hybrid EXECMOD and EXECMEM behavior is intentional and
> >+	 * reflects the nature of enclaves and the EPC, e.g. EPC is effectively
> >+	 * a non-persistent shared file, but each enclave is a private domain
> >+	 * within that shared file, so delegate to the source of the enclave.
> >+	 */
> >+	if ((*allowed_prot & PROT_EXEC) && (*allowed_prot & PROT_WRITE)) {
> >+		if (vma->vm_file && !IS_PRIVATE(file_inode(vma->vm_file)))
> >+			rc = file_has_perm(cred, vma->vm_file, FILE__EXECMOD);
> >+		else
> >+			rc = avc_has_perm(&selinux_state,
> >+					  sid, sid, SECCLASS_PROCESS,
> >+					  PROCESS__EXECMEM, NULL);
> 
> Same issue wrt auditing here.  Could also potentially skip the EXECMEM check
> this time if we performed it above (if so, then we must have passed it
> because *allowed_prot still had PROT_EXEC set).

Skipping the second EXECMEM check crossed my mind as well.  I'll play with
the code to see if I can come up with clean way of avoiding multiple
EXECMEM (or EXECMOD) checks.

> >+		/*
> >+		 * Clear ALLOW_EXEC instead of ALLOWED_WRITE if permissions are
> >+		 * lacking and @prot has neither PROT_WRITE or PROT_EXEC.  If
> >+		 * userspace wanted RX they would have requested RX, and due to
> >+		 * lack of permissions they can never get RW->RX, i.e. the only
> >+		 * useful transition is R->RW.
> >+		 */
> >+		if (rc) {
> >+			if ((prot & PROT_EXEC) && (prot & PROT_WRITE))
> >+				return rc;
> >+
> >+			if (prot & PROT_EXEC)
> >+				*allowed_prot &= ~PROT_WRITE;
> >+			else
> >+				*allowed_prot &= ~PROT_EXEC;
> >+		}
> >+	}
> >+
> >+	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 +7049,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_load, selinux_enclave_load),
> >+#endif
> >  };
> >  static __init int selinux_init(void)
> >
> 

Re: [RFC PATCH 0/9] security: x86/sgx: SGX vs. LSM

[Sean Christopherson]

On Sun, Jun 02, 2019 at 12:29:35AM -0700, Xing, Cedric wrote:
> Hi Sean,
> 
> > From: Christopherson, Sean J
> > Sent: Friday, May 31, 2019 4:32 PM
> > 
> > This series is the result of a rather absurd amount of discussion over how to get SGX to play
> > nice with LSM policies, without having to resort to evil shenanigans or put undue burden on
> > userspace.  The discussion definitely wandered into completely insane territory at times, but
> > I think/hope we ended up with something reasonable.
> > 
> > The basic gist of the approach is to require userspace to declare what protections are
> > maximally allowed for any given page, e.g. add a flags field for loading enclave pages that
> > takes ALLOW_{READ,WRITE,EXEC}.  LSMs can then adjust the allowed protections, e.g. clear
> > ALLOW_EXEC to prevent ever mapping the page with PROT_EXEC.  SGX enforces the allowed perms
> > via a new mprotect() vm_ops hook, e.g. like regular mprotect() uses MAY_{READ,WRITE,EXEC}.
> > 
> > ALLOW_EXEC is used to deny hings like loading an enclave from a noexec file system or from a
> > file without EXECUTE permissions, e.g. without the ALLOW_EXEC concept, on SGX2 hardware
> > (regardless of kernel support) userspace could EADD from a noexec file using read-only
> > permissions, and later use mprotect() and ENCLU[EMODPE] to gain execute permissions.
> > 
> > ALLOW_WRITE is used in conjuction with ALLOW_EXEC to enforce SELinux's EXECMOD (or EXECMEM).
> > 
> > This is very much an RFC series.  It's only compile tested, likely has obvious bugs, the
> > SELinux patch could be completely harebrained, etc...
> > My goal at this point is to get feedback at a macro level, e.g. is the core concept
> > viable/acceptable, are there objection to hooking mprotect(), etc...
> > 
> > Andy and Cedric, hopefully this aligns with your general expectations based on our last
> > discussion.
> 
> I couldn't understand the real intentions of ALLOW_* flags until I saw them
> in code. I have to say C is more expressive than English in that regard :)
> 
> Generally I agree with your direction but think ALLOW_* flags are completely
> internal to LSM because they can be both produced and consumed inside an LSM
> module. So spilling them into SGX driver and also user mode code makes the
> solution ugly and in some cases impractical because not every enclave host
> process has a priori knowledge on whether or not an enclave page would be
> EMODPE'd at runtime.

In this case, the host process should tag *all* pages it *might* convert
to executable as ALLOW_EXEC.  LSMs can (and should/will) be written in
such a way that denying ALLOW_EXEC is fatal to the enclave if and only if
the enclave actually attempts mprotect(PROT_EXEC).

Take the SELinux path for example.  The only scenario in which PROT_WRITE
is cleared from @allowed_prot is if the page *starts* with PROT_EXEC.
If PROT_EXEC is denied on a page that starts RW, e.g. an EAUG'd page,
then PROT_EXEC will be cleared from @allowed_prot.

As Stephen pointed out, auditing the denials on @allowed_prot means the
log will contain false positives of a sort.  But this is more of a noise
issue than true false positives.  E.g. there are three possible outcomes
for the enclave.

  - The enclave does not do EMODPE[PROT_EXEC] in any scenario, ever.
    Requesting ALLOW_EXEC is either a straightforward a userspace bug or
    a poorly written generic enclave loader.

  - The enclave conditionally performs EMODPE[PROT_EXEC].  In this case
    the denial is a true false positive.
  
  - The enclave does EMODPE[PROT_EXEC] and its host userspace then fails
    on mprotect(PROT_EXEC), i.e. the LSM denial is working as intended.
    The audit log will be noisy, but viewed as a whole the denials aren't
    false positives.

The potential for noisy audit logs and/or false positives is unfortunate,
but it's (by far) the lesser of many evils.

> Theoretically speaking, what you really need is a per page flag (let's name
> it WRITTEN?) indicating whether a page has ever been written to (or more
> precisely, granted PROT_WRITE), which will be used to decide whether to grant
> PROT_EXEC when requested in future. Given the fact that all mprotect() goes
> through LSM and mmap() is limited to PROT_NONE, it's easy for LSM to capture
> that flag by itself instead of asking user mode code to provide it.
>
> That said, here is the summary of what I think is a better approach.
> * In hook security_file_alloc(), if @file is an enclave, allocate some data
>   structure to store for every page, the WRITTEN flag as described above.
>   WRITTEN is cleared initially for all pages.

This would effectively require *every* LSM to duplicate the SGX driver's
functionality, e.g. track per-page metadata, implement locking to prevent
races between multiple mm structs, etc...

>   Open: Given a file of type struct file *, how to tell if it is an enclave (i.e. /dev/sgx/enclave)?
> * In hook security_mmap_file(), if @file is an enclave, make sure @prot can
>   only be PROT_NONE. This is to force all protection changes to go through
>   security_file_mprotect().
> * In the newly introduced hook security_enclave_load(), set WRITTEN for pages
>   that are requested PROT_WRITE.

How would an LSM associate a page with a specific enclave?  vma->vm_file
will point always point at /dev/sgx/enclave.  vma->vm_mm is useless
because we're allowing multiple processes to map a single enclave, not to
mention that by mm would require holding a reference to the mm.

> * In hook security_file_mprotect(), if @vma->vm_file is an enclave, look up
>   and use WRITTEN flags for all pages within @vma, along with other global
>   flags (e.g. PROCESS__EXECMEM/FILE__EXECMOD in the case of SELinux) to decide
>   on allowing/rejecting @prot.

vma->vm_file will always be /dev/sgx/enclave at this point, which means
LSMs don't have the necessary anchor back to the source file, e.g. to
enforce FILE__EXECUTE.  The noexec file system case is also unaddressed.

> * In hook security_file_free(), if @file is an  enclave, free storage
>   allocated for WRITTEN flags.

Re: [RFC PATCH 0/9] security: x86/sgx: SGX vs. LSM

[Stephen Smalley]

On 6/2/19 3:29 AM, Xing, Cedric wrote:
> Hi Sean,
> 
>> From: Christopherson, Sean J
>> Sent: Friday, May 31, 2019 4:32 PM
>>
>> This series is the result of a rather absurd amount of discussion over how to get SGX to play
>> nice with LSM policies, without having to resort to evil shenanigans or put undue burden on
>> userspace.  The discussion definitely wandered into completely insane territory at times, but
>> I think/hope we ended up with something reasonable.
>>
>> The basic gist of the approach is to require userspace to declare what protections are
>> maximally allowed for any given page, e.g. add a flags field for loading enclave pages that
>> takes ALLOW_{READ,WRITE,EXEC}.  LSMs can then adjust the allowed protections, e.g. clear
>> ALLOW_EXEC to prevent ever mapping the page with PROT_EXEC.  SGX enforces the allowed perms
>> via a new mprotect() vm_ops hook, e.g. like regular mprotect() uses MAY_{READ,WRITE,EXEC}.
>>
>> ALLOW_EXEC is used to deny hings like loading an enclave from a noexec file system or from a
>> file without EXECUTE permissions, e.g. without the ALLOW_EXEC concept, on SGX2 hardware
>> (regardless of kernel support) userspace could EADD from a noexec file using read-only
>> permissions, and later use mprotect() and ENCLU[EMODPE] to gain execute permissions.
>>
>> ALLOW_WRITE is used in conjuction with ALLOW_EXEC to enforce SELinux's EXECMOD (or EXECMEM).
>>
>> This is very much an RFC series.  It's only compile tested, likely has obvious bugs, the
>> SELinux patch could be completely harebrained, etc...
>> My goal at this point is to get feedback at a macro level, e.g. is the core concept
>> viable/acceptable, are there objection to hooking mprotect(), etc...
>>
>> Andy and Cedric, hopefully this aligns with your general expectations based on our last
>> discussion.
> 
> I couldn't understand the real intentions of ALLOW_* flags until I saw them in code. I have to say C is more expressive than English in that regard :)
> 
> Generally I agree with your direction but think ALLOW_* flags are completely internal to LSM because they can be both produced and consumed inside an LSM module. So spilling them into SGX driver and also user mode code makes the solution ugly and in some cases impractical because not every enclave host process has a priori knowledge on whether or not an enclave page would be EMODPE'd at runtime.
> 
> Theoretically speaking, what you really need is a per page flag (let's name it WRITTEN?) indicating whether a page has ever been written to (or more precisely, granted PROT_WRITE), which will be used to decide whether to grant PROT_EXEC when requested in future. Given the fact that all mprotect() goes through LSM and mmap() is limited to PROT_NONE, it's easy for LSM to capture that flag by itself instead of asking user mode code to provide it.
> 
> That said, here is the summary of what I think is a better approach.
> * In hook security_file_alloc(), if @file is an enclave, allocate some data structure to store for every page, the WRITTEN flag as described above. WRITTEN is cleared initially for all pages.
>    Open: Given a file of type struct file *, how to tell if it is an enclave (i.e. /dev/sgx/enclave)?
> * In hook security_mmap_file(), if @file is an enclave, make sure @prot can only be PROT_NONE. This is to force all protection changes to go through security_file_mprotect().
> * In the newly introduced hook security_enclave_load(), set WRITTEN for pages that are requested PROT_WRITE.
> * In hook security_file_mprotect(), if @vma->vm_file is an enclave, look up and use WRITTEN flags for all pages within @vma, along with other global flags (e.g. PROCESS__EXECMEM/FILE__EXECMOD in the case of SELinux) to decide on 
allowing/rejecting @prot.

At this point we have no knowledge of the source vma/file, right?  So 
what do we check FILE__EXECUTE and/or FILE__EXECMOD against? 
vma->vm_file at this point is /dev/sgx/enclave, right?

> * In hook security_file_free(), if @file is an enclave, free storage allocated for WRITTEN flags.
> 
> I'll try to make more detailed comments in my replies to individual patches sometime tomorrow.
> 
>>
>> Lastly, I added a patch to allow userspace to add multiple pages in a single ioctl().  It's
>> obviously not directly related to the security stuff, but the idea tangentially came up during
>> earlier discussions and it's something I think the UAPI should provide (it's a tiny change).
>> Since I was modifying the UAPI anyways, I threw it in.
>>
>> Sean Christopherson (9):
>>    x86/sgx: Remove unused local variable in sgx_encl_release()
>>    x86/sgx: Do not naturally align MAP_FIXED address
>>    x86/sgx: Allow userspace to add multiple pages in single ioctl()
>>    mm: Introduce vm_ops->mprotect()
>>    x86/sgx: Restrict mapping without an enclave page to PROT_NONE
>>    x86/sgx: Require userspace to provide allowed prots to ADD_PAGES
>>    x86/sgx: Enforce noexec filesystem restriction for enclaves
>>    LSM: x86/sgx: Introduce ->enclave_load() hook for Intel SGX
>>    security/selinux: Add enclave_load() implementation
>>
>>   arch/x86/include/uapi/asm/sgx.h        |  30 ++++--
>>   arch/x86/kernel/cpu/sgx/driver/ioctl.c | 143 +++++++++++++++++--------
>> arch/x86/kernel/cpu/sgx/driver/main.c  |  13 ++-
>>   arch/x86/kernel/cpu/sgx/encl.c         |  31 +++++-
>>   arch/x86/kernel/cpu/sgx/encl.h         |   4 +
>>   include/linux/lsm_hooks.h              |  16 +++
>>   include/linux/mm.h                     |   2 +
>>   include/linux/security.h               |   2 +
>>   mm/mprotect.c                          |  15 ++-
>>   security/security.c                    |   8 ++
>>   security/selinux/hooks.c               |  85 +++++++++++++++
>>   11 files changed, 290 insertions(+), 59 deletions(-)
>>
>> --
>> 2.21.0
> 
> -Cedric
> 

RE: [RFC PATCH 0/9] security: x86/sgx: SGX vs. LSM

[Xing, Cedric]

> From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx-
> owner@vger.kernel.org] On Behalf Of Stephen Smalley
> Sent: Monday, June 03, 2019 10:47 AM
> 
> On 6/2/19 3:29 AM, Xing, Cedric wrote:
> > Hi Sean,
> >
> >> From: Christopherson, Sean J
> >> Sent: Friday, May 31, 2019 4:32 PM
> >>
> >> This series is the result of a rather absurd amount of discussion
> >> over how to get SGX to play nice with LSM policies, without having to
> >> resort to evil shenanigans or put undue burden on userspace.  The
> >> discussion definitely wandered into completely insane territory at
> times, but I think/hope we ended up with something reasonable.
> >>
> >> The basic gist of the approach is to require userspace to declare
> >> what protections are maximally allowed for any given page, e.g. add a
> >> flags field for loading enclave pages that takes
> >> ALLOW_{READ,WRITE,EXEC}.  LSMs can then adjust the allowed
> >> protections, e.g. clear ALLOW_EXEC to prevent ever mapping the page
> with PROT_EXEC.  SGX enforces the allowed perms via a new mprotect()
> vm_ops hook, e.g. like regular mprotect() uses MAY_{READ,WRITE,EXEC}.
> >>
> >> ALLOW_EXEC is used to deny hings like loading an enclave from a
> >> noexec file system or from a file without EXECUTE permissions, e.g.
> >> without the ALLOW_EXEC concept, on SGX2 hardware (regardless of
> >> kernel support) userspace could EADD from a noexec file using read-
> only permissions, and later use mprotect() and ENCLU[EMODPE] to gain
> execute permissions.
> >>
> >> ALLOW_WRITE is used in conjuction with ALLOW_EXEC to enforce
> SELinux's EXECMOD (or EXECMEM).
> >>
> >> This is very much an RFC series.  It's only compile tested, likely
> >> has obvious bugs, the SELinux patch could be completely harebrained,
> etc...
> >> My goal at this point is to get feedback at a macro level, e.g. is
> >> the core concept viable/acceptable, are there objection to hooking
> mprotect(), etc...
> >>
> >> Andy and Cedric, hopefully this aligns with your general expectations
> >> based on our last discussion.
> >
> > I couldn't understand the real intentions of ALLOW_* flags until I saw
> > them in code. I have to say C is more expressive than English in that
> > regard :)
> >
> > Generally I agree with your direction but think ALLOW_* flags are
> completely internal to LSM because they can be both produced and
> consumed inside an LSM module. So spilling them into SGX driver and also
> user mode code makes the solution ugly and in some cases impractical
> because not every enclave host process has a priori knowledge on whether
> or not an enclave page would be EMODPE'd at runtime.
> >
> > Theoretically speaking, what you really need is a per page flag (let's
> name it WRITTEN?) indicating whether a page has ever been written to (or
> more precisely, granted PROT_WRITE), which will be used to decide
> whether to grant PROT_EXEC when requested in future. Given the fact that
> all mprotect() goes through LSM and mmap() is limited to PROT_NONE, it's
> easy for LSM to capture that flag by itself instead of asking user mode
> code to provide it.
> >
> > That said, here is the summary of what I think is a better approach.
> > * In hook security_file_alloc(), if @file is an enclave, allocate some
> data structure to store for every page, the WRITTEN flag as described
> above. WRITTEN is cleared initially for all pages.
> >    Open: Given a file of type struct file *, how to tell if it is an
> enclave (i.e. /dev/sgx/enclave)?
> > * In hook security_mmap_file(), if @file is an enclave, make sure
> @prot can only be PROT_NONE. This is to force all protection changes to
> go through security_file_mprotect().
> > * In the newly introduced hook security_enclave_load(), set WRITTEN
> for pages that are requested PROT_WRITE.
> > * In hook security_file_mprotect(), if @vma->vm_file is an enclave,
> > look up and use WRITTEN flags for all pages within @vma, along with
> > other global flags (e.g. PROCESS__EXECMEM/FILE__EXECMOD in the case of
> > SELinux) to decide on
> allowing/rejecting @prot.
> 
> At this point we have no knowledge of the source vma/file, right?  So
> what do we check FILE__EXECUTE and/or FILE__EXECMOD against?
> vma->vm_file at this point is /dev/sgx/enclave, right?

My apology to the confusions here.

Yes, vma->vm_file is always /dev/sgx/enclave, but each open("/dev/sgx/enclave") returns a *new* file struct (let's denote it as @enclave_fd) that uniquely identifies one enclave instance, and the expectation is that @enclave_fd->f_security would be used by LSM to store enclave specific information, including ALLOW_* flags and whatever deemed appropriate by an LSM module.

In the case of SELinux, and if the choice is to use FILE__EXECMOD of .sigstruct file to authorize RW->RX at runtime, then SELinux could cache that flag in @enclave_fd->f_security upon security_enclave_init().

RE: [RFC PATCH 0/9] security: x86/sgx: SGX vs. LSM

[Xing, Cedric]

> From: Christopherson, Sean J
> Sent: Monday, June 03, 2019 10:16 AM
> 
> On Sun, Jun 02, 2019 at 12:29:35AM -0700, Xing, Cedric wrote:
> > Hi Sean,
> >
> > Generally I agree with your direction but think ALLOW_* flags are
> > completely internal to LSM because they can be both produced and
> > consumed inside an LSM module. So spilling them into SGX driver and
> > also user mode code makes the solution ugly and in some cases
> > impractical because not every enclave host process has a priori
> > knowledge on whether or not an enclave page would be EMODPE'd at
> runtime.
> 
> In this case, the host process should tag *all* pages it *might* convert
> to executable as ALLOW_EXEC.  LSMs can (and should/will) be written in
> such a way that denying ALLOW_EXEC is fatal to the enclave if and only
> if the enclave actually attempts mprotect(PROT_EXEC).

What if those pages contain self-modifying code but the host doesn't know ahead of time? Would it require ALLOW_WRITE|ALLOW_EXEC at EADD? Then would it prevent those pages to start with PROT_EXEC?

Anyway, my point is that it is unnecessary even if it works.

> 
> Take the SELinux path for example.  The only scenario in which
> PROT_WRITE is cleared from @allowed_prot is if the page *starts* with
> PROT_EXEC.
> If PROT_EXEC is denied on a page that starts RW, e.g. an EAUG'd page,
> then PROT_EXEC will be cleared from @allowed_prot.
> 
> As Stephen pointed out, auditing the denials on @allowed_prot means the
> log will contain false positives of a sort.  But this is more of a noise
> issue than true false positives.  E.g. there are three possible outcomes
> for the enclave.
> 
>   - The enclave does not do EMODPE[PROT_EXEC] in any scenario, ever.
>     Requesting ALLOW_EXEC is either a straightforward a userspace bug or
>     a poorly written generic enclave loader.
> 
>   - The enclave conditionally performs EMODPE[PROT_EXEC].  In this case
>     the denial is a true false positive.
> 
>   - The enclave does EMODPE[PROT_EXEC] and its host userspace then fails
>     on mprotect(PROT_EXEC), i.e. the LSM denial is working as intended.
>     The audit log will be noisy, but viewed as a whole the denials
> aren't
>     false positives.

What I was talking about was EMODPE[PROT_WRITE] on an RX page.

> 
> The potential for noisy audit logs and/or false positives is unfortunate,
> but it's (by far) the lesser of many evils.
> 
> > Theoretically speaking, what you really need is a per page flag (let's
> > name it WRITTEN?) indicating whether a page has ever been written to
> > (or more precisely, granted PROT_WRITE), which will be used to decide
> > whether to grant PROT_EXEC when requested in future. Given the fact
> > that all mprotect() goes through LSM and mmap() is limited to
> > PROT_NONE, it's easy for LSM to capture that flag by itself instead of
> asking user mode code to provide it.
> >
> > That said, here is the summary of what I think is a better approach.
> > * In hook security_file_alloc(), if @file is an enclave, allocate some
> data
> >   structure to store for every page, the WRITTEN flag as described
> above.
> >   WRITTEN is cleared initially for all pages.
> 
> This would effectively require *every* LSM to duplicate the SGX driver's
> functionality, e.g. track per-page metadata, implement locking to
> prevent races between multiple mm structs, etc...

Architecturally we shouldn't dictate how LSM makes decisions. ALLOW_* are no difference than PROCESS__* or FILE__* flags, which are just artifacts to assist particular LSMs in decision making. They are never considered part of the LSM interface, even if other LSMs than SELinux may adopt the same/similar approach.

If code duplication is what you are worrying about, you can put them in a library, or implement/export them in some new file (maybe security/enclave.c?) as utility functions. But spilling them into user mode is what I think is unacceptable.

> 
> >   Open: Given a file of type struct file *, how to tell if it is an
> enclave (i.e. /dev/sgx/enclave)?
> > * In hook security_mmap_file(), if @file is an enclave, make sure
> @prot can
> >   only be PROT_NONE. This is to force all protection changes to go
> through
> >   security_file_mprotect().
> > * In the newly introduced hook security_enclave_load(), set WRITTEN
> for pages
> >   that are requested PROT_WRITE.
> 
> How would an LSM associate a page with a specific enclave?  vma->vm_file
> will point always point at /dev/sgx/enclave.  vma->vm_mm is useless
> because we're allowing multiple processes to map a single enclave, not
> to mention that by mm would require holding a reference to the mm.

Each open("/dev/sgx/enclave") syscall creates a *new* instance of struct file to uniquely identify one enclave instance. What I mean is @vma->vm_file, not @vma->vm_file->f_path or @vma->vm_file->f_inode.

> 
> > * In hook security_file_mprotect(), if @vma->vm_file is an enclave,
> look up
> >   and use WRITTEN flags for all pages within @vma, along with other
> global
> >   flags (e.g. PROCESS__EXECMEM/FILE__EXECMOD in the case of SELinux)
> to decide
> >   on allowing/rejecting @prot.
> 
> vma->vm_file will always be /dev/sgx/enclave at this point, which means
> LSMs don't have the necessary anchor back to the source file, e.g. to
> enforce FILE__EXECUTE.  The noexec file system case is also unaddressed.

vma->vm_file identifies an enclave instance uniquely. FILE__EXECUTE is checked by security_enclave_load() using @source_vma->vm_file. Once a page has been EADD'ed, whether to allow RW->RX depends on .sigstruct file (more precisely, the file backing SIGSTRUCT), whose FILE__* attributes could be cached in vma->vm_file->f_security by security_enclave_init().
 
The noexec case should be addressed in IOC_ADD_PAGES by testing @source_vma->vm_flags & VM_MAYEXEC.

> 
> > * In hook security_file_free(), if @file is an  enclave, free storage
> >   allocated for WRITTEN flags.

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

[Stephen Smalley]

On 6/3/19 10:42 AM, Sean Christopherson wrote:
> On Mon, Jun 03, 2019 at 10:19:18AM -0400, Stephen Smalley wrote:
>> On 5/31/19 7:31 PM, 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, 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.  The notable
>>> difference from file_mprotect() is the allowed_prot parameter, which
>>> is essentially an SGX-specific version of a VMA's MAY_{READ,WRITE,EXEC}
>>> flags.  The purpose of allowed_prot is to enable checks such as
>>> SELinux's FILE__EXECMOD permission without having to track and update
>>> VMAs across multiple mm structs, i.e. SGX can ensure userspace doesn't
>>> overstep its bounds simply by restricting an enclave VMA's protections
>>> by vetting what is maximally allowed during build time.
>>>
>>> An alternative to the allowed_prot approach would be to use an enclave's
>>> SIGSTRUCT (a smallish structure that can uniquely identify an enclave)
>>> as a proxy for the enclave.  For example, SGX could take and hold a
>>> reference to the file containing the SIGSTRUCT (if it's in a file) and
>>> call security_enclave_load() during mprotect().  While the SIGSTRUCT
>>> approach would provide better precision, the actual value added was
>>> deemed to be negligible.  On the other hand, pinning a file for the
>>> lifetime of the enclave is ugly, and essentially caching LSM policies
>>> in each page's allowed_prot avoids having to make an extra LSM upcall
>>> during mprotect().
>>>
>>> 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 | 14 +++++++++-----
>>>   include/linux/lsm_hooks.h              | 16 ++++++++++++++++
>>>   include/linux/security.h               |  2 ++
>>>   security/security.c                    |  8 ++++++++
>>>   4 files changed, 35 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
>>> index 5f71be7cbb01..260417ecbcff 100644
>>> --- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
>>> +++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
>>> @@ -8,6 +8,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>
>>> @@ -580,21 +581,24 @@ static int sgx_encl_page_protect(unsigned long src, unsigned long prot,
>>>   				 unsigned long *allowed_prot)
>>>   {
>>>   	struct vm_area_struct *vma;
>>> +	int ret = 0;
>>> -	if (!(*allowed_prot & VM_EXEC))
>>> +	if (!(*allowed_prot & VM_EXEC) && !IS_ENABLED(CONFIG_SECURITY))
>>>   		goto do_check;
>>>   	down_read(&current->mm->mmap_sem);
>>>   	vma = find_vma(current->mm, src);
>>>   	if (!vma || (vma->vm_file && path_noexec(&vma->vm_file->f_path)))
>>>   		*allowed_prot &= ~VM_EXEC;
>>> +#ifdef CONFIG_SECURITY
>>> +	ret = security_enclave_load(vma, prot, allowed_prot);
>>> +#endif
>>
>> Normally you'd define a static inline stub for the hook in the #else clause
>> for CONFIG_SECURITY in include/linux/security.h and avoid any ifdef here.
> 
> Ah, right.
>   
>> What ensures that the mapping referenced by src can't be changed to an
>> entirely different one (with a different vm_file) between the time of check
>> (here) and the time of use?
> 
> Nothing.  Holding mmap_sem across copy_from_user() would suffice, correct?

I don't believe you can do that; copy_from_user() could stall 
indefinitely.  Not sure how to do what you want here or if it requires 
changing the interface.

> 
>>>   	up_read(&current->mm->mmap_sem);
>>>   do_check:
>>> -	if (prot & ~*allowed_prot)
>>> -		return -EACCES;
>>> -
>>> -	return 0;
>>> +	if (!ret && (prot & ~*allowed_prot))
>>> +		ret = -EACCES;
>>> +	return ret;
>>>   }
>>>   static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,

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

[Dave Hansen]

...
>>> What ensures that the mapping referenced by src can't be changed
>>> to an entirely different one (with a different vm_file) between
>>> the time of check (here) and the time of use?
>> 
>> Nothing.  Holding mmap_sem across copy_from_user() would suffice, 
>> correct?
> 
> I don't believe you can do that; copy_from_user() could stall 
> indefinitely.  Not sure how to do what you want here or if it requires
> changing the interface.

Holding mmap_sem for *read* is OK since you can handle page faults
underneath it.  Holding it for write is not.

But, holding it for read also locks out the writers which might be
messing with vm_file or other parts of the VMA.

Holding it for read for a long time is OK.  It's obviously not ideal,
but it is something we do widely today.

Re: [RFC PATCH 3/9] x86/sgx: Allow userspace to add multiple pages in single ioctl()

[Sean Christopherson]

On Sun, Jun 02, 2019 at 11:26:09PM -0700, Xing, Cedric wrote:
> > From: Christopherson, Sean J
> > Sent: Friday, May 31, 2019 4:32 PM
> > 
> > +/**
> > + * sgx_ioc_enclave_add_pages - handler for %SGX_IOC_ENCLAVE_ADD_PAGES
> > + *
> > + * @filep:	open file to /dev/sgx
> > + * @cmd:	the command value
> > + * @arg:	pointer to an &sgx_enclave_add_page instance
> > + *
> > + * Add a range of pages to an uninitialized enclave (EADD), and
> > +optionally
> > + * extend the enclave's measurement with the contents of the page (EEXTEND).
> > + * The range of pages must be virtually contiguous.  The SECINFO and
> > + * measurement maskare applied to all pages, i.e. pages with different
> > + * properties must be added in separate calls.
> > + *
> > + * EADD and EEXTEND are done asynchronously via worker threads.  A
> > +successful
> > + * sgx_ioc_enclave_add_page() only indicates the pages have been added
> > +to the
> > + * work queue, it does not guarantee adding the pages to the enclave
> > +will
> > + * succeed.
> > + *
> > + * Return:
> > + *   0 on success,
> > + *   -errno otherwise
> > + */
> > +static long sgx_ioc_enclave_add_pages(struct file *filep, unsigned int cmd,
> > +				      unsigned long arg)
> > +{
> > +	struct sgx_enclave_add_pages *addp = (void *)arg;
> > +	struct sgx_encl *encl = filep->private_data;
> > +	struct sgx_secinfo secinfo;
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	if (copy_from_user(&secinfo, (void __user *)addp->secinfo,
> > +			   sizeof(secinfo)))
> > +		return -EFAULT;
> > +
> > +	for (i = 0, ret = 0; i < addp->nr_pages && !ret; i++) {
> > +		if (signal_pending(current))
> > +			return -ERESTARTSYS;
> 
> If interrupted, how would user mode code know how many pages have been EADD'ed?

Hmm, updating nr_pages would be fairly simple and shouldn't confuse
userspace, e.g. as opposed to overloading the return value.

> > +
> > +		if (need_resched())
> > +			cond_resched();
> > +
> > +		ret = sgx_encl_add_page(encl, addp->addr + i*PAGE_SIZE,
> > +					addp->src + i*PAGE_SIZE,
> > +					&secinfo, addp->mrmask);
> > +	}
> >  	return ret;
> >  }
> > 
> > @@ -823,8 +845,8 @@ long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
> >  	case SGX_IOC_ENCLAVE_CREATE:
> >  		handler = sgx_ioc_enclave_create;
> >  		break;
> > -	case SGX_IOC_ENCLAVE_ADD_PAGE:
> > -		handler = sgx_ioc_enclave_add_page;
> > +	case SGX_IOC_ENCLAVE_ADD_PAGES:
> > +		handler = sgx_ioc_enclave_add_pages;
> >  		break;
> >  	case SGX_IOC_ENCLAVE_INIT:
> >  		handler = sgx_ioc_enclave_init;
> > --
> > 2.21.0
> 

Re: [RFC PATCH 3/9] x86/sgx: Allow userspace to add multiple pages in single ioctl()

[Dave Hansen]

On 5/31/19 4:31 PM, Sean Christopherson wrote:
> -struct sgx_enclave_add_page {
> +struct sgx_enclave_add_pages {
>  	__u64	addr;
>  	__u64	src;
>  	__u64	secinfo;
> +	__u32	nr_pages;
>  	__u16	mrmask;
>  } __attribute__((__packed__));

IMNHO this follows a user interface anti-pattern: exposing page sizes
where not strictly required.

Think of how this would look to an application if page size was
variable.  With this interface, they always need to scale their
operations by page size instead of just aligning it.

BTW, why is nr_pages a u32?  Do we never envision a case where you can
add more than 4TB of memory to an enclave? ;)

Re: [RFC PATCH 3/9] x86/sgx: Allow userspace to add multiple pages in single ioctl()

[Sean Christopherson]

On Mon, Jun 03, 2019 at 01:14:45PM -0700, Dave Hansen wrote:
> On 5/31/19 4:31 PM, Sean Christopherson wrote:
> > -struct sgx_enclave_add_page {
> > +struct sgx_enclave_add_pages {
> >  	__u64	addr;
> >  	__u64	src;
> >  	__u64	secinfo;
> > +	__u32	nr_pages;
> >  	__u16	mrmask;
> >  } __attribute__((__packed__));
> 
> IMNHO this follows a user interface anti-pattern: exposing page sizes
> where not strictly required.
> 
> Think of how this would look to an application if page size was
> variable.  With this interface, they always need to scale their
> operations by page size instead of just aligning it.

I briefly considered taking size in bytes, but I took a shortcut because
EPC pages are architecturally defined to be 4k sized and aligned.  That
being said, I don't necessarily disagree, especially if nr_pages isn't
squeezed into a u32.
 
> BTW, why is nr_pages a u32?  Do we never envision a case where you can
> add more than 4TB of memory to an enclave? ;)

Heh, fair enough.  IIRC, a while back someone posted about having problems
building a 512gb enclave in a 92mb EPC...

How about this for the intermediate patch:

	struct sgx_enclave_add_region {
		__u64	addr;
		__u64	src;
		__u64	size;
		__u64	secinfo;
		__u16	mrmask;
		__u16	reserved16;
		__u32	reserved;
	}

and with the flags field:

	struct sgx_enclave_add_region {
		__u64	addr;
		__u64	src;
		__u64	size;
		__u64	secinfo;
		__u16	mrmask;
		__u16	flags;
		__u32	reserved;
	}

Re: [RFC PATCH 3/9] x86/sgx: Allow userspace to add multiple pages in single ioctl()

[Sean Christopherson]

On Mon, Jun 03, 2019 at 01:08:04PM -0700, Sean Christopherson wrote:
> On Sun, Jun 02, 2019 at 11:26:09PM -0700, Xing, Cedric wrote:
> > > From: Christopherson, Sean J
> > > Sent: Friday, May 31, 2019 4:32 PM
> > > 
> > > +/**
> > > + * sgx_ioc_enclave_add_pages - handler for %SGX_IOC_ENCLAVE_ADD_PAGES
> > > + *
> > > + * @filep:	open file to /dev/sgx
> > > + * @cmd:	the command value
> > > + * @arg:	pointer to an &sgx_enclave_add_page instance
> > > + *
> > > + * Add a range of pages to an uninitialized enclave (EADD), and
> > > +optionally
> > > + * extend the enclave's measurement with the contents of the page (EEXTEND).
> > > + * The range of pages must be virtually contiguous.  The SECINFO and
> > > + * measurement maskare applied to all pages, i.e. pages with different
> > > + * properties must be added in separate calls.
> > > + *
> > > + * EADD and EEXTEND are done asynchronously via worker threads.  A
> > > +successful
> > > + * sgx_ioc_enclave_add_page() only indicates the pages have been added
> > > +to the
> > > + * work queue, it does not guarantee adding the pages to the enclave
> > > +will
> > > + * succeed.
> > > + *
> > > + * Return:
> > > + *   0 on success,
> > > + *   -errno otherwise
> > > + */
> > > +static long sgx_ioc_enclave_add_pages(struct file *filep, unsigned int cmd,
> > > +				      unsigned long arg)
> > > +{
> > > +	struct sgx_enclave_add_pages *addp = (void *)arg;
> > > +	struct sgx_encl *encl = filep->private_data;
> > > +	struct sgx_secinfo secinfo;
> > > +	unsigned int i;
> > > +	int ret;
> > > +
> > > +	if (copy_from_user(&secinfo, (void __user *)addp->secinfo,
> > > +			   sizeof(secinfo)))
> > > +		return -EFAULT;
> > > +
> > > +	for (i = 0, ret = 0; i < addp->nr_pages && !ret; i++) {
> > > +		if (signal_pending(current))
> > > +			return -ERESTARTSYS;
> > 
> > If interrupted, how would user mode code know how many pages have been EADD'ed?
> 
> Hmm, updating nr_pages would be fairly simple and shouldn't confuse
> userspace, e.g. as opposed to overloading the return value.

Or maybe update @addr and @src as well?  That would allow userspace to
re-invoke the ioctl() without having to modify the struct.

Re: [RFC PATCH 3/9] x86/sgx: Allow userspace to add multiple pages in single ioctl()

[Dave Hansen]

On 6/3/19 1:37 PM, Sean Christopherson wrote:
> Heh, fair enough.  IIRC, a while back someone posted about having problems
> building a 512gb enclave in a 92mb EPC...
> 
> How about this for the intermediate patch:
> 
> 	struct sgx_enclave_add_region {
> 		__u64	addr;
> 		__u64	src;
> 		__u64	size;
> 		__u64	secinfo;
> 		__u16	mrmask;
> 		__u16	reserved16;
> 		__u32	reserved;
> 	}

Looks fine to me.

RE: [RFC PATCH 3/9] x86/sgx: Allow userspace to add multiple pages in single ioctl()

[Xing, Cedric]

> From: Christopherson, Sean J
> Sent: Monday, June 03, 2019 1:40 PM
> 
> On Mon, Jun 03, 2019 at 01:08:04PM -0700, Sean Christopherson wrote:
> > On Sun, Jun 02, 2019 at 11:26:09PM -0700, Xing, Cedric wrote:
> > > > From: Christopherson, Sean J
> > > > Sent: Friday, May 31, 2019 4:32 PM
> > > >
> > > > +/**
> > > > + * sgx_ioc_enclave_add_pages - handler for
> > > > +%SGX_IOC_ENCLAVE_ADD_PAGES
> > > > + *
> > > > + * @filep:	open file to /dev/sgx
> > > > + * @cmd:	the command value
> > > > + * @arg:	pointer to an &sgx_enclave_add_page instance
> > > > + *
> > > > + * Add a range of pages to an uninitialized enclave (EADD), and
> > > > +optionally
> > > > + * extend the enclave's measurement with the contents of the page
> (EEXTEND).
> > > > + * The range of pages must be virtually contiguous.  The SECINFO
> > > > +and
> > > > + * measurement maskare applied to all pages, i.e. pages with
> > > > +different
> > > > + * properties must be added in separate calls.
> > > > + *
> > > > + * EADD and EEXTEND are done asynchronously via worker threads.
> > > > +A successful
> > > > + * sgx_ioc_enclave_add_page() only indicates the pages have been
> > > > +added to the
> > > > + * work queue, it does not guarantee adding the pages to the
> > > > +enclave will
> > > > + * succeed.
> > > > + *
> > > > + * Return:
> > > > + *   0 on success,
> > > > + *   -errno otherwise
> > > > + */
> > > > +static long sgx_ioc_enclave_add_pages(struct file *filep,
> unsigned int cmd,
> > > > +				      unsigned long arg)
> > > > +{
> > > > +	struct sgx_enclave_add_pages *addp = (void *)arg;
> > > > +	struct sgx_encl *encl = filep->private_data;
> > > > +	struct sgx_secinfo secinfo;
> > > > +	unsigned int i;
> > > > +	int ret;
> > > > +
> > > > +	if (copy_from_user(&secinfo, (void __user *)addp->secinfo,
> > > > +			   sizeof(secinfo)))
> > > > +		return -EFAULT;
> > > > +
> > > > +	for (i = 0, ret = 0; i < addp->nr_pages && !ret; i++) {
> > > > +		if (signal_pending(current))
> > > > +			return -ERESTARTSYS;
> > >
> > > If interrupted, how would user mode code know how many pages have
> been EADD'ed?
> >
> > Hmm, updating nr_pages would be fairly simple and shouldn't confuse
> > userspace, e.g. as opposed to overloading the return value.
> 
> Or maybe update @addr and @src as well?  That would allow userspace to
> re-invoke the ioctl() without having to modify the struct.

How about returning the number of pages (or bytes) EADD'ed, similar to write() syscall? 

RE: [RFC PATCH 3/9] x86/sgx: Allow userspace to add multiple pages in single ioctl()

[Xing, Cedric]

> -----Original Message-----
> From: Christopherson, Sean J
> Sent: Monday, June 03, 2019 1:37 PM
> To: Hansen, Dave <dave.hansen@intel.com>
> Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>; Andy Lutomirski
> <luto@kernel.org>; Xing, Cedric <cedric.xing@intel.com>; Stephen Smalley
> <sds@tycho.nsa.gov>; James Morris <jmorris@namei.org>; Serge E . Hallyn
> <serge@hallyn.com>; LSM List <linux-security-module@vger.kernel.org>;
> Paul Moore <paul@paul-moore.com>; Eric Paris <eparis@parisplace.org>;
> selinux@vger.kernel.org; Jethro Beekman <jethro@fortanix.com>; Thomas
> Gleixner <tglx@linutronix.de>; Linus Torvalds <torvalds@linux-
> foundation.org>; LKML <linux-kernel@vger.kernel.org>; X86 ML
> <x86@kernel.org>; linux-sgx@vger.kernel.org; Andrew Morton <akpm@linux-
> foundation.org>; nhorman@redhat.com; npmccallum@redhat.com; Ayoun, Serge
> <serge.ayoun@intel.com>; Katz-zamir, Shay <shay.katz-zamir@intel.com>;
> Huang, Haitao <haitao.huang@intel.com>; Andy Shevchenko
> <andriy.shevchenko@linux.intel.com>; Svahn, Kai <kai.svahn@intel.com>;
> Borislav Petkov <bp@alien8.de>; Josh Triplett <josh@joshtriplett.org>;
> Huang, Kai <kai.huang@intel.com>; David Rientjes <rientjes@google.com>;
> Roberts, William C <william.c.roberts@intel.com>; Tricca, Philip B
> <philip.b.tricca@intel.com>
> Subject: Re: [RFC PATCH 3/9] x86/sgx: Allow userspace to add multiple
> pages in single ioctl()
> 
> On Mon, Jun 03, 2019 at 01:14:45PM -0700, Dave Hansen wrote:
> > On 5/31/19 4:31 PM, Sean Christopherson wrote:
> > > -struct sgx_enclave_add_page {
> > > +struct sgx_enclave_add_pages {
> > >  	__u64	addr;
> > >  	__u64	src;
> > >  	__u64	secinfo;
> > > +	__u32	nr_pages;
> > >  	__u16	mrmask;
> > >  } __attribute__((__packed__));
> >
> > IMNHO this follows a user interface anti-pattern: exposing page sizes
> > where not strictly required.
> >
> > Think of how this would look to an application if page size was
> > variable.  With this interface, they always need to scale their
> > operations by page size instead of just aligning it.
> 
> I briefly considered taking size in bytes, but I took a shortcut because
> EPC pages are architecturally defined to be 4k sized and aligned.  That
> being said, I don't necessarily disagree, especially if nr_pages isn't
> squeezed into a u32.
> 
> > BTW, why is nr_pages a u32?  Do we never envision a case where you can
> > add more than 4TB of memory to an enclave? ;)
> 
> Heh, fair enough.  IIRC, a while back someone posted about having
> problems building a 512gb enclave in a 92mb EPC...
> 
> How about this for the intermediate patch:
> 
> 	struct sgx_enclave_add_region {
> 		__u64	addr;
> 		__u64	src;
> 		__u64	size;
> 		__u64	secinfo;
> 		__u16	mrmask;
> 		__u16	reserved16;
> 		__u32	reserved;
> 	}
> 
> and with the flags field:
> 
> 	struct sgx_enclave_add_region {
> 		__u64	addr;
> 		__u64	src;
> 		__u64	size;
> 		__u64	secinfo;
> 		__u16	mrmask;
> 		__u16	flags;

What is "flags" here?

> 		__u32	reserved;
> 	}

Re: [RFC PATCH 3/9] x86/sgx: Allow userspace to add multiple pages in single ioctl()

[Sean Christopherson]

On Mon, Jun 03, 2019 at 04:45:45PM -0700, Xing, Cedric wrote:
> > From: Christopherson, Sean J
> > Sent: Monday, June 03, 2019 1:40 PM
> > 
> > On Mon, Jun 03, 2019 at 01:08:04PM -0700, Sean Christopherson wrote:
> > > On Sun, Jun 02, 2019 at 11:26:09PM -0700, Xing, Cedric wrote:
> > > > > From: Christopherson, Sean J
> > > > > Sent: Friday, May 31, 2019 4:32 PM
> > > > >
> > > > > +/**
> > > > > + * sgx_ioc_enclave_add_pages - handler for
> > > > > +%SGX_IOC_ENCLAVE_ADD_PAGES
> > > > > + *
> > > > > + * @filep:	open file to /dev/sgx
> > > > > + * @cmd:	the command value
> > > > > + * @arg:	pointer to an &sgx_enclave_add_page instance
> > > > > + *
> > > > > + * Add a range of pages to an uninitialized enclave (EADD), and
> > > > > +optionally
> > > > > + * extend the enclave's measurement with the contents of the page
> > (EEXTEND).
> > > > > + * The range of pages must be virtually contiguous.  The SECINFO
> > > > > +and
> > > > > + * measurement maskare applied to all pages, i.e. pages with
> > > > > +different
> > > > > + * properties must be added in separate calls.
> > > > > + *
> > > > > + * EADD and EEXTEND are done asynchronously via worker threads.
> > > > > +A successful
> > > > > + * sgx_ioc_enclave_add_page() only indicates the pages have been
> > > > > +added to the
> > > > > + * work queue, it does not guarantee adding the pages to the
> > > > > +enclave will
> > > > > + * succeed.
> > > > > + *
> > > > > + * Return:
> > > > > + *   0 on success,
> > > > > + *   -errno otherwise
> > > > > + */
> > > > > +static long sgx_ioc_enclave_add_pages(struct file *filep,
> > unsigned int cmd,
> > > > > +				      unsigned long arg)
> > > > > +{
> > > > > +	struct sgx_enclave_add_pages *addp = (void *)arg;
> > > > > +	struct sgx_encl *encl = filep->private_data;
> > > > > +	struct sgx_secinfo secinfo;
> > > > > +	unsigned int i;
> > > > > +	int ret;
> > > > > +
> > > > > +	if (copy_from_user(&secinfo, (void __user *)addp->secinfo,
> > > > > +			   sizeof(secinfo)))
> > > > > +		return -EFAULT;
> > > > > +
> > > > > +	for (i = 0, ret = 0; i < addp->nr_pages && !ret; i++) {
> > > > > +		if (signal_pending(current))
> > > > > +			return -ERESTARTSYS;
> > > >
> > > > If interrupted, how would user mode code know how many pages have
> > been EADD'ed?
> > >
> > > Hmm, updating nr_pages would be fairly simple and shouldn't confuse
> > > userspace, e.g. as opposed to overloading the return value.
> > 
> > Or maybe update @addr and @src as well?  That would allow userspace to
> > re-invoke the ioctl() without having to modify the struct.
> 
> How about returning the number of pages (or bytes) EADD'ed, similar to
> write() syscall? 

I thought about that as well, but I think it'd be useful to update the
offset on any failure, not just -ERESTARTSYS.

Re: [RFC PATCH 3/9] x86/sgx: Allow userspace to add multiple pages in single ioctl()

[Sean Christopherson]

On Mon, Jun 03, 2019 at 04:48:47PM -0700, Xing, Cedric wrote:
> > How about this for the intermediate patch:
> > 
> > 	struct sgx_enclave_add_region {
> > 		__u64	addr;
> > 		__u64	src;
> > 		__u64	size;
> > 		__u64	secinfo;
> > 		__u16	mrmask;
> > 		__u16	reserved16;
> > 		__u32	reserved;
> > 	}
> > 
> > and with the flags field:
> > 
> > 	struct sgx_enclave_add_region {
> > 		__u64	addr;
> > 		__u64	src;
> > 		__u64	size;
> > 		__u64	secinfo;
> > 		__u16	mrmask;
> > 		__u16	flags;
> 
> What is "flags" here?

In the RFC, @flags holds SGX_ALLOW_{READ,WRITE,EXEC}.

> 
> > 		__u32	reserved;
> > 	}

Re: [RFC PATCH 0/9] security: x86/sgx: SGX vs. LSM

[Sean Christopherson]

On Mon, Jun 03, 2019 at 11:30:54AM -0700, Xing, Cedric wrote:
> > From: Christopherson, Sean J
> > Sent: Monday, June 03, 2019 10:16 AM
> > 
> > On Sun, Jun 02, 2019 at 12:29:35AM -0700, Xing, Cedric wrote:
> > > Hi Sean,
> > >
> > > Generally I agree with your direction but think ALLOW_* flags are
> > > completely internal to LSM because they can be both produced and
> > > consumed inside an LSM module. So spilling them into SGX driver and
> > > also user mode code makes the solution ugly and in some cases
> > > impractical because not every enclave host process has a priori
> > > knowledge on whether or not an enclave page would be EMODPE'd at
> > runtime.
> > 
> > In this case, the host process should tag *all* pages it *might* convert
> > to executable as ALLOW_EXEC.  LSMs can (and should/will) be written in
> > such a way that denying ALLOW_EXEC is fatal to the enclave if and only
> > if the enclave actually attempts mprotect(PROT_EXEC).
> 
> What if those pages contain self-modifying code but the host doesn't know
> ahead of time? Would it require ALLOW_WRITE|ALLOW_EXEC at EADD? Then would it
> prevent those pages to start with PROT_EXEC?

Without ALLOW_WRITE+ALLOW_EXEC, the enclave would build and launch, but
fail at mprotect(..., PROT_WRITE), e.g. when it attempted to gain write
access to do self-modifying code.  And it would would fail irrespective of
LSM restrictions.

> Anyway, my point is that it is unnecessary even if it works.

Unnecessary in an ideal world, yes.  Realistically, it's the least bad
option.

> > Take the SELinux path for example.  The only scenario in which
> > PROT_WRITE is cleared from @allowed_prot is if the page *starts* with
> > PROT_EXEC.
> > If PROT_EXEC is denied on a page that starts RW, e.g. an EAUG'd page,
> > then PROT_EXEC will be cleared from @allowed_prot.
> > 
> > As Stephen pointed out, auditing the denials on @allowed_prot means the
> > log will contain false positives of a sort.  But this is more of a noise
> > issue than true false positives.  E.g. there are three possible outcomes
> > for the enclave.
> > 
> >   - The enclave does not do EMODPE[PROT_EXEC] in any scenario, ever.
> >     Requesting ALLOW_EXEC is either a straightforward a userspace bug or
> >     a poorly written generic enclave loader.
> > 
> >   - The enclave conditionally performs EMODPE[PROT_EXEC].  In this case
> >     the denial is a true false positive.
> > 
> >   - The enclave does EMODPE[PROT_EXEC] and its host userspace then fails
> >     on mprotect(PROT_EXEC), i.e. the LSM denial is working as intended.
> >     The audit log will be noisy, but viewed as a whole the denials
> > aren't
> >     false positives.
> 
> What I was talking about was EMODPE[PROT_WRITE] on an RX page.

As above, mprotect(..., PROT_WRITE) would fail without ALLOW_WRITE.

> > The potential for noisy audit logs and/or false positives is unfortunate,
> > but it's (by far) the lesser of many evils.
> > 
> > > Theoretically speaking, what you really need is a per page flag (let's
> > > name it WRITTEN?) indicating whether a page has ever been written to
> > > (or more precisely, granted PROT_WRITE), which will be used to decide
> > > whether to grant PROT_EXEC when requested in future. Given the fact
> > > that all mprotect() goes through LSM and mmap() is limited to
> > > PROT_NONE, it's easy for LSM to capture that flag by itself instead of
> > asking user mode code to provide it.
> > >
> > > That said, here is the summary of what I think is a better approach.
> > > * In hook security_file_alloc(), if @file is an enclave, allocate some
> > data
> > >   structure to store for every page, the WRITTEN flag as described
> > above.
> > >   WRITTEN is cleared initially for all pages.
> > 
> > This would effectively require *every* LSM to duplicate the SGX driver's
> > functionality, e.g. track per-page metadata, implement locking to
> > prevent races between multiple mm structs, etc...
> 
> Architecturally we shouldn't dictate how LSM makes decisions. ALLOW_* are no
> difference than PROCESS__* or FILE__* flags, which are just artifacts to
> assist particular LSMs in decision making. They are never considered part of
> the LSM interface, even if other LSMs than SELinux may adopt the same/similar
> approach.

No, the flags are tracked and managed by SGX.  We are not dictating LSM
behavior in any way, e.g. an LSM could completely ignore @allowed_prot and
nothing would break.

> If code duplication is what you are worrying about, you can put them in a
> library, or implement/export them in some new file (maybe
> security/enclave.c?) as utility functions.

Code duplication is the least of my concerns.  Tracking file pointers
would require a global list/tree of some form, along with a locking and/or
RCU scheme to protect accesses to that container.  Another lock would be
needed to prevent races between mprotect() calls from different processes.

> But spilling them into user mode is what I think is unacceptable.

Why is it unacceptable?  There's effectively no cost to userspace for SGX1.
The ALLOW_* flags only come into play in the event of a noexec or LSM
restriction, i.e. worst case scenario an enclave that wants to do arbitrary
self-modifying code can declare RWX on everything.

Re: [RFC PATCH 0/9] security: x86/sgx: SGX vs. LSM

[Jarkko Sakkinen]

On Fri, May 31, 2019 at 04:31:50PM -0700, Sean Christopherson wrote:
> This series is the result of a rather absurd amount of discussion over
> how to get SGX to play nice with LSM policies, without having to resort
> to evil shenanigans or put undue burden on userspace.  The discussion
> definitely wandered into completely insane territory at times, but I
> think/hope we ended up with something reasonable.

By definition this is a broken series because it does not apply to
mainline. Even RFC series should at least apply. Would be better idea to
discuss design ideas and use snippets instead. Now you have to take
original v20 and apply to these patches to evaluate anything.

> The basic gist of the approach is to require userspace to declare what
> protections are maximally allowed for any given page, e.g. add a flags
> field for loading enclave pages that takes ALLOW_{READ,WRITE,EXEC}.  LSMs
> can then adjust the allowed protections, e.g. clear ALLOW_EXEC to prevent
> ever mapping the page with PROT_EXEC.  SGX enforces the allowed perms
> via a new mprotect() vm_ops hook, e.g. like regular mprotect() uses
> MAY_{READ,WRITE,EXEC}.

mprotect() does not use MAY_{READ,WRITE,EXEC} constants. It uses
VM_MAY{READ,WRITE,EXEC,SHARED} constants.

What are ALLOW_{READ,WRITE,EXEC} and how they are used? What does the
hook do and why it is in vm_ops and not in file_operations? Are they
arguments to the ioctl or internal variables that are set based on
SECINFO?

/Jarkko

Re: [RFC PATCH 1/9] x86/sgx: Remove unused local variable in sgx_encl_release()

[Jarkko Sakkinen]

On Fri, May 31, 2019 at 04:31:51PM -0700, Sean Christopherson wrote:
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---

How this patch is essential to demonstrate anything?

/Jarkko

Re: [RFC PATCH 2/9] x86/sgx: Do not naturally align MAP_FIXED address

[Jarkko Sakkinen]

On Fri, May 31, 2019 at 04:31:52PM -0700, Sean Christopherson wrote:
> 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>

Why you want to allow mmap() to be called multiple times? mmap() could
be allowed only once with PROT_NONE and denied afterwards. Is this for
sending fd to another process that would map already existing enclave?

I don't see any checks for whether the is enclave underneath. Also, I
think that in all cases mmap() callback should allow only PROT_NONE
as permissions for clarity even if it could called multiple times.

/Jarkko

Re: [RFC PATCH 3/9] x86/sgx: Allow userspace to add multiple pages in single ioctl()

[Jarkko Sakkinen]

On Fri, May 31, 2019 at 04:31:53PM -0700, Sean Christopherson wrote:
> ...to improve performance when building enclaves by reducing the number
> of user<->system transitions.  Rather than provide arbitrary batching,
> e.g. with per-page SECINFO and mrmask, take advantage of the fact that
> any sane enclave will have large swaths of pages with identical
> properties, e.g. code vs. data sections.
> 
> For simplicity and stability in the initial implementation, loop over
> the existing add page flow instead of taking a more agressive approach,
> which would require tracking transitions between VMAs and holding
> mmap_sem for an extended duration.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

I think this completely ruins the rest of the series. We should first
get the model for security done (including documentation). I would even
send v21 with just that update because this series does not even apply
to the mainline.

I would request an update to the series with just the change to the
security model. Also the very first should be dropped as it is
completely unrelated cosmetic fix.

/Jarkko

Re: [RFC PATCH 4/9] mm: Introduce vm_ops->mprotect()

[Jarkko Sakkinen]

On Fri, May 31, 2019 at 04:31:54PM -0700, Sean Christopherson wrote:
> SGX will use the mprotect() hook to prevent userspace from circumventing
> various security checks, i.e. Linux Security Modules.
> 
> Enclaves are built by copying data from normal memory into the Enclave
> Page Cache (EPC).  Due to the nature of SGX, the EPC is represented by a
> single file that must be MAP_SHARED, i.e. mprotect() only ever sees a
> single MAP_SHARED vm_file.  Furthermore, all enclaves will need read,
> write and execute pages in the EPC.

What does the last sentence is pointing out? Enclaves read, write and
execute pages, so?

> As a result, LSM policies cannot be meaningfully applied, e.g. an LSM
> can deny access to the EPC as a whole, but can't deny PROT_EXEC on page
> that originated in a non-EXECUTE file (which is long gone by the time
> mprotect() is called).

I'm not sure what kind of scenario this is describing where some LSM
can't dent PROT_EXEC. Kind of cryptic paragraph, have to say.

> By hooking mprotect(), SGX can make explicit LSM upcalls while an
> enclave is being built, i.e. when the kernel has a handle to origin of
> each enclave page, and enforce the result of the LSM policy whenever
> userspace maps the enclave page in the future.

How does mprotect() enabled adding new LSM hooks?

> Alternatively, SGX could play games with MAY_{READ,WRITE,EXEC}, but
> that approach is quite ugly, e.g. would require userspace to call an
> SGX ioctl() prior to using mprotect() to extend a page's protections.

Not really sure I got this. SGX gets page permissions in SECINFO.
Also recurring comment about MAY_* constants.

> 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..50a42364a885 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 (*mprotect)(struct vm_area_struct * area, unsigned long start,
> +			unsigned long end, unsigned long prot);

Right, the hook must be here obviously because mprotect() can be called
when /dev/sgx/enclave is closed. Can you describe start and end i.e.
what range they are in?

>  	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..e466ca5e4fe0 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->mprotect) {
> +			error = vma->vm_ops->mprotect(vma, nstart, tmp, prot);
> +			if (error)
> +				goto out;
> +		}
> +
> +		error = security_file_mprotect(vma, reqprot, prot);

Why is mprotect callback called post the LSM hook?

> +		if (error)
> +			goto out;

/Jarkko

Re: [RFC PATCH 4/9] mm: Introduce vm_ops->mprotect()

[Andy Lutomirski]

On Fri, May 31, 2019 at 4:32 PM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> SGX will use the mprotect() hook to prevent userspace from circumventing
> various security checks, i.e. Linux Security Modules.
>
> Enclaves are built by copying data from normal memory into the Enclave
> Page Cache (EPC).  Due to the nature of SGX, the EPC is represented by a
> single file that must be MAP_SHARED, i.e. mprotect() only ever sees a
> single MAP_SHARED vm_file.  Furthermore, all enclaves will need read,
> write and execute pages in the EPC.
>
> As a result, LSM policies cannot be meaningfully applied, e.g. an LSM
> can deny access to the EPC as a whole, but can't deny PROT_EXEC on page
> that originated in a non-EXECUTE file (which is long gone by the time
> mprotect() is called).
>
> By hooking mprotect(), SGX can make explicit LSM upcalls while an
> enclave is being built, i.e. when the kernel has a handle to origin of
> each enclave page, and enforce the result of the LSM policy whenever
> userspace maps the enclave page in the future.
>
> Alternatively, SGX could play games with MAY_{READ,WRITE,EXEC}, but
> that approach is quite ugly, e.g. would require userspace to call an
> SGX ioctl() prior to using mprotect() to extend a page's protections.
>
> 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..50a42364a885 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 (*mprotect)(struct vm_area_struct * area, 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..e466ca5e4fe0 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->mprotect) {
> +                       error = vma->vm_ops->mprotect(vma, nstart, tmp, prot);
> +                       if (error)
> +                               goto out;
> +               }
> +
> +               error = security_file_mprotect(vma, reqprot, prot);
> +               if (error)
> +                       goto out;
> +

I think that, if you're going to do it like this, you need to call it
mprotect_and_check_security or something.  Or you could just add
.may_mprotect, which is allowed to fail but, on success, falls through
to call security_file_mprotect and mprotect_fixup().

--Andy

Re: [RFC PATCH 5/9] x86/sgx: Restrict mapping without an enclave page to PROT_NONE

[Jarkko Sakkinen]

On Fri, May 31, 2019 at 04:31:55PM -0700, Sean Christopherson wrote:
> To support LSM integration, SGX will require userspace to explicitly
> specify the allowed protections for each page.  The allowed protections
> will be supplied to and modified by LSMs (based on their policies).

How the allowed protections are modified by LSMs? AFAIK they don't touch
the PROT_* flags but I could be wrong too.

> To prevent userspace from circumventing the allowed protections, do not
> allow PROT_{READ,WRITE,EXEC} mappings to an enclave without an
> associated enclave page (which will track the allowed protections).
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/kernel/cpu/sgx/driver/main.c |  5 +++++
>  arch/x86/kernel/cpu/sgx/encl.c        | 30 +++++++++++++++++++++++++++
>  arch/x86/kernel/cpu/sgx/encl.h        |  3 +++
>  3 files changed, 38 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/driver/main.c b/arch/x86/kernel/cpu/sgx/driver/main.c
> index 129d356aff30..65a87c2fdf02 100644
> --- a/arch/x86/kernel/cpu/sgx/driver/main.c
> +++ b/arch/x86/kernel/cpu/sgx/driver/main.c
> @@ -63,6 +63,11 @@ static long sgx_compat_ioctl(struct file *filep, unsigned int cmd,
>  static int sgx_mmap(struct file *file, struct vm_area_struct *vma)
>  {
>  	struct sgx_encl *encl = file->private_data;
> +	int ret;
> +
> +	ret = sgx_map_allowed(encl, vma->vm_start, vma->vm_end, vma->vm_flags);
> +	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 f23ea0fbaa47..955d4f430adc 100644
> --- a/arch/x86/kernel/cpu/sgx/encl.c
> +++ b/arch/x86/kernel/cpu/sgx/encl.c
> @@ -235,6 +235,35 @@ static void sgx_vma_close(struct vm_area_struct *vma)
>  	kref_put(&encl->refcount, sgx_encl_release);
>  }
>  
> +int sgx_map_allowed(struct sgx_encl *encl, unsigned long start,
> +		    unsigned long end, unsigned long prot)

Documentation missing.

> +{
> +	struct sgx_encl_page *page;
> +	unsigned long addr;
> +
> +	prot &= (VM_READ | VM_WRITE | VM_EXEC);
> +	if (!prot || !encl)
> +		return 0;
> +
> +	mutex_lock(&encl->lock);
> +
> +	for (addr = start; addr < end; addr += PAGE_SIZE) {
> +		page = radix_tree_lookup(&encl->page_tree, addr >> PAGE_SHIFT);
> +		if (!page)
> +			return -EACCES;
> +	}
> +
> +	mutex_unlock(&encl->lock);
> +
> +	return 0;
> +}
> +
> +static int sgx_vma_mprotect(struct vm_area_struct *vma, unsigned long start,
> +			    unsigned long end, unsigned long prot)
> +{
> +	return sgx_map_allowed(vma->vm_private_data, start, end, prot);
> +}
> +
>  static unsigned int sgx_vma_fault(struct vm_fault *vmf)
>  {
>  	unsigned long addr = (unsigned long)vmf->address;
> @@ -372,6 +401,7 @@ static int sgx_vma_access(struct vm_area_struct *vma, unsigned long addr,
>  const struct vm_operations_struct sgx_vm_ops = {
>  	.close = sgx_vma_close,
>  	.open = sgx_vma_open,
> +	.mprotect = sgx_vma_mprotect,
>  	.fault = sgx_vma_fault,
>  	.access = sgx_vma_access,
>  };
> diff --git a/arch/x86/kernel/cpu/sgx/encl.h b/arch/x86/kernel/cpu/sgx/encl.h
> index c557f0374d74..6e310e3b3fff 100644
> --- a/arch/x86/kernel/cpu/sgx/encl.h
> +++ b/arch/x86/kernel/cpu/sgx/encl.h
> @@ -106,6 +106,9 @@ static inline unsigned long sgx_pcmd_offset(pgoff_t page_index)
>  	       sizeof(struct sgx_pcmd);
>  }
>  
> +int sgx_map_allowed(struct sgx_encl *encl, unsigned long start,
> +		    unsigned long end, unsigned long prot);
> +
>  enum sgx_encl_mm_iter {
>  	SGX_ENCL_MM_ITER_DONE		= 0,
>  	SGX_ENCL_MM_ITER_NEXT		= 1,
> -- 
> 2.21.0

This is missing explanation why it is OK to have a mismatch between
the SECINFO flags and VM_* flags. Maybe that could be explained in
sgx_map_allowed() documentation.

/Jarkko

Re: [RFC PATCH 0/9] security: x86/sgx: SGX vs. LSM

[Stephen Smalley]

On 6/3/19 2:30 PM, Xing, Cedric wrote:
>> From: Christopherson, Sean J
>> Sent: Monday, June 03, 2019 10:16 AM
>>
>> On Sun, Jun 02, 2019 at 12:29:35AM -0700, Xing, Cedric wrote:
>>> Hi Sean,
>>>
>>> Generally I agree with your direction but think ALLOW_* flags are
>>> completely internal to LSM because they can be both produced and
>>> consumed inside an LSM module. So spilling them into SGX driver and
>>> also user mode code makes the solution ugly and in some cases
>>> impractical because not every enclave host process has a priori
>>> knowledge on whether or not an enclave page would be EMODPE'd at
>> runtime.
>>
>> In this case, the host process should tag *all* pages it *might* convert
>> to executable as ALLOW_EXEC.  LSMs can (and should/will) be written in
>> such a way that denying ALLOW_EXEC is fatal to the enclave if and only
>> if the enclave actually attempts mprotect(PROT_EXEC).
> 
> What if those pages contain self-modifying code but the host doesn't know ahead of time? Would it require ALLOW_WRITE|ALLOW_EXEC at EADD? Then would it prevent those pages to start with PROT_EXEC?
> 
> Anyway, my point is that it is unnecessary even if it works.
> 
>>
>> Take the SELinux path for example.  The only scenario in which
>> PROT_WRITE is cleared from @allowed_prot is if the page *starts* with
>> PROT_EXEC.
>> If PROT_EXEC is denied on a page that starts RW, e.g. an EAUG'd page,
>> then PROT_EXEC will be cleared from @allowed_prot.
>>
>> As Stephen pointed out, auditing the denials on @allowed_prot means the
>> log will contain false positives of a sort.  But this is more of a noise
>> issue than true false positives.  E.g. there are three possible outcomes
>> for the enclave.
>>
>>    - The enclave does not do EMODPE[PROT_EXEC] in any scenario, ever.
>>      Requesting ALLOW_EXEC is either a straightforward a userspace bug or
>>      a poorly written generic enclave loader.
>>
>>    - The enclave conditionally performs EMODPE[PROT_EXEC].  In this case
>>      the denial is a true false positive.
>>
>>    - The enclave does EMODPE[PROT_EXEC] and its host userspace then fails
>>      on mprotect(PROT_EXEC), i.e. the LSM denial is working as intended.
>>      The audit log will be noisy, but viewed as a whole the denials
>> aren't
>>      false positives.
> 
> What I was talking about was EMODPE[PROT_WRITE] on an RX page.
> 
>>
>> The potential for noisy audit logs and/or false positives is unfortunate,
>> but it's (by far) the lesser of many evils.
>>
>>> Theoretically speaking, what you really need is a per page flag (let's
>>> name it WRITTEN?) indicating whether a page has ever been written to
>>> (or more precisely, granted PROT_WRITE), which will be used to decide
>>> whether to grant PROT_EXEC when requested in future. Given the fact
>>> that all mprotect() goes through LSM and mmap() is limited to
>>> PROT_NONE, it's easy for LSM to capture that flag by itself instead of
>> asking user mode code to provide it.
>>>
>>> That said, here is the summary of what I think is a better approach.
>>> * In hook security_file_alloc(), if @file is an enclave, allocate some
>> data
>>>    structure to store for every page, the WRITTEN flag as described
>> above.
>>>    WRITTEN is cleared initially for all pages.
>>
>> This would effectively require *every* LSM to duplicate the SGX driver's
>> functionality, e.g. track per-page metadata, implement locking to
>> prevent races between multiple mm structs, etc...
> 
> Architecturally we shouldn't dictate how LSM makes decisions. ALLOW_* are no difference than PROCESS__* or FILE__* flags, which are just artifacts to assist particular LSMs in decision making. They are never considered part of the LSM interface, even if other LSMs than SELinux may adopt the same/similar approach.
> 
> If code duplication is what you are worrying about, you can put them in a library, or implement/export them in some new file (maybe security/enclave.c?) as utility functions. But spilling them into user mode is what I think is unacceptable.
> 
>>
>>>    Open: Given a file of type struct file *, how to tell if it is an
>> enclave (i.e. /dev/sgx/enclave)?
>>> * In hook security_mmap_file(), if @file is an enclave, make sure
>> @prot can
>>>    only be PROT_NONE. This is to force all protection changes to go
>> through
>>>    security_file_mprotect().
>>> * In the newly introduced hook security_enclave_load(), set WRITTEN
>> for pages
>>>    that are requested PROT_WRITE.
>>
>> How would an LSM associate a page with a specific enclave?  vma->vm_file
>> will point always point at /dev/sgx/enclave.  vma->vm_mm is useless
>> because we're allowing multiple processes to map a single enclave, not
>> to mention that by mm would require holding a reference to the mm.
> 
> Each open("/dev/sgx/enclave") syscall creates a *new* instance of struct file to uniquely identify one enclave instance. What I mean is @vma->vm_file, not @vma->vm_file->f_path or @vma->vm_file->f_inode.
> 
>>
>>> * In hook security_file_mprotect(), if @vma->vm_file is an enclave,
>> look up
>>>    and use WRITTEN flags for all pages within @vma, along with other
>> global
>>>    flags (e.g. PROCESS__EXECMEM/FILE__EXECMOD in the case of SELinux)
>> to decide
>>>    on allowing/rejecting @prot.
>>
>> vma->vm_file will always be /dev/sgx/enclave at this point, which means
>> LSMs don't have the necessary anchor back to the source file, e.g. to
>> enforce FILE__EXECUTE.  The noexec file system case is also unaddressed.
> 
> vma->vm_file identifies an enclave instance uniquely. FILE__EXECUTE is checked by security_enclave_load() using @source_vma->vm_file. Once a page has been EADD'ed, whether to allow RW->RX depends on .sigstruct file (more precisely, the file backing SIGSTRUCT), whose FILE__* attributes could be cached in vma->vm_file->f_security by security_enclave_init().

The RFC series seemed to dispense with the use of the sigstruct file and 
just used the source file throughout IIUC.  That allowed for reuse of 
FILE__* permissions without ambiguity rather than introducing separate 
ENCLAVE__* permissions or using /dev/sgx/enclave inode as the target of 
all checks.

Regardless, IIUC, your approach requires that we always check 
FILE__EXECMOD, and FILE__EXECUTE up front during security_enclave_load() 
irrespective of prot so that we can save the result in the f_security 
for later use by the mprotect hook.  This may generate many spurious 
audit messages for cases where PROT_EXEC will never be requested, and 
users will be prone to just always allowing it since they cannot tell 
when it was actually needed.

>   
> The noexec case should be addressed in IOC_ADD_PAGES by testing @source_vma->vm_flags & VM_MAYEXEC.
> 
>>
>>> * In hook security_file_free(), if @file is an  enclave, free storage
>>>    allocated for WRITTEN flags.

Re: [RFC PATCH 6/9] x86/sgx: Require userspace to provide allowed prots to ADD_PAGES

[Jarkko Sakkinen]

On Fri, May 31, 2019 at 04:31:56PM -0700, Sean Christopherson wrote:
> ...to support (the equivalent) of existing Linux Security Module
> functionality.

Long and short descriptions should be separate. Also this does not
make any sense. LSM is a framework with a set of hook to make access
decisions and there various implementations of it.

How this replicates LSMs and why that even would be a goal?

My guess is that you are trying to do something else. I'm just saying
that the idea to do equivalent of LSMs to another subsystems would be
insane if it was done.

> always be MAP_SHARED.  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 any reasonable granularity, e.g. an LSM can
> deny access to EPC altogether, but can't deny potentially dangerous
> behavior such as mapping pages RW->RW or RWX.

The mapping must be shared given that it is iomem but why enclave pages
would need RWX for all pages? The information that is missing from this
paragraph is the explanation why an LSM could not deny dangerous
behavior in PTE level.

> To give LSMs enough information to implement their policies without
> having to resort to ugly things, e.g. holding a reference to the vm_file
> of each enclave page, require userspace to explicitly state the allowed
> protections for each page (region), i.e. take ALLOW_{READ,WRITE,EXEC}
> in the ADD_PAGES ioctl.

I would keep descriptions such as "ugly things" away from commit
messages as it is easy way to be not clear and explicit what you are
trying to say.

> The ALLOW_* flags will be passed to LSMs so that they can make informed
> decisions when the enclave is being built, i.e. when the source vm_file
> is available.  For example, SELinux's EXECMOD permission can be
> required if an enclave is requesting both ALLOW_WRITE and ALLOW_EXEC.

There should be some explanation what ALLOW_* flag are. It is now like
as it was in common knowledge. SECINFO already has protection flags to
name an example and without any explanation all of this is just very
confusing.

This should address SECINFO and ALLOW_* relationship and differences.

> Update the mmap()/mprotect() hooks to enforce the ALLOW_* protections,
> a la the standard VM_MAY{READ,WRITE,EXEC} flags.
> 
> The ALLOW_EXEC flag also has a second (important) 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 from a noexec path using read-only permissions and later mprotect()
> and ENCLU[EMODPE] the page to gain execute permissions.  By requiring
> ALLOW_EXEC up front, SGX will be able to enforce noexec paths when
> building the enclave.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/include/uapi/asm/sgx.h        |  9 ++++++++-
>  arch/x86/kernel/cpu/sgx/driver/ioctl.c | 23 +++++++++++++++++------
>  arch/x86/kernel/cpu/sgx/encl.c         |  2 +-
>  arch/x86/kernel/cpu/sgx/encl.h         |  1 +
>  4 files changed, 27 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
> index 4a12d6abbcb7..4489e92fa0dc 100644
> --- a/arch/x86/include/uapi/asm/sgx.h
> +++ b/arch/x86/include/uapi/asm/sgx.h
> @@ -31,6 +31,11 @@ struct sgx_enclave_create  {
>  	__u64	src;
>  };
>  
> +/* Supported flags for struct sgx_enclave_add_pages. */
> +#define SGX_ALLOW_READ	VM_READ
> +#define SGX_ALLOW_WRITE	VM_WRITE
> +#define SGX_ALLOW_EXEC	VM_EXEC

Why these flags are even defined if they are the same as VM_* flags?

> +
>  /**
>   * struct sgx_enclave_add_pages - parameter structure for the
>   *                                %SGX_IOC_ENCLAVE_ADD_PAGES ioctl
> @@ -39,6 +44,7 @@ struct sgx_enclave_create  {
>   * @secinfo:	address for the SECINFO data (common to all pages)
>   * @nr_pages:	number of pages (must be virtually contiguous)
>   * @mrmask:	bitmask for the measured 256 byte chunks (common to all pages)
> + * @flags:	flags, e.g. SGX_ALLOW_{READ,WRITE,EXEC} (common to all pages)
>   */
>  struct sgx_enclave_add_pages {
>  	__u64	addr;
> @@ -46,7 +52,8 @@ struct sgx_enclave_add_pages {
>  	__u64	secinfo;
>  	__u32	nr_pages;
>  	__u16	mrmask;
> -} __attribute__((__packed__));
> +	__u16	flags;
> +};
>  
>  /**
>   * struct sgx_enclave_init - parameter structure for the
> diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> index 6acfcbdeca9a..c30acd3fbbdd 100644
> --- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> +++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> @@ -235,7 +235,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 allowed_prot)
>  {
>  	struct sgx_encl_page *encl_page;
>  	int ret;
> @@ -247,6 +248,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->allowed_prot = allowed_prot;
>  	ret = radix_tree_insert(&encl->page_tree, PFN_DOWN(encl_page->desc),
>  				encl_page);
>  	if (ret) {
> @@ -530,7 +532,7 @@ static int sgx_encl_queue_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 allowed_prot)
>  {
>  	u64 page_type = secinfo->flags & SGX_SECINFO_PAGE_TYPE_MASK;
>  	struct sgx_encl_page *encl_page;
> @@ -556,7 +558,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, allowed_prot);
>  	if (IS_ERR(encl_page)) {
>  		ret = PTR_ERR(encl_page);
>  		goto out;
> @@ -576,12 +578,20 @@ static int __sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
>  
>  static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
>  			     unsigned long src, struct sgx_secinfo *secinfo,
> -			     unsigned int mrmask)
> +			     unsigned int mrmask, unsigned int flags)
>  {
> +	unsigned long prot = secinfo->flags & (VM_READ | VM_WRITE | VM_EXEC);

Even if the secinfo flags have the exactly the same values you should
not do this as they are kind of from different type. This is confusing
to read.

> +	unsigned long allowed_prot = flags & (VM_READ | VM_WRITE | VM_EXEC);

Why you take the trouble defining those macros and do not then use them
even yourself?

>  	struct page *data_page;
>  	void *data;
>  	int ret;
>  
> +	BUILD_BUG_ON(SGX_SECINFO_R != VM_READ || SGX_SECINFO_W != VM_WRITE ||
> +		     SGX_SECINFO_X != VM_EXEC);

Why this check?

> +
> +	if (prot & ~allowed_prot)
> +		return -EACCES;
> +
>  	data_page = alloc_page(GFP_HIGHUSER);
>  	if (!data_page)
>  		return -ENOMEM;
> @@ -593,7 +603,8 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
>  		goto out;
>  	}
>  
> -	ret = __sgx_encl_add_page(encl, addr, data, secinfo, mrmask);
> +	ret = __sgx_encl_add_page(encl, addr, data, secinfo, mrmask,
> +				  allowed_prot);
>  out:
>  	kunmap(data_page);
>  	__free_page(data_page);
> @@ -645,7 +656,7 @@ static long sgx_ioc_enclave_add_pages(struct file *filep, unsigned int cmd,
>  
>  		ret = sgx_encl_add_page(encl, addp->addr + i*PAGE_SIZE,
>  					addp->src + i*PAGE_SIZE,
> -					&secinfo, addp->mrmask);
> +					&secinfo, addp->mrmask, addp->flags);
>  	}
>  	return ret;
>  }
> diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
> index 955d4f430adc..e5847571a265 100644
> --- a/arch/x86/kernel/cpu/sgx/encl.c
> +++ b/arch/x86/kernel/cpu/sgx/encl.c
> @@ -249,7 +249,7 @@ int sgx_map_allowed(struct sgx_encl *encl, unsigned long start,
>  
>  	for (addr = start; addr < end; addr += PAGE_SIZE) {
>  		page = radix_tree_lookup(&encl->page_tree, addr >> PAGE_SHIFT);
> -		if (!page)
> +		if (!page || (prot & ~page->allowed_prot))
>  			return -EACCES;
>  	}

However this goes it would be good idea to have only ony patch in the
patch set that fully defines this function. Impossible to review
properly with this split.

>  
> 
> diff --git a/arch/x86/kernel/cpu/sgx/encl.h b/arch/x86/kernel/cpu/sgx/encl.h
> index 6e310e3b3fff..7cca076a4987 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 allowed_prot;
>  	struct sgx_epc_page *epc_page;
>  	struct sgx_va_page *va_page;
>  	struct sgx_encl *encl;
> -- 
> 2.21.0
> 

This patch left me very confused. I don't get it.

/Jarkko

Re: [RFC PATCH 7/9] x86/sgx: Enforce noexec filesystem restriction for enclaves

[Jarkko Sakkinen]

On Fri, May 31, 2019 at 04:31:57PM -0700, Sean Christopherson wrote:
> Do not allow an enclave page to be mapped with PROT_EXEC if the source
> page is backed by a file on a noexec file system.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

Why don't you just check in sgx_encl_add_page() that whether the path
comes from noexec and deny if SECINFO contains X?

/Jarkko

Re: [RFC PATCH 0/9] security: x86/sgx: SGX vs. LSM

[Sean Christopherson]

On Tue, Jun 04, 2019 at 11:33:44AM -0400, Stephen Smalley wrote:
> The RFC series seemed to dispense with the use of the sigstruct file and
> just used the source file throughout IIUC.  That allowed for reuse of
> FILE__* permissions without ambiguity rather than introducing separate
> ENCLAVE__* permissions or using /dev/sgx/enclave inode as the target of all
> checks.

Drat, I meant to explicitly call that out in the cover letter.  Yes, the
concept of using sigstruct as a proxy was dropped for this RFC.  The
primary motivation was to avoid having to take a hold a reference to the
sigstruct file for the lifetime of the enclave, and in general so that
userspace isn't forced to put sigstruct into a file.

> Regardless, IIUC, your approach requires that we always check FILE__EXECMOD,
> and FILE__EXECUTE up front during security_enclave_load() irrespective of
> prot so that we can save the result in the f_security for later use by the
> mprotect hook.

Correct, this approach requires up front checks.

> This may generate many spurious audit messages for cases
> where PROT_EXEC will never be requested, and users will be prone to just
> always allowing it since they cannot tell when it was actually needed.

Userspace will be able to understand when PROT_EXEC is actually needed
as mprotect() will (eventually) fail.  Of course that assumes userspace
is being intelligent and isn't blindly declaring permissions they don't
need, e.g. declaring RWX on all pages even though the enclave never
actually maps a RWX or RW->RX page.

One thought for handling this in a more user friendly fashion would be
to immediately return -EACCES instead of modifying @allowed_prot.  An
enclave that truly needs the permission would fail immediately.

An enclave loader that wants/needs to speculatively declare PROT_EXEC,
e.g. because the exact requirements of the enclave are unknown, could
handle -EACCESS gracefully by retrying the SGX ioctl() with different
@allowed_prot, e.g.:

  region.flags = SGX_ALLOW_READ | SGX_ALLOW_WRITE | SGX_ALLOW_EXEC;

  ret = ioctl(fd, SGX_IOC_ENCLAVE_ADD_REGION, &region);
  if (ret && errno == EACCES && !(prot & PROT_EXEC)) {
      region.flags &= ~SGX_ALLOW_EXEC;
      ret = ioctl(fd, SGX_IOC_ENCLAVE_ADD_REGION, &region);
  }

This type of enclave loader would still generate spurious audit messages,
but the spurious messages would be limited to enclave loaders that are
deliberately probing the allowed permissions.

> >The noexec case should be addressed in IOC_ADD_PAGES by testing
> >@source_vma->vm_flags & VM_MAYEXEC.
> >
> >>
> >>>* In hook security_file_free(), if @file is an  enclave, free storage
> >>>   allocated for WRITTEN flags.
> 

Re: [RFC PATCH 6/9] x86/sgx: Require userspace to provide allowed prots to ADD_PAGES

[Sean Christopherson]

On Tue, Jun 04, 2019 at 07:23:06PM +0300, Jarkko Sakkinen wrote:
> On Fri, May 31, 2019 at 04:31:56PM -0700, Sean Christopherson wrote:
> > ...to support (the equivalent) of existing Linux Security Module
> > functionality.
> 
> Long and short descriptions should be separate. Also this does not
> make any sense. LSM is a framework with a set of hook to make access
> decisions and there various implementations of it.
> 
> How this replicates LSMs and why that even would be a goal?
> 
> My guess is that you are trying to do something else. I'm just saying
> that the idea to do equivalent of LSMs to another subsystems would be
> insane if it was done.

Heh, yeah, it's not duplicating LSM functionality.  What I was trying to
say is that this patch allows LSMs to implement policies that are
equivalent to their existing functionality, e.g. paves the way to add
security_enclave_load() as an equivalent to security_file_mprotect().

> > always be MAP_SHARED.  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 any reasonable granularity, e.g. an LSM can
> > deny access to EPC altogether, but can't deny potentially dangerous
> > behavior such as mapping pages RW->RW or RWX.
> 
> The mapping must be shared given that it is iomem but why enclave pages
> would need RWX for all pages? The information that is missing from this
> paragraph is the explanation why an LSM could not deny dangerous
> behavior in PTE level.

I'll add that.

> > To give LSMs enough information to implement their policies without
> > having to resort to ugly things, e.g. holding a reference to the vm_file
> > of each enclave page, require userspace to explicitly state the allowed
> > protections for each page (region), i.e. take ALLOW_{READ,WRITE,EXEC}
> > in the ADD_PAGES ioctl.
> 
> I would keep descriptions such as "ugly things" away from commit
> messages as it is easy way to be not clear and explicit what you are
> trying to say.
> 
> > The ALLOW_* flags will be passed to LSMs so that they can make informed
> > decisions when the enclave is being built, i.e. when the source vm_file
> > is available.  For example, SELinux's EXECMOD permission can be
> > required if an enclave is requesting both ALLOW_WRITE and ALLOW_EXEC.
> 
> There should be some explanation what ALLOW_* flag are. It is now like
> as it was in common knowledge. SECINFO already has protection flags to
> name an example and without any explanation all of this is just very
> confusing.

Noted.

> This should address SECINFO and ALLOW_* relationship and differences.
> 
> > Update the mmap()/mprotect() hooks to enforce the ALLOW_* protections,
> > a la the standard VM_MAY{READ,WRITE,EXEC} flags.
> > 
> > The ALLOW_EXEC flag also has a second (important) 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 from a noexec path using read-only permissions and later mprotect()
> > and ENCLU[EMODPE] the page to gain execute permissions.  By requiring
> > ALLOW_EXEC up front, SGX will be able to enforce noexec paths when
> > building the enclave.
> > 
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > ---
> >  arch/x86/include/uapi/asm/sgx.h        |  9 ++++++++-
> >  arch/x86/kernel/cpu/sgx/driver/ioctl.c | 23 +++++++++++++++++------
> >  arch/x86/kernel/cpu/sgx/encl.c         |  2 +-
> >  arch/x86/kernel/cpu/sgx/encl.h         |  1 +
> >  4 files changed, 27 insertions(+), 8 deletions(-)
> > 
> > diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
> > index 4a12d6abbcb7..4489e92fa0dc 100644
> > --- a/arch/x86/include/uapi/asm/sgx.h
> > +++ b/arch/x86/include/uapi/asm/sgx.h
> > @@ -31,6 +31,11 @@ struct sgx_enclave_create  {
> >  	__u64	src;
> >  };
> >  
> > +/* Supported flags for struct sgx_enclave_add_pages. */
> > +#define SGX_ALLOW_READ	VM_READ
> > +#define SGX_ALLOW_WRITE	VM_WRITE
> > +#define SGX_ALLOW_EXEC	VM_EXEC
> 
> Why these flags are even defined if they are the same as VM_* flags?

Brain fart.  Flags can just take PROT_{READ,WRITE,EXEC}.

> > +
> >  /**
> >   * struct sgx_enclave_add_pages - parameter structure for the
> >   *                                %SGX_IOC_ENCLAVE_ADD_PAGES ioctl
> > @@ -39,6 +44,7 @@ struct sgx_enclave_create  {
> >   * @secinfo:	address for the SECINFO data (common to all pages)
> >   * @nr_pages:	number of pages (must be virtually contiguous)
> >   * @mrmask:	bitmask for the measured 256 byte chunks (common to all pages)
> > + * @flags:	flags, e.g. SGX_ALLOW_{READ,WRITE,EXEC} (common to all pages)
> >   */
> >  struct sgx_enclave_add_pages {
> >  	__u64	addr;
> > @@ -46,7 +52,8 @@ struct sgx_enclave_add_pages {
> >  	__u64	secinfo;
> >  	__u32	nr_pages;
> >  	__u16	mrmask;
> > -} __attribute__((__packed__));
> > +	__u16	flags;
> > +};
> >  

...

> > @@ -576,12 +578,20 @@ static int __sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
> >  
> >  static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
> >  			     unsigned long src, struct sgx_secinfo *secinfo,
> > -			     unsigned int mrmask)
> > +			     unsigned int mrmask, unsigned int flags)
> >  {
> > +	unsigned long prot = secinfo->flags & (VM_READ | VM_WRITE | VM_EXEC);
> 
> Even if the secinfo flags have the exactly the same values you should
> not do this as they are kind of from different type. This is confusing
> to read.

I can add a dummy helper to translate flags and encapsulate the below
assert.

> > +	unsigned long allowed_prot = flags & (VM_READ | VM_WRITE | VM_EXEC);
> 
> Why you take the trouble defining those macros and do not then use them
> even yourself?

The original thought was to define them for userspace, but that's broken
because VM_* aren't defined for userspace.

> >  	struct page *data_page;
> >  	void *data;
> >  	int ret;
> >  
> > +	BUILD_BUG_ON(SGX_SECINFO_R != VM_READ || SGX_SECINFO_W != VM_WRITE ||
> > +		     SGX_SECINFO_X != VM_EXEC);
> 
> Why this check?

To assert that the hardware defined SECINFO flags are interchangeable with
Linux's software defined flags, i.e. don't need to be translated.

> 
> > +
> > +	if (prot & ~allowed_prot)
> > +		return -EACCES;
> > +
> >  	data_page = alloc_page(GFP_HIGHUSER);
> >  	if (!data_page)
> >  		return -ENOMEM;
> > @@ -593,7 +603,8 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
> >  		goto out;
> >  	}
> >  
> > -	ret = __sgx_encl_add_page(encl, addr, data, secinfo, mrmask);
> > +	ret = __sgx_encl_add_page(encl, addr, data, secinfo, mrmask,
> > +				  allowed_prot);
> >  out:
> >  	kunmap(data_page);
> >  	__free_page(data_page);
> > @@ -645,7 +656,7 @@ static long sgx_ioc_enclave_add_pages(struct file *filep, unsigned int cmd,
> >  
> >  		ret = sgx_encl_add_page(encl, addp->addr + i*PAGE_SIZE,
> >  					addp->src + i*PAGE_SIZE,
> > -					&secinfo, addp->mrmask);
> > +					&secinfo, addp->mrmask, addp->flags);
> >  	}
> >  	return ret;
> >  }
> > diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
> > index 955d4f430adc..e5847571a265 100644
> > --- a/arch/x86/kernel/cpu/sgx/encl.c
> > +++ b/arch/x86/kernel/cpu/sgx/encl.c
> > @@ -249,7 +249,7 @@ int sgx_map_allowed(struct sgx_encl *encl, unsigned long start,
> >  
> >  	for (addr = start; addr < end; addr += PAGE_SIZE) {
> >  		page = radix_tree_lookup(&encl->page_tree, addr >> PAGE_SHIFT);
> > -		if (!page)
> > +		if (!page || (prot & ~page->allowed_prot))
> >  			return -EACCES;
> >  	}
> 
> However this goes it would be good idea to have only ony patch in the
> patch set that fully defines this function. Impossible to review
> properly with this split.

Sorry, I don't understand what you're suggesting.

> 
> >  
> > 
> > diff --git a/arch/x86/kernel/cpu/sgx/encl.h b/arch/x86/kernel/cpu/sgx/encl.h
> > index 6e310e3b3fff..7cca076a4987 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 allowed_prot;
> >  	struct sgx_epc_page *epc_page;
> >  	struct sgx_va_page *va_page;
> >  	struct sgx_encl *encl;
> > -- 
> > 2.21.0
> > 
> 
> This patch left me very confused. I don't get it.
> 
> /Jarkko

Re: [RFC PATCH 2/9] x86/sgx: Do not naturally align MAP_FIXED address

[Andy Lutomirski]

On Tue, Jun 4, 2019 at 4:50 AM Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
>
> On Fri, May 31, 2019 at 04:31:52PM -0700, Sean Christopherson wrote:
> > 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>
>
> Why you want to allow mmap() to be called multiple times? mmap() could
> be allowed only once with PROT_NONE and denied afterwards. Is this for
> sending fd to another process that would map already existing enclave?
>
> I don't see any checks for whether the is enclave underneath. Also, I
> think that in all cases mmap() callback should allow only PROT_NONE
> as permissions for clarity even if it could called multiple times.
>

What's the advantage to only allowing PROT_NONE?  The idea here is to
allow a PROT_NONE map followed by some replacemets that overlay it for
the individual segments.  Admittedly, mprotect() can do the same
thing, but disallowing mmap() seems at least a bit surprising.

Re: [RFC PATCH 3/9] x86/sgx: Allow userspace to add multiple pages in single ioctl()

[Andy Lutomirski]

On Mon, Jun 3, 2019 at 1:39 PM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> On Mon, Jun 03, 2019 at 01:08:04PM -0700, Sean Christopherson wrote:
> > On Sun, Jun 02, 2019 at 11:26:09PM -0700, Xing, Cedric wrote:
> > > > From: Christopherson, Sean J
> > > > Sent: Friday, May 31, 2019 4:32 PM
> > > >
> > > > +/**
> > > > + * sgx_ioc_enclave_add_pages - handler for %SGX_IOC_ENCLAVE_ADD_PAGES
> > > > + *
> > > > + * @filep:       open file to /dev/sgx
> > > > + * @cmd: the command value
> > > > + * @arg: pointer to an &sgx_enclave_add_page instance
> > > > + *
> > > > + * Add a range of pages to an uninitialized enclave (EADD), and
> > > > +optionally
> > > > + * extend the enclave's measurement with the contents of the page (EEXTEND).
> > > > + * The range of pages must be virtually contiguous.  The SECINFO and
> > > > + * measurement maskare applied to all pages, i.e. pages with different
> > > > + * properties must be added in separate calls.
> > > > + *
> > > > + * EADD and EEXTEND are done asynchronously via worker threads.  A
> > > > +successful
> > > > + * sgx_ioc_enclave_add_page() only indicates the pages have been added
> > > > +to the
> > > > + * work queue, it does not guarantee adding the pages to the enclave
> > > > +will
> > > > + * succeed.
> > > > + *
> > > > + * Return:
> > > > + *   0 on success,
> > > > + *   -errno otherwise
> > > > + */
> > > > +static long sgx_ioc_enclave_add_pages(struct file *filep, unsigned int cmd,
> > > > +                               unsigned long arg)
> > > > +{
> > > > + struct sgx_enclave_add_pages *addp = (void *)arg;
> > > > + struct sgx_encl *encl = filep->private_data;
> > > > + struct sgx_secinfo secinfo;
> > > > + unsigned int i;
> > > > + int ret;
> > > > +
> > > > + if (copy_from_user(&secinfo, (void __user *)addp->secinfo,
> > > > +                    sizeof(secinfo)))
> > > > +         return -EFAULT;
> > > > +
> > > > + for (i = 0, ret = 0; i < addp->nr_pages && !ret; i++) {
> > > > +         if (signal_pending(current))
> > > > +                 return -ERESTARTSYS;
> > >
> > > If interrupted, how would user mode code know how many pages have been EADD'ed?
> >
> > Hmm, updating nr_pages would be fairly simple and shouldn't confuse
> > userspace, e.g. as opposed to overloading the return value.
>
> Or maybe update @addr and @src as well?  That would allow userspace to
> re-invoke the ioctl() without having to modify the struct.

If you're going to use -ERESTARTSYS, that's the way to go.  -EINTR
would be an alternative.  A benefit of -ERESTARTSYS is that, with
-EINTR, it wouldn't be that surprising for user code to simply fail to
handle it.

Re: [RFC PATCH 6/9] x86/sgx: Require userspace to provide allowed prots to ADD_PAGES

[Andy Lutomirski]

On Fri, May 31, 2019 at 4:32 PM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> ...to support (the equivalent) of existing Linux Security Module
> functionality.
>
> Because SGX manually manages EPC memory, all enclave VMAs are backed by
> the same vm_file, i.e. /dev/sgx/enclave, so that SGX can implement the
> necessary hooks to move pages in/out of the EPC.  And because EPC pages
> for any given enclave are fundamentally shared between processes, i.e.
> CoW semantics are not possible with EPC pages, /dev/sgx/enclave must
> always be MAP_SHARED.  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 any reasonable granularity, e.g. an LSM can
> deny access to EPC altogether, but can't deny potentially dangerous
> behavior such as mapping pages RW->RW or RWX.
>
> To give LSMs enough information to implement their policies without
> having to resort to ugly things, e.g. holding a reference to the vm_file
> of each enclave page, require userspace to explicitly state the allowed
> protections for each page (region), i.e. take ALLOW_{READ,WRITE,EXEC}
> in the ADD_PAGES ioctl.
>
> The ALLOW_* flags will be passed to LSMs so that they can make informed
> decisions when the enclave is being built, i.e. when the source vm_file
> is available.  For example, SELinux's EXECMOD permission can be
> required if an enclave is requesting both ALLOW_WRITE and ALLOW_EXEC.
>
> Update the mmap()/mprotect() hooks to enforce the ALLOW_* protections,
> a la the standard VM_MAY{READ,WRITE,EXEC} flags.
>
> The ALLOW_EXEC flag also has a second (important) 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 from a noexec path using read-only permissions and later mprotect()
> and ENCLU[EMODPE] the page to gain execute permissions.  By requiring
> ALLOW_EXEC up front, SGX will be able to enforce noexec paths when
> building the enclave.
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/include/uapi/asm/sgx.h        |  9 ++++++++-
>  arch/x86/kernel/cpu/sgx/driver/ioctl.c | 23 +++++++++++++++++------
>  arch/x86/kernel/cpu/sgx/encl.c         |  2 +-
>  arch/x86/kernel/cpu/sgx/encl.h         |  1 +
>  4 files changed, 27 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
> index 4a12d6abbcb7..4489e92fa0dc 100644
> --- a/arch/x86/include/uapi/asm/sgx.h
> +++ b/arch/x86/include/uapi/asm/sgx.h
> @@ -31,6 +31,11 @@ struct sgx_enclave_create  {
>         __u64   src;
>  };
>
> +/* Supported flags for struct sgx_enclave_add_pages. */
> +#define SGX_ALLOW_READ VM_READ
> +#define SGX_ALLOW_WRITE        VM_WRITE
> +#define SGX_ALLOW_EXEC VM_EXEC
> +
>  /**
>   * struct sgx_enclave_add_pages - parameter structure for the
>   *                                %SGX_IOC_ENCLAVE_ADD_PAGES ioctl
> @@ -39,6 +44,7 @@ struct sgx_enclave_create  {
>   * @secinfo:   address for the SECINFO data (common to all pages)
>   * @nr_pages:  number of pages (must be virtually contiguous)
>   * @mrmask:    bitmask for the measured 256 byte chunks (common to all pages)
> + * @flags:     flags, e.g. SGX_ALLOW_{READ,WRITE,EXEC} (common to all pages)
>   */
>  struct sgx_enclave_add_pages {
>         __u64   addr;
> @@ -46,7 +52,8 @@ struct sgx_enclave_add_pages {
>         __u64   secinfo;
>         __u32   nr_pages;
>         __u16   mrmask;
> -} __attribute__((__packed__));
> +       __u16   flags;

Minor nit: I would use at least u32 for flags.  It's not like the size
saving is important.

>  static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
>                              unsigned long src, struct sgx_secinfo *secinfo,
> -                            unsigned int mrmask)
> +                            unsigned int mrmask, unsigned int flags)
>  {
> +       unsigned long prot = secinfo->flags & (VM_READ | VM_WRITE | VM_EXEC);
> +       unsigned long allowed_prot = flags & (VM_READ | VM_WRITE | VM_EXEC);

...

> +       if (prot & ~allowed_prot)
> +               return -EACCES;

This seems like a well-intentioned sanity check, but it doesn't really
accomplish anything with SGX2, so I tend to think it would be better
to omit it.

Re: [RFC PATCH 7/9] x86/sgx: Enforce noexec filesystem restriction for enclaves

[Andy Lutomirski]

On Tue, Jun 4, 2019 at 9:26 AM Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
>
> On Fri, May 31, 2019 at 04:31:57PM -0700, Sean Christopherson wrote:
> > Do not allow an enclave page to be mapped with PROT_EXEC if the source
> > page is backed by a file on a noexec file system.
> >
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
>
> Why don't you just check in sgx_encl_add_page() that whether the path
> comes from noexec and deny if SECINFO contains X?
>

SECINFO seems almost entirely useless for this kind of thing because
of SGX2.  I'm thinking that SECINFO should be completely ignored for
anything other than its required architectural purpose.

Re: [RFC PATCH 7/9] x86/sgx: Enforce noexec filesystem restriction for enclaves

[Andy Lutomirski]

On Sun, Jun 2, 2019 at 11:29 PM Xing, Cedric <cedric.xing@intel.com> wrote:
>
> > From: Christopherson, Sean J
> > Sent: Friday, May 31, 2019 4:32 PM
> >
> > Do not allow an enclave page to be mapped with PROT_EXEC if the source page is backed by a
> > file on a noexec file system.
> >
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > ---
> >  arch/x86/kernel/cpu/sgx/driver/ioctl.c | 26 ++++++++++++++++++++++++--
> >  1 file changed, 24 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> > b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> > index c30acd3fbbdd..5f71be7cbb01 100644
> > --- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> > +++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> > @@ -576,6 +576,27 @@ static int __sgx_encl_add_page(struct sgx_encl *encl, unsigned long
> > addr,
> >       return ret;
> >  }
> >
> > +static int sgx_encl_page_protect(unsigned long src, unsigned long prot,
> > +                              unsigned long *allowed_prot)
> > +{
> > +     struct vm_area_struct *vma;
> > +
> > +     if (!(*allowed_prot & VM_EXEC))
> > +             goto do_check;
> > +
> > +     down_read(&current->mm->mmap_sem);
> > +     vma = find_vma(current->mm, src);
> > +     if (!vma || (vma->vm_file && path_noexec(&vma->vm_file->f_path)))
> > +             *allowed_prot &= ~VM_EXEC;
>
> Testing (vma->vm_flags & VM_MAYEXEC) == 0 should be a better approach.

I think I agree, although that would need a comment explaining why it works.

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

[Andy Lutomirski]

On Fri, May 31, 2019 at 4:32 PM Sean Christopherson
<sean.j.christopherson@intel.com> 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, 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.  The notable
> difference from file_mprotect() is the allowed_prot parameter, which
> is essentially an SGX-specific version of a VMA's MAY_{READ,WRITE,EXEC}
> flags.  The purpose of allowed_prot is to enable checks such as
> SELinux's FILE__EXECMOD permission without having to track and update
> VMAs across multiple mm structs, i.e. SGX can ensure userspace doesn't
> overstep its bounds simply by restricting an enclave VMA's protections
> by vetting what is maximally allowed during build time.
>
> An alternative to the allowed_prot approach would be to use an enclave's
> SIGSTRUCT (a smallish structure that can uniquely identify an enclave)
> as a proxy for the enclave.  For example, SGX could take and hold a
> reference to the file containing the SIGSTRUCT (if it's in a file) and
> call security_enclave_load() during mprotect().  While the SIGSTRUCT
> approach would provide better precision, the actual value added was
> deemed to be negligible.  On the other hand, pinning a file for the
> lifetime of the enclave is ugly, and essentially caching LSM policies
> in each page's allowed_prot avoids having to make an extra LSM upcall
> during mprotect().
>
> 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 | 14 +++++++++-----
>  include/linux/lsm_hooks.h              | 16 ++++++++++++++++
>  include/linux/security.h               |  2 ++
>  security/security.c                    |  8 ++++++++
>  4 files changed, 35 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> index 5f71be7cbb01..260417ecbcff 100644
> --- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> +++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> @@ -8,6 +8,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>
> @@ -580,21 +581,24 @@ static int sgx_encl_page_protect(unsigned long src, unsigned long prot,
>                                  unsigned long *allowed_prot)
>  {
>         struct vm_area_struct *vma;
> +       int ret = 0;
>
> -       if (!(*allowed_prot & VM_EXEC))
> +       if (!(*allowed_prot & VM_EXEC) && !IS_ENABLED(CONFIG_SECURITY))
>                 goto do_check;
>
>         down_read(&current->mm->mmap_sem);
>         vma = find_vma(current->mm, src);
>         if (!vma || (vma->vm_file && path_noexec(&vma->vm_file->f_path)))
>                 *allowed_prot &= ~VM_EXEC;
> +#ifdef CONFIG_SECURITY
> +       ret = security_enclave_load(vma, prot, allowed_prot);
> +#endif
>         up_read(&current->mm->mmap_sem);
>
>  do_check:
> -       if (prot & ~*allowed_prot)
> -               return -EACCES;
> -
> -       return 0;
> +       if (!ret && (prot & ~*allowed_prot))
> +               ret = -EACCES;
> +       return ret;
>  }
>
>  static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 47f58cfb6a19..0562775424a0 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -1446,6 +1446,14 @@
>   * @bpf_prog_free_security:
>   *     Clean up the security information stored inside bpf prog.
>   *
> + * Security hooks for Intel SGX enclaves.
> + *
> + * @enclave_load:
> + *     On success, returns 0 and optionally adjusts @allowed_prot
> + *     @vma: the source memory region of the enclave page being loaded.
> + *     @prot: the initial protection of the enclave page.

What do you mean "initial"?  The page is always mapped PROT_NONE when
this is called, right?  I feel like I must be missing something here.

Re: [RFC PATCH 7/9] x86/sgx: Enforce noexec filesystem restriction for enclaves

[Sean Christopherson]

On Tue, Jun 04, 2019 at 01:25:10PM -0700, Andy Lutomirski wrote:
> On Tue, Jun 4, 2019 at 9:26 AM Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> >
> > On Fri, May 31, 2019 at 04:31:57PM -0700, Sean Christopherson wrote:
> > > Do not allow an enclave page to be mapped with PROT_EXEC if the source
> > > page is backed by a file on a noexec file system.
> > >
> > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> >
> > Why don't you just check in sgx_encl_add_page() that whether the path
> > comes from noexec and deny if SECINFO contains X?
> >
> 
> SECINFO seems almost entirely useless for this kind of thing because
> of SGX2.  I'm thinking that SECINFO should be completely ignored for
> anything other than its required architectural purpose.

Agreed.

I've already (somewhat unknowingly) reworked the SELinux patch such that
it ignores @prot (the SECINFO protections) and only looks at @allowed_prot
(the declared protections).  If the kernel ignores SECINFO protections
entirely then the LSM hook can simply be:

  int selinux_enclave_load(struct vm_area_struct *vma, unsigned long prot)

I.e. LSMs can be blissfully unaware that @prot isn't technically what's
going into the PTEs *now*.

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

[Sean Christopherson]

On Tue, Jun 04, 2019 at 01:29:10PM -0700, Andy Lutomirski wrote:
> On Fri, May 31, 2019 at 4:32 PM Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
> >  static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
> > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> > index 47f58cfb6a19..0562775424a0 100644
> > --- a/include/linux/lsm_hooks.h
> > +++ b/include/linux/lsm_hooks.h
> > @@ -1446,6 +1446,14 @@
> >   * @bpf_prog_free_security:
> >   *     Clean up the security information stored inside bpf prog.
> >   *
> > + * Security hooks for Intel SGX enclaves.
> > + *
> > + * @enclave_load:
> > + *     On success, returns 0 and optionally adjusts @allowed_prot
> > + *     @vma: the source memory region of the enclave page being loaded.
> > + *     @prot: the initial protection of the enclave page.
> 
> What do you mean "initial"?  The page is always mapped PROT_NONE when
> this is called, right?  I feel like I must be missing something here.

Initial protection in the EPCM.  Yet another reason to ignore SECINFO.

RE: [RFC PATCH 0/9] security: x86/sgx: SGX vs. LSM

[Xing, Cedric]

Hi Stephen,

> From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx-
> owner@vger.kernel.org] On Behalf Of Stephen Smalley
> Sent: Tuesday, June 04, 2019 8:34 AM
> 
> On 6/3/19 2:30 PM, Xing, Cedric wrote:
> >> From: Christopherson, Sean J
> >> Sent: Monday, June 03, 2019 10:16 AM
> >>
> >> On Sun, Jun 02, 2019 at 12:29:35AM -0700, Xing, Cedric wrote:
> >>> Hi Sean,
> >>>
> >>> Generally I agree with your direction but think ALLOW_* flags are
> >>> completely internal to LSM because they can be both produced and
> >>> consumed inside an LSM module. So spilling them into SGX driver and
> >>> also user mode code makes the solution ugly and in some cases
> >>> impractical because not every enclave host process has a priori
> >>> knowledge on whether or not an enclave page would be EMODPE'd at
> >> runtime.
> >>
> >> In this case, the host process should tag *all* pages it *might*
> convert
> >> to executable as ALLOW_EXEC.  LSMs can (and should/will) be written
> in
> >> such a way that denying ALLOW_EXEC is fatal to the enclave if and
> only
> >> if the enclave actually attempts mprotect(PROT_EXEC).
> >
> > What if those pages contain self-modifying code but the host doesn't
> know ahead of time? Would it require ALLOW_WRITE|ALLOW_EXEC at EADD?
> Then would it prevent those pages to start with PROT_EXEC?
> >
> > Anyway, my point is that it is unnecessary even if it works.
> >
> >>
> >> Take the SELinux path for example.  The only scenario in which
> >> PROT_WRITE is cleared from @allowed_prot is if the page *starts* with
> >> PROT_EXEC.
> >> If PROT_EXEC is denied on a page that starts RW, e.g. an EAUG'd page,
> >> then PROT_EXEC will be cleared from @allowed_prot.
> >>
> >> As Stephen pointed out, auditing the denials on @allowed_prot means
> the
> >> log will contain false positives of a sort.  But this is more of a
> noise
> >> issue than true false positives.  E.g. there are three possible
> outcomes
> >> for the enclave.
> >>
> >>    - The enclave does not do EMODPE[PROT_EXEC] in any scenario, ever.
> >>      Requesting ALLOW_EXEC is either a straightforward a userspace
> bug or
> >>      a poorly written generic enclave loader.
> >>
> >>    - The enclave conditionally performs EMODPE[PROT_EXEC].  In this
> case
> >>      the denial is a true false positive.
> >>
> >>    - The enclave does EMODPE[PROT_EXEC] and its host userspace then
> fails
> >>      on mprotect(PROT_EXEC), i.e. the LSM denial is working as
> intended.
> >>      The audit log will be noisy, but viewed as a whole the denials
> >> aren't
> >>      false positives.
> >
> > What I was talking about was EMODPE[PROT_WRITE] on an RX page.
> >
> >>
> >> The potential for noisy audit logs and/or false positives is
> unfortunate,
> >> but it's (by far) the lesser of many evils.
> >>
> >>> Theoretically speaking, what you really need is a per page flag
> (let's
> >>> name it WRITTEN?) indicating whether a page has ever been written to
> >>> (or more precisely, granted PROT_WRITE), which will be used to
> decide
> >>> whether to grant PROT_EXEC when requested in future. Given the fact
> >>> that all mprotect() goes through LSM and mmap() is limited to
> >>> PROT_NONE, it's easy for LSM to capture that flag by itself instead
> of
> >> asking user mode code to provide it.
> >>>
> >>> That said, here is the summary of what I think is a better approach.
> >>> * In hook security_file_alloc(), if @file is an enclave, allocate
> some
> >> data
> >>>    structure to store for every page, the WRITTEN flag as described
> >> above.
> >>>    WRITTEN is cleared initially for all pages.
> >>
> >> This would effectively require *every* LSM to duplicate the SGX
> driver's
> >> functionality, e.g. track per-page metadata, implement locking to
> >> prevent races between multiple mm structs, etc...
> >
> > Architecturally we shouldn't dictate how LSM makes decisions. ALLOW_*
> are no difference than PROCESS__* or FILE__* flags, which are just
> artifacts to assist particular LSMs in decision making. They are never
> considered part of the LSM interface, even if other LSMs than SELinux
> may adopt the same/similar approach.
> >
> > If code duplication is what you are worrying about, you can put them
> in a library, or implement/export them in some new file (maybe
> security/enclave.c?) as utility functions. But spilling them into user
> mode is what I think is unacceptable.
> >
> >>
> >>>    Open: Given a file of type struct file *, how to tell if it is an
> >> enclave (i.e. /dev/sgx/enclave)?
> >>> * In hook security_mmap_file(), if @file is an enclave, make sure
> >> @prot can
> >>>    only be PROT_NONE. This is to force all protection changes to go
> >> through
> >>>    security_file_mprotect().
> >>> * In the newly introduced hook security_enclave_load(), set WRITTEN
> >> for pages
> >>>    that are requested PROT_WRITE.
> >>
> >> How would an LSM associate a page with a specific enclave?  vma-
> >vm_file
> >> will point always point at /dev/sgx/enclave.  vma->vm_mm is useless
> >> because we're allowing multiple processes to map a single enclave,
> not
> >> to mention that by mm would require holding a reference to the mm.
> >
> > Each open("/dev/sgx/enclave") syscall creates a *new* instance of
> struct file to uniquely identify one enclave instance. What I mean is
> @vma->vm_file, not @vma->vm_file->f_path or @vma->vm_file->f_inode.
> >
> >>
> >>> * In hook security_file_mprotect(), if @vma->vm_file is an enclave,
> >> look up
> >>>    and use WRITTEN flags for all pages within @vma, along with other
> >> global
> >>>    flags (e.g. PROCESS__EXECMEM/FILE__EXECMOD in the case of SELinux)
> >> to decide
> >>>    on allowing/rejecting @prot.
> >>
> >> vma->vm_file will always be /dev/sgx/enclave at this point, which
> means
> >> LSMs don't have the necessary anchor back to the source file, e.g. to
> >> enforce FILE__EXECUTE.  The noexec file system case is also
> unaddressed.
> >
> > vma->vm_file identifies an enclave instance uniquely. FILE__EXECUTE is
> checked by security_enclave_load() using @source_vma->vm_file. Once a
> page has been EADD'ed, whether to allow RW->RX depends on .sigstruct
> file (more precisely, the file backing SIGSTRUCT), whose FILE__*
> attributes could be cached in vma->vm_file->f_security by
> security_enclave_init().
> 
> The RFC series seemed to dispense with the use of the sigstruct file and
> just used the source file throughout IIUC.  That allowed for reuse of
> FILE__* permissions without ambiguity rather than introducing separate
> ENCLAVE__* permissions or using /dev/sgx/enclave inode as the target of
> all checks.

That's right. But that's not the distinction between Sean's patch and my proposal.

My point here is, from the perspective of LSM architecture, LSM hooks shall be defined to pass adequate information to allow *all* possible implementations to make reasonable decisions. In particular, all parameters to EADD (i.e. target linear address, SECINFO, source page) could affect (current and future) decisions but Sean's definition of security_enclave_load() passes only the source, which limits the possibility of other implementations. Another point I'm trying to make is, different LSM implementations may need different information at any given decision point, therefore it's *not* possible to always pass "right" information at "right" time. And that's why I think .f_security was added to struct file to allow stateful LSMs. Sean's patch however is trying the opposite, as ALLOW_* flags should otherwise be part of internal state of LSMs, but they are spilled into SGX driver and also userspace. 

> 
> Regardless, IIUC, your approach requires that we always check
> FILE__EXECMOD, and FILE__EXECUTE up front during security_enclave_load()
> irrespective of prot so that we can save the result in the f_security
> for later use by the mprotect hook.  This may generate many spurious
> audit messages for cases where PROT_EXEC will never be requested, and
> users will be prone to just always allowing it since they cannot tell
> when it was actually needed.

Yes and no. 

For those not following this discussion closely, here's the prototype of security_enclave_load() that I proposed in one of my earlier emails.

int security_enclave_load(struct file *enclave_fd, unsigned long linear_address, unsigned long nr_pages, int prot, struct vm_area_struct *source_vma);

@enclave_fd identifies the enclave to which new pages are being added.
@linear_address/@nr_pages specifies the linear range of pages being added.
@prot specifies the initial protection of those newly added pages. It is taken from the vma covering the target range.
@source_vma covers the source pages in the case of EADD. An LSM is expected to make sure security_file_mprotect(source_vma, prot, prot) would succeed before checking anything else, unless @source_vma is NULL, indicating pages are being EAUG'ed. In all cases, LSM is expected to "remember" @prot for all those pages to be checked in future security_file_mprotect() invocations.

Architecture wise, the idea here is for SGX driver to pass in all information relevant for an LSM's decision. 

Implementation wise, LSM may allow PROT_EXEC depending on FILE__EXECUTE of the source file (@source_vma->vm_file), or the sigstruct file (will be provided to LSM at security_enclave_init), or /dev/sgx/enclave. It makes most sense to me to use the source file, hence the check would most likely be done here. For future security_file_mprotect(), the source file's FILE__EXECMOD could also be cached here, or it could use sigstruct file's FILE__EXECMOD instead. Given the fact that no EPC pages could be accessed before EINIT, the major purpose of security_enclave_load() is for LSM to cache whatever information deemed appropriate for the pages being EADD'ed (i.e. @source_vma != NULL) so that it has necessary information to make decisions in security_file_mprotect() in future. And in that regard the parameter @prot is unnecessary but I decided to have it here for 2 reasons: 1) Pages may be EADD'ed within an existing VMA (so no upcoming mprotect) so LSM's decision is needed right away and 2) @source_vma may not be able to mprotect(@prot) and in that case it'd be better to fail EADD instead of failing at mprotect() later.

> 
> >
> > The noexec case should be addressed in IOC_ADD_PAGES by testing
> @source_vma->vm_flags & VM_MAYEXEC.
> >
> >>
> >>> * In hook security_file_free(), if @file is an  enclave, free
> storage
> >>>    allocated for WRITTEN flags.

RE: [RFC PATCH 8/9] LSM: x86/sgx: Introduce ->enclave_load() hook for Intel SGX

[Xing, Cedric]

> From: Christopherson, Sean J
> Sent: Tuesday, June 04, 2019 1:37 PM
> 
> On Tue, Jun 04, 2019 at 01:29:10PM -0700, Andy Lutomirski wrote:
> > On Fri, May 31, 2019 at 4:32 PM Sean Christopherson
> > <sean.j.christopherson@intel.com> wrote:
> > >  static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long
> > > addr, diff --git a/include/linux/lsm_hooks.h
> > > b/include/linux/lsm_hooks.h index 47f58cfb6a19..0562775424a0 100644
> > > --- a/include/linux/lsm_hooks.h
> > > +++ b/include/linux/lsm_hooks.h
> > > @@ -1446,6 +1446,14 @@
> > >   * @bpf_prog_free_security:
> > >   *     Clean up the security information stored inside bpf prog.
> > >   *
> > > + * Security hooks for Intel SGX enclaves.
> > > + *
> > > + * @enclave_load:
> > > + *     On success, returns 0 and optionally adjusts @allowed_prot
> > > + *     @vma: the source memory region of the enclave page being
> loaded.
> > > + *     @prot: the initial protection of the enclave page.
> >
> > What do you mean "initial"?  The page is always mapped PROT_NONE when
> > this is called, right?  I feel like I must be missing something here.
> 
> Initial protection in the EPCM.  Yet another reason to ignore SECINFO.

I know you guys are talking in the background that all pages are mmap()'ed PROT_NONE. But that's an unnecessary limitation. And @prot here should be @target_vma->vm_flags&(VM_READ|VM_WRITE|VM_EXEC). 

RE: [RFC PATCH 7/9] x86/sgx: Enforce noexec filesystem restriction for enclaves

[Xing, Cedric]

> From: Andy Lutomirski [mailto:luto@kernel.org]
> Sent: Tuesday, June 04, 2019 1:25 PM
> To: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> 
> On Tue, Jun 4, 2019 at 9:26 AM Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> >
> > On Fri, May 31, 2019 at 04:31:57PM -0700, Sean Christopherson wrote:
> > > Do not allow an enclave page to be mapped with PROT_EXEC if the
> > > source page is backed by a file on a noexec file system.
> > >
> > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> >
> > Why don't you just check in sgx_encl_add_page() that whether the path
> > comes from noexec and deny if SECINFO contains X?
> >
> 
> SECINFO seems almost entirely useless for this kind of thing because of
> SGX2.  I'm thinking that SECINFO should be completely ignored for
> anything other than its required architectural purpose.

For the purpose of allowing/denying EADD/EAUG, SECINFO is useless. 

But SECINFO contains also the page type. What's coming as new feature of SGX2 is CONFIGID, which is a 512-bit value inside SECS, provided by untrusted code at ECREATE. Usually CONFIGID is a hash of something that would affect the behavior of the enclave. For example, the "main" enclave could be a JVM with the actual applet being loaded hashed into SECS.CONFIGID. In that case the enclave's measurements (MRENCLAVE) will stay the same for all applets yet individual applet will have distinct CONFIGID and receive distinct keys. When it comes to LSM, a policy may want to whitelist/blacklist applets for a JVM so a hook at ECREATE may be desirable. We could either define a new hook, or overload security_enclave_load() by providing SECINFO as one of its parameters.

RE: [RFC PATCH 3/9] x86/sgx: Allow userspace to add multiple pages in single ioctl()

[Xing, Cedric]

> From: Andy Lutomirski [mailto:luto@kernel.org]
> Sent: Tuesday, June 04, 2019 1:18 PM
> 
> On Mon, Jun 3, 2019 at 1:39 PM Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
> >
> > On Mon, Jun 03, 2019 at 01:08:04PM -0700, Sean Christopherson wrote:
> > > On Sun, Jun 02, 2019 at 11:26:09PM -0700, Xing, Cedric wrote:
> > > > > From: Christopherson, Sean J
> > > > > Sent: Friday, May 31, 2019 4:32 PM
> > > > >
> > > > > +/**
> > > > > + * sgx_ioc_enclave_add_pages - handler for
> > > > > +%SGX_IOC_ENCLAVE_ADD_PAGES
> > > > > + *
> > > > > + * @filep:       open file to /dev/sgx
> > > > > + * @cmd: the command value
> > > > > + * @arg: pointer to an &sgx_enclave_add_page instance
> > > > > + *
> > > > > + * Add a range of pages to an uninitialized enclave (EADD), and
> > > > > +optionally
> > > > > + * extend the enclave's measurement with the contents of the
> page (EEXTEND).
> > > > > + * The range of pages must be virtually contiguous.  The
> > > > > +SECINFO and
> > > > > + * measurement maskare applied to all pages, i.e. pages with
> > > > > +different
> > > > > + * properties must be added in separate calls.
> > > > > + *
> > > > > + * EADD and EEXTEND are done asynchronously via worker threads.
> > > > > +A successful
> > > > > + * sgx_ioc_enclave_add_page() only indicates the pages have
> > > > > +been added to the
> > > > > + * work queue, it does not guarantee adding the pages to the
> > > > > +enclave will
> > > > > + * succeed.
> > > > > + *
> > > > > + * Return:
> > > > > + *   0 on success,
> > > > > + *   -errno otherwise
> > > > > + */
> > > > > +static long sgx_ioc_enclave_add_pages(struct file *filep,
> unsigned int cmd,
> > > > > +                               unsigned long arg) {  struct
> > > > > +sgx_enclave_add_pages *addp = (void *)arg;  struct sgx_encl
> > > > > +*encl = filep->private_data;  struct sgx_secinfo secinfo;
> > > > > +unsigned int i;  int ret;
> > > > > +
> > > > > + if (copy_from_user(&secinfo, (void __user *)addp->secinfo,
> > > > > +                    sizeof(secinfo)))
> > > > > +         return -EFAULT;
> > > > > +
> > > > > + for (i = 0, ret = 0; i < addp->nr_pages && !ret; i++) {
> > > > > +         if (signal_pending(current))
> > > > > +                 return -ERESTARTSYS;
> > > >
> > > > If interrupted, how would user mode code know how many pages have
> been EADD'ed?
> > >
> > > Hmm, updating nr_pages would be fairly simple and shouldn't confuse
> > > userspace, e.g. as opposed to overloading the return value.
> >
> > Or maybe update @addr and @src as well?  That would allow userspace to
> > re-invoke the ioctl() without having to modify the struct.
> 
> If you're going to use -ERESTARTSYS, that's the way to go.  -EINTR would
> be an alternative.  A benefit of -ERESTARTSYS is that, with -EINTR, it
> wouldn't be that surprising for user code to simply fail to handle it.

-EINTR means the call was interrupted before anything could be done. Am I correct?

But in this case some pages have been processed already so I guess we cannot return any error code. I think it more reasonable to return the number of pages (or bytes) processed.

RE: [RFC PATCH 2/9] x86/sgx: Do not naturally align MAP_FIXED address

[Xing, Cedric]

> From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx-
> owner@vger.kernel.org] On Behalf Of Andy Lutomirski
> Sent: Tuesday, June 04, 2019 1:16 PM
> 
> On Tue, Jun 4, 2019 at 4:50 AM Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> >
> > On Fri, May 31, 2019 at 04:31:52PM -0700, Sean Christopherson wrote:
> > > 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>
> >
> > Why you want to allow mmap() to be called multiple times? mmap() could
> > be allowed only once with PROT_NONE and denied afterwards. Is this for
> > sending fd to another process that would map already existing enclave?
> >
> > I don't see any checks for whether the is enclave underneath. Also, I
> > think that in all cases mmap() callback should allow only PROT_NONE as
> > permissions for clarity even if it could called multiple times.
> >
> 
> What's the advantage to only allowing PROT_NONE?  The idea here is to
> allow a PROT_NONE map followed by some replacemets that overlay it for
> the individual segments.  Admittedly, mprotect() can do the same thing,
> but disallowing mmap() seems at least a bit surprising.

Disallowing mmap() is not only surprising but also unnecessary.

A bit off topic here. This mmap()/mprotect() discussion reminds me a question (guess for Jarkko): Now that vma->vm_file->private_data keeps a pointer to the enclave, why do we store it again in vma->vm_private? It isn't a big deal but non-NULL vm_private does prevent mprotect() from merging adjacent VMAs. 

RE: [RFC PATCH 6/9] x86/sgx: Require userspace to provide allowed prots to ADD_PAGES

[Ayoun, Serge]

> From: Christopherson, Sean J
> Sent: Saturday, June 01, 2019 02:32
> 
>  /**
>   * struct sgx_enclave_add_pages - parameter structure for the
>   *                                %SGX_IOC_ENCLAVE_ADD_PAGES ioctl
> @@ -39,6 +44,7 @@ struct sgx_enclave_create  {
>   * @secinfo:	address for the SECINFO data (common to all pages)
>   * @nr_pages:	number of pages (must be virtually contiguous)
>   * @mrmask:	bitmask for the measured 256 byte chunks (common to all
> pages)
> + * @flags:	flags, e.g. SGX_ALLOW_{READ,WRITE,EXEC} (common to all
> pages)
>   */
>  struct sgx_enclave_add_pages {
>  	__u64	addr;
> @@ -46,7 +52,8 @@ struct sgx_enclave_add_pages {
>  	__u64	secinfo;
>  	__u32	nr_pages;
>  	__u16	mrmask;
> -} __attribute__((__packed__));
> +	__u16	flags;
> +};

You are adding a flags member. The secinfo structure has already a flags member in it.
Why do you need both - they are both coming from user mode. What kind of scenario would
require having different values. Seems confusing.

---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [RFC PATCH 2/9] x86/sgx: Do not naturally align MAP_FIXED address

[Sean Christopherson]

On Tue, Jun 04, 2019 at 03:10:22PM -0700, Xing, Cedric wrote:
> A bit off topic here. This mmap()/mprotect() discussion reminds me a question
> (guess for Jarkko): Now that vma->vm_file->private_data keeps a pointer to
> the enclave, why do we store it again in vma->vm_private? It isn't a big deal
> but non-NULL vm_private does prevent mprotect() from merging adjacent VMAs. 

vma->vm_ops->close also prevents merging, and we need that to refcount the
enclave and mm.

We also rely on nullifying vma->vm_private_data in the unlikely event that
adding a new mm to the enclave fails on kzalloc().

Re: [RFC PATCH 6/9] x86/sgx: Require userspace to provide allowed prots to ADD_PAGES

[Jarkko Sakkinen]

On Tue, Jun 04, 2019 at 09:45:14AM -0700, Sean Christopherson wrote:
> Heh, yeah, it's not duplicating LSM functionality.  What I was trying to
> say is that this patch allows LSMs to implement policies that are
> equivalent to their existing functionality, e.g. paves the way to add
> security_enclave_load() as an equivalent to security_file_mprotect().

I would suggest describing explicitly in the commit message what you
want to do, which you said here e.g. "I do this because I want to add
LSM hooks". This also relevant information for the LKM discussion.

Lets see how the next version looks like now that you have some
feedback.

In the whole scope of the patch set, in order to make it more
readable, I'll give following suggestions on how it is organized:

1. Leave out anything that is not strictly necessary (cosmetic
fix, batch operation if possible). Better to focus one thing at
a time.
2. Try to organize it so that each function is fully defined in
the scope of one patch even if it would mean larger patches.
3. Do not add one call site helpers unless there is a good
reason to do so. A good reason would be something like needing
to extensive work in error rollback, which would make the
caller a mess.

/Jarkko

Re: [RFC PATCH 7/9] x86/sgx: Enforce noexec filesystem restriction for enclaves

[Jarkko Sakkinen]

On Tue, Jun 04, 2019 at 01:25:10PM -0700, Andy Lutomirski wrote:
> On Tue, Jun 4, 2019 at 9:26 AM Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> >
> > On Fri, May 31, 2019 at 04:31:57PM -0700, Sean Christopherson wrote:
> > > Do not allow an enclave page to be mapped with PROT_EXEC if the source
> > > page is backed by a file on a noexec file system.
> > >
> > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> >
> > Why don't you just check in sgx_encl_add_page() that whether the path
> > comes from noexec and deny if SECINFO contains X?
> >
> 
> SECINFO seems almost entirely useless for this kind of thing because
> of SGX2.  I'm thinking that SECINFO should be completely ignored for
> anything other than its required architectural purpose.

Not exactly sure why using it to pass the RWX bits to EADD ioctl would
cause anything to SGX2 support.

/Jarkko

Re: [RFC PATCH 2/9] x86/sgx: Do not naturally align MAP_FIXED address

[Jarkko Sakkinen]

On Tue, Jun 04, 2019 at 01:16:04PM -0700, Andy Lutomirski wrote:
> On Tue, Jun 4, 2019 at 4:50 AM Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> >
> > On Fri, May 31, 2019 at 04:31:52PM -0700, Sean Christopherson wrote:
> > > 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>
> >
> > Why you want to allow mmap() to be called multiple times? mmap() could
> > be allowed only once with PROT_NONE and denied afterwards. Is this for
> > sending fd to another process that would map already existing enclave?
> >
> > I don't see any checks for whether the is enclave underneath. Also, I
> > think that in all cases mmap() callback should allow only PROT_NONE
> > as permissions for clarity even if it could called multiple times.
> >
> 
> What's the advantage to only allowing PROT_NONE?  The idea here is to
> allow a PROT_NONE map followed by some replacemets that overlay it for
> the individual segments.  Admittedly, mprotect() can do the same
> thing, but disallowing mmap() seems at least a bit surprising.

I was merely wondering if it is specifically for the application where a
client process would mmap(MAP_FIXED) an enclave created by a server
process.

/Jarkko

Re: [RFC PATCH 2/9] x86/sgx: Do not naturally align MAP_FIXED address

[Jarkko Sakkinen]

On Tue, Jun 04, 2019 at 10:10:22PM +0000, Xing, Cedric wrote:
> A bit off topic here. This mmap()/mprotect() discussion reminds me a
> question (guess for Jarkko): Now that vma->vm_file->private_data keeps
> a pointer to the enclave, why do we store it again in vma->vm_private?
> It isn't a big deal but non-NULL vm_private does prevent mprotect()
> from merging adjacent VMAs. 

Same semantics as with a regular mmap i.e. you can close the file and
still use the mapping.

/Jarkko

Re: [RFC PATCH 2/9] x86/sgx: Do not naturally align MAP_FIXED address

[Andy Lutomirski]

> On Jun 5, 2019, at 8:17 AM, Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> 
>> On Tue, Jun 04, 2019 at 10:10:22PM +0000, Xing, Cedric wrote:
>> A bit off topic here. This mmap()/mprotect() discussion reminds me a
>> question (guess for Jarkko): Now that vma->vm_file->private_data keeps
>> a pointer to the enclave, why do we store it again in vma->vm_private?
>> It isn't a big deal but non-NULL vm_private does prevent mprotect()
>> from merging adjacent VMAs. 
> 
> Same semantics as with a regular mmap i.e. you can close the file and
> still use the mapping.
> 
> 

The file should be properly refcounted — vm_file should not go away while it’s mapped.

Re: [RFC PATCH 6/9] x86/sgx: Require userspace to provide allowed prots to ADD_PAGES

[Sean Christopherson]

On Wed, Jun 05, 2019 at 04:10:44AM -0700, Ayoun, Serge wrote:
> > From: Christopherson, Sean J
> > Sent: Saturday, June 01, 2019 02:32
> > 
> >  /**
> >   * struct sgx_enclave_add_pages - parameter structure for the
> >   *                                %SGX_IOC_ENCLAVE_ADD_PAGES ioctl
> > @@ -39,6 +44,7 @@ struct sgx_enclave_create  {
> >   * @secinfo:	address for the SECINFO data (common to all pages)
> >   * @nr_pages:	number of pages (must be virtually contiguous)
> >   * @mrmask:	bitmask for the measured 256 byte chunks (common to all
> > pages)
> > + * @flags:	flags, e.g. SGX_ALLOW_{READ,WRITE,EXEC} (common to all
> > pages)
> >   */
> >  struct sgx_enclave_add_pages {
> >  	__u64	addr;
> > @@ -46,7 +52,8 @@ struct sgx_enclave_add_pages {
> >  	__u64	secinfo;
> >  	__u32	nr_pages;
> >  	__u16	mrmask;
> > -} __attribute__((__packed__));
> > +	__u16	flags;
> > +};
> 
> You are adding a flags member. The secinfo structure has already a flags member in it.
> Why do you need both - they are both coming from user mode. What kind of scenario would
> require having different values. Seems confusing.

The format of SECINFO.FLAGS is hardware defined, e.g. we can't add a flag
to tag the page as being a zero page for optimization purposes, at least
not without breaking future compatibility or doing tricky overloading.

If you're specifically talking about SECINFO.FLAGS.RWX, due to SGX2 there
are scenarios where userspace will initially want the page to be RW, and
will later want to convert the page to RX.  Making decisions based solely
on the initial EPCM permissions would either create a security hole or
force SGX to track "dirty" pages along with a implementing a pre-check
scheme for LSMs (or restricting LSMs to tieing permissions to the host
process and not the enclave).

Re: [RFC PATCH 7/9] x86/sgx: Enforce noexec filesystem restriction for enclaves

[Sean Christopherson]

On Wed, Jun 05, 2019 at 06:10:18PM +0300, Jarkko Sakkinen wrote:
> On Tue, Jun 04, 2019 at 01:25:10PM -0700, Andy Lutomirski wrote:
> > On Tue, Jun 4, 2019 at 9:26 AM Jarkko Sakkinen
> > <jarkko.sakkinen@linux.intel.com> wrote:
> > >
> > > On Fri, May 31, 2019 at 04:31:57PM -0700, Sean Christopherson wrote:
> > > > Do not allow an enclave page to be mapped with PROT_EXEC if the source
> > > > page is backed by a file on a noexec file system.
> > > >
> > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > >
> > > Why don't you just check in sgx_encl_add_page() that whether the path
> > > comes from noexec and deny if SECINFO contains X?
> > >
> > 
> > SECINFO seems almost entirely useless for this kind of thing because
> > of SGX2.  I'm thinking that SECINFO should be completely ignored for
> > anything other than its required architectural purpose.
> 
> Not exactly sure why using it to pass the RWX bits to EADD ioctl would
> cause anything to SGX2 support.

Andy was pointing out that with SGX2 the enclave can do ENCLU[EMODPE] to
make the page executable, e.g. add the page with SECINFO.R and then
mprotect() the enclave VMA (whose vm_file == /dev/sgx/enclave) PROT_EXEC.
We could hard enforce SECINFO, i.e. set the enclave page's protection bits
directly from SECINFO, but that would neuter SGX2, e.g. would break
converting RW to RX.

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

[Sean Christopherson]

On Tue, Jun 04, 2019 at 02:43:09PM -0700, Xing, Cedric wrote:
> > From: Christopherson, Sean J
> > Sent: Tuesday, June 04, 2019 1:37 PM
> > 
> > On Tue, Jun 04, 2019 at 01:29:10PM -0700, Andy Lutomirski wrote:
> > > On Fri, May 31, 2019 at 4:32 PM Sean Christopherson
> > > <sean.j.christopherson@intel.com> wrote:
> > > >  static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long
> > > > addr, diff --git a/include/linux/lsm_hooks.h
> > > > b/include/linux/lsm_hooks.h index 47f58cfb6a19..0562775424a0 100644
> > > > --- a/include/linux/lsm_hooks.h
> > > > +++ b/include/linux/lsm_hooks.h
> > > > @@ -1446,6 +1446,14 @@
> > > >   * @bpf_prog_free_security:
> > > >   *     Clean up the security information stored inside bpf prog.
> > > >   *
> > > > + * Security hooks for Intel SGX enclaves.
> > > > + *
> > > > + * @enclave_load:
> > > > + *     On success, returns 0 and optionally adjusts @allowed_prot
> > > > + *     @vma: the source memory region of the enclave page being
> > loaded.
> > > > + *     @prot: the initial protection of the enclave page.
> > >
> > > What do you mean "initial"?  The page is always mapped PROT_NONE when
> > > this is called, right?  I feel like I must be missing something here.
> > 
> > Initial protection in the EPCM.  Yet another reason to ignore SECINFO.
> 
> I know you guys are talking in the background that all pages are mmap()'ed
> PROT_NONE. But that's an unnecessary limitation.

Not all pages have to be mmap()'d PROT_NONE, only pages that do not have
an associated enclave page.

> And @prot here should be @target_vma->vm_flags&(VM_READ|VM_WRITE|VM_EXEC). 

I don't follow, there is no target_vma at this point.

Re: [RFC PATCH 2/9] x86/sgx: Do not naturally align MAP_FIXED address

[Jarkko Sakkinen]

On Wed, Jun 05, 2019 at 01:14:04PM -0700, Andy Lutomirski wrote:
> 
> 
> > On Jun 5, 2019, at 8:17 AM, Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> > 
> >> On Tue, Jun 04, 2019 at 10:10:22PM +0000, Xing, Cedric wrote:
> >> A bit off topic here. This mmap()/mprotect() discussion reminds me a
> >> question (guess for Jarkko): Now that vma->vm_file->private_data keeps
> >> a pointer to the enclave, why do we store it again in vma->vm_private?
> >> It isn't a big deal but non-NULL vm_private does prevent mprotect()
> >> from merging adjacent VMAs. 
> > 
> > Same semantics as with a regular mmap i.e. you can close the file and
> > still use the mapping.
> > 
> > 
> 
> The file should be properly refcounted — vm_file should not go away while it’s mapped.

Right, makes sense. It is easy one to change essentially just removing
internal refcount from sgx_encl and using file for the same. I'll update
this to my tree along with the changes to remove LKM/ACPI bits ASAP.

/Jarkko

Re: [RFC PATCH 2/9] x86/sgx: Do not naturally align MAP_FIXED address

[Jarkko Sakkinen]

On Thu, Jun 06, 2019 at 06:37:10PM +0300, Jarkko Sakkinen wrote:
> On Wed, Jun 05, 2019 at 01:14:04PM -0700, Andy Lutomirski wrote:
> > 
> > 
> > > On Jun 5, 2019, at 8:17 AM, Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> > > 
> > >> On Tue, Jun 04, 2019 at 10:10:22PM +0000, Xing, Cedric wrote:
> > >> A bit off topic here. This mmap()/mprotect() discussion reminds me a
> > >> question (guess for Jarkko): Now that vma->vm_file->private_data keeps
> > >> a pointer to the enclave, why do we store it again in vma->vm_private?
> > >> It isn't a big deal but non-NULL vm_private does prevent mprotect()
> > >> from merging adjacent VMAs. 
> > > 
> > > Same semantics as with a regular mmap i.e. you can close the file and
> > > still use the mapping.
> > > 
> > > 
> > 
> > The file should be properly refcounted — vm_file should not go away while it’s mapped.

mm already takes care of that so vm_file does not go away. Still
we need an internal refcount for enclaves to synchronize with the
swapper. In summary nothing needs to be done.

> Right, makes sense. It is easy one to change essentially just removing
> internal refcount from sgx_encl and using file for the same. I'll update
> this to my tree along with the changes to remove LKM/ACPI bits ASAP.

/Jarkko

RE: [RFC PATCH 2/9] x86/sgx: Do not naturally align MAP_FIXED address

[Xing, Cedric]

> From: Jarkko Sakkinen [mailto:jarkko.sakkinen@linux.intel.com]
> Sent: Thursday, June 13, 2019 6:48 AM
> 
> On Thu, Jun 06, 2019 at 06:37:10PM +0300, Jarkko Sakkinen wrote:
> > On Wed, Jun 05, 2019 at 01:14:04PM -0700, Andy Lutomirski wrote:
> > >
> > >
> > > > On Jun 5, 2019, at 8:17 AM, Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> > > >
> > > >> On Tue, Jun 04, 2019 at 10:10:22PM +0000, Xing, Cedric wrote:
> > > >> A bit off topic here. This mmap()/mprotect() discussion reminds
> > > >> me a question (guess for Jarkko): Now that
> > > >> vma->vm_file->private_data keeps a pointer to the enclave, why do
> we store it again in vma->vm_private?
> > > >> It isn't a big deal but non-NULL vm_private does prevent
> > > >> mprotect() from merging adjacent VMAs.
> > > >
> > > > Same semantics as with a regular mmap i.e. you can close the file
> > > > and still use the mapping.
> > > >
> > > >
> > >
> > > The file should be properly refcounted — vm_file should not go away
> while it’s mapped.
> 
> mm already takes care of that so vm_file does not go away. Still we need
> an internal refcount for enclaves to synchronize with the swapper. In
> summary nothing needs to be done.

I don't get this. The swapper takes a read lock on mm->mmap_sem, which locks the vma, which in turn reference counts vma->vm_file. Why is the internal refcount still needed? 

> 
> > Right, makes sense. It is easy one to change essentially just removing
> > internal refcount from sgx_encl and using file for the same. I'll
> > update this to my tree along with the changes to remove LKM/ACPI bits
> ASAP.
> 
> /Jarkko

Re: [RFC PATCH 2/9] x86/sgx: Do not naturally align MAP_FIXED address

[Sean Christopherson]

On Thu, Jun 13, 2019 at 09:47:06AM -0700, Xing, Cedric wrote:
> > From: Jarkko Sakkinen [mailto:jarkko.sakkinen@linux.intel.com]
> > Sent: Thursday, June 13, 2019 6:48 AM
> > 
> > On Thu, Jun 06, 2019 at 06:37:10PM +0300, Jarkko Sakkinen wrote:
> > > On Wed, Jun 05, 2019 at 01:14:04PM -0700, Andy Lutomirski wrote:
> > > >
> > > >
> > > > > On Jun 5, 2019, at 8:17 AM, Jarkko Sakkinen
> > <jarkko.sakkinen@linux.intel.com> wrote:
> > > > >
> > > > >> On Tue, Jun 04, 2019 at 10:10:22PM +0000, Xing, Cedric wrote:
> > > > >> A bit off topic here. This mmap()/mprotect() discussion reminds
> > > > >> me a question (guess for Jarkko): Now that
> > > > >> vma->vm_file->private_data keeps a pointer to the enclave, why do
> > we store it again in vma->vm_private?
> > > > >> It isn't a big deal but non-NULL vm_private does prevent
> > > > >> mprotect() from merging adjacent VMAs.
> > > > >
> > > > > Same semantics as with a regular mmap i.e. you can close the file
> > > > > and still use the mapping.
> > > > >
> > > > >
> > > >
> > > > The file should be properly refcounted — vm_file should not go away
> > while it’s mapped.
> > 
> > mm already takes care of that so vm_file does not go away. Still we need
> > an internal refcount for enclaves to synchronize with the swapper. In
> > summary nothing needs to be done.
> 
> I don't get this. The swapper takes a read lock on mm->mmap_sem, which locks
> the vma, which in turn reference counts vma->vm_file. Why is the internal
> refcount still needed? 

mmap_sem is only held when reclaim is touching PTEs, e.g. to test/clear
its accessed bit and to zap the PTE.  The liveliness of the enclave needs
to be guaranteed for the entire duration of reclaim, e.g. we can't have
the enclave disappearing when we go to do EWB.  It's also worth nothing
that a single reclaim may operate on more than one mmap_sem, as enclaves
can be shared across processes (mm_structs).

Re: [RFC PATCH 2/9] x86/sgx: Do not naturally align MAP_FIXED address

[Jarkko Sakkinen]

On Thu, Jun 13, 2019 at 10:14:51AM -0700, Sean Christopherson wrote:
> > I don't get this. The swapper takes a read lock on mm->mmap_sem, which locks
> > the vma, which in turn reference counts vma->vm_file. Why is the internal
> > refcount still needed? 
> 
> mmap_sem is only held when reclaim is touching PTEs, e.g. to test/clear
> its accessed bit and to zap the PTE.  The liveliness of the enclave needs
> to be guaranteed for the entire duration of reclaim, e.g. we can't have
> the enclave disappearing when we go to do EWB.  It's also worth nothing
> that a single reclaim may operate on more than one mmap_sem, as enclaves
> can be shared across processes (mm_structs).

Anyway, the takeaway I got from this is that encl->refcount does not
need to be updated for VMAs (sent a patch to linux-sgx that I plan
merge).

/Jarkko