Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)

[Stephen Smalley]

On 5/17/19 1:29 PM, Sean Christopherson wrote:
> On Fri, May 17, 2019 at 12:37:40PM -0400, Stephen Smalley wrote:
> > On 5/17/19 12:20 PM, Stephen Smalley wrote:
> > > On 5/17/19 11:09 AM, Sean Christopherson wrote:
> > > > I think we may want to change the SGX API to alloc an anon inode for each
> > > > enclave instead of hanging every enclave off of the /dev/sgx/enclave
> > > > inode.
> > > > Because /dev/sgx/enclave is NOT private, SELinux's file_map_prot_check()
> > > > will only require FILE__WRITE and FILE__EXECUTE to mprotect() enclave
> > > > VMAs
> > > > to RWX. Backing each enclave with an anon inode will make SELinux treat
> > > > EPC memory like anonymous mappings, which is what we want (I think), e.g.
> > > > making *any* EPC page executable will require PROCESS__EXECMEM (SGX is
> > > > 64-bit only at this point, so SELinux will always have default_noexec).
> > >
> > > I don't think we want to require EXECMEM (or equivalently both FILE__WRITE
> > > and FILE__EXECUTE to /dev/sgx/enclave) for making any EPC page executable,
> > > only if the page is also writable or previously modified. The intent is
> > > to prevent arbitrary code execution without EXECMEM (or
> > > FILE__WRITE|FILE__EXECUTE), while still allowing enclaves to be created
> > > without EXECMEM as long as the EPC page mapping is only ever mapped RX and
> > > its initial contents came from an unmodified file mapping that was
> > > PROT_EXEC (and hence already checked via FILE__EXECUTE).
>
> The idea is that by providing an SGX ioctl() to propagate VMA permissions
> from a source VMA, EXECMEM wouldn't be required to make an EPC page
> executable.  E.g. userspace establishes an enclave in non-EPC memory from
> an unmodified file (with FILE__EXECUTE perms), and the uses the SGX ioctl()
> to copy the contents and permissions into EPC memory.
>
> > Also, just to be clear, there is nothing inherently better about checking
> > EXECMEM instead of checking both FILE__WRITE and FILE__EXECUTE to the
> > /dev/sgx/enclave inode, so I wouldn't switch to using anon inodes for that
> > reason.  Using anon inodes also unfortunately disables SELinux inode-based
> > checking since we no longer have any useful inode information, so you'd lose
> > out on SELinux ioctl whitelisting on those enclave inodes if that matters.
>
> The problem is that all enclaves are associated with a single inode, i.e.
> /dev/sgx/enclave.  /dev/sgx/enclave is a char device whose purpose is to
> provide ioctls() and to allow mmap()'ing EPC memory.  In no way is it
> associated with the content that actually gets loaded into EPC memory.
>
> The actual file that contains the enclave's contents (assuming the enclave
> came from a file) is a separate regular file that the SGX subsystem never
> sees.
>
> AIUI, having FILE__WRITE and FILE__EXECUTE on /dev/sgx/enclave would allow
> *any* enclave/process to map EPC as RWX.  Moving to anon inodes and thus
> PROCESS__EXECMEM achieves per-process granularity.

No, FILE__WRITE and FILE__EXECUTE are a check between a process and a 
file, so you can ensure that only whitelisted processes are allowed both 
to /dev/sgx/enclave.