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

[Sean Christopherson <sean.j.christopherson@intel.com>]

On Fri, Jun 07, 2019 at 03:58:34PM -0400, Stephen Smalley wrote:
> On 6/5/19 10:11 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, 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.  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 | 12 ++++++------
> >  include/linux/lsm_hooks.h              | 13 +++++++++++++
> >  include/linux/security.h               | 12 ++++++++++++
> >  security/security.c                    |  7 +++++++
> >  4 files changed, 38 insertions(+), 6 deletions(-)
> >
> >diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> >index 44b2d73de7c3..29c0df672250 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>
> >@@ -582,9 +583,6 @@ static int sgx_encl_page_copy(void *dst, unsigned long src, unsigned long prot)
> >  	struct vm_area_struct *vma;
> >  	int ret;
> >-	if (!(prot & VM_EXEC))
> >-		return 0;
> >-
> 
> Is there a real use case where LSM will want to be called if !(prot &
> VM_EXEC)?

I don't think so?  I have no objection to conditioning the LSM calls on
the page being executable.  I actually had the code written that way in
the first RFC, but it felt weird for SGX to be making assumptions about
LSM use cases.

> Also, you seem to be mixing prot and PROT_EXEC with vm_flags and
> VM_EXEC; other code does not appear to assume they are identical and
> explicitly converts, e.g. calc_vm_prot_bits().

Argh, I'll clean that up.
 
> >  	/* Hold mmap_sem across copy_from_user() to avoid a TOCTOU race. */
> >  	down_read(&current->mm->mmap_sem);