| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20190603144251.GB13384@linux.intel.com>
Date: Mon, 3 Jun 2019 07:42:51 -0700
From: Sean Christopherson <sean.j.christopherson@...el.com>
To: Stephen Smalley <sds@...ho.nsa.gov>
Cc: Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>,
Andy Lutomirski <luto@...nel.org>,
Cedric Xing <cedric.xing@...el.com>,
James Morris <jmorris@...ei.org>,
"Serge E . Hallyn" <serge@...lyn.com>,
LSM List <linux-security-module@...r.kernel.org>,
Paul Moore <paul@...l-moore.com>,
Eric Paris <eparis@...isplace.org>, selinux@...r.kernel.org,
Jethro Beekman <jethro@...tanix.com>,
Dave Hansen <dave.hansen@...el.com>,
Thomas Gleixner <tglx@...utronix.de>,
Linus Torvalds <torvalds@...ux-foundation.org>,
LKML <linux-kernel@...r.kernel.org>, X86 ML <x86@...nel.org>,
linux-sgx@...r.kernel.org,
Andrew Morton <akpm@...ux-foundation.org>, nhorman@...hat.com,
npmccallum@...hat.com, Serge Ayoun <serge.ayoun@...el.com>,
Shay Katz-zamir <shay.katz-zamir@...el.com>,
Haitao Huang <haitao.huang@...el.com>,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
Kai Svahn <kai.svahn@...el.com>,
Borislav Petkov <bp@...en8.de>,
Josh Triplett <josh@...htriplett.org>,
Kai Huang <kai.huang@...el.com>,
David Rientjes <rientjes@...gle.com>,
William Roberts <william.c.roberts@...el.com>,
Philip Tricca <philip.b.tricca@...el.com>
Subject: Re: [RFC PATCH 8/9] LSM: x86/sgx: Introduce ->enclave_load() hook
for Intel SGX
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@...el.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(¤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;
> >+#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(¤t->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,
Powered by blists - more mailing lists