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

From: "Xing, Cedric" <cedric.xing@intel.com>
To: Stephen Smalley <sds@tycho.nsa.gov>,
	Andy Lutomirski <luto@kernel.org>,
	"Christopherson, Sean J" <sean.j.christopherson@intel.com>
Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>,
	James Morris <jmorris@namei.org>,
	"Serge E. Hallyn" <serge@hallyn.com>,
	LSM List <linux-security-module@vger.kernel.org>,
	Paul Moore <paul@paul-moore.com>,
	Eric Paris <eparis@parisplace.org>,
	"selinux@vger.kernel.org" <selinux@vger.kernel.org>,
	Jethro Beekman <jethro@fortanix.com>,
	"Hansen, Dave" <dave.hansen@intel.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	"Dr. Greg" <greg@enjellic.com>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	LKML <linux-kernel@vger.kernel.org>, X86 ML <x86@kernel.org>,
	"linux-sgx@vger.kernel.org" <linux-sgx@vger.kernel.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	"nhorman@redhat.com" <nhorman@redhat.com>,
	"npmccallum@redhat.com" <npmccallum@redhat.com>,
	"Ayoun, Serge" <serge.ayoun@intel.com>,
	"Katz-zamir, Shay" <shay.katz-zamir@intel.com>,
	"Huang, Haitao" <haitao.huang@intel.com>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	"Svahn, Kai" <kai.svahn@intel.com>,
	Borislav Petkov <bp@alien8.de>,
	Josh Triplett <josh@joshtriplett.org>,
	"Huang, Kai" <kai.huang@intel.com>,
	David Rientjes <rientjes@google.com>
Subject: RE: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
Date: Fri, 24 May 2019 16:57:26 +0000	[thread overview]
Message-ID: <960B34DE67B9E140824F1DCDEC400C0F654E8D1E@ORSMSX116.amr.corp.intel.com> (raw)
In-Reply-To: <dda0912b-cb15-3c07-d368-345159e995f7@tycho.nsa.gov>

Hi Stephen,

> On 5/24/19 3:24 AM, Xing, Cedric wrote:
> >
> > When we talk about EPC page permissions with SGX2 in mind, I think we
> should distinguish between initial permissions and runtime permissions.
> Initial permissions refer to the page permissions set at EADD. They are
> technically set by "untrusted" code so should go by policies similar to
> those applicable to regular shared objects. Runtime permissions refer to
> the permissions granted by EMODPE, EAUG and EACCEPTCOPY. They are
> resulted from inherent behavior of the enclave, which in theory is
> determined by the enclave's measurements (MRENCLAVE and/or MRSIGNER).
> >
> > And we have 2 distinct files to work with - the enclave file and
> /dev/sgx/enclave. And I consider the enclave file a logical source for
> initial permissions while /dev/sgx/enclave is a means to control runtime
> permissions. Then we can have a simpler approach like the pseudo code
> below.
> >
> > /**
> >   * Summary:
> >   * - The enclave file resembles a shared object that contains
> RO/RX/RW segments
> >   * - FILE__* are assigned to /dev/sgx/enclave, to determine
> acceptable permissions to mmap()/mprotect(), valid combinations are
> >   *   + FILE__READ - Allow SGX1 enclaves only
> >   *   + FILE__READ|FILE__WRITE - Allow SGX2 enclaves to expand data
> segments (e.g. heaps, stacks, etc.)
> >   *   + FILE__READ|FILE__WRITE|FILE__EXECUTE - Allow SGX2 enclaves to
> expend both data and code segments. This is necessary to support
> dynamically linked enclaves (e.g. Graphene)
> >   *   + FILE__READ|FILE__EXECUTE - Allow RW->RX changes for SGX1
> enclaves - necessary to support dynamically linked enclaves (e.g.
> Graphene) on SGX1. EXECMEM is also required for this to work
> 
> I think EXECMOD would fit better than EXECMEM for this case; the former
> is applied for RW->RX changes for private file mappings while the latter
> is applied for WX private file mappings.
> 
> >   *   + <None> - Disallow the calling process to launch any enclaves
> >   */
> >
> > /* Step 1: mmap() the enclave file according to the segment attributes
> > (similar to what dlopen() would do for regular shared objects) */ int
> > image_fd = open("/path/to/enclave/file", O_RDONLY);
> 
> FILE__READ checked to enclave file upon open().

Yes. We'd like to have the enclave file pass LSM/IMA checks and let EPC pages "inherit" the permissions from it as "initial" permissions.

> 
> > foreach phdr in loadable segments /* phdr->p_type == PT_LOAD */ {
> >      /* <segment permission> below is subject to LSM checks */
> >      loadable_segments[i] = mmap(NULL, phdr->p_memsz, MAP_PRIATE,
> > <segment permission>, image_fd, phdr->p_offset);
> 
> FILE__READ revalidated and FILE__EXECUTE checked to enclave file upon
> mmap() for PROT_READ and PROT_EXEC respectively.  FILE__WRITE not
> checked even for PROT_WRITE mappings since it is a private file mapping
> and writes do not reach the file.  EXECMEM checked if any segment
> permission has both W and X simultaneously.  EXECMOD checked on any
> subsequent mprotect() RW->RX changes (if modified).

Yes. The intention here is to make sure all X pages come directly from file (unless EXECMEM or EXECMOD is granted). And because the driver will grant X only if the source page also has X, we can assert that all executable EPC pages are loaded from a file that has passed LSM/IMA checks.

> 
> > }
> >
> > /* Step 2: Create enclave */
> > int enclave_fd = open("/dev/sgx/enclave", O_RDONLY /* or O_RDWR for
> > SGX2 enclaves */);
> 
> FILE__READ checked (SGX1) or both FILE__READ and FILE__WRITE checked
> (SGX2) to /dev/sgx/enclave upon open().  Assuming that we are returning
> an open file referencing the /dev/sgx/enclave inode and not an anon
> inode, else we lose all subsequent FILE__* checking on mmap/mprotect and
> trigger EXECMEM on any mmap/mprotect PROT_EXEC.

Yes, the returned fd will be referencing /dev/sgx/enclave. The intention here is to limit EPC "runtime" permissions by the permissions granted to /dev/sgx/enclave, in order to allow user/administrator to specify what kinds of enclaves a given process can launch. Per your earlier comments, FILE__EXECMOD is probably also needed to support dynamically linked enclaves (that require RW->RX changes).

> 
> > void *enclave_base = mmap(NULL, <enclave size>, MAP_SHARED, PROT_READ,
> > enclave_fd, 0); /* Only FILE__READ is required here */
> 
> FILE__READ revalidated to /dev/sgx/enclave upon mmap().

Yes. This mmap() is to set "default" permissions for regions that do *not* have EPC pages populated. It is significant only for SGX2, to specify what action to take by the SGX driver upon #PF with those regions. For example, a R attempt (usually triggered by EACCEPT) within a RW region will cause SGX driver to EAUG a page at the fault address.

> 
> > ioctl(enclave_fd, IOC_ECREATE, ...);
> >
> > /* Step 3: EADD and map initial EPC pages */ foreach s in
> > loadable_segments {
> >      /* IOC_EADD_AND_MAP_SEGMENT will make sure s->perm is a subset of
> VMA permissions of the source pages, and use that as *both* EPCM and VMA
> permissions).
> >       * Given enclave_fd may have FILE__READ only, LSM has to be
> bypassed so the "mmap" part has to be done inside the driver.
> >       * Initial EPC pages will be mapped only once, so no inode is
> needed to remember the initial permissions. mmap/mprotect afterwards are
> subject to FILE__* on /dev/sgx/enclave
> >       * The key point here is: permissions of source pages govern
> initial permissions of EADD'ed pages, regardless FILE__* on
> /dev/sgx/enclave
> >       */
> >      ioctl(enclave_fd, IOC_EADD_AND_MAP_SEGMENT, s->base, s->size,
> > s->perm...); }
> > /* EADD other enclave components, e.g. TCS, stacks, heaps, etc. */
> > ioctl(enclave_fd, IOC_EADD_AND_MAP_SEGMENT, tcs, 0x1000, RW |
> > PT_TCS...); ioctl(enclave_fd, IOC_EADD_AND_MAP_SEGMENT, <zero page>,
> > <stack size>, RW...); ...
> >
> > /* Step 4 (SGX2 only): Reserve ranges for additional heaps, stacks,
> > etc. */
> > /* FILE__WRITE required to allow expansion of data segments at runtime
> > */
> > /* Key point here is: permissions, if needed to change at runtime, are
> > subject to FILL__* on /dev/sgx/enclave */ mprotect(<heap address>,
> > <heap size>, PROT_READ | PROT_WRITE);
> 
> FILE__READ and FILE__WRITE revalidated to /dev/sgx/enclave upon
> mprotect().

Yes. The intention here is to limit "runtime" permissions by accesses granted to the calling process to /dev/sgx/enclave. The "initial" permissions are set by ioctl to bypass LSM, because they are derived/determined by the enclave file.

Alternatively, the driver can remember "initial" permissions for each EPC page at IOC_EADD, to be committed at IOC_EINIT. Then this new IOC_EADD_AND_MAP will not be needed. 

> 
> >
> > /* Step 5: EINIT */
> > ioctl(IOC_EINIT, <sigstruct>...);
> >
> > /* Step 6 (SGX2 only): Set RX for dynamically loaded code pages (e.g.
> > Graphene, encrypted enclaves, etc.) as needed, at runtime */
> > /* FILE__EXECUTE required */
> > mprotect(<RX address>, <RX size>, PROT_READ | PROT_EXEC);
> 
> FILE__READ revalidated and FILE__EXECUTE checked to /dev/sgx/enclave
> upon mprotect().  Cumulative set of checks at this point is
> FILE__READ|FILE__WRITE|FILE__EXECUTE.
> 
> What would the step be for a SGX1 RW->RX change?  How would that trigger
> EXECMOD?  Do we really need to distinguish it from the SGX2 dynamically
> loaded code case?

Per your earlier comments, FILE__EXECMOD is also needed I think to allow RW->RX changes.

FILE__WRITE controls EAUG. I'm not judging its necessity, but just saying they are both valid combinations. To minimize impact to LSM, I don't want to special-case /dev/sgx/enclave. And the current semantics of FILE__* distinguish those two naturally.

BTW, there are usages, such as encrypted enclaves (https://github.com/intel/linux-sgx-pcl), requiring RW->RX but not EAUG. Graphene could also run on SGX1, provided that pages needed by shared objects are all pre-allocated before EINIT. All those could run without FILE__WRITE.

> 
> >
> > -Cedric
> >