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

Message ID 20190617222438.2080-10-sean.j.christopherson@intel.com
State New
Headers show
Series
  • security: x86/sgx: SGX vs. LSM, round 3
Related show

Commit Message

Sean Christopherson June 17, 2019, 10:24 p.m. UTC
enclave_load() is roughly analogous to the existing file_mprotect().

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

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

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

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

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

Comments

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

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

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

/Jarkko
James Morris June 19, 2019, 9:13 p.m. UTC | #2
On Wed, 19 Jun 2019, Jarkko Sakkinen wrote:

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

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

If the hooks could be generalized beyond just SGX, that would be ideal, 
but it's not clear if that's feasible.
Dr. Greg June 20, 2019, 9:28 a.m. UTC | #3
On Thu, Jun 20, 2019 at 07:13:50AM +1000, James Morris wrote:

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

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

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

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

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

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

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

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

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

> James Morris

Have a good day.

Dr. Greg

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

 A 64bit number incremented every millisecond can grow for half a
 billion years.  As far as I'm concerned, that is forever."
                                -- Neil Brown
                                   linux-raid
Jarkko Sakkinen June 20, 2019, 10:22 p.m. UTC | #4
On Thu, Jun 20, 2019 at 07:13:50AM +1000, James Morris wrote:
> On Wed, 19 Jun 2019, Jarkko Sakkinen wrote:
> 
> > Can LSM callbacks ever non-generic when it comes to hardware? This is
> > the very first time I ever see such callbacks being introduced.
> > 
> > I suspect that from maintainers perspective, accepting such changes for
> > Intel hardware, could open a pandoras box.
> 
> If there's a major distro/userbase committing to ship with these hooks 
> enabled via a supported in-tree LSM, the case for inclusion is clear.

I think there is.

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

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

/Jarkko
Dr. Greg June 23, 2019, 5:16 p.m. UTC | #5
On Thu, Jun 20, 2019 at 07:13:50AM +1000, James Morris wrote:

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

> James Morris

Hopefully these comments are a useful basis for further discussion.

Best wishes for a productive week to everyone.

Dr. Greg

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

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

Thanks, this is helpful.

Patch
diff mbox series

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