RE: [RFC PATCH 0/9] security: x86/sgx: SGX vs. LSM

["Xing, Cedric" <cedric.xing@intel.com>]

> From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx-
> owner@vger.kernel.org] On Behalf Of Stephen Smalley
> Sent: Monday, June 03, 2019 10:47 AM
> 
> On 6/2/19 3:29 AM, Xing, Cedric wrote:
> > Hi Sean,
> >
> >> From: Christopherson, Sean J
> >> Sent: Friday, May 31, 2019 4:32 PM
> >>
> >> This series is the result of a rather absurd amount of discussion
> >> over how to get SGX to play nice with LSM policies, without having to
> >> resort to evil shenanigans or put undue burden on userspace.  The
> >> discussion definitely wandered into completely insane territory at
> times, but I think/hope we ended up with something reasonable.
> >>
> >> The basic gist of the approach is to require userspace to declare
> >> what protections are maximally allowed for any given page, e.g. add a
> >> flags field for loading enclave pages that takes
> >> ALLOW_{READ,WRITE,EXEC}.  LSMs can then adjust the allowed
> >> protections, e.g. clear ALLOW_EXEC to prevent ever mapping the page
> with PROT_EXEC.  SGX enforces the allowed perms via a new mprotect()
> vm_ops hook, e.g. like regular mprotect() uses MAY_{READ,WRITE,EXEC}.
> >>
> >> ALLOW_EXEC is used to deny hings like loading an enclave from a
> >> noexec file system or from a file without EXECUTE permissions, e.g.
> >> without the ALLOW_EXEC concept, on SGX2 hardware (regardless of
> >> kernel support) userspace could EADD from a noexec file using read-
> only permissions, and later use mprotect() and ENCLU[EMODPE] to gain
> execute permissions.
> >>
> >> ALLOW_WRITE is used in conjuction with ALLOW_EXEC to enforce
> SELinux's EXECMOD (or EXECMEM).
> >>
> >> This is very much an RFC series.  It's only compile tested, likely
> >> has obvious bugs, the SELinux patch could be completely harebrained,
> etc...
> >> My goal at this point is to get feedback at a macro level, e.g. is
> >> the core concept viable/acceptable, are there objection to hooking
> mprotect(), etc...
> >>
> >> Andy and Cedric, hopefully this aligns with your general expectations
> >> based on our last discussion.
> >
> > I couldn't understand the real intentions of ALLOW_* flags until I saw
> > them in code. I have to say C is more expressive than English in that
> > regard :)
> >
> > Generally I agree with your direction but think ALLOW_* flags are
> completely internal to LSM because they can be both produced and
> consumed inside an LSM module. So spilling them into SGX driver and also
> user mode code makes the solution ugly and in some cases impractical
> because not every enclave host process has a priori knowledge on whether
> or not an enclave page would be EMODPE'd at runtime.
> >
> > Theoretically speaking, what you really need is a per page flag (let's
> name it WRITTEN?) indicating whether a page has ever been written to (or
> more precisely, granted PROT_WRITE), which will be used to decide
> whether to grant PROT_EXEC when requested in future. Given the fact that
> all mprotect() goes through LSM and mmap() is limited to PROT_NONE, it's
> easy for LSM to capture that flag by itself instead of asking user mode
> code to provide it.
> >
> > That said, here is the summary of what I think is a better approach.
> > * In hook security_file_alloc(), if @file is an enclave, allocate some
> data structure to store for every page, the WRITTEN flag as described
> above. WRITTEN is cleared initially for all pages.
> >    Open: Given a file of type struct file *, how to tell if it is an
> enclave (i.e. /dev/sgx/enclave)?
> > * In hook security_mmap_file(), if @file is an enclave, make sure
> @prot can only be PROT_NONE. This is to force all protection changes to
> go through security_file_mprotect().
> > * In the newly introduced hook security_enclave_load(), set WRITTEN
> for pages that are requested PROT_WRITE.
> > * In hook security_file_mprotect(), if @vma->vm_file is an enclave,
> > look up and use WRITTEN flags for all pages within @vma, along with
> > other global flags (e.g. PROCESS__EXECMEM/FILE__EXECMOD in the case of
> > SELinux) to decide on
> allowing/rejecting @prot.
> 
> At this point we have no knowledge of the source vma/file, right?  So
> what do we check FILE__EXECUTE and/or FILE__EXECMOD against?
> vma->vm_file at this point is /dev/sgx/enclave, right?

My apology to the confusions here.

Yes, vma->vm_file is always /dev/sgx/enclave, but each open("/dev/sgx/enclave") returns a *new* file struct (let's denote it as @enclave_fd) that uniquely identifies one enclave instance, and the expectation is that @enclave_fd->f_security would be used by LSM to store enclave specific information, including ALLOW_* flags and whatever deemed appropriate by an LSM module.

In the case of SELinux, and if the choice is to use FILE__EXECMOD of .sigstruct file to authorize RW->RX at runtime, then SELinux could cache that flag in @enclave_fd->f_security upon security_enclave_init().