| Message ID | 20190531233159.30992-8-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | security: x86/sgx: SGX vs. LSM | expand |
> 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(¤t->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(¤t->mm->mmap_sem); > + > +do_check: > + if (prot & ~*allowed_prot) > + return -EACCES; > + > + return 0; > +}
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
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.
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(¤t->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.
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*.
> 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.
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
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.
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(¤t->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(¤t->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)
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(-)