[RFC,7/9] x86/sgx: Enforce noexec filesystem restriction for enclaves
diff mbox series

Message ID 20190531233159.30992-8-sean.j.christopherson@intel.com
State New
Headers show
Series
  • security: x86/sgx: SGX vs. LSM
Related show

Commit Message

Sean Christopherson May 31, 2019, 11:31 p.m. UTC
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(-)

Comments

Xing, Cedric June 3, 2019, 6:29 a.m. UTC | #1
> 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;
> +}
Jarkko Sakkinen June 4, 2019, 4:25 p.m. UTC | #2
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
Andy Lutomirski June 4, 2019, 8:25 p.m. UTC | #3
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.
Andy Lutomirski June 4, 2019, 8:26 p.m. UTC | #4
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.
Sean Christopherson June 4, 2019, 8:34 p.m. UTC | #5
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*.
Xing, Cedric June 4, 2019, 9:54 p.m. UTC | #6
> 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.
Jarkko Sakkinen June 5, 2019, 3:10 p.m. UTC | #7
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
Sean Christopherson June 6, 2019, 1:01 a.m. UTC | #8
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.

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 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)