On Fri, May 24, 2019 at 12:24 AM Xing, Cedric [off-list ref] wrote:

Hi Andy,

quoted

From: Andy Lutomirski [mailto:luto@kernel.org]
Sent: Thursday, May 23, 2019 6:18 PM

On Thu, May 23, 2019 at 4:40 PM Sean Christopherson [off-list ref]
wrote:

quoted

On Thu, May 23, 2019 at 08:38:17AM -0700, Andy Lutomirski wrote:

quoted

On Thu, May 23, 2019 at 7:17 AM Sean Christopherson
[off-list ref] wrote:

quoted

On Thu, May 23, 2019 at 01:26:28PM +0300, Jarkko Sakkinen wrote:

quoted

On Wed, May 22, 2019 at 07:35:17PM -0700, Sean Christopherson wrote:

quoted

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.

quoted

quoted

quoted

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()?

quoted

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.

quoted

 - 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.

quoted

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.

quoted

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().

quoted

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.

quoted

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?

quoted

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.