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

[Andy Lutomirski]

On Fri, May 24, 2019 at 12:24 AM Xing, Cedric <cedric.xing@xxxxxxxxx> wrote:
>
> Hi Andy,
>
> > From: Andy Lutomirski [mailto:luto@xxxxxxxxxx]
> > Sent: Thursday, May 23, 2019 6:18 PM
> >
> > On Thu, May 23, 2019 at 4:40 PM Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> > wrote:
> > >
> > > On Thu, May 23, 2019 at 08:38:17AM -0700, Andy Lutomirski wrote:
> > > > On Thu, May 23, 2019 at 7:17 AM Sean Christopherson
> > > > <sean.j.christopherson@xxxxxxxxx> wrote:
> > > > >
> > > > > On Thu, May 23, 2019 at 01:26:28PM +0300, Jarkko Sakkinen wrote:
> > > > > > On Wed, May 22, 2019 at 07:35:17PM -0700, Sean Christopherson wrote:
> > > > > > > But actually, there's no need to disallow mmap() after ECREATE
> > > > > > > since the LSM checks also apply to mmap(), e.g. FILE__EXECUTE
> > > > > > > would be needed to
> > > > > > > mmap() any enclave pages PROT_EXEC.  I guess my past self
> > > > > > > thought mmap() bypassed LSM checks?  The real problem is that
> > > > > > > mmap()'ng an existing enclave would require FILE__WRITE and
> > > > > > > FILE__EXECUTE, which puts us back at square one.
> > > > > >
> > > > > > I'm lost with the constraints we want to set.
> > > > >
> > > > > As is today, SELinux policies would require enclave loaders to
> > > > > have FILE__WRITE and FILE__EXECUTE permissions on
> > > > > /dev/sgx/enclave.  Presumably other LSMs have similar
> > > > > requirements.  Requiring all processes to have
> > > > > FILE__{WRITE,EXECUTE} permissions means the permissions don't add
> > > > > much value, e.g. they can't be used to distinguish between an
> > > > > enclave that is being loaded from an unmodified file and an enclave that is being
> > generated on the fly, e.g. Graphene.
> > > > >
> > > > > Looking back at Andy's mail, he was talking about requiring
> > > > > FILE__EXECUTE to run an enclave, so perhaps it's only FILE__WRITE
> > > > > that we're trying to special case.
> > > > >
> > > >
> > > > I thought about this some more, and I have a new proposal that helps
> > > > address the ELRANGE alignment issue and the permission issue at the
> > > > cost of some extra verbosity.  Maybe you all can poke holes in it :)
> > > > The basic idea is to make everything more explicit from a user's
> > > > perspective.  Here's how it works:
> > > >
> > > > Opening /dev/sgx/enclave gives an enclave_fd that, by design,
> > > > doesn't give EXECUTE or WRITE.  mmap() on the enclave_fd only works
> > > > if you pass PROT_NONE and gives the correct alignment.  The
> > > > resulting VMA cannot be mprotected or mremapped.  It can't be
> > > > mmapped at all until
> > >
> > > I assume you're thinking of clearing all VM_MAY* flags in sgx_mmap()?
> > >
> > > > after ECREATE because the alignment isn't known before that.
> > >
> > > I don't follow.  The alignment is known because userspace knows the
> > > size of its enclave.  The initial unknown is the address, but that
> > > becomes known once the initial mmap() completes.
> >
> > [...]
> >
> > I think I made the mistake of getting too carried away with implementation details rather
> > than just getting to the point.  And I misremembered the ECREATE flow -- oops.  Let me try
> > again.  First, here are some problems with some earlier proposals (mine, yours
> > Cedric's):
> >
> >  - Having the EADD operation always work but have different effects depending on the
> > source memory permissions is, at the very least, confusing.
>
> Inheriting permissions from source pages IMHO is the easiest way to validate the EPC permissions without any changes to LSM. And the argument about its security is also easy to make.
>
> I understand that it may take some effort to document it properly but otherwise don't see any practical issues with it.

My objection is to the fact that it's implicit.  I have no problem
with some operation succeeding if the source address is X and failing
if it's !X, but I don't think it's fantastic to have it succeed in
either case but do different things.

For what it's worth, while this is a bit of a theoretical issue for X,
but I think it's a real problem with W.  To avoid accidentally mapping
an enclave page X and then later mapping the same page W (potentially
in a different VMA), I think it will be a lot simpler if the driver
can track which pages are allowed to ever be W.  We definitely *don't*
want an interface in which the eventual writability of a page is
inferred from the W permission on the source address, since we do
*not* want to force anyone to map their enclave file PROT_WRITE or
even to open it O_RDWR.

With the explicit MAXPERM passed in, this issue goes away.  You can
specify W if you want W.

>
> >
> >  - If we want to encourage user programs to be well-behaved, we want to make it easy to
> > map the RX parts of an enclave RX, the RW parts RW, the RO parts R, etc.  But this
> > interacts poorly with the sgx_mmap() alignment magic, as you've pointed out.
> >
> >  - We don't want to couple LSMs with SGX too tightly.
> >
> > So here's how a nice interface might work:
> >
> > int enclave_fd = open("/dev/sgx/enclave", O_RDWR);
> >
> > /* enclave_fd points to a totally blank enclave. Before ECREATE, we need to decide on an
> > address. */
> >
> > void *addr = mmap(NULL, size, PROT_NONE, MAP_SHARED, enclave_fd, 0);
> >
> > /* we have an address! */
> >
> > ioctl(enclave_fd, ECREATE, ...);
> >
> > /* now add some data to the enclave.  We want the RWX addition to fail
> > immediately unless we have the relevant LSM pemission.   Similarly, we
> > want the RX addition to fail immediately unless the source VMA is appropriate. */
> >
> > ioctl(enclave_fd, EADD, rx_source_1, MAXPERM=RX, ...);  [the ...
> > includes SECINFO, which the kernel doesn't really care about] ioctl(enclave_fd, EADD,
> > ro_source_1, MAXPERM=RX ...); ioctl(enclave_fd, EADD, rw_source_1, MAXPERM=RW ...);
> > ioctl(enclave_fd, EADD, rwx_source_1, MAXPERM=RWX ...);
>
> If MAXPERM is taken from ioctl parameters, the real question here is how to validate MAXPERM. Guess we shouldn't allow arbitrary MAXPERM to be specified by user code, and the only logical source I can think of is from the source pages (or from the enclave source file, but memory mapping is preferred because it offers more flexibility).

That's exactly what I intended here.  If you specify MAXPERM=RX, then
the kernel can validate that the source address is executable.

>
> >
> > ioctl(enclave_fd, EINIT, ...);  /* presumably pass sigstruct_fd here, too. */
> >
> > /* at this point, all is well except that the enclave is mapped PROT_NONE. There are a
> > couple ways I can imagine to fix this. */
> >
> > We could use mmap:
> >
> > mmap(baseaddr+offset, len, PROT_READ, MAP_SHARED | MAP_FIXED, enclave_fd, 0);  /* only
> > succeeds if MAXPERM & R == R */
> >
> > But this has some annoying implications with regard to sgx_get_unmapped_area().  We could
> > use an ioctl:
>
> There's an easy fix. Just let sgx_get_unmapped_area() do the natural alignment only if MAP_FIXED is *not* set, otherwise, honor both address and len.
>
> But mmap() is subject to LSM check (probably against /dev/sgx/enclave?). How to do mmap(RX) if FILE__EXECUTE is *not* granted for /dev/sgx/enclave, even if MAXPERM=RX?

I think we just let /dev/sgx/enclave be FILE__EXECUTE.  We don't
*have* to make it so that FILE__WRITE and FILE__EXECUTE on
/dev/sgx/enclave means you can create RWX enclave mappings.

>
> >
> > ioctl(enclave_fd, SGX_IOC_MPROTECT, offset, len, PROT_READ);
> >
> > which has the potentially nice property that we can completely bypass the LSM hooks,
> > because the LSM has *already* vetted everything when the EADD calls were allowed.  Or we
> > could maybe even just use
> > mprotect() itself:
> >
> > mprotect(baseaddr + offset, len, PROT_READ);
>
> How to bypass LSM hooks in this mprotect()?

Hmm.  I guess we either use FILE__WRITE and FILE__EXECUTE or we use ioctl().

>
> >
> > Or, for the really evil option, we could use a bit of magic in .fault and do nothing here.
> > Instead we'd make the initial mapping PROT_READ|PROT_WRITE|PROT_EXEC and have .fault
> > actually instantiate the PTEs with the intersection of the VMA permissions and MAXPERM.  I
> > don't think I like this alternative, since it feels more magical than needed and it will
> > be harder to debug.  I like the fact that /proc/self/maps shows the actual permissions in
> > all the other variants.
>
> Agreed.
>
> >
> >
> > All of the rest of the crud in my earlier email was just implementation details.  The
> > point I was trying to make was that I think it's possible to implement this without making
> > too much of a mess internally.  I think I favor the mprotect() approach since it makes the
> > behavior fairly obvious.
> >
> > I don't think any of this needs to change for SGX2.  We'd have an
> > ioctl() that does EAUG and specifies MAXPERM.  Trying to mprotect() a page that hasn't
> > been added yet with any permission other than PROT_NONE would fail.  I suppose we might
> > end up needing a way to let the EAUG operation *change* MAXPERM, and this operation would
> > have to do some more LSM checks and walk all the existing mappings to make sure they're
> > consistent with the new MAXPERM.
>
> EAUG ioctl could be a solution, but isn't optimal at least. What we've done is #PF based. Specifically, an SGX2 enclave will have its heap mapped as RW, but without any pages populated before EINIT. Then when the enclave needs a new page in its heap, it issues EACCEPT, which will cause a #PF and the driver will respond by EAUG a new EPC page. And then the enclave will be resumed and the faulted EACCEPT will be retried (and succeed).
>

If the driver works like that, then whatever call sets up this lazily
allocated heap could do the MAXPERM part.

That being said, is the performance advantage from putting this logic
in the kernel instead of in the untrusted part of the SDK really
worthwhile?

> >
> > As an aside, I wonder if Linus et all would be okay with a new MAP_FULLY_ALIGNED mmap()
> > flag that allocated memory aligned to the requested size.  Then we could get rid of yet
> > another bit of magic.
> >
> > --Andy
>
> I've also got a chance to think more about it lately.
>
> 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.)

I think this is a non-starter :(  FILE__WRITE also means that you can
write to the file, and the admin / policy author will almost never
want to allow that.