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

Hi, LSM and SELinux people-

We're trying to figure out how SGX fits in with LSMs.  For background,
an SGX library is functionally a bit like a DSO, except that it's
nominally resistant to attack from outside and the process of loading
it is complicated.  To load an enclave, a program can open
/dev/sgx/enclave, do some ioctls to load the code and data segments
into the enclave, call a special ioctl to "initialize" the enclave,
and then call into the enclave (using special CPU instructions).

One nastiness is that there is not actually a universally agreed upon,
documented file format for enclaves.  Windows has an undocumented
format, and there are probably a few others out there.  No one really
wants to teach the kernel to parse enclave files.

There are two issues with how this interacts with LSMs:

1) LSMs might want to be able to whitelist, blacklist, or otherwise
restrict what enclaves can run at all.  The current proposal that
everyone seems to dislike the least is to have a .sigstruct file on
disk that contains a hash and signature of the enclave in a
CPU-defined format.  To initialize an enclave, a program will pass an
fd to this file, and a new LSM hook can be called to allow or disallow
the operation.  In a SELinux context, the idea is that policy could
require the .sigstruct file to be labeled with a type like
sgx_sigstruct_t, and only enclaves that have a matching .sigstruct
with such a label could run.

2) Just like any other DSO, there are potential issues with how
enclaves deal with writable vs executable memory.  This takes two
forms.  First, a task should probably require EXECMEM, EXECMOD, or
similar permission to run an enclave that can modify its own text.
Second, it would be nice if a task that did *not* have EXECMEM,
EXECMOD, or similar could still run the enclave if it had EXECUTE
permission on the file containing the enclave.

Currently, this all works because DSOs are run by mmapping the file to
create multiple VMAs, some of which are executable, non-writable, and
non-CoWed, and some of which are writable but not executable.  With
SGX, there's only really one inode per enclave (the anon_inode that
comes form /dev/sgx/enclave), and it can only be sensibly mapped
MAP_SHARED.

With the current version of the SGX driver, to run an enclave, I think
you'll need either EXECUTE rights to /dev/sgx/enclave or EXECMOD or
similar, all of which more or less mean that you can run any modified
code you want, and none of which is useful to prevent enclaves from
contain RWX segments.

So my question is: what, if anything, should change to make this work better?

Here's a very vague proposal that's kind of like what I've been
thinking over the past few days.  The SGX inode could track, for each
page, a "safe-to-execute" bit.  When you first open /dev/sgx/enclave,
you get a blank enclave and all pages are safe-to-execute.  When you
do the ioctl to load context (which could be code, data, or anything
else), the kernel will check whether the *source* VMA is executable
and, if not, mark the page of the enclave being loaded as unsafe.
Once the enclave is initialized, the driver will clear the
safe-to-execute bit for any page that is successfully mapped writably.

The intent is that a page of the enclave is safe-to-execute if that
page was populated from executable memory and not modified since then.
LSMs could then enforce a policy that you can map an enclave page RX
if the page is safe-to-execute, you can map any page you want for
write if there are no executable mappings, and you can only map a page
for write and execute simultaneously if you can EXECMOD permission.
This should allow an enclave to be loaded by userspace from a file
with EXECUTE rights.

So here are my questions:

Are the goals I mentioned reasonable?

Is the design I just outlined reasonable?  Would SELinux support this?

Is there a better solution that works well enough?

Thanks, all!

On May 14, 2019, at 6:30 PM, Sean Christopherson [off-list ref] wrote:

quoted

But thinking this all through, it's a bit more complicated than any of
this.  Looking at the SELinux code for inspiration, there are quite a
few paths, but they boil down to two cases: EXECUTE is the right to
map an unmodified file executably, and EXECMOD/EXECMEM (the
distinction seems mostly irrelevant) is the right to create (via mmap
or mprotect) a modified anonymous file mapping or a non-file-backed
mapping that is executable.  So, if we do nothing, then mapping an
enclave with execute permission will require either EXECUTE on the
enclave inode or EXECMOD/EXECMEM, depending on exactly how this gets
set up.

If we do literally nothing, then I'm pretty sure mapping an enclave will
require PROCESS__EXECMEM.  The mmap() for the actual enclave is done
using an anon inode, e.g. from /dev/sgx/enclave.  Anon inodes are marked
private, which means inode_has_perm() will always return "success".  The
only effective check is in file_map_prot_check() when default_noexec is
true, in which case requesting PROT_EXEC on private inodes requires
PROCESS__EXECMEM.

quoted

So all is well, sort of.  The problem is that I expect there will be
people who want enclaves to work in a process that does not have these
rights.  To make this work, we probably need do some surgery on
SELinux.  ISTM the act of copying (via the EADD ioctl) data from a
PROT_EXEC mapping to an enclave should not be construed as "modifying"
the enclave for SELinux purposes.  Actually doing this could be
awkward, since the same inode will have executable parts and
non-executable parts, and SELinux can't really tell the difference.

Rather the do surgery on SELinux, why not go with Cedric's original
proposal and propagate the permissions from the source VMA to the EPC
VMA?

Which EPC VMA?  Users can map the enclave fd again after EADD,
resulting in a new VMA.  And any realistic enclave will presumably
have RO, RW, and RX pages.

 The enclave mmap() from userspace could then be done with RO
permissions so as to not run afoul of LSMs.  Adding PROT_EXEC after
EADD would require PROCESS__EXECMEM, but that's in line with mprotect()
on regular memory.

How does this help anything?  The driver currently only works with
EXECMEM and, with this change, it still needs EXECMEM.

I think that, if we’re going to make changes along these lines, the
goal should be that you can have an enclave serialized in a file on
disk such that you have EXECUTE on the file, and you should be able to
load and run the enclave without needing EXECMEM.  (Unless the enclave
is self-modifying, of course.)

On Wed, 15 May 2019, Andy Lutomirski wrote:

There are two issues with how this interacts with LSMs:

1) LSMs might want to be able to whitelist, blacklist, or otherwise
restrict what enclaves can run at all.  The current proposal that
everyone seems to dislike the least is to have a .sigstruct file on
disk that contains a hash and signature of the enclave in a
CPU-defined format.  To initialize an enclave, a program will pass an
fd to this file, and a new LSM hook can be called to allow or disallow
the operation.  In a SELinux context, the idea is that policy could
require the .sigstruct file to be labeled with a type like
sgx_sigstruct_t, and only enclaves that have a matching .sigstruct
with such a label could run.

The .sigstruct file is for the CPU to consume, not the kernel correct?

How is it bound to the enclave file?

Why not just use an xattr, like security.sgx ?

2) Just like any other DSO, there are potential issues with how
enclaves deal with writable vs executable memory.  This takes two
forms.  First, a task should probably require EXECMEM, EXECMOD, or
similar permission to run an enclave that can modify its own text.
Second, it would be nice if a task that did *not* have EXECMEM,
EXECMOD, or similar could still run the enclave if it had EXECUTE
permission on the file containing the enclave.

Currently, this all works because DSOs are run by mmapping the file to
create multiple VMAs, some of which are executable, non-writable, and
non-CoWed, and some of which are writable but not executable.  With
SGX, there's only really one inode per enclave (the anon_inode that
comes form /dev/sgx/enclave), and it can only be sensibly mapped
MAP_SHARED.

With the current version of the SGX driver, to run an enclave, I think
you'll need either EXECUTE rights to /dev/sgx/enclave or EXECMOD or
similar, all of which more or less mean that you can run any modified
code you want, and none of which is useful to prevent enclaves from
contain RWX segments.

So my question is: what, if anything, should change to make this work better?

Would it be possible to provide multiple fds (perhaps via a pseudo fs 
interface) which can be mapped to different types of VMAs?

-- 
James Morris
[off-list ref]

On Wed, May 15, 2019 at 12:58 PM James Morris [off-list ref] wrote:

On Wed, 15 May 2019, Andy Lutomirski wrote:

quoted

There are two issues with how this interacts with LSMs:

1) LSMs might want to be able to whitelist, blacklist, or otherwise
restrict what enclaves can run at all.  The current proposal that
everyone seems to dislike the least is to have a .sigstruct file on
disk that contains a hash and signature of the enclave in a
CPU-defined format.  To initialize an enclave, a program will pass an
fd to this file, and a new LSM hook can be called to allow or disallow
the operation.  In a SELinux context, the idea is that policy could
require the .sigstruct file to be labeled with a type like
sgx_sigstruct_t, and only enclaves that have a matching .sigstruct
with such a label could run.

The .sigstruct file is for the CPU to consume, not the kernel correct?

Yes, unless an LSM wants to examine it to make a decision.

How is it bound to the enclave file?

It's not bound to the enclave *file* at all, but it contains a hash
that covers the enclave, so two different files in two different
formats representing exactly the same enclave would get the same hash,
but any change to the enclave would get a different hash.

Why not just use an xattr, like security.sgx ?

Wouldn't this make it so that only someone with CAP_MAC_ADMIN could
install an enclave?  I think that this decision should be left up the
administrator, and it should be easy to set up a loose policy where
anyone can load whatever enclave they want.  That's what would happen
in my proposal if there was no LSM loaded or of the LSM policy didn't
restrict what .sigstruct files were acceptable.

quoted

2) Just like any other DSO, there are potential issues with how
enclaves deal with writable vs executable memory.  This takes two
forms.  First, a task should probably require EXECMEM, EXECMOD, or
similar permission to run an enclave that can modify its own text.
Second, it would be nice if a task that did *not* have EXECMEM,
EXECMOD, or similar could still run the enclave if it had EXECUTE
permission on the file containing the enclave.

Currently, this all works because DSOs are run by mmapping the file to
create multiple VMAs, some of which are executable, non-writable, and
non-CoWed, and some of which are writable but not executable.  With
SGX, there's only really one inode per enclave (the anon_inode that
comes form /dev/sgx/enclave), and it can only be sensibly mapped
MAP_SHARED.

With the current version of the SGX driver, to run an enclave, I think
you'll need either EXECUTE rights to /dev/sgx/enclave or EXECMOD or
similar, all of which more or less mean that you can run any modified
code you want, and none of which is useful to prevent enclaves from
contain RWX segments.

So my question is: what, if anything, should change to make this work better?

Would it be possible to provide multiple fds (perhaps via a pseudo fs
interface) which can be mapped to different types of VMAs?

Maybe.  The tricky bit is that, even if there was a separate inode for
the writable and the executable parts of the enclave, I think that
both would have to be mapped MAP_SHARED since MAP_ANONYMOUS is
nonsensical for SGX.  This would certainly push more complexity into
the user code.  Jarkko?

On Wed, May 15, 2019 at 11:27:04AM -0700, Andy Lutomirski wrote:

2) Just like any other DSO, there are potential issues with how
enclaves deal with writable vs executable memory.  This takes two
forms.  First, a task should probably require EXECMEM, EXECMOD, or
similar permission to run an enclave that can modify its own text.
Second, it would be nice if a task that did *not* have EXECMEM,
EXECMOD, or similar could still run the enclave if it had EXECUTE
permission on the file containing the enclave.

Currently, this all works because DSOs are run by mmapping the file to
create multiple VMAs, some of which are executable, non-writable, and
non-CoWed, and some of which are writable but not executable.  With
SGX, there's only really one inode per enclave (the anon_inode that
comes form /dev/sgx/enclave), and it can only be sensibly mapped
MAP_SHARED.

I was wrong when I said /dev/sgx/enclave creates and returns an anon
inode.  I was thinking of the KVM model for creating VMs.  SGX creates
an enclave when /dev/sgx/enclave is opened and associates the enclave
with the newly opened /dev/sgx/enclave fd.

Regardless, the fundamental problem remains, mmap() of EPC works on a
single inode.

On Wed, 15 May 2019, Andy Lutomirski wrote:

quoted

Why not just use an xattr, like security.sgx ?

Wouldn't this make it so that only someone with CAP_MAC_ADMIN could
install an enclave?  I think that this decision should be left up the
administrator, and it should be easy to set up a loose policy where
anyone can load whatever enclave they want.  That's what would happen
in my proposal if there was no LSM loaded or of the LSM policy didn't
restrict what .sigstruct files were acceptable.

You could try user.sigstruct, which does not require any privs.

-- 
James Morris
[off-list ref]

On Wed, May 15, 2019 at 3:46 PM James Morris [off-list ref] wrote:

On Wed, 15 May 2019, Andy Lutomirski wrote:

quoted

quoted

Why not just use an xattr, like security.sgx ?

Wouldn't this make it so that only someone with CAP_MAC_ADMIN could
install an enclave?  I think that this decision should be left up the
administrator, and it should be easy to set up a loose policy where
anyone can load whatever enclave they want.  That's what would happen
in my proposal if there was no LSM loaded or of the LSM policy didn't
restrict what .sigstruct files were acceptable.

You could try user.sigstruct, which does not require any privs.

I don't think I understand your proposal.  What file would this
attribute be on?  What would consume it?

I'm imagining that there's some enclave in a file
crypto_thingy.enclave.  There's also a file crypto_thingy.sigstruct.
crypto_thingy.enclave has type lib_t or similar so that it's
executable.  crypto_thingy.sigstruct has type sgx_sigstruct_t.  The
enclave loader does, in effect:

void *source_data = mmap(crypto_thingy.enclave, PROT_READ | PROT_EXEC, ...);
int sigstruct_fd = open("crypto_thingy.sigstruct", O_RDONLY);
int enclave_fd = open("/dev/sgx/enclave", O_RDWR);

ioctl(enclave_fd, SGX_IOC_ADD_SOME_DATA, source_data + source_offset,
enclave_offset, len, ...);
ioctl(enclave_fd, SGX_IOC_ADD_SOME_DATA, source_data + source_offset2,
enclave_offset2, len, ...);
etc.

/* Here's where LSMs get to check that the sigstruct is acceptable.
The CPU will check that the sigstruct matches the enclave. */
ioctl(enclave_fd, SGX_INIT_THE_ENCLAVE, sigstruct_fd);

/* Actually map the thing */
mmap(enclave_fd RO section, PROT_READ, ...);
mmap(enclave_fd RW section, PROT_READ | PROT_WRITE, ...);
mmap(enclave_fd RX section, PROT_READ | PROT_EXEC, ...);

/* This should fail unless EXECMOD is available, I think */
mmap(enclave_fd RWX section, PROT_READ | PROT_WRITE | PROT_EXEC);

And the idea here is that, if the .enclave file isn't mapped
PROT_EXEC, then mmapping the RX section will also require EXECMEM or
EXECMOD.

On Wed, 15 May 2019 16:38:58 -0500, Sean Christopherson [off-list ref] wrote:

On Wed, May 15, 2019 at 11:27:04AM -0700, Andy Lutomirski wrote:

quoted

2) Just like any other DSO, there are potential issues with how
enclaves deal with writable vs executable memory.  This takes two
forms.  First, a task should probably require EXECMEM, EXECMOD, or
similar permission to run an enclave that can modify its own text.
Second, it would be nice if a task that did *not* have EXECMEM,
EXECMOD, or similar could still run the enclave if it had EXECUTE
permission on the file containing the enclave.

Currently, this all works because DSOs are run by mmapping the file to
create multiple VMAs, some of which are executable, non-writable, and
non-CoWed, and some of which are writable but not executable.  With
SGX, there's only really one inode per enclave (the anon_inode that
comes form /dev/sgx/enclave), and it can only be sensibly mapped
MAP_SHARED.

I was wrong when I said /dev/sgx/enclave creates and returns an anon
inode.  I was thinking of the KVM model for creating VMs.  SGX creates
an enclave when /dev/sgx/enclave is opened and associates the enclave
with the newly opened /dev/sgx/enclave fd.

Regardless, the fundamental problem remains, mmap() of EPC works on a
single inode.

If I read code in file_map_prot_check() correctly, only when you request W+X at the same time that EXECMEM would be required for MAP_SHARED, right?
If so, I believe SGX enclaves would never need that.

Hi Andy,

From: Andy Lutomirski [mailto:luto@kernel.org]

On Wed, May 15, 2019 at 3:46 PM James Morris [off-list ref] wrote:

quoted

On Wed, 15 May 2019, Andy Lutomirski wrote:

quoted

quoted

Why not just use an xattr, like security.sgx ?

Wouldn't this make it so that only someone with CAP_MAC_ADMIN could
install an enclave?  I think that this decision should be left up the
administrator, and it should be easy to set up a loose policy where
anyone can load whatever enclave they want.  That's what would happen
in my proposal if there was no LSM loaded or of the LSM policy didn't
restrict what .sigstruct files were acceptable.

You could try user.sigstruct, which does not require any privs.

I don't think I understand your proposal.  What file would this
attribute be on?  What would consume it?

I'm imagining that there's some enclave in a file
crypto_thingy.enclave.  There's also a file crypto_thingy.sigstruct.
crypto_thingy.enclave has type lib_t or similar so that it's
executable.  crypto_thingy.sigstruct has type sgx_sigstruct_t.  The
enclave loader does, in effect:

void *source_data = mmap(crypto_thingy.enclave, PROT_READ | PROT_EXEC, ...);
int sigstruct_fd = open("crypto_thingy.sigstruct", O_RDONLY);
int enclave_fd = open("/dev/sgx/enclave", O_RDWR);

ioctl(enclave_fd, SGX_IOC_ADD_SOME_DATA, source_data + source_offset,
enclave_offset, len, ...);
ioctl(enclave_fd, SGX_IOC_ADD_SOME_DATA, source_data + source_offset2,
enclave_offset2, len, ...);
etc.

/* Here's where LSMs get to check that the sigstruct is acceptable.
The CPU will check that the sigstruct matches the enclave. */
ioctl(enclave_fd, SGX_INIT_THE_ENCLAVE, sigstruct_fd);

SIGSTRUCT isn't necessarily stored on disk so may not always have a fd. How about the following?
void *ss_pointer = mmap(sigstruct_fd, PROT_READ,...);
ioctl(enclave_fd, SGX_INIT_THE_ENCLAVE, ss_pointer);

The idea here is SIGSTRUCT will still be passed in memory so it works the same way when no LSM modules are loaded or basing its decision on the .sigstruct file. Otherwise, an LSM module can figure out the backing file (and offset within that file) by looking into the VMA covering ss_pointer.

/* Actually map the thing */
mmap(enclave_fd RO section, PROT_READ, ...);
mmap(enclave_fd RW section, PROT_READ | PROT_WRITE, ...);
mmap(enclave_fd RX section, PROT_READ | PROT_EXEC, ...);

/* This should fail unless EXECMOD is available, I think */
mmap(enclave_fd RWX section, PROT_READ | PROT_WRITE | PROT_EXEC);

And the idea here is that, if the .enclave file isn't mapped
PROT_EXEC, then mmapping the RX section will also require EXECMEM or
EXECMOD.

From security perspective, I think it reasonable to give EXECMEM and EXECMOD to /dev/sgx/enclave because the actual permissions are guarded by EPCM permissions, which are "inherited" from the source pages, whose permissions have passed LSM checks.

Alternatively, I think we could mark enclave VMAs somewhat differently, such as defining a new VM_SGX flag. The reason behind that is, enclave ranges differ from "regular" virtual ranges in terms of both functionality (i.e. #PF will have to be handled quite differently) and security, so I believe demand will come up to distinguish them eventually - e.g., LSM modules can then enforce different policies on them (by a new security_sgx_mprot() hook?).

-Cedric

On May 15, 2019, at 8:03 PM, Xing, Cedric [off-list ref] wrote:

Hi Andy,

quoted

From: Andy Lutomirski [mailto:luto@kernel.org]

quoted

On Wed, May 15, 2019 at 3:46 PM James Morris [off-list ref] wrote:

On Wed, 15 May 2019, Andy Lutomirski wrote:

quoted

quoted

Why not just use an xattr, like security.sgx ?

Wouldn't this make it so that only someone with CAP_MAC_ADMIN could
install an enclave?  I think that this decision should be left up the
administrator, and it should be easy to set up a loose policy where
anyone can load whatever enclave they want.  That's what would happen
in my proposal if there was no LSM loaded or of the LSM policy didn't
restrict what .sigstruct files were acceptable.

You could try user.sigstruct, which does not require any privs.

I don't think I understand your proposal.  What file would this
attribute be on?  What would consume it?

I'm imagining that there's some enclave in a file
crypto_thingy.enclave.  There's also a file crypto_thingy.sigstruct.
crypto_thingy.enclave has type lib_t or similar so that it's
executable.  crypto_thingy.sigstruct has type sgx_sigstruct_t.  The
enclave loader does, in effect:

void *source_data = mmap(crypto_thingy.enclave, PROT_READ | PROT_EXEC, ...);
int sigstruct_fd = open("crypto_thingy.sigstruct", O_RDONLY);
int enclave_fd = open("/dev/sgx/enclave", O_RDWR);

ioctl(enclave_fd, SGX_IOC_ADD_SOME_DATA, source_data + source_offset,
enclave_offset, len, ...);
ioctl(enclave_fd, SGX_IOC_ADD_SOME_DATA, source_data + source_offset2,
enclave_offset2, len, ...);
etc.

/* Here's where LSMs get to check that the sigstruct is acceptable.
The CPU will check that the sigstruct matches the enclave. */
ioctl(enclave_fd, SGX_INIT_THE_ENCLAVE, sigstruct_fd);

SIGSTRUCT isn't necessarily stored on disk so may not always have a fd. How about the following?
void *ss_pointer = mmap(sigstruct_fd, PROT_READ,...);
ioctl(enclave_fd, SGX_INIT_THE_ENCLAVE, ss_pointer);

The idea here is SIGSTRUCT will still be passed in memory so it works the same way when no LSM modules are loaded or basing its decision on the .sigstruct file. Otherwise, an LSM module can figure out the backing file (and offset within that file) by looking into the VMA covering ss_pointer.

I don’t love this approach.  Application authors seem likely to use
read() instead of mmap(), and it’ll still work in many cares. It would
also complicate the kernel implementation, and looking at the inode
backing the vma that backs a pointer is at least rather unusual.
Instead, if the sigstruct isn’t on disk because it’s dynamic or came
from a network, the application can put it in a memfd.

quoted

/* Actually map the thing */
mmap(enclave_fd RO section, PROT_READ, ...);
mmap(enclave_fd RW section, PROT_READ | PROT_WRITE, ...);
mmap(enclave_fd RX section, PROT_READ | PROT_EXEC, ...);

/* This should fail unless EXECMOD is available, I think */
mmap(enclave_fd RWX section, PROT_READ | PROT_WRITE | PROT_EXEC);

And the idea here is that, if the .enclave file isn't mapped
PROT_EXEC, then mmapping the RX section will also require EXECMEM or
EXECMOD.

From security perspective, I think it reasonable to give EXECMEM and EXECMOD to /dev/sgx/enclave because the actual permissions are guarded by EPCM permissions, which are "inherited" from the source pages, whose permissions have passed LSM checks.

I disagree.  If you deny a program EXECMOD, it’s not because you
distrust the program. It’s because you want to enforce good security
practices.  (Or you’re Apple and want to disallow third-party JITs.)
A policy that accepts any sigstruct but requires that enclaves come
from disk and respect W^X seems entirely reasonable.

I think that blocking EXECMOD has likely served two very real security
purposes. It helps force application and library developers to write
and compile their code in a way that doesn’t rely on dangerous tricks
like putting executable trampolines on the stack.  It also makes it
essentially impossible for an exploit to run actual downloaded machine
code — if there is no way to run code that isn’t appropriately
labeled, then attackers are more limited in what they can do.

I don’t think that SGX should become an exception to either of these.
Code should not have an excuse to use WX memory just because it’s in
an enclave. Similarly, an exploit should not be able to run an
attacker-supplied enclave as a way around a policy that would
otherwise prevent downloaded code from running.


—Andy

On Wed, May 15, 2019 at 11:27:04AM -0700, Andy Lutomirski wrote:

Hi, LSM and SELinux people-

We're trying to figure out how SGX fits in with LSMs.  For background,
an SGX library is functionally a bit like a DSO, except that it's
nominally resistant to attack from outside and the process of loading
it is complicated.  To load an enclave, a program can open
/dev/sgx/enclave, do some ioctls to load the code and data segments
into the enclave, call a special ioctl to "initialize" the enclave,
and then call into the enclave (using special CPU instructions).

One nastiness is that there is not actually a universally agreed upon,
documented file format for enclaves.  Windows has an undocumented
format, and there are probably a few others out there.  No one really
wants to teach the kernel to parse enclave files.

There are two issues with how this interacts with LSMs:

1) LSMs might want to be able to whitelist, blacklist, or otherwise
restrict what enclaves can run at all.  The current proposal that
everyone seems to dislike the least is to have a .sigstruct file on
disk that contains a hash and signature of the enclave in a
CPU-defined format.  To initialize an enclave, a program will pass an
fd to this file, and a new LSM hook can be called to allow or disallow
the operation.  In a SELinux context, the idea is that policy could
require the .sigstruct file to be labeled with a type like
sgx_sigstruct_t, and only enclaves that have a matching .sigstruct
with such a label could run.

Similarly if we could take data for the enclave from fd and enforce
it with sgx_enclave_t label.

Here's a very vague proposal that's kind of like what I've been
thinking over the past few days.  The SGX inode could track, for each
page, a "safe-to-execute" bit.  When you first open /dev/sgx/enclave,
you get a blank enclave and all pages are safe-to-execute.  When you
do the ioctl to load context (which could be code, data, or anything
else), the kernel will check whether the *source* VMA is executable
and, if not, mark the page of the enclave being loaded as unsafe.
Once the enclave is initialized, the driver will clear the
safe-to-execute bit for any page that is successfully mapped writably.

With the fd based model for source I'd mark SECINFO.W pages as unsafe
to execute and then check unsafe bit before applying lets say EMODT
or EMODPR.

There is a problem here though. Usually the enclave itself is just a
loader that then loads the application from outside source and creates
the executable pages from the content.

A great example of this is Graphene that bootstraps unmodified Linux
applications to an enclave:

https://github.com/oscarlab/graphene

/Jarkko

On Wed, 15 May 2019, Andy Lutomirski wrote:

On Wed, May 15, 2019 at 3:46 PM James Morris [off-list ref] wrote:

quoted

You could try user.sigstruct, which does not require any privs.

I don't think I understand your proposal.  What file would this
attribute be on?  What would consume it?

It would be on the enclave file, so you keep the sigstruct bound to it, 
rather than needing a separate file to manage.  It would simplify any LSM 
policy check.

It would be consumed by (I guess) the SGX_INIT_THE_ENCLAVE ioctl in your 
example, instead of having a 2nd fd.


-- 
James Morris
[off-list ref]

On May 16, 2019, at 12:24 AM, James Morris [off-list ref] wrote:

quoted

On Wed, 15 May 2019, Andy Lutomirski wrote:

quoted

On Wed, May 15, 2019 at 3:46 PM James Morris [off-list ref] wrote:

You could try user.sigstruct, which does not require any privs.

I don't think I understand your proposal.  What file would this
attribute be on?  What would consume it?

It would be on the enclave file, so you keep the sigstruct bound to it,
rather than needing a separate file to manage.  It would simplify any LSM
policy check.

It would be consumed by (I guess) the SGX_INIT_THE_ENCLAVE ioctl in your
example, instead of having a 2nd fd.

Okay, I think I see what you’re suggesting. I don’t think it works
well, though, since loading the data from the enclave file will almost
always be done in multiple chunks, and it’s not clear when the kernel
should look for the xattr or what to do if the xattr changes part way
through.

On May 15, 2019, at 10:16 PM, Jarkko Sakkinen [off-list ref] wrote:

quoted

On Wed, May 15, 2019 at 11:27:04AM -0700, Andy Lutomirski wrote:
Hi, LSM and SELinux people-

We're trying to figure out how SGX fits in with LSMs.  For background,
an SGX library is functionally a bit like a DSO, except that it's
nominally resistant to attack from outside and the process of loading
it is complicated.  To load an enclave, a program can open
/dev/sgx/enclave, do some ioctls to load the code and data segments
into the enclave, call a special ioctl to "initialize" the enclave,
and then call into the enclave (using special CPU instructions).

One nastiness is that there is not actually a universally agreed upon,
documented file format for enclaves.  Windows has an undocumented
format, and there are probably a few others out there.  No one really
wants to teach the kernel to parse enclave files.

There are two issues with how this interacts with LSMs:

1) LSMs might want to be able to whitelist, blacklist, or otherwise
restrict what enclaves can run at all.  The current proposal that
everyone seems to dislike the least is to have a .sigstruct file on
disk that contains a hash and signature of the enclave in a
CPU-defined format.  To initialize an enclave, a program will pass an
fd to this file, and a new LSM hook can be called to allow or disallow
the operation.  In a SELinux context, the idea is that policy could
require the .sigstruct file to be labeled with a type like
sgx_sigstruct_t, and only enclaves that have a matching .sigstruct
with such a label could run.

Similarly if we could take data for the enclave from fd and enforce
it with sgx_enclave_t label.

That certainly *could* be done, and I guess the decision could be left
to the LSMs, but I'm not convinced this adds value.  What security use
case does this cover that isn't already covered by requiring EXECUTE
(e.g. lib_t) on the enclave file and some new SIGSTRUCT right on the
.sigstruct?

quoted

Here's a very vague proposal that's kind of like what I've been
thinking over the past few days.  The SGX inode could track, for each
page, a "safe-to-execute" bit.  When you first open /dev/sgx/enclave,
you get a blank enclave and all pages are safe-to-execute.  When you
do the ioctl to load context (which could be code, data, or anything
else), the kernel will check whether the *source* VMA is executable
and, if not, mark the page of the enclave being loaded as unsafe.
Once the enclave is initialized, the driver will clear the
safe-to-execute bit for any page that is successfully mapped writably.

With the fd based model for source I'd mark SECINFO.W pages as unsafe
to execute and then check unsafe bit before applying lets say EMODT
or EMODPR.

There is a problem here though. Usually the enclave itself is just a
loader that then loads the application from outside source and creates
the executable pages from the content.

A great example of this is Graphene that bootstraps unmodified Linux
applications to an enclave:

https://github.com/oscarlab/graphene

ISTM you should need EXECMEM or similar to run Graphene, then.

Hi Andy,

quoted

SIGSTRUCT isn't necessarily stored on disk so may not always have a fd.

How about the following?

quoted

void *ss_pointer = mmap(sigstruct_fd, PROT_READ,...);
ioctl(enclave_fd, SGX_INIT_THE_ENCLAVE, ss_pointer);

The idea here is SIGSTRUCT will still be passed in memory so it works

the same way when no LSM modules are loaded or basing its decision on
the .sigstruct file. Otherwise, an LSM module can figure out the backing
file (and offset within that file) by looking into the VMA covering
ss_pointer.

I don’t love this approach.  Application authors seem likely to use
read() instead of mmap(), and it’ll still work in many cares. It would
also complicate the kernel implementation, and looking at the inode
backing the vma that backs a pointer is at least rather unusual.
Instead, if the sigstruct isn’t on disk because it’s dynamic or came
from a network, the application can put it in a memfd.

I understand your concern here. But I guess we are making too much assumption on how enclaves are structured/packaged. My concern is, what if a SIGSTRUCT really has to be from memory? For example, an enclave (along with its SIGSTRUCT) could be embedded inside a shared object (or even the "main" executable) so it shows up in memory to begin with. Of course it could be copied to a memfd but whatever "attributes" (e.g. path, or SELinux class/type) associated with the original file would be lost, so I'm not sure if that would work.

I'm also with you that applications tend to use read() instead of mmap() for accessing files. But in our case that'd be necessary only if .sigstruct is a separate file (hence needs to be read separately). What if (and I guess most implementations would) the SIGSTRUCT is embedded in the same file as the enclave? mmap() is the more common practice when dealing with executable images, and in that case SIGSTRUCT will have already been mmap()'d. 

I'm with you again that it's kind of unprecedented to look at the backing inode. But I believe we should strive to allow as large variety of applications/usages as possible and I don't see any alternatives without losing flexibility.

quoted

quoted

/* Actually map the thing */
mmap(enclave_fd RO section, PROT_READ, ...);
mmap(enclave_fd RW section, PROT_READ | PROT_WRITE, ...);
mmap(enclave_fd RX section, PROT_READ | PROT_EXEC, ...);

/* This should fail unless EXECMOD is available, I think */
mmap(enclave_fd RWX section, PROT_READ | PROT_WRITE | PROT_EXEC);

And the idea here is that, if the .enclave file isn't mapped
PROT_EXEC, then mmapping the RX section will also require EXECMEM or
EXECMOD.

From security perspective, I think it reasonable to give EXECMEM and

EXECMOD to /dev/sgx/enclave because the actual permissions are guarded
by EPCM permissions, which are "inherited" from the source pages, whose
permissions have passed LSM checks.

I disagree.  If you deny a program EXECMOD, it’s not because you
distrust the program. It’s because you want to enforce good security
practices.  (Or you’re Apple and want to disallow third-party JITs.)
A policy that accepts any sigstruct but requires that enclaves come
from disk and respect W^X seems entirely reasonable.

I think that blocking EXECMOD has likely served two very real security
purposes. It helps force application and library developers to write
and compile their code in a way that doesn’t rely on dangerous tricks
like putting executable trampolines on the stack.  It also makes it
essentially impossible for an exploit to run actual downloaded machine
code — if there is no way to run code that isn’t appropriately
labeled, then attackers are more limited in what they can do.

I don’t think that SGX should become an exception to either of these.
Code should not have an excuse to use WX memory just because it’s in
an enclave. Similarly, an exploit should not be able to run an
attacker-supplied enclave as a way around a policy that would
otherwise prevent downloaded code from running.

My apology for the confusion here.

I thought EXECMOD applied to files (and memory mappings backed by them) but I was probably wrong. It sounds like EXECMOD applies to the whole process so would allow all pages within a process's address space to be modified then executed, regardless the backing files. Am I correct this time?

I was not saying enclaves were exempt to good security practices. What I was trying to say was, EPC pages are *not* subject to the same attacks as regular pages so I suspect there will be a desire to enforce different policies on them, especially after new SGX2 features/applications become available. So I think it beneficial to distinguish between regular vs. enclave virtual ranges. And to do that, a new VM_SGX flag in VMA is probably a very simple/easy way. And with that VM_SGX flag, we could add a new security_sgx_mprot() hook so that LSM modules/policies could act differently.

And if you are with me on that bigger picture, the next question is: what should be the default behavior of security_sgx_mprot() for existing/non-SGX-aware LSM modules/policies? I'd say a reasonable default is to allow R, RW and RX, but not anything else. It'd suffice to get rid of EXECMEM/EXECMOD requirements on enclave applications. For SGX1, EPCM permissions are immutable so it really doesn't matter what security_sgx_mprot() does. For SGX2 and beyond, there's still time and new SGX-aware LSM modules/policies will probably have emerged by then.

-Cedric

On Thu, May 16, 2019 at 02:02:58PM -0700, Andy Lutomirski wrote:

quoted

On May 15, 2019, at 10:16 PM, Jarkko Sakkinen [off-list ref] wrote:
There is a problem here though. Usually the enclave itself is just a
loader that then loads the application from outside source and creates
the executable pages from the content.

A great example of this is Graphene that bootstraps unmodified Linux
applications to an enclave:

https://github.com/oscarlab/graphene

ISTM you should need EXECMEM or similar to run Graphene, then.

Agreed, Graphene is effectively running arbitrary enclave code.  I'm
guessing there is nothing that prevents extending/reworking Graphene to
allow generating the enclave ahead of time so as to avoid populating the
guts of the enclave at runtime, i.e. it's likely possible to run an
unmodified application in an enclave without EXECMEM if that's something
Graphene or its users really care about.

quoted

quoted

There is a problem here though. Usually the enclave itself is just a
loader that then loads the application from outside source and
creates the executable pages from the content.

A great example of this is Graphene that bootstraps unmodified Linux
applications to an enclave:

https://github.com/oscarlab/graphene

ISTM you should need EXECMEM or similar to run Graphene, then.

Agreed, Graphene is effectively running arbitrary enclave code.  I'm
guessing there is nothing that prevents extending/reworking Graphene to
allow generating the enclave ahead of time so as to avoid populating the
guts of the enclave at runtime, i.e. it's likely possible to run an
unmodified application in an enclave without EXECMEM if that's something
Graphene or its users really care about.

Inefficient use of memory is a problem of running Graphene on SGX1, from at least 2 aspects: 1) heaps/stacks have to be pre-allocated but only a small portion of those pages will be actually used; and 2) dynamic linking is commonly used in *unmodified* applications and all dependent libraries have to be loaded, but only a subset of those pages will actually be used - e.g. most applications use only a small set of functions in libc.so but the whole library still has to be loaded. Hence a practical/efficient solution will require/involve EDMM features available in SGX2. I guess we shall look a bit further into future in order to address this problem properly. And I think it necessary to distinguish enclave virtual ranges from regular ones (probably at VMA level) before we could have a practical solution.

On Wed, May 15, 2019 at 11:27:04AM -0700, Andy Lutomirski wrote:

Here's a very vague proposal that's kind of like what I've been
thinking over the past few days.  The SGX inode could track, for each
page, a "safe-to-execute" bit.  When you first open /dev/sgx/enclave,
you get a blank enclave and all pages are safe-to-execute.  When you
do the ioctl to load context (which could be code, data, or anything
else), the kernel will check whether the *source* VMA is executable
and, if not, mark the page of the enclave being loaded as unsafe.
Once the enclave is initialized, the driver will clear the
safe-to-execute bit for any page that is successfully mapped writably.

The intent is that a page of the enclave is safe-to-execute if that
page was populated from executable memory and not modified since then.
LSMs could then enforce a policy that you can map an enclave page RX
if the page is safe-to-execute, you can map any page you want for
write if there are no executable mappings, and you can only map a page
for write and execute simultaneously if you can EXECMOD permission.
This should allow an enclave to be loaded by userspace from a file
with EXECUTE rights.

I'm still confused as to why you want to track execute permissions on the
enclave pages and add SGX-specific LSM hooks.  Is there anything that
prevents userspace from building the enclave like any other DSO and then
copying it into enclave memory?  I feel like I'm missing something.

  1. Userspace loads enclave into regular memory, e.g. like a normal DSO.
     All mmap(), mprotect(), etc... calls are subject to all existing
     LSM policies.

  2. Userspace opens /dev/sgx/enclave to instantiate a new enclave.

  3. Userspace uses mmap() to allocate virtual memory for its enclave,
     again subject to all existing LSM policies (sane userspaces map it RO
     since the permissions eventually get tossed anyways).

  4. SGX subsystem refuses to service page faults for enclaves that have
     not yet been initialized, e.g. signals SIGBUS or SIGSEGV.

  5. Userspace invokes SGX ioctl() to copy enclave from regulary VMA to
     enclave VMA.

  6. SGX ioctl() propagates VMA protection-related flags from source VMA
     to enclave VMA, e.g. invokes mprotect_fixup().  Enclave VMA(s) may
     be split as part of this process.

  7. At all times, mprotect() calls on the enclave VMA are subject to
     existing LSM policies, i.e. it's not special cased for enclaves.


The SGX ioctl() would need to take mmap_sem for write, but we can mitigate
that issue by changing the ioctl() to take a range of memory instead of a
single page.  That'd also provide "EADD batching" that folks have
requested.

On Thu, May 16, 2019 at 5:03 PM Sean Christopherson
[off-list ref] wrote:

On Wed, May 15, 2019 at 11:27:04AM -0700, Andy Lutomirski wrote:

quoted

Here's a very vague proposal that's kind of like what I've been
thinking over the past few days.  The SGX inode could track, for each
page, a "safe-to-execute" bit.  When you first open /dev/sgx/enclave,
you get a blank enclave and all pages are safe-to-execute.  When you
do the ioctl to load context (which could be code, data, or anything
else), the kernel will check whether the *source* VMA is executable
and, if not, mark the page of the enclave being loaded as unsafe.
Once the enclave is initialized, the driver will clear the
safe-to-execute bit for any page that is successfully mapped writably.

The intent is that a page of the enclave is safe-to-execute if that
page was populated from executable memory and not modified since then.
LSMs could then enforce a policy that you can map an enclave page RX
if the page is safe-to-execute, you can map any page you want for
write if there are no executable mappings, and you can only map a page
for write and execute simultaneously if you can EXECMOD permission.
This should allow an enclave to be loaded by userspace from a file
with EXECUTE rights.

I'm still confused as to why you want to track execute permissions on the
enclave pages and add SGX-specific LSM hooks.  Is there anything that
prevents userspace from building the enclave like any other DSO and then
copying it into enclave memory?

It's entirely possible that I'm the one missing something.  But here's
why I think this:

I feel like I'm missing something.

  1. Userspace loads enclave into regular memory, e.g. like a normal DSO.
     All mmap(), mprotect(), etc... calls are subject to all existing
     LSM policies.

  2. Userspace opens /dev/sgx/enclave to instantiate a new enclave.

  3. Userspace uses mmap() to allocate virtual memory for its enclave,
     again subject to all existing LSM policies (sane userspaces map it RO
     since the permissions eventually get tossed anyways).

Is userspace actually requred to mmap() the enclave prior to EADDing things?

  4. SGX subsystem refuses to service page faults for enclaves that have
     not yet been initialized, e.g. signals SIGBUS or SIGSEGV.

  5. Userspace invokes SGX ioctl() to copy enclave from regulary VMA to
     enclave VMA.

  6. SGX ioctl() propagates VMA protection-related flags from source VMA
     to enclave VMA, e.g. invokes mprotect_fixup().  Enclave VMA(s) may
     be split as part of this process.

Does this also call the LSM?  If so, what is it expected to do?  What
happens if there are different regions with different permissions on
the same page?  SGX has 256-byte granularity right?

  7. At all times, mprotect() calls on the enclave VMA are subject to
     existing LSM policies, i.e. it's not special cased for enclaves.

I don't think the normal behavior actually works here.  An enclave is
always MAP_SHARED, so (with SELinux) mprotecting() to X or RX requires
EXECUTE and mprotecting() to RWX requires extra permissions.  But user
code can also mmap() the enclave again.  What is supposed to happen in
that case?

On Thu, May 16, 2019 at 3:23 PM Xing, Cedric [off-list ref] wrote:

Hi Andy,

quoted

quoted

SIGSTRUCT isn't necessarily stored on disk so may not always have a fd.

How about the following?

quoted

void *ss_pointer = mmap(sigstruct_fd, PROT_READ,...);
ioctl(enclave_fd, SGX_INIT_THE_ENCLAVE, ss_pointer);

The idea here is SIGSTRUCT will still be passed in memory so it works

the same way when no LSM modules are loaded or basing its decision on
the .sigstruct file. Otherwise, an LSM module can figure out the backing
file (and offset within that file) by looking into the VMA covering
ss_pointer.

I don’t love this approach.  Application authors seem likely to use
read() instead of mmap(), and it’ll still work in many cares. It would
also complicate the kernel implementation, and looking at the inode
backing the vma that backs a pointer is at least rather unusual.
Instead, if the sigstruct isn’t on disk because it’s dynamic or came
from a network, the application can put it in a memfd.

I understand your concern here. But I guess we are making too much assumption on how enclaves are structured/packaged. My concern is, what if a SIGSTRUCT really has to be from memory? For example, an enclave (along with its SIGSTRUCT) could be embedded inside a shared object (or even the "main" executable) so it shows up in memory to begin with.

Hmm.  That's a fair point, although opening /proc/self/exe could be
somewhat of a workaround.  It does suffer from a bit of an in-band
signaling problem, though, in that it's possible that some other
random bytes in the library resemble a SIGSTRUCT.

I was not saying enclaves were exempt to good security practices. What I was trying to say was, EPC pages are *not* subject to the same attacks as regular pages so I suspect there will be a desire to enforce different policies on them, especially after new SGX2 features/applications become available. So I think it beneficial to distinguish between regular vs. enclave virtual ranges. And to do that, a new VM_SGX flag in VMA is probably a very simple/easy way. And with that VM_SGX flag, we could add a new security_sgx_mprot() hook so that LSM modules/policies could act differently.

I'm not opposed to this, but I also don't think this needs to be in
the initial upstream driver.  VM_SGX also isn't strictly necessary --
an LSM could inspect the VMA to decide whether it's an SGX VMA if it
really wanted to.

That being said, do you have any specific behavior differences in mind
aside from the oddities involved in loading the enclave.

And if you are with me on that bigger picture, the next question is: what should be the default behavior of security_sgx_mprot() for existing/non-SGX-aware LSM modules/policies? I'd say a reasonable default is to allow R, RW and RX, but not anything else. It'd suffice to get rid of EXECMEM/EXECMOD requirements on enclave applications. For SGX1, EPCM permissions are immutable so it really doesn't matter what security_sgx_mprot() does. For SGX2 and beyond, there's still time and new SGX-aware LSM modules/policies will probably have emerged by then.

I hadn't thought about the SGX1 vs SGX2 difference.  If the driver
initially only wants to support SGX1, then I guess we really could get
away with constraining the EPC flags based on the source page
permission and not restricting mprotect() and mmap() permissions on
/dev/sgx/enclave at all.

From: Andy Lutomirski [mailto:luto@kernel.org]

On Thu, May 16, 2019 at 3:23 PM Xing, Cedric [off-list ref]
wrote:

quoted

Hi Andy,

quoted

quoted

SIGSTRUCT isn't necessarily stored on disk so may not always have

a fd.

quoted

quoted

How about the following?

quoted

void *ss_pointer = mmap(sigstruct_fd, PROT_READ,...);
ioctl(enclave_fd, SGX_INIT_THE_ENCLAVE, ss_pointer);

The idea here is SIGSTRUCT will still be passed in memory so it
works

the same way when no LSM modules are loaded or basing its decision
on the .sigstruct file. Otherwise, an LSM module can figure out the
backing file (and offset within that file) by looking into the VMA
covering ss_pointer.

I don’t love this approach.  Application authors seem likely to use
read() instead of mmap(), and it’ll still work in many cares. It
would also complicate the kernel implementation, and looking at the
inode backing the vma that backs a pointer is at least rather

unusual.

quoted

quoted

Instead, if the sigstruct isn’t on disk because it’s dynamic or came
from a network, the application can put it in a memfd.

I understand your concern here. But I guess we are making too much

assumption on how enclaves are structured/packaged. My concern is, what
if a SIGSTRUCT really has to be from memory? For example, an enclave
(along with its SIGSTRUCT) could be embedded inside a shared object (or
even the "main" executable) so it shows up in memory to begin with.

Hmm.  That's a fair point, although opening /proc/self/exe could be
somewhat of a workaround.  It does suffer from a bit of an in-band
signaling problem, though, in that it's possible that some other random
bytes in the library resemble a SIGSTRUCT.

quoted

I was not saying enclaves were exempt to good security practices. What

I was trying to say was, EPC pages are *not* subject to the same attacks
as regular pages so I suspect there will be a desire to enforce
different policies on them, especially after new SGX2
features/applications become available. So I think it beneficial to
distinguish between regular vs. enclave virtual ranges. And to do that,
a new VM_SGX flag in VMA is probably a very simple/easy way. And with
that VM_SGX flag, we could add a new security_sgx_mprot() hook so that
LSM modules/policies could act differently.

I'm not opposed to this, but I also don't think this needs to be in the
initial upstream driver.  VM_SGX also isn't strictly necessary -- an LSM
could inspect the VMA to decide whether it's an SGX VMA if it really
wanted to.

VM_SGX is just what I think is the easiest way for any module to tell enclave VMAs from all others. I agree totally with you that doesn't have to be in the initial release!

That being said, do you have any specific behavior differences in mind
aside from the oddities involved in loading the enclave.

The major thing is dynamically linked enclaves. Say if you want something like dlopen() inside an enclave, the driver would need to EAUG a page as RW initially, and then change to RX after it has been EACCEPTCOPY'ed by the enclave. So it's like a RW->RX transition and an LSM module/policy may want to allow it only if it's within an enclave range (ELRANGE), or deny it otherwise.

quoted

And if you are with me on that bigger picture, the next question is:

what should be the default behavior of security_sgx_mprot() for
existing/non-SGX-aware LSM modules/policies? I'd say a reasonable
default is to allow R, RW and RX, but not anything else. It'd suffice to
get rid of EXECMEM/EXECMOD requirements on enclave applications. For
SGX1, EPCM permissions are immutable so it really doesn't matter what
security_sgx_mprot() does. For SGX2 and beyond, there's still time and
new SGX-aware LSM modules/policies will probably have emerged by then.

I hadn't thought about the SGX1 vs SGX2 difference.  If the driver
initially only wants to support SGX1, then I guess we really could get
away with constraining the EPC flags based on the source page permission
and not restricting mprotect() and mmap() permissions on
/dev/sgx/enclave at all.

This is exactly what I'm going after! 

But I have to apologize for this silly question because I don't know much about SELinux: Wouldn't it require changes to existing SELinux policies to *not* restrict mprotect() on /dev/sgx/enclave?

-Cedric

On Thu, May 16, 2019 at 6:06 PM Xing, Cedric [off-list ref] wrote:

quoted

From: Andy Lutomirski [mailto:luto@kernel.org]

On Thu, May 16, 2019 at 3:23 PM Xing, Cedric [off-list ref]
wrote:

quoted

Hi Andy,

quoted

quoted

SIGSTRUCT isn't necessarily stored on disk so may not always have

a fd.

quoted

quoted

How about the following?

quoted

void *ss_pointer = mmap(sigstruct_fd, PROT_READ,...);
ioctl(enclave_fd, SGX_INIT_THE_ENCLAVE, ss_pointer);

The idea here is SIGSTRUCT will still be passed in memory so it
works

the same way when no LSM modules are loaded or basing its decision
on the .sigstruct file. Otherwise, an LSM module can figure out the
backing file (and offset within that file) by looking into the VMA
covering ss_pointer.

I don’t love this approach.  Application authors seem likely to use
read() instead of mmap(), and it’ll still work in many cares. It
would also complicate the kernel implementation, and looking at the
inode backing the vma that backs a pointer is at least rather

unusual.

quoted

quoted

Instead, if the sigstruct isn’t on disk because it’s dynamic or came
from a network, the application can put it in a memfd.

I understand your concern here. But I guess we are making too much

assumption on how enclaves are structured/packaged. My concern is, what
if a SIGSTRUCT really has to be from memory? For example, an enclave
(along with its SIGSTRUCT) could be embedded inside a shared object (or
even the "main" executable) so it shows up in memory to begin with.

Hmm.  That's a fair point, although opening /proc/self/exe could be
somewhat of a workaround.  It does suffer from a bit of an in-band
signaling problem, though, in that it's possible that some other random
bytes in the library resemble a SIGSTRUCT.

quoted

I was not saying enclaves were exempt to good security practices. What

I was trying to say was, EPC pages are *not* subject to the same attacks
as regular pages so I suspect there will be a desire to enforce
different policies on them, especially after new SGX2
features/applications become available. So I think it beneficial to
distinguish between regular vs. enclave virtual ranges. And to do that,
a new VM_SGX flag in VMA is probably a very simple/easy way. And with
that VM_SGX flag, we could add a new security_sgx_mprot() hook so that
LSM modules/policies could act differently.

I'm not opposed to this, but I also don't think this needs to be in the
initial upstream driver.  VM_SGX also isn't strictly necessary -- an LSM
could inspect the VMA to decide whether it's an SGX VMA if it really
wanted to.

VM_SGX is just what I think is the easiest way for any module to tell enclave VMAs from all others. I agree totally with you that doesn't have to be in the initial release!

quoted

That being said, do you have any specific behavior differences in mind
aside from the oddities involved in loading the enclave.

The major thing is dynamically linked enclaves. Say if you want something like dlopen() inside an enclave, the driver would need to EAUG a page as RW initially, and then change to RX after it has been EACCEPTCOPY'ed by the enclave. So it's like a RW->RX transition and an LSM module/policy may want to allow it only if it's within an enclave range (ELRANGE), or deny it otherwise.

I'm not convinced.  Given that the kernel has no way to tell that the
dynamically loaded code wasn't dynamically generated, I don't think it
makes sense to allow this in an enclave but disallow it outside an
enclave.

quoted

quoted

And if you are with me on that bigger picture, the next question is:

what should be the default behavior of security_sgx_mprot() for
existing/non-SGX-aware LSM modules/policies? I'd say a reasonable
default is to allow R, RW and RX, but not anything else. It'd suffice to
get rid of EXECMEM/EXECMOD requirements on enclave applications. For
SGX1, EPCM permissions are immutable so it really doesn't matter what
security_sgx_mprot() does. For SGX2 and beyond, there's still time and
new SGX-aware LSM modules/policies will probably have emerged by then.

I hadn't thought about the SGX1 vs SGX2 difference.  If the driver
initially only wants to support SGX1, then I guess we really could get
away with constraining the EPC flags based on the source page permission
and not restricting mprotect() and mmap() permissions on
/dev/sgx/enclave at all.

This is exactly what I'm going after!

But I have to apologize for this silly question because I don't know much about SELinux: Wouldn't it require changes to existing SELinux policies to *not* restrict mprotect() on /dev/sgx/enclave?

I'm assuming we'd make a small in-kernel change to SELinux to make it
work without policy changes, assuming the SELinux maintainers would be
okay with this.

On 5/16/19 6:23 PM, Xing, Cedric wrote:

Hi Andy,

quoted

quoted

SIGSTRUCT isn't necessarily stored on disk so may not always have a fd.

How about the following?

quoted

void *ss_pointer = mmap(sigstruct_fd, PROT_READ,...);
ioctl(enclave_fd, SGX_INIT_THE_ENCLAVE, ss_pointer);

The idea here is SIGSTRUCT will still be passed in memory so it works

the same way when no LSM modules are loaded or basing its decision on
the .sigstruct file. Otherwise, an LSM module can figure out the backing
file (and offset within that file) by looking into the VMA covering
ss_pointer.

I don’t love this approach.  Application authors seem likely to use
read() instead of mmap(), and it’ll still work in many cares. It would
also complicate the kernel implementation, and looking at the inode
backing the vma that backs a pointer is at least rather unusual.
Instead, if the sigstruct isn’t on disk because it’s dynamic or came
from a network, the application can put it in a memfd.

I understand your concern here. But I guess we are making too much assumption on how enclaves are structured/packaged. My concern is, what if a SIGSTRUCT really has to be from memory? For example, an enclave (along with its SIGSTRUCT) could be embedded inside a shared object (or even the "main" executable) so it shows up in memory to begin with. Of course it could be copied to a memfd but whatever "attributes" (e.g. path, or SELinux class/type) associated with the original file would be lost, so I'm not sure if that would work.

I'm also with you that applications tend to use read() instead of mmap() for accessing files. But in our case that'd be necessary only if .sigstruct is a separate file (hence needs to be read separately). What if (and I guess most implementations would) the SIGSTRUCT is embedded in the same file as the enclave? mmap() is the more common practice when dealing with executable images, and in that case SIGSTRUCT will have already been mmap()'d.

I'm with you again that it's kind of unprecedented to look at the backing inode. But I believe we should strive to allow as large variety of applications/usages as possible and I don't see any alternatives without losing flexibility.

quoted

quoted

quoted

/* Actually map the thing */
mmap(enclave_fd RO section, PROT_READ, ...);
mmap(enclave_fd RW section, PROT_READ | PROT_WRITE, ...);
mmap(enclave_fd RX section, PROT_READ | PROT_EXEC, ...);

/* This should fail unless EXECMOD is available, I think */
mmap(enclave_fd RWX section, PROT_READ | PROT_WRITE | PROT_EXEC);

And the idea here is that, if the .enclave file isn't mapped
PROT_EXEC, then mmapping the RX section will also require EXECMEM or
EXECMOD.

 From security perspective, I think it reasonable to give EXECMEM and

EXECMOD to /dev/sgx/enclave because the actual permissions are guarded
by EPCM permissions, which are "inherited" from the source pages, whose
permissions have passed LSM checks.

I disagree.  If you deny a program EXECMOD, it’s not because you
distrust the program. It’s because you want to enforce good security
practices.  (Or you’re Apple and want to disallow third-party JITs.)
A policy that accepts any sigstruct but requires that enclaves come
from disk and respect W^X seems entirely reasonable.

I think that blocking EXECMOD has likely served two very real security
purposes. It helps force application and library developers to write
and compile their code in a way that doesn’t rely on dangerous tricks
like putting executable trampolines on the stack.  It also makes it
essentially impossible for an exploit to run actual downloaded machine
code — if there is no way to run code that isn’t appropriately
labeled, then attackers are more limited in what they can do.

quoted

I don’t think that SGX should become an exception to either of these.
Code should not have an excuse to use WX memory just because it’s in
an enclave. Similarly, an exploit should not be able to run an
attacker-supplied enclave as a way around a policy that would
otherwise prevent downloaded code from running.

My apology for the confusion here.

I thought EXECMOD applied to files (and memory mappings backed by them) but I was probably wrong. It sounds like EXECMOD applies to the whole process so would allow all pages within a process's address space to be modified then executed, regardless the backing files. Am I correct this time?

No, you were correct the first time I think; EXECMOD is used to control whether a process can make executable a private file mapping that has previously been modified (e.g. text relocation); it is a special case to support text relocations without having to allow full EXECMEM (i.e. execute arbitrary memory).

SELinux checks relevant to W^X include:

- EXECMEM: mmap/mprotect PROT_EXEC an anonymous mapping (regardless of PROT_WRITE, since we know the content has to have been written at some point) or a private file mapping that is also PROT_WRITE.
- EXECMOD: mprotect PROT_EXEC a private file mapping that has been previously modified, typically for text relocations,
- FILE__WRITE: mmap/mprotect PROT_WRITE a shared file mapping,
- FILE__EXECUTE: mmap/mprotect PROT_EXEC a file mapping.

(ignoring EXECSTACK and EXECHEAP here since they aren't really relevant to this discussion)

So if you want to ensure W^X, then you wouldn't allow EXECMEM for the process, EXECMOD by the process to any file, and the combination of both FILE__WRITE and FILE__EXECUTE by the process to any file.

If the /dev/sgx/enclave mappings are MAP_SHARED and you aren't using an anonymous inode, then I would expect that only the FILE__WRITE and FILE__EXECUTE checks are relevant.

I was not saying enclaves were exempt to good security practices. What I was trying to say was, EPC pages are *not* subject to the same attacks as regular pages so I suspect there will be a desire to enforce different policies on them, especially after new SGX2 features/applications become available. So I think it beneficial to distinguish between regular vs. enclave virtual ranges. And to do that, a new VM_SGX flag in VMA is probably a very simple/easy way. And with that VM_SGX flag, we could add a new security_sgx_mprot() hook so that LSM modules/policies could act differently.

And if you are with me on that bigger picture, the next question is: what should be the default behavior of security_sgx_mprot() for existing/non-SGX-aware LSM modules/policies? I'd say a reasonable default is to allow R, RW and RX, but not anything else. It'd suffice to get rid of EXECMEM/EXECMOD requirements on enclave applications. For SGX1, EPCM permissions are immutable so it really doesn't matter what security_sgx_mprot() does. For SGX2 and beyond, there's still time and new SGX-aware LSM modules/policies will probably have emerged by then.

-Cedric

On Fri, May 17, 2019 at 09:53:06AM -0400, Stephen Smalley wrote:

On 5/16/19 6:23 PM, Xing, Cedric wrote:

quoted

I thought EXECMOD applied to files (and memory mappings backed by them) but
I was probably wrong. It sounds like EXECMOD applies to the whole process so
would allow all pages within a process's address space to be modified then
executed, regardless the backing files. Am I correct this time?

No, you were correct the first time I think; EXECMOD is used to control
whether a process can make executable a private file mapping that has
previously been modified (e.g. text relocation); it is a special case to
support text relocations without having to allow full EXECMEM (i.e. execute
arbitrary memory).

SELinux checks relevant to W^X include:

- EXECMEM: mmap/mprotect PROT_EXEC an anonymous mapping (regardless of
PROT_WRITE, since we know the content has to have been written at some
point) or a private file mapping that is also PROT_WRITE.
- EXECMOD: mprotect PROT_EXEC a private file mapping that has been
previously modified, typically for text relocations,
- FILE__WRITE: mmap/mprotect PROT_WRITE a shared file mapping,
- FILE__EXECUTE: mmap/mprotect PROT_EXEC a file mapping.

(ignoring EXECSTACK and EXECHEAP here since they aren't really relevant to
this discussion)

So if you want to ensure W^X, then you wouldn't allow EXECMEM for the
process, EXECMOD by the process to any file, and the combination of both
FILE__WRITE and FILE__EXECUTE by the process to any file.

If the /dev/sgx/enclave mappings are MAP_SHARED and you aren't using an
anonymous inode, then I would expect that only the FILE__WRITE and
FILE__EXECUTE checks are relevant.

Yep, I was just typing this up in a different thread:

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

On Thu, May 16, 2019 at 05:26:15PM -0700, Andy Lutomirski wrote:

On Thu, May 16, 2019 at 5:03 PM Sean Christopherson
[off-list ref] wrote:

quoted

On Wed, May 15, 2019 at 11:27:04AM -0700, Andy Lutomirski wrote:

quoted

Here's a very vague proposal that's kind of like what I've been
thinking over the past few days.  The SGX inode could track, for each
page, a "safe-to-execute" bit.  When you first open /dev/sgx/enclave,
you get a blank enclave and all pages are safe-to-execute.  When you
do the ioctl to load context (which could be code, data, or anything
else), the kernel will check whether the *source* VMA is executable
and, if not, mark the page of the enclave being loaded as unsafe.
Once the enclave is initialized, the driver will clear the
safe-to-execute bit for any page that is successfully mapped writably.

The intent is that a page of the enclave is safe-to-execute if that
page was populated from executable memory and not modified since then.
LSMs could then enforce a policy that you can map an enclave page RX
if the page is safe-to-execute, you can map any page you want for
write if there are no executable mappings, and you can only map a page
for write and execute simultaneously if you can EXECMOD permission.
This should allow an enclave to be loaded by userspace from a file
with EXECUTE rights.

I'm still confused as to why you want to track execute permissions on the
enclave pages and add SGX-specific LSM hooks.  Is there anything that
prevents userspace from building the enclave like any other DSO and then
copying it into enclave memory?

It's entirely possible that I'm the one missing something.  But here's
why I think this:

quoted

I feel like I'm missing something.

  1. Userspace loads enclave into regular memory, e.g. like a normal DSO.
     All mmap(), mprotect(), etc... calls are subject to all existing
     LSM policies.

  2. Userspace opens /dev/sgx/enclave to instantiate a new enclave.

  3. Userspace uses mmap() to allocate virtual memory for its enclave,
     again subject to all existing LSM policies (sane userspaces map it RO
     since the permissions eventually get tossed anyways).

Is userspace actually requred to mmap() the enclave prior to EADDing things?

It was a requirement prior to the API rework in v20, i.e. unless someone
was really quick on the draw after the v20 update all existing userspace
implementations mmap() the enclave before ECREATE.   Requiring a valid
enclave VMA for EADD shoudn't be too onerous.

quoted

  4. SGX subsystem refuses to service page faults for enclaves that have
     not yet been initialized, e.g. signals SIGBUS or SIGSEGV.

  5. Userspace invokes SGX ioctl() to copy enclave from regulary VMA to
     enclave VMA.

  6. SGX ioctl() propagates VMA protection-related flags from source VMA
     to enclave VMA, e.g. invokes mprotect_fixup().  Enclave VMA(s) may
     be split as part of this process.

Does this also call the LSM?  If so, what is it expected to do?

Nope.  My reasoning behind skipping LSM checks is that the LSMs have
already ok'd the source VMAs, similar to how dup_mmap() doesn't redo LSM
checks.

What happens if there are different regions with different permissions on
the same page?  SGX has 256-byte granularity right?

No, EPC pages have 4k granularity.

  The EPC is divided into EPC pages. An EPC page is 4KB in size and always
  aligned on a 4KB boundary

EEXTEND is the only aspect of SGX that works on 256-byte chunks, and that
goofiness is primarily to keep the latency of EEXTEND low enough so that
the instruction doesn't have to be interruptible, a la EINIT.

quoted

  7. At all times, mprotect() calls on the enclave VMA are subject to
     existing LSM policies, i.e. it's not special cased for enclaves.

I don't think the normal behavior actually works here.  An enclave is
always MAP_SHARED, so (with SELinux) mprotecting() to X or RX requires
EXECUTE and mprotecting() to RWX requires extra permissions.

Requiring extra permissions is good though, right?  My thinking is to make
the EADD "VMA copy" the happy/easy path, while using mprotect() to convert
EPC memory to executable would require PROCESS__EXECMEM (assuming we back
enclaves with anon inodes instead of /dev/sgx/enclave).

But user code can also mmap() the enclave again.  What is supposed to
happen in that case?

Hmm, it can't effectively re-mmap() the enclave as executable since
entering the enclave requires using the correct virtual address range,
i.e. EENTER would fail.  It could, I think, do munmap()->mmap() to change
the permissions.  We could handle that case fairly easily by invoking
security_file_mprotect() in SGX's mmap() hook if any pages have been added
to the enclave, i.e. treat mmap() like mprotect().

On Thu, May 16, 2019 at 05:35:16PM -0700, Andy Lutomirski wrote:

On Thu, May 16, 2019 at 3:23 PM Xing, Cedric [off-list ref] wrote:

quoted

And if you are with me on that bigger picture, the next question is: what
should be the default behavior of security_sgx_mprot() for
existing/non-SGX-aware LSM modules/policies? I'd say a reasonable default
is to allow R, RW and RX, but not anything else. It'd suffice to get rid of
EXECMEM/EXECMOD requirements on enclave applications. For SGX1, EPCM
permissions are immutable so it really doesn't matter what
security_sgx_mprot() does. For SGX2 and beyond, there's still time and new
SGX-aware LSM modules/policies will probably have emerged by then.

I hadn't thought about the SGX1 vs SGX2 difference.  If the driver
initially only wants to support SGX1, then I guess we really could get
away with constraining the EPC flags based on the source page
permission and not restricting mprotect() and mmap() permissions on
/dev/sgx/enclave at all.

No, SGX1 vs SGX2 support in the kernel is irrelevant.  Well, unless the
driver simply refuses to load on SGX2 hardware, but I don't think anyone
wants to go that route.  There is no enabling or attribute bit required
to execute ENCLU[EMODPE], e.g. an enclave can effect RW->RWX in the EPCM
on SGX2 hardware regardless of what the kernel is doing.

IMO the kernel should ignore the EPCM from an LSM perspective.

On May 17, 2019, at 9:20 AM, Stephen Smalley [off-list ref] wrote:

quoted

On 5/17/19 11:09 AM, Sean Christopherson wrote:

quoted

On Fri, May 17, 2019 at 09:53:06AM -0400, Stephen Smalley wrote:

quoted

On 5/16/19 6:23 PM, Xing, Cedric wrote:
I thought EXECMOD applied to files (and memory mappings backed by them) but
I was probably wrong. It sounds like EXECMOD applies to the whole process so
would allow all pages within a process's address space to be modified then
executed, regardless the backing files. Am I correct this time?

No, you were correct the first time I think; EXECMOD is used to control
whether a process can make executable a private file mapping that has
previously been modified (e.g. text relocation); it is a special case to
support text relocations without having to allow full EXECMEM (i.e. execute
arbitrary memory).

SELinux checks relevant to W^X include:

- EXECMEM: mmap/mprotect PROT_EXEC an anonymous mapping (regardless of
PROT_WRITE, since we know the content has to have been written at some
point) or a private file mapping that is also PROT_WRITE.
- EXECMOD: mprotect PROT_EXEC a private file mapping that has been
previously modified, typically for text relocations,
- FILE__WRITE: mmap/mprotect PROT_WRITE a shared file mapping,
- FILE__EXECUTE: mmap/mprotect PROT_EXEC a file mapping.

(ignoring EXECSTACK and EXECHEAP here since they aren't really relevant to
this discussion)

So if you want to ensure W^X, then you wouldn't allow EXECMEM for the
process, EXECMOD by the process to any file, and the combination of both
FILE__WRITE and FILE__EXECUTE by the process to any file.

If the /dev/sgx/enclave mappings are MAP_SHARED and you aren't using an
anonymous inode, then I would expect that only the FILE__WRITE and
FILE__EXECUTE checks are relevant.

Yep, I was just typing this up in a different thread:
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).

That agrees with my thoughts. Actually plumbing everything together so this works could be a bit interesting.  I assume it’ll need a special case in SELinux or maybe a new vm_op.

On 5/17/19 11:09 AM, Sean Christopherson wrote:

On Fri, May 17, 2019 at 09:53:06AM -0400, Stephen Smalley wrote:

quoted

On 5/16/19 6:23 PM, Xing, Cedric wrote:

quoted

I thought EXECMOD applied to files (and memory mappings backed by them) but
I was probably wrong. It sounds like EXECMOD applies to the whole process so
would allow all pages within a process's address space to be modified then
executed, regardless the backing files. Am I correct this time?

No, you were correct the first time I think; EXECMOD is used to control
whether a process can make executable a private file mapping that has
previously been modified (e.g. text relocation); it is a special case to
support text relocations without having to allow full EXECMEM (i.e. execute
arbitrary memory).

SELinux checks relevant to W^X include:

- EXECMEM: mmap/mprotect PROT_EXEC an anonymous mapping (regardless of
PROT_WRITE, since we know the content has to have been written at some
point) or a private file mapping that is also PROT_WRITE.
- EXECMOD: mprotect PROT_EXEC a private file mapping that has been
previously modified, typically for text relocations,
- FILE__WRITE: mmap/mprotect PROT_WRITE a shared file mapping,
- FILE__EXECUTE: mmap/mprotect PROT_EXEC a file mapping.

(ignoring EXECSTACK and EXECHEAP here since they aren't really relevant to
this discussion)

So if you want to ensure W^X, then you wouldn't allow EXECMEM for the
process, EXECMOD by the process to any file, and the combination of both
FILE__WRITE and FILE__EXECUTE by the process to any file.

If the /dev/sgx/enclave mappings are MAP_SHARED and you aren't using an
anonymous inode, then I would expect that only the FILE__WRITE and
FILE__EXECUTE checks are relevant.

Yep, I was just typing this up in a different thread:

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

On 5/17/19 12:20 PM, Stephen Smalley wrote:

On 5/17/19 11:09 AM, Sean Christopherson wrote:

quoted

On Fri, May 17, 2019 at 09:53:06AM -0400, Stephen Smalley wrote:

quoted

On 5/16/19 6:23 PM, Xing, Cedric wrote:

quoted

I thought EXECMOD applied to files (and memory mappings backed by them) but
I was probably wrong. It sounds like EXECMOD applies to the whole process so
would allow all pages within a process's address space to be modified then
executed, regardless the backing files. Am I correct this time?

No, you were correct the first time I think; EXECMOD is used to control
whether a process can make executable a private file mapping that has
previously been modified (e.g. text relocation); it is a special case to
support text relocations without having to allow full EXECMEM (i.e. execute
arbitrary memory).

SELinux checks relevant to W^X include:

- EXECMEM: mmap/mprotect PROT_EXEC an anonymous mapping (regardless of
PROT_WRITE, since we know the content has to have been written at some
point) or a private file mapping that is also PROT_WRITE.
- EXECMOD: mprotect PROT_EXEC a private file mapping that has been
previously modified, typically for text relocations,
- FILE__WRITE: mmap/mprotect PROT_WRITE a shared file mapping,
- FILE__EXECUTE: mmap/mprotect PROT_EXEC a file mapping.

(ignoring EXECSTACK and EXECHEAP here since they aren't really relevant to
this discussion)

So if you want to ensure W^X, then you wouldn't allow EXECMEM for the
process, EXECMOD by the process to any file, and the combination of both
FILE__WRITE and FILE__EXECUTE by the process to any file.

If the /dev/sgx/enclave mappings are MAP_SHARED and you aren't using an
anonymous inode, then I would expect that only the FILE__WRITE and
FILE__EXECUTE checks are relevant.

Yep, I was just typing this up in a different thread:

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

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.

On May 17, 2019, at 9:37 AM, Stephen Smalley [off-list ref] wrote:

quoted

On 5/17/19 12:20 PM, Stephen Smalley wrote:

quoted

On 5/17/19 11:09 AM, Sean Christopherson wrote:

quoted

On Fri, May 17, 2019 at 09:53:06AM -0400, Stephen Smalley wrote:

quoted

On 5/16/19 6:23 PM, Xing, Cedric wrote:
I thought EXECMOD applied to files (and memory mappings backed by them) but
I was probably wrong. It sounds like EXECMOD applies to the whole process so
would allow all pages within a process's address space to be modified then
executed, regardless the backing files. Am I correct this time?

No, you were correct the first time I think; EXECMOD is used to control
whether a process can make executable a private file mapping that has
previously been modified (e.g. text relocation); it is a special case to
support text relocations without having to allow full EXECMEM (i.e. execute
arbitrary memory).

SELinux checks relevant to W^X include:

- EXECMEM: mmap/mprotect PROT_EXEC an anonymous mapping (regardless of
PROT_WRITE, since we know the content has to have been written at some
point) or a private file mapping that is also PROT_WRITE.
- EXECMOD: mprotect PROT_EXEC a private file mapping that has been
previously modified, typically for text relocations,
- FILE__WRITE: mmap/mprotect PROT_WRITE a shared file mapping,
- FILE__EXECUTE: mmap/mprotect PROT_EXEC a file mapping.

(ignoring EXECSTACK and EXECHEAP here since they aren't really relevant to
this discussion)

So if you want to ensure W^X, then you wouldn't allow EXECMEM for the
process, EXECMOD by the process to any file, and the combination of both
FILE__WRITE and FILE__EXECUTE by the process to any file.

If the /dev/sgx/enclave mappings are MAP_SHARED and you aren't using an
anonymous inode, then I would expect that only the FILE__WRITE and
FILE__EXECUTE checks are relevant.

Yep, I was just typing this up in a different thread:

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

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.

How can that work?  Unless the API changes fairly radically, users fundamentally need to both write and execute the enclave.  Some of it will be written only from already executable pages, and some privilege should be needed to execute any enclave page that was not loaded like this.

On Fri, May 17, 2019 at 12:37:40PM -0400, Stephen Smalley wrote:

On 5/17/19 12:20 PM, Stephen Smalley wrote:

quoted

On 5/17/19 11:09 AM, Sean Christopherson wrote:

quoted

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.

On May 17, 2019, at 10:29 AM, Sean Christopherson [off-list ref] wrote:

quoted

On Fri, May 17, 2019 at 12:37:40PM -0400, Stephen Smalley wrote:

quoted

On 5/17/19 12:20 PM, Stephen Smalley wrote:

quoted

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.

quoted

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.

How does anon_inode make any difference?  Anon_inode is not the same thing as anon_vma.

On 5/17/19 1:29 PM, Sean Christopherson wrote:

On Fri, May 17, 2019 at 12:37:40PM -0400, Stephen Smalley wrote:

quoted

On 5/17/19 12:20 PM, Stephen Smalley wrote:

quoted

On 5/17/19 11:09 AM, Sean Christopherson wrote:

quoted

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.

quoted

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.

On Fri, May 17, 2019 at 01:42:50PM -0400, Stephen Smalley wrote:

On 5/17/19 1:29 PM, Sean Christopherson wrote:

quoted

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.

Ah, so each process has its own FILE__* permissions for a specific set of
files?

Does that allow differentiating between a process making an EPC page RWX
and a process making two separate EPC pages RW and RX?

On Fri, May 17, 2019 at 10:43:01AM -0700, Andy Lutomirski wrote:

quoted

On May 17, 2019, at 10:29 AM, Sean Christopherson [off-list ref] wrote:

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.

How does anon_inode make any difference?  Anon_inode is not the same thing as
anon_vma.

In this snippet, IS_PRIVATE() is true for anon inodes, false for
/dev/sgx/enclave.  Because EPC memory is always shared, SELinux will never
check PROCESS__EXECMEM for mprotect() on/dev/sgx/enclave.

static int file_map_prot_check(struct file *file, unsigned long prot, int shared)
{
        const struct cred *cred = current_cred();
        u32 sid = cred_sid(cred);
        int rc = 0;

        if (default_noexec &&
            (prot & PROT_EXEC) && (!file || IS_PRIVATE(file_inode(file)) ||
                                   (!shared && (prot & PROT_WRITE)))) {
                /*
                 * We are making executable an anonymous mapping or a
                 * private file mapping that will also be writable.
                 * This has an additional check.
                 */
                rc = avc_has_perm(&selinux_state,
                                  sid, sid, SECCLASS_PROCESS,
                                  PROCESS__EXECMEM, NULL);
                if (rc)
                        goto error;
        }

	...
}

On 5/17/19 1:12 PM, Andy Lutomirski wrote:

quoted

On May 17, 2019, at 9:37 AM, Stephen Smalley [off-list ref] wrote:

quoted

On 5/17/19 12:20 PM, Stephen Smalley wrote:

quoted

On 5/17/19 11:09 AM, Sean Christopherson wrote:

quoted

On Fri, May 17, 2019 at 09:53:06AM -0400, Stephen Smalley wrote:

quoted

On 5/16/19 6:23 PM, Xing, Cedric wrote:
I thought EXECMOD applied to files (and memory mappings backed by them) but
I was probably wrong. It sounds like EXECMOD applies to the whole process so
would allow all pages within a process's address space to be modified then
executed, regardless the backing files. Am I correct this time?

No, you were correct the first time I think; EXECMOD is used to control
whether a process can make executable a private file mapping that has
previously been modified (e.g. text relocation); it is a special case to
support text relocations without having to allow full EXECMEM (i.e. execute
arbitrary memory).

SELinux checks relevant to W^X include:

- EXECMEM: mmap/mprotect PROT_EXEC an anonymous mapping (regardless of
PROT_WRITE, since we know the content has to have been written at some
point) or a private file mapping that is also PROT_WRITE.
- EXECMOD: mprotect PROT_EXEC a private file mapping that has been
previously modified, typically for text relocations,
- FILE__WRITE: mmap/mprotect PROT_WRITE a shared file mapping,
- FILE__EXECUTE: mmap/mprotect PROT_EXEC a file mapping.

(ignoring EXECSTACK and EXECHEAP here since they aren't really relevant to
this discussion)

So if you want to ensure W^X, then you wouldn't allow EXECMEM for the
process, EXECMOD by the process to any file, and the combination of both
FILE__WRITE and FILE__EXECUTE by the process to any file.

If the /dev/sgx/enclave mappings are MAP_SHARED and you aren't using an
anonymous inode, then I would expect that only the FILE__WRITE and
FILE__EXECUTE checks are relevant.

Yep, I was just typing this up in a different thread:

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

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.

How can that work?  Unless the API changes fairly radically, users fundamentally need to both write and execute the enclave.  Some of it will be written only from already executable pages, and some privilege should be needed to execute any enclave page that was not loaded like this.

I'm not sure what the API is. Let's say they do something like this:

fd = open("/dev/sgx/enclave", O_RDONLY);
addr = mmap(NULL, size, PROT_READ | PROT_EXEC, MAP_SHARED, fd, 0);
stuff addr into ioctl args
ioctl(fd, ENCLAVE_CREATE, &ioctlargs);
ioctl(fd, ENCLAVE_ADD_PAGE, &ioctlargs);
ioctl(fd, ENCLAVE_INIT, &ioctlargs);

The important points are that they do not open /dev/sgx/enclave with write access (otherwise they will trigger FILE__WRITE at open time, and later encounter FILE__EXECUTE as well during mmap, thereby requiring both to be allowed to /dev/sgx/enclave), and that they do not request PROT_WRITE to the resulting mapping (otherwise they will trigger FILE__WRITE at mmap time).  Then only FILE__READ and FILE__EXECUTE are required to /dev/sgx/enclave in policy.

If they switch to an anon inode, then any mmap PROT_EXEC of the opened file will trigger an EXECMEM check, at least as currently implemented, as we have no useful backing inode information.

On Fri, May 17, 2019 at 10:55 AM Sean Christopherson
[off-list ref] wrote:

In this snippet, IS_PRIVATE() is true for anon inodes, false for
/dev/sgx/enclave.  Because EPC memory is always shared, SELinux will never
check PROCESS__EXECMEM for mprotect() on/dev/sgx/enclave.

Why _does_ the memory have to be shared? Shared mmap() is
fundamentally less secure than private mmap, since by definition it
means "oh, somebody else has access to it too and might modify it
under us".

Why does the SGX logic care about things like that? Normal executables
are just private mappings of an underlying file, I'm not sure why the
SGX interface has to have that shared thing, and why the interface has
to have a device node in the first place when  you have system calls
for setup anyway.

So why don't the system calls just work on perfectly normal anonymous
mmap's? Why a device node, and why must it be shared to begin with?

                  Linus

On 5/17/19 1:50 PM, Sean Christopherson wrote:

On Fri, May 17, 2019 at 01:42:50PM -0400, Stephen Smalley wrote:

quoted

On 5/17/19 1:29 PM, Sean Christopherson wrote:

quoted

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.

Ah, so each process has its own FILE__* permissions for a specific set of
files?

That's correct.

Does that allow differentiating between a process making an EPC page RWX
and a process making two separate EPC pages RW and RX?

Not if they are backed by the same inode, nor if they are all backed by anon inodes, at least not as currently implemented.

On Fri, May 17, 2019 at 11:04:22AM -0700, Linus Torvalds wrote:

On Fri, May 17, 2019 at 10:55 AM Sean Christopherson
[off-list ref] wrote:

quoted

In this snippet, IS_PRIVATE() is true for anon inodes, false for
/dev/sgx/enclave.  Because EPC memory is always shared, SELinux will never
check PROCESS__EXECMEM for mprotect() on/dev/sgx/enclave.

Why _does_ the memory have to be shared? Shared mmap() is
fundamentally less secure than private mmap, since by definition it
means "oh, somebody else has access to it too and might modify it
under us".

Why does the SGX logic care about things like that? Normal executables
are just private mappings of an underlying file, I'm not sure why the
SGX interface has to have that shared thing, and why the interface has
to have a device node in the first place when  you have system calls
for setup anyway.

So why don't the system calls just work on perfectly normal anonymous
mmap's? Why a device node, and why must it be shared to begin with?

I agree that conceptually EPC is private memory, but because EPC is
managed as a separate memory pool, SGX tags it VM_PFNMAP and manually
inserts PFNs, i.e. EPC effectively it gets classified as IO memory. 

And vmf_insert_pfn_prot() doesn't like writable private IO mappings:

   BUG_ON((vma->vm_flags & VM_PFNMAP) && is_cow_mapping(vma->vm_flags));

On Fri, May 17, 2019 at 11:21 AM Sean Christopherson
[off-list ref] wrote:

I agree that conceptually EPC is private memory, but because EPC is
managed as a separate memory pool, SGX tags it VM_PFNMAP and manually
inserts PFNs, i.e. EPC effectively it gets classified as IO memory.

And vmf_insert_pfn_prot() doesn't like writable private IO mappings:

   BUG_ON((vma->vm_flags & VM_PFNMAP) && is_cow_mapping(vma->vm_flags));

Hmm. I haven't looked into why you want to do your own page insertion
and not just "use existing pages", but I'm sure there's some reason.

It looks like the "shared vs private" inode part is a red herring,
though. You might as well give each opener of the sgx node its own
inode - and you probably should. Then you can keep track of the pages
that have been added in the inode->i_mapping, and you could avoid the
whole PFN thing entirely. I still am not a huge fan of the device node
in the first place, but I guess it's just one more place where a
system admin can then give (or deny) access to a kernel feature from
users. I guess the kvm people do the same thing, for not necessarily
any better reasons.

With the PFNMAP model I guess the SGX memory ends up being unswappable
- at least done the obvious way.

Again, the way I'd expect it to be done is as a shmem inode - that
would I think be a better model. But I think that's a largely internal
design decision, and the device node could just do that eventually
(and the mmap could just map the populated shmem information into
memory, no PFNMAP needed - the inode and the mapping could be
"read-only" as far as the _user_ is concerned, but the i_mapping then
gets populated by the ioctl's).

I have not actually looked at any of the SGX patches, so maybe you're
already doing something like that (although the PFNMAP comment makes
me think not), and quite possibly there's some fundamental reason why
you can't just use the shmem approach.

So my high-level reaction here may be just the rantings of somebody
who just isn't familiar with what you do. My "why not shmem and
regular mmap" questions come from a 30000ft view without knowing any
of the details.

                   Linus

On Fri, May 17, 2019 at 11:33:30AM -0700, Linus Torvalds wrote:

On Fri, May 17, 2019 at 11:21 AM Sean Christopherson
[off-list ref] wrote:

quoted

I agree that conceptually EPC is private memory, but because EPC is
managed as a separate memory pool, SGX tags it VM_PFNMAP and manually
inserts PFNs, i.e. EPC effectively it gets classified as IO memory.

And vmf_insert_pfn_prot() doesn't like writable private IO mappings:

   BUG_ON((vma->vm_flags & VM_PFNMAP) && is_cow_mapping(vma->vm_flags));

Hmm. I haven't looked into why you want to do your own page insertion
and not just "use existing pages", but I'm sure there's some reason.

Outside of the SGX subsystem, the kernel is unaware of EPC memory, e.g.
BIOS enumerates it as reserved memory in the e820 tables, or not at all.

On current hardware, EPC is backed by system memory, but it's protected
by a range registers (and other stuff) and can't be accessed directly
except when the CPU is in "enclave mode", i.e. executing an enclave in
CPL3.  To execute an enclave it must first be built, and because EPC
memory can't be written outside of enclave mode, the only way to build
the enclave is via dedicated CPL0 ISA, e.g. ENCLS[EADD].

It looks like the "shared vs private" inode part is a red herring,
though. You might as well give each opener of the sgx node its own
inode - and you probably should. Then you can keep track of the pages
that have been added in the inode->i_mapping, and you could avoid the
whole PFN thing entirely. I still am not a huge fan of the device node
in the first place, but I guess it's just one more place where a
system admin can then give (or deny) access to a kernel feature from
users. I guess the kvm people do the same thing, for not necessarily
any better reasons.

With the PFNMAP model I guess the SGX memory ends up being unswappable
- at least done the obvious way.

EPC memory is swappable in it's own terms, e.g. pages can be swapped
from EPC to system RAM and vice versa, but again moving pages in and out
of the EPC can only be done through dedicated CPL0 ISA.  And there are
additional TLB flushing requirements, evicted pages need to be refcounted
against the enclave, evicted pages need an anchor in the EPC to ensure
freshness, etc...

Long story short, we decided to manage EPC in the SGX subsystem as a
separate memory pool rather than modify the kernel's MMU to teach it
how to deal with EPC.

Again, the way I'd expect it to be done is as a shmem inode - that
would I think be a better model. But I think that's a largely internal
design decision, and the device node could just do that eventually
(and the mmap could just map the populated shmem information into
memory, no PFNMAP needed - the inode and the mapping could be
"read-only" as far as the _user_ is concerned, but the i_mapping then
gets populated by the ioctl's).

I have not actually looked at any of the SGX patches, so maybe you're
already doing something like that (although the PFNMAP comment makes
me think not), and quite possibly there's some fundamental reason why
you can't just use the shmem approach.

So my high-level reaction here may be just the rantings of somebody
who just isn't familiar with what you do. My "why not shmem and
regular mmap" questions come from a 30000ft view without knowing any
of the details.

                   Linus

On May 17, 2019, at 11:21 AM, Sean Christopherson [off-list ref] wrote:

quoted

On Fri, May 17, 2019 at 11:04:22AM -0700, Linus Torvalds wrote:
On Fri, May 17, 2019 at 10:55 AM Sean Christopherson
[off-list ref] wrote:

quoted

In this snippet, IS_PRIVATE() is true for anon inodes, false for
/dev/sgx/enclave.  Because EPC memory is always shared, SELinux will never
check PROCESS__EXECMEM for mprotect() on/dev/sgx/enclave.

Why _does_ the memory have to be shared? Shared mmap() is
fundamentally less secure than private mmap, since by definition it
means "oh, somebody else has access to it too and might modify it
under us".

Why does the SGX logic care about things like that? Normal executables
are just private mappings of an underlying file, I'm not sure why the
SGX interface has to have that shared thing, and why the interface has
to have a device node in the first place when  you have system calls
for setup anyway.

So why don't the system calls just work on perfectly normal anonymous
mmap's? Why a device node, and why must it be shared to begin with?

I agree that conceptually EPC is private memory, but because EPC is
managed as a separate memory pool, SGX tags it VM_PFNMAP and manually
inserts PFNs, i.e. EPC effectively it gets classified as IO memory. 

And vmf_insert_pfn_prot() doesn't like writable private IO mappings:

  BUG_ON((vma->vm_flags & VM_PFNMAP) && is_cow_mapping(vma->vm_flags));

I don’t see how it could be anonymous even in principle.  The kernel can’t *read* the memory — how could we possibly CoW it?  And we can’t share an RO backing pages between two different enclaves because the CPU won’t let us — each EPC page belongs to a particular enclave.  And fork()ing an enclave is right out.

So I agree that MAP_ANONYMOUS would be nice conceptually, but I don’t see how it would work.

On 5/17/19 2:05 PM, Stephen Smalley wrote:

On 5/17/19 1:12 PM, Andy Lutomirski wrote:

quoted

quoted

On May 17, 2019, at 9:37 AM, Stephen Smalley [off-list ref] wrote:

quoted

On 5/17/19 12:20 PM, Stephen Smalley wrote:

quoted

On 5/17/19 11:09 AM, Sean Christopherson wrote:

quoted

On Fri, May 17, 2019 at 09:53:06AM -0400, Stephen Smalley wrote:

quoted

On 5/16/19 6:23 PM, Xing, Cedric wrote:
I thought EXECMOD applied to files (and memory mappings backed by them) but
I was probably wrong. It sounds like EXECMOD applies to the whole process so
would allow all pages within a process's address space to be modified then
executed, regardless the backing files. Am I correct this time?

No, you were correct the first time I think; EXECMOD is used to control
whether a process can make executable a private file mapping that has
previously been modified (e.g. text relocation); it is a special case to
support text relocations without having to allow full EXECMEM (i.e. execute
arbitrary memory).

SELinux checks relevant to W^X include:

- EXECMEM: mmap/mprotect PROT_EXEC an anonymous mapping (regardless of
PROT_WRITE, since we know the content has to have been written at some
point) or a private file mapping that is also PROT_WRITE.
- EXECMOD: mprotect PROT_EXEC a private file mapping that has been
previously modified, typically for text relocations,
- FILE__WRITE: mmap/mprotect PROT_WRITE a shared file mapping,
- FILE__EXECUTE: mmap/mprotect PROT_EXEC a file mapping.

(ignoring EXECSTACK and EXECHEAP here since they aren't really relevant to
this discussion)

So if you want to ensure W^X, then you wouldn't allow EXECMEM for the
process, EXECMOD by the process to any file, and the combination of both
FILE__WRITE and FILE__EXECUTE by the process to any file.

If the /dev/sgx/enclave mappings are MAP_SHARED and you aren't using an
anonymous inode, then I would expect that only the FILE__WRITE and
FILE__EXECUTE checks are relevant.

Yep, I was just typing this up in a different thread:

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

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.

How can that work?  Unless the API changes fairly radically, users fundamentally need to both write and execute the enclave.  Some of it will be written only from already executable pages, and some privilege should be needed to execute any enclave page that was not loaded like this.

I'm not sure what the API is. Let's say they do something like this:

fd = open("/dev/sgx/enclave", O_RDONLY);
addr = mmap(NULL, size, PROT_READ | PROT_EXEC, MAP_SHARED, fd, 0);
stuff addr into ioctl args
ioctl(fd, ENCLAVE_CREATE, &ioctlargs);
ioctl(fd, ENCLAVE_ADD_PAGE, &ioctlargs);
ioctl(fd, ENCLAVE_INIT, &ioctlargs);

The important points are that they do not open /dev/sgx/enclave with write access (otherwise they will trigger FILE__WRITE at open time, and later encounter FILE__EXECUTE as well during mmap, thereby requiring both to be allowed to /dev/sgx/enclave), and that they do not request PROT_WRITE to the resulting mapping (otherwise they will trigger FILE__WRITE at mmap time).  Then only FILE__READ and FILE__EXECUTE are required to /dev/sgx/enclave in policy.

If they switch to an anon inode, then any mmap PROT_EXEC of the opened file will trigger an EXECMEM check, at least as currently implemented, as we have no useful backing inode information.

FWIW, looking at the selftest for SGX in the patch series, they open /dev/sgx/enclave O_RDWR (probably not necessary?) and mmap the open file RWX.  If that is necessary then I'd rather it show up as FILE__WRITE and FILE__EXECUTE to /dev/sgx/enclave instead of EXECMEM, so that we can allow the process the ability to perform that mmap without allowing it to make other mappings WX.  So staying with the single /dev/sgx/enclave inode is better in that regard.

On Fri, May 17, 2019 at 02:05:39PM -0400, Stephen Smalley wrote:

On 5/17/19 1:12 PM, Andy Lutomirski wrote:

quoted

How can that work?  Unless the API changes fairly radically, users
fundamentally need to both write and execute the enclave.  Some of it will
be written only from already executable pages, and some privilege should be
needed to execute any enclave page that was not loaded like this.

I'm not sure what the API is. Let's say they do something like this:

fd = open("/dev/sgx/enclave", O_RDONLY);
addr = mmap(NULL, size, PROT_READ | PROT_EXEC, MAP_SHARED, fd, 0);
stuff addr into ioctl args
ioctl(fd, ENCLAVE_CREATE, &ioctlargs);
ioctl(fd, ENCLAVE_ADD_PAGE, &ioctlargs);
ioctl(fd, ENCLAVE_INIT, &ioctlargs);

That's rougly the flow, except that that all enclaves need to have RW and
X EPC pages.

The important points are that they do not open /dev/sgx/enclave with write
access (otherwise they will trigger FILE__WRITE at open time, and later
encounter FILE__EXECUTE as well during mmap, thereby requiring both to be
allowed to /dev/sgx/enclave), and that they do not request PROT_WRITE to the
resulting mapping (otherwise they will trigger FILE__WRITE at mmap time).
Then only FILE__READ and FILE__EXECUTE are required to /dev/sgx/enclave in
policy.

If they switch to an anon inode, then any mmap PROT_EXEC of the opened file
will trigger an EXECMEM check, at least as currently implemented, as we have
no useful backing inode information.

Yep, and that's by design in the overall proposal.  The trick is that
ENCLAVE_ADD takes a source VMA and copies the contents *and* the
permissions from the source VMA.  The source VMA points at regular memory
that was mapped and populated using existing mechanisms for loading DSOs.

E.g. at a high level:

source_fd = open("/home/sean/path/to/my/enclave", O_RDONLY);
for_each_chunk {
        <hand waving - mmap()/mprotect() the enclave file into regular memory>
}

enclave_fd = open("/dev/sgx/enclave", O_RDWR); /* allocs anon inode */
enclave_addr = mmap(NULL, size, PROT_READ, MAP_SHARED, enclave_fd, 0);

ioctl(enclave_fd, ENCLAVE_CREATE, {enclave_addr});
for_each_chunk {
        struct sgx_enclave_add ioctlargs = {
                .offset = chunk.offset,
                .source = chunk.addr,
                .size   = chunk.size,
                .type   = chunk.type, /* SGX specific metadata */
        }
        ioctl(fd, ENCLAVE_ADD, &ioctlargs); /* modifies enclave's VMAs */
}
ioctl(fd, ENCLAVE_INIT, ...);


Userspace never explicitly requests PROT_EXEC on enclave_fd, but SGX also
ensures userspace isn't bypassing LSM policies by virtue of copying the
permissions for EPC VMAs from regular VMAs that have already gone through
LSM checks.

On 5/17/19 3:28 PM, Sean Christopherson wrote:

On Fri, May 17, 2019 at 02:05:39PM -0400, Stephen Smalley wrote:

quoted

On 5/17/19 1:12 PM, Andy Lutomirski wrote:

quoted

How can that work?  Unless the API changes fairly radically, users
fundamentally need to both write and execute the enclave.  Some of it will
be written only from already executable pages, and some privilege should be
needed to execute any enclave page that was not loaded like this.

I'm not sure what the API is. Let's say they do something like this:

fd = open("/dev/sgx/enclave", O_RDONLY);
addr = mmap(NULL, size, PROT_READ | PROT_EXEC, MAP_SHARED, fd, 0);
stuff addr into ioctl args
ioctl(fd, ENCLAVE_CREATE, &ioctlargs);
ioctl(fd, ENCLAVE_ADD_PAGE, &ioctlargs);
ioctl(fd, ENCLAVE_INIT, &ioctlargs);

That's rougly the flow, except that that all enclaves need to have RW and
X EPC pages.

quoted

The important points are that they do not open /dev/sgx/enclave with write
access (otherwise they will trigger FILE__WRITE at open time, and later
encounter FILE__EXECUTE as well during mmap, thereby requiring both to be
allowed to /dev/sgx/enclave), and that they do not request PROT_WRITE to the
resulting mapping (otherwise they will trigger FILE__WRITE at mmap time).
Then only FILE__READ and FILE__EXECUTE are required to /dev/sgx/enclave in
policy.

If they switch to an anon inode, then any mmap PROT_EXEC of the opened file
will trigger an EXECMEM check, at least as currently implemented, as we have
no useful backing inode information.

Yep, and that's by design in the overall proposal.  The trick is that
ENCLAVE_ADD takes a source VMA and copies the contents *and* the
permissions from the source VMA.  The source VMA points at regular memory
that was mapped and populated using existing mechanisms for loading DSOs.

E.g. at a high level:

source_fd = open("/home/sean/path/to/my/enclave", O_RDONLY);
for_each_chunk {
         <hand waving - mmap()/mprotect() the enclave file into regular memory>
}

enclave_fd = open("/dev/sgx/enclave", O_RDWR); /* allocs anon inode */
enclave_addr = mmap(NULL, size, PROT_READ, MAP_SHARED, enclave_fd, 0);

ioctl(enclave_fd, ENCLAVE_CREATE, {enclave_addr});
for_each_chunk {
         struct sgx_enclave_add ioctlargs = {
                 .offset = chunk.offset,
                 .source = chunk.addr,
                 .size   = chunk.size,
                 .type   = chunk.type, /* SGX specific metadata */
         }
         ioctl(fd, ENCLAVE_ADD, &ioctlargs); /* modifies enclave's VMAs */
}
ioctl(fd, ENCLAVE_INIT, ...);


Userspace never explicitly requests PROT_EXEC on enclave_fd, but SGX also
ensures userspace isn't bypassing LSM policies by virtue of copying the
permissions for EPC VMAs from regular VMAs that have already gone through
LSM checks.

Is O_RDWR required for /dev/sgx/enclave or would O_RDONLY suffice?  Do you do anything other than ioctl() calls on it?

What's the advantage of allocating an anon inode in the above?  At present anon inodes are exempted from inode-based checking, thereby losing the ability to perform SELinux ioctl whitelisting, unlike the file-backed /dev/sgx/enclave inode.

How would SELinux (or other security modules) restrict the authorized enclaves that can be loaded via this interface?  Would the sgx driver invoke a new LSM hook with the regular/source VMAs as parameters and allow the security module to reject the ENCLAVE_ADD operation?  That could be just based on the vm_file (e.g. whitelist what enclave files are permitted in general) or it could be based on both the process and the vm_file (e.g. only allow specific enclaves to be loaded into specific processes).

On May 17, 2019, at 1:09 PM, Stephen Smalley [off-list ref] wrote:

quoted

On 5/17/19 3:28 PM, Sean Christopherson wrote:

quoted

On Fri, May 17, 2019 at 02:05:39PM -0400, Stephen Smalley wrote:

quoted

On 5/17/19 1:12 PM, Andy Lutomirski wrote:

How can that work?  Unless the API changes fairly radically, users
fundamentally need to both write and execute the enclave.  Some of it will
be written only from already executable pages, and some privilege should be
needed to execute any enclave page that was not loaded like this.

I'm not sure what the API is. Let's say they do something like this:

fd = open("/dev/sgx/enclave", O_RDONLY);
addr = mmap(NULL, size, PROT_READ | PROT_EXEC, MAP_SHARED, fd, 0);
stuff addr into ioctl args
ioctl(fd, ENCLAVE_CREATE, &ioctlargs);
ioctl(fd, ENCLAVE_ADD_PAGE, &ioctlargs);
ioctl(fd, ENCLAVE_INIT, &ioctlargs);

That's rougly the flow, except that that all enclaves need to have RW and
X EPC pages.

quoted

The important points are that they do not open /dev/sgx/enclave with write
access (otherwise they will trigger FILE__WRITE at open time, and later
encounter FILE__EXECUTE as well during mmap, thereby requiring both to be
allowed to /dev/sgx/enclave), and that they do not request PROT_WRITE to the
resulting mapping (otherwise they will trigger FILE__WRITE at mmap time).
Then only FILE__READ and FILE__EXECUTE are required to /dev/sgx/enclave in
policy.

If they switch to an anon inode, then any mmap PROT_EXEC of the opened file
will trigger an EXECMEM check, at least as currently implemented, as we have
no useful backing inode information.

Yep, and that's by design in the overall proposal.  The trick is that
ENCLAVE_ADD takes a source VMA and copies the contents *and* the
permissions from the source VMA.  The source VMA points at regular memory
that was mapped and populated using existing mechanisms for loading DSOs.
E.g. at a high level:
source_fd = open("/home/sean/path/to/my/enclave", O_RDONLY);
for_each_chunk {
        <hand waving - mmap()/mprotect() the enclave file into regular memory>
}
enclave_fd = open("/dev/sgx/enclave", O_RDWR); /* allocs anon inode */
enclave_addr = mmap(NULL, size, PROT_READ, MAP_SHARED, enclave_fd, 0);
ioctl(enclave_fd, ENCLAVE_CREATE, {enclave_addr});
for_each_chunk {
        struct sgx_enclave_add ioctlargs = {
                .offset = chunk.offset,
                .source = chunk.addr,
                .size   = chunk.size,
                .type   = chunk.type, /* SGX specific metadata */
        }
        ioctl(fd, ENCLAVE_ADD, &ioctlargs); /* modifies enclave's VMAs */
}
ioctl(fd, ENCLAVE_INIT, ...);
Userspace never explicitly requests PROT_EXEC on enclave_fd, but SGX also
ensures userspace isn't bypassing LSM policies by virtue of copying the
permissions for EPC VMAs from regular VMAs that have already gone through
LSM checks.

Is O_RDWR required for /dev/sgx/enclave or would O_RDONLY suffice?  Do you do anything other than ioctl() calls on it?

What's the advantage of allocating an anon inode in the above?  At present anon inodes are exempted from inode-based checking, thereby losing the ability to perform SELinux ioctl whitelisting, unlike the file-backed /dev/sgx/enclave inode.

How would SELinux (or other security modules) restrict the authorized enclaves that can be loaded via this interface?  Would the sgx driver invoke a new LSM hook with the regular/source VMAs as parameters and allow the security module to reject the ENCLAVE_ADD operation?  That could be just based on the vm_file (e.g. whitelist what enclave files are permitted in general) or it could be based on both the process and the vm_file (e.g. only allow specific enclaves to be loaded into specific processes).

This is the idea behind the .sigstruct file. The driver could call a new hook to approve or reject the .sigstruct. The sigstruct contains a hash of the whole enclave and a signature by the author.

On 5/17/19 4:14 PM, Andy Lutomirski wrote:

quoted

On May 17, 2019, at 1:09 PM, Stephen Smalley [off-list ref] wrote:

quoted

On 5/17/19 3:28 PM, Sean Christopherson wrote:

quoted

On Fri, May 17, 2019 at 02:05:39PM -0400, Stephen Smalley wrote:

quoted

On 5/17/19 1:12 PM, Andy Lutomirski wrote:

How can that work?  Unless the API changes fairly radically, users
fundamentally need to both write and execute the enclave.  Some of it will
be written only from already executable pages, and some privilege should be
needed to execute any enclave page that was not loaded like this.

I'm not sure what the API is. Let's say they do something like this:

fd = open("/dev/sgx/enclave", O_RDONLY);
addr = mmap(NULL, size, PROT_READ | PROT_EXEC, MAP_SHARED, fd, 0);
stuff addr into ioctl args
ioctl(fd, ENCLAVE_CREATE, &ioctlargs);
ioctl(fd, ENCLAVE_ADD_PAGE, &ioctlargs);
ioctl(fd, ENCLAVE_INIT, &ioctlargs);

That's rougly the flow, except that that all enclaves need to have RW and
X EPC pages.

quoted

The important points are that they do not open /dev/sgx/enclave with write
access (otherwise they will trigger FILE__WRITE at open time, and later
encounter FILE__EXECUTE as well during mmap, thereby requiring both to be
allowed to /dev/sgx/enclave), and that they do not request PROT_WRITE to the
resulting mapping (otherwise they will trigger FILE__WRITE at mmap time).
Then only FILE__READ and FILE__EXECUTE are required to /dev/sgx/enclave in
policy.

If they switch to an anon inode, then any mmap PROT_EXEC of the opened file
will trigger an EXECMEM check, at least as currently implemented, as we have
no useful backing inode information.

Yep, and that's by design in the overall proposal.  The trick is that
ENCLAVE_ADD takes a source VMA and copies the contents *and* the
permissions from the source VMA.  The source VMA points at regular memory
that was mapped and populated using existing mechanisms for loading DSOs.
E.g. at a high level:
source_fd = open("/home/sean/path/to/my/enclave", O_RDONLY);
for_each_chunk {
         <hand waving - mmap()/mprotect() the enclave file into regular memory>
}
enclave_fd = open("/dev/sgx/enclave", O_RDWR); /* allocs anon inode */
enclave_addr = mmap(NULL, size, PROT_READ, MAP_SHARED, enclave_fd, 0);
ioctl(enclave_fd, ENCLAVE_CREATE, {enclave_addr});
for_each_chunk {
         struct sgx_enclave_add ioctlargs = {
                 .offset = chunk.offset,
                 .source = chunk.addr,
                 .size   = chunk.size,
                 .type   = chunk.type, /* SGX specific metadata */
         }
         ioctl(fd, ENCLAVE_ADD, &ioctlargs); /* modifies enclave's VMAs */
}
ioctl(fd, ENCLAVE_INIT, ...);
Userspace never explicitly requests PROT_EXEC on enclave_fd, but SGX also
ensures userspace isn't bypassing LSM policies by virtue of copying the
permissions for EPC VMAs from regular VMAs that have already gone through
LSM checks.

Is O_RDWR required for /dev/sgx/enclave or would O_RDONLY suffice?  Do you do anything other than ioctl() calls on it?

What's the advantage of allocating an anon inode in the above?  At present anon inodes are exempted from inode-based checking, thereby losing the ability to perform SELinux ioctl whitelisting, unlike the file-backed /dev/sgx/enclave inode.

How would SELinux (or other security modules) restrict the authorized enclaves that can be loaded via this interface?  Would the sgx driver invoke a new LSM hook with the regular/source VMAs as parameters and allow the security module to reject the ENCLAVE_ADD operation?  That could be just based on the vm_file (e.g. whitelist what enclave files are permitted in general) or it could be based on both the process and the vm_file (e.g. only allow specific enclaves to be loaded into specific processes).

This is the idea behind the .sigstruct file. The driver could call a new hook to approve or reject the .sigstruct. The sigstruct contains a hash of the whole enclave and a signature by the author.

Ok, so same idea but moved to ENCLAVE_INIT and passing the vma or file for the sigstruct instead of the enclave.

On Fri, May 17, 2019 at 04:09:22PM -0400, Stephen Smalley wrote:

On 5/17/19 3:28 PM, Sean Christopherson wrote:

quoted

On Fri, May 17, 2019 at 02:05:39PM -0400, Stephen Smalley wrote:
Yep, and that's by design in the overall proposal.  The trick is that
ENCLAVE_ADD takes a source VMA and copies the contents *and* the
permissions from the source VMA.  The source VMA points at regular memory
that was mapped and populated using existing mechanisms for loading DSOs.

E.g. at a high level:

source_fd = open("/home/sean/path/to/my/enclave", O_RDONLY);
for_each_chunk {
        <hand waving - mmap()/mprotect() the enclave file into regular memory>
}

enclave_fd = open("/dev/sgx/enclave", O_RDWR); /* allocs anon inode */
enclave_addr = mmap(NULL, size, PROT_READ, MAP_SHARED, enclave_fd, 0);

ioctl(enclave_fd, ENCLAVE_CREATE, {enclave_addr});
for_each_chunk {
        struct sgx_enclave_add ioctlargs = {
                .offset = chunk.offset,
                .source = chunk.addr,
                .size   = chunk.size,
                .type   = chunk.type, /* SGX specific metadata */
        }
        ioctl(fd, ENCLAVE_ADD, &ioctlargs); /* modifies enclave's VMAs */
}
ioctl(fd, ENCLAVE_INIT, ...);


Userspace never explicitly requests PROT_EXEC on enclave_fd, but SGX also
ensures userspace isn't bypassing LSM policies by virtue of copying the
permissions for EPC VMAs from regular VMAs that have already gone through
LSM checks.

Is O_RDWR required for /dev/sgx/enclave or would O_RDONLY suffice?  Do you
do anything other than ioctl() calls on it?

Hmm, in the current implementation, yes, O_RDWR is required.  An enclave
and its associated EPC memory are represented and referenced by its fd,
which is backed by /dev/sgx/enclave.  An enclave is not just code, e.g.
also has a heap, stack, variables, etc..., which need to be mapped
accordingly.  In the current implementation, userspace directly does
mprotect() or mmap() on EPC VMAs, and so setting PROT_WRITE for the heap
and whatnot requires opening /dev/sgx/enclave with O_RDWR.

I *think* /dev/sgx/enclave could be opened O_RDONLY if ENCLAVE_ADD stuffed
the EPC VMA permissions, assuming the use case doesn't require changing
permissions after the enclave has been created.

The other reason userspace would need to open /dev/sgx/enclave O_RDWR
would be to debug an enclave, e.g. pwrite() works on the enclave fd due
to SGX restrictions on modifying EPC memory from outside the enclave.
But that's an obvious case where FILE__WRITE should be required.

What's the advantage of allocating an anon inode in the above?  At present
anon inodes are exempted from inode-based checking, thereby losing the
ability to perform SELinux ioctl whitelisting, unlike the file-backed
/dev/sgx/enclave inode.

Purely to trigger the EXECMEM check on any PROT_EXEC mapping.  However,
the motiviation for that was due to my bad assumption that FILE__WRITE
and FILE__EXECUTE are global and not per process.  If we can do as you
suggest and allow creation of enclaves with O_RDONLY, then keeping a
file-backed inode is definitely better as it means most processes only
need FILE__READ and FILE__* in general has actual meaning.

Thanks a bunch for your help!

On Thu, May 16, 2019 at 05:24:33PM +1000, James Morris wrote:

Good morning, I hope everyone had a pleasant weekend.

James, I believe the last time our paths crossed was at the Linux
Security Summit in Seattle, I trust you have been well since then.

On Wed, 15 May 2019, Andy Lutomirski wrote:

quoted

On Wed, May 15, 2019 at 3:46 PM James Morris [off-list ref] wrote:

quoted

You could try user.sigstruct, which does not require any privs.

I don't think I understand your proposal.  What file would this
attribute be on?  What would consume it?

It would be on the enclave file, so you keep the sigstruct bound to
it, rather than needing a separate file to manage.  It would
simplify any LSM policy check.

It would be consumed by (I guess) the SGX_INIT_THE_ENCLAVE ioctl in your 
example, instead of having a 2nd fd.

I've watched this discussion regarding LSM, sigstructs and file
descriptors with some fascination, since all of this infrastructure
already exists and should be well understood by anyone who has been
active in SGX runtime development.  There would thus seem to be a
disconnect between SGX driver developers and the consumers of the
services of the driver.

The existing enclave format, codified by the silo within Intel that is
responsible for the existing SDK/PSW, implements a notes section
stored inside a standard ELF shared library image.  The notes section
contains a significant amount of metadata that is used to direct the
instantiation of what will be the initialized enclave image.  Said
metadata includes a copy of the sigstruct that was generated when the
enclave was signed, which is the event that triggers metadata
generation.

All of this means that any enclave that gets loaded effectively
triggers both LSM and IMA checks.

James, if you remember, the paper that we presented in Seattle
described the initial implementation of an extension to the Linux IMA
infrastructure that tracks whether or not processes can be 'trusted'.
That work has gone on to include running the trust modeling and
disciplining engine inside of a namespace specific SGX enclave.  We
would be happy to make available execution trajectory logs that
clearly document IMA and LSM checks being conducted on enclaves.

There is a strong probability that we will be maintaining and
supporting a modified version of whatever driver that goes upstream.
In support of this we are putting together a white paper discussing
security architecture concerns inherent in an SGX driver.  With the
intent of avoiding LKML verbosity we will post a URL to the paper when
it is available if there is interest.

The issue of EDMM has already come up, suffice it to say that EDMM
makes LSM inspection of enclave content, while desirable, largely
irrelevant from a security perspective.

James Morris

Best wishes for a productive week.

Dr. Greg

As always,
Dr. G.W. Wettstein, Ph.D.   Enjellic Systems Development, LLC.
4206 N. 19th Ave.           Specializing in information infra-structure
Fargo, ND  58102            development.
PH: 701-281-1686            EMAIL: greg@enjellic.com
------------------------------------------------------------------------------
"If you plugged up your nose and mouth right before you sneezed, would
 the sneeze go out your ears or would your head explode?  Either way I'm
 afraid to try."
                                -- Nick Kean

On Thu, May 16, 2019 at 03:45:50PM -0700, Sean Christopherson wrote:

On Thu, May 16, 2019 at 02:02:58PM -0700, Andy Lutomirski wrote:

quoted

quoted

On May 15, 2019, at 10:16 PM, Jarkko Sakkinen [off-list ref] wrote:
There is a problem here though. Usually the enclave itself is just a
loader that then loads the application from outside source and creates
the executable pages from the content.

A great example of this is Graphene that bootstraps unmodified Linux
applications to an enclave:

https://github.com/oscarlab/graphene

ISTM you should need EXECMEM or similar to run Graphene, then.

Agreed, Graphene is effectively running arbitrary enclave code.  I'm
guessing there is nothing that prevents extending/reworking Graphene to
allow generating the enclave ahead of time so as to avoid populating the
guts of the enclave at runtime, i.e. it's likely possible to run an
unmodified application in an enclave without EXECMEM if that's something
Graphene or its users really care about.

I'd guess that also people adding SGX support to containers want
somewhat similar framework to work on so that you can just wrap a
container with an enclave.

/Jarkko

On Thu, May 16, 2019 at 02:02:58PM -0700, Andy Lutomirski wrote:

That certainly *could* be done, and I guess the decision could be left
to the LSMs, but I'm not convinced this adds value.  What security use
case does this cover that isn't already covered by requiring EXECUTE
(e.g. lib_t) on the enclave file and some new SIGSTRUCT right on the
.sigstruct?

I guess you are right as SIGSTRUCT completely shields the memory layout
and contents of an enclave.

/Jarkko

On Thu, May 16, 2019 at 05:03:31PM -0700, Sean Christopherson wrote:

The SGX ioctl() would need to take mmap_sem for write, but we can mitigate
that issue by changing the ioctl() to take a range of memory instead of a
single page.  That'd also provide "EADD batching" that folks have
requested.

This should be easy enough to add as the EADD operations are already
batched internally to a worker thread.

/Jarkko

On Thu, May 16, 2019 at 05:26:15PM -0700, Andy Lutomirski wrote:

Is userspace actually requred to mmap() the enclave prior to EADDing things?

Nope, not since v20. Here is what I wrote about API to the kernel
documentation:

"The enclave life-cycle starts by opening `/dev/sgx/enclave`. After this
there is already a data structure inside kernel tracking the enclave
that is initially uncreated. After this a set of ioctl's can be used to
create, populate and initialize the enclave.

You can close (if you want) the fd after you've mmap()'d. As long as the
file is open the enclave stays alive so you might want to do that after
you don't need it anymore. Even munmap() won't destruct the enclave if
the file is open.  Neither will closing the fd as long as you have
mmap() done over the fd (even if it does not across the range defined in
SECS)."

Enclave can be created and initialized without doing a single mmap()
call.

/Jarkko

On Fri, May 17, 2019 at 08:41:28AM -0700, Sean Christopherson wrote:

It was a requirement prior to the API rework in v20, i.e. unless someone
was really quick on the draw after the v20 update all existing userspace
implementations mmap() the enclave before ECREATE.   Requiring a valid
enclave VMA for EADD shoudn't be too onerous.

Still underlining: it is not required.

/Jarkko

On Mon, May 20, 2019 at 02:41:05PM +0300, Jarkko Sakkinen wrote:

On Thu, May 16, 2019 at 05:26:15PM -0700, Andy Lutomirski wrote:

quoted

Is userspace actually requred to mmap() the enclave prior to EADDing things?

Nope, not since v20. Here is what I wrote about API to the kernel
documentation:

"The enclave life-cycle starts by opening `/dev/sgx/enclave`. After this
there is already a data structure inside kernel tracking the enclave
that is initially uncreated. After this a set of ioctl's can be used to
create, populate and initialize the enclave.

You can close (if you want) the fd after you've mmap()'d. As long as the
file is open the enclave stays alive so you might want to do that after
you don't need it anymore. Even munmap() won't destruct the enclave if
the file is open.  Neither will closing the fd as long as you have
mmap() done over the fd (even if it does not across the range defined in
SECS)."

Enclave can be created and initialized without doing a single mmap()
call.

We could even disallow mmap() before EINIT done. The way enclave
management internally works right now is quite robust and completely
detached from requiring process address space for anything.

/Jarkko

On 2019-05-21 08:19, Jarkko Sakkinen wrote:

We could even disallow mmap() before EINIT done.

This would be extremely annoying in software because now you have to save the all the page permissions somewhere between EADD and mprotect.

--
Jethro Beekman | Fortanix

On Tue, May 21, 2019 at 06:19:37PM +0300, Jarkko Sakkinen wrote:

On Mon, May 20, 2019 at 02:41:05PM +0300, Jarkko Sakkinen wrote:

quoted

On Thu, May 16, 2019 at 05:26:15PM -0700, Andy Lutomirski wrote:

quoted

Is userspace actually requred to mmap() the enclave prior to EADDing things?

Nope, not since v20. Here is what I wrote about API to the kernel
documentation:

"The enclave life-cycle starts by opening `/dev/sgx/enclave`. After this
there is already a data structure inside kernel tracking the enclave
that is initially uncreated. After this a set of ioctl's can be used to
create, populate and initialize the enclave.

You can close (if you want) the fd after you've mmap()'d. As long as the
file is open the enclave stays alive so you might want to do that after
you don't need it anymore. Even munmap() won't destruct the enclave if
the file is open.  Neither will closing the fd as long as you have
mmap() done over the fd (even if it does not across the range defined in
SECS)."

Enclave can be created and initialized without doing a single mmap()
call.

We could even disallow mmap() before EINIT done. The way enclave
management internally works right now is quite robust and completely
detached from requiring process address space for anything.

Except that mmap() is more or less required to guarantee that ELRANGE
established by ECREATE is available.  And we want to disallow mmap() as
soon as the first EADD is done so that userspace can't remap the enclave's
VMAs via munmap()->mmap() and gain execute permissions to pages that were
EADD'd as NX.

Actually, conceptually it's probably more intuitive to disallow mmap() at
ECREATE, i.e. the act of creating an enclave pins the associated virtual
address range until the enclave is destroyed.

On Tue, May 21, 2019 at 03:24:18PM +0000, Jethro Beekman wrote:

On 2019-05-21 08:19, Jarkko Sakkinen wrote:

quoted

We could even disallow mmap() before EINIT done.

This would be extremely annoying in software because now you have to save
the all the page permissions somewhere between EADD and mprotect.

Actually you don't have to use mprotect anymore that much.

You can just do multiple mmap's even with v20 after EINIT, one
for each region (albeit it does not enforce above).

/Jarkko

On Tue, May 21, 2019 at 08:51:40AM -0700, Sean Christopherson wrote:

Except that mmap() is more or less required to guarantee that ELRANGE
established by ECREATE is available.  And we want to disallow mmap() as
soon as the first EADD is done so that userspace can't remap the enclave's
VMAs via munmap()->mmap() and gain execute permissions to pages that were
EADD'd as NX.

We don't want to guarantee such thing and it is not guaranteed. It does
not fit at all to the multi process work done. Enclaves are detached
from any particular process addresse spaces. It is responsibility of
process to open windows to them.

That would be completely against work that we've done lately.

Actually, conceptually it's probably more intuitive to disallow mmap() at
ECREATE, i.e. the act of creating an enclave pins the associated virtual
address range until the enclave is destroyed.

/Jarkko

On Wed, May 22, 2019 at 04:20:22PM +0300, Jarkko Sakkinen wrote:

On Tue, May 21, 2019 at 08:51:40AM -0700, Sean Christopherson wrote:

quoted

Except that mmap() is more or less required to guarantee that ELRANGE
established by ECREATE is available.  And we want to disallow mmap() as
soon as the first EADD is done so that userspace can't remap the enclave's
VMAs via munmap()->mmap() and gain execute permissions to pages that were
EADD'd as NX.

We don't want to guarantee such thing and it is not guaranteed. It does
not fit at all to the multi process work done. Enclaves are detached
from any particular process addresse spaces. It is responsibility of
process to open windows to them.

That would be completely against work that we've done lately.

Example use case: you have a process that just constructs an enclave
and sends it to another process or processes for use. The constructor
process could have basically anything on that range. This was the key
goal of the fd based enclave work.

/Jarkko

On 5/22/19 9:22 AM, Jarkko Sakkinen wrote:

On Wed, May 22, 2019 at 04:20:22PM +0300, Jarkko Sakkinen wrote:

quoted

On Tue, May 21, 2019 at 08:51:40AM -0700, Sean Christopherson wrote:

quoted

Except that mmap() is more or less required to guarantee that ELRANGE
established by ECREATE is available.  And we want to disallow mmap() as
soon as the first EADD is done so that userspace can't remap the enclave's
VMAs via munmap()->mmap() and gain execute permissions to pages that were
EADD'd as NX.

We don't want to guarantee such thing and it is not guaranteed. It does
not fit at all to the multi process work done. Enclaves are detached
from any particular process addresse spaces. It is responsibility of
process to open windows to them.

That would be completely against work that we've done lately.

Example use case: you have a process that just constructs an enclave
and sends it to another process or processes for use. The constructor
process could have basically anything on that range. This was the key
goal of the fd based enclave work.

What exactly happens in the constructor versus the recipient processes? Which process performs each of the necessary open(), mmap(), and ioctl() calls for setting up the enclave?  Can you provide a high level overview of the sequence of userspace calls by the constructor and by the recipient similar to what Sean showed earlier for just a single process?

On Wed, May 22, 2019 at 09:56:30AM -0400, Stephen Smalley wrote:

On 5/22/19 9:22 AM, Jarkko Sakkinen wrote:

quoted

On Wed, May 22, 2019 at 04:20:22PM +0300, Jarkko Sakkinen wrote:

quoted

On Tue, May 21, 2019 at 08:51:40AM -0700, Sean Christopherson wrote:

quoted

Except that mmap() is more or less required to guarantee that ELRANGE
established by ECREATE is available.  And we want to disallow mmap() as
soon as the first EADD is done so that userspace can't remap the enclave's
VMAs via munmap()->mmap() and gain execute permissions to pages that were
EADD'd as NX.

We don't want to guarantee such thing and it is not guaranteed. It does
not fit at all to the multi process work done. Enclaves are detached
from any particular process addresse spaces. It is responsibility of
process to open windows to them.

That would be completely against work that we've done lately.

Example use case: you have a process that just constructs an enclave
and sends it to another process or processes for use. The constructor
process could have basically anything on that range. This was the key
goal of the fd based enclave work.

What exactly happens in the constructor versus the recipient processes?
Which process performs each of the necessary open(), mmap(), and ioctl()
calls for setting up the enclave?  Can you provide a high level overview of
the sequence of userspace calls by the constructor and by the recipient
similar to what Sean showed earlier for just a single process?

Hmm, what we had talked about was allowing the SGX ioctls to work without
an associated VMA, with the end goal of letting userspace restrict access
to /dev/sgx/enclave.   Very roughly...

Enclave Owner:

  connect(builder, ...);
  send(builder, "/home/sean/path/to/my/enclave");

  recv(builder, &enclave_fd);

  for_each_chunk {
          mmap(enclave_addr + offset, size, ..., MAP_SHARED, enclave_fd, 0);
  }
  

Enclave Builder:

  recv(sock, &enclave_path);

  source_fd = open(enclave_path, O_RDONLY);
  for_each_chunk {
          <hand waving - mmap()/mprotect() the enclave file into regular memory>
  }

  enclave_fd = open("/dev/sgx/enclave", O_RDWR);

  ioctl(enclave_fd, ENCLAVE_CREATE, ...);
  for_each_chunk {
      struct sgx_enclave_add ioctlargs = {
          .offset = chunk.offset,
          .source = chunk.addr,
          .size   = chunk.size,
          .type   = chunk.type, /* SGX specific metadata */
      }
      ioctl(fd, ENCLAVE_ADD, &ioctlargs); /* modifies enclave's VMAs */
  }
  ioctl(enclave_fd, ENCLAVE_INIT, ...);

  write(sock, enclave_fd);


But the above flow is flawed because there'a catch-22: ENCLAVE_ECREATE
takes the virtual address of the enclave, but in the above flow that's
not established until "mmap(..., enclave_fd)".  And because an enclave's
virtual range needs to be naturally aligned (hardware requirements), the
enclave owner would need to do something like:

  source_fd = open("/home/sean/path/to/my/enclave", O_RDONLY);
  size = <parse size from source_fd>
  
  enclave_range = mmap(NULL, size*2, PROT_READ, ???, NULL, 0);
  enclave_addr = (enclave_range + (size - 1)) & ~(size - 1);

  connect(builder, ...);
  send(builder, {"/home/sean/path/to/my/enclave", enclave_addr});

  recv(builder, &enclave_fd);

  munmap(enclave_range);

  for_each_chunk {
      addr = mmap(enclave_addr + c.offset, c.size, ..., MAP_SHARED, enclave_fd, 0);
      if (addr != enclave_addr + c.offset)
           exit(1);
  } 

And that straight up doesn't work with the v20 driver because mmap() with
the enclave_fd will run through sgx_get_unmapped_area(), which also does
the natural alignment adjustments (the idea being that mmap() is mapping
the entire enclave).  E.g. mmap() will map the wrong address if the offset
of a chunk is less than its size due to the driver adjusting the address.

Eliminating sgx_get_unmapped_area() means userspace is once again on the
hook for naturally aligning the enclave, which is less than desirable.

Looking back at the original API discussions around a builder process[1],
we never fleshed out the end-to-end flow.  While having a builder process
*sounds* reasonable, in practice it adds a lot of complexity without
providing much in the way of added security.  E.g. in addition to the
above mmap() issues, since the order of EADDs affects the enclave
measurement, the enclave owner would need to communicate the exact steps
to build the enclave, or the builder would need a priori knowledge of the
enclave format.

Userspace can still restrict access to /dev/sgx/enclave, e.g. by having a
daemon that requires additional credentials to obtain a new enclave_fd.
So AFAICT, the only benefit to having a dedicated builder is that it can
do its own whitelisting of enclaves, but since we're trending towards
supporting whitelisting enclaves in the kernel, e.g. via sigstruct,
whitelisting in userspace purely in userspace also provides marginal value.

TL;DR: Requiring VMA backing to build an enclave seems reasonable and sane.

[1] https://lkml.kernel.org/r/CALCETrX+KisMCbptrnPSO79-YF4E3nR1XHt+a7hCs1GXsxAbtw@mail.gmail.com

On Wed, May 22, 2019 at 8:38 AM Sean Christopherson
[off-list ref] wrote:

On Wed, May 22, 2019 at 09:56:30AM -0400, Stephen Smalley wrote:

quoted

On 5/22/19 9:22 AM, Jarkko Sakkinen wrote:

quoted

On Wed, May 22, 2019 at 04:20:22PM +0300, Jarkko Sakkinen wrote:

quoted

On Tue, May 21, 2019 at 08:51:40AM -0700, Sean Christopherson wrote:

quoted

Except that mmap() is more or less required to guarantee that ELRANGE
established by ECREATE is available.  And we want to disallow mmap() as
soon as the first EADD is done so that userspace can't remap the enclave's
VMAs via munmap()->mmap() and gain execute permissions to pages that were
EADD'd as NX.

We don't want to guarantee such thing and it is not guaranteed. It does
not fit at all to the multi process work done. Enclaves are detached
from any particular process addresse spaces. It is responsibility of
process to open windows to them.

That would be completely against work that we've done lately.

Example use case: you have a process that just constructs an enclave
and sends it to another process or processes for use. The constructor
process could have basically anything on that range. This was the key
goal of the fd based enclave work.

What exactly happens in the constructor versus the recipient processes?
Which process performs each of the necessary open(), mmap(), and ioctl()
calls for setting up the enclave?  Can you provide a high level overview of
the sequence of userspace calls by the constructor and by the recipient
similar to what Sean showed earlier for just a single process?

Hmm, what we had talked about was allowing the SGX ioctls to work without
an associated VMA, with the end goal of letting userspace restrict access
to /dev/sgx/enclave.   Very roughly...

Enclave Owner:

  connect(builder, ...);
  send(builder, "/home/sean/path/to/my/enclave");

  recv(builder, &enclave_fd);

  for_each_chunk {
          mmap(enclave_addr + offset, size, ..., MAP_SHARED, enclave_fd, 0);
  }


Enclave Builder:

  recv(sock, &enclave_path);

  source_fd = open(enclave_path, O_RDONLY);
  for_each_chunk {
          <hand waving - mmap()/mprotect() the enclave file into regular memory>
  }

  enclave_fd = open("/dev/sgx/enclave", O_RDWR);

  ioctl(enclave_fd, ENCLAVE_CREATE, ...);
  for_each_chunk {
      struct sgx_enclave_add ioctlargs = {
          .offset = chunk.offset,
          .source = chunk.addr,
          .size   = chunk.size,
          .type   = chunk.type, /* SGX specific metadata */
      }
      ioctl(fd, ENCLAVE_ADD, &ioctlargs); /* modifies enclave's VMAs */
  }
  ioctl(enclave_fd, ENCLAVE_INIT, ...);

  write(sock, enclave_fd);


But the above flow is flawed because there'a catch-22: ENCLAVE_ECREATE
takes the virtual address of the enclave, but in the above flow that's
not established until "mmap(..., enclave_fd)".  And because an enclave's
virtual range needs to be naturally aligned (hardware requirements), the
enclave owner would need to do something like:

  source_fd = open("/home/sean/path/to/my/enclave", O_RDONLY);
  size = <parse size from source_fd>

  enclave_range = mmap(NULL, size*2, PROT_READ, ???, NULL, 0);
  enclave_addr = (enclave_range + (size - 1)) & ~(size - 1);

  connect(builder, ...);
  send(builder, {"/home/sean/path/to/my/enclave", enclave_addr});

  recv(builder, &enclave_fd);

  munmap(enclave_range);

  for_each_chunk {
      addr = mmap(enclave_addr + c.offset, c.size, ..., MAP_SHARED, enclave_fd, 0);
      if (addr != enclave_addr + c.offset)
           exit(1);
  }

And that straight up doesn't work with the v20 driver because mmap() with
the enclave_fd will run through sgx_get_unmapped_area(), which also does
the natural alignment adjustments (the idea being that mmap() is mapping
the entire enclave).  E.g. mmap() will map the wrong address if the offset
of a chunk is less than its size due to the driver adjusting the address.

That presumably needs to change.

Are we entirely missing an API to allocate a naturally aligned VA
range?  That's kind of annoying.

Eliminating sgx_get_unmapped_area() means userspace is once again on the
hook for naturally aligning the enclave, which is less than desirable.

Looking back at the original API discussions around a builder process[1],
we never fleshed out the end-to-end flow.  While having a builder process
*sounds* reasonable, in practice it adds a lot of complexity without
providing much in the way of added security.  E.g. in addition to the
above mmap() issues, since the order of EADDs affects the enclave
measurement, the enclave owner would need to communicate the exact steps
to build the enclave, or the builder would need a priori knowledge of the
enclave format.

Userspace can still restrict access to /dev/sgx/enclave, e.g. by having a
daemon that requires additional credentials to obtain a new enclave_fd.
So AFAICT, the only benefit to having a dedicated builder is that it can
do its own whitelisting of enclaves, but since we're trending towards
supporting whitelisting enclaves in the kernel, e.g. via sigstruct,
whitelisting in userspace purely in userspace also provides marginal value.

TL;DR: Requiring VMA backing to build an enclave seems reasonable and sane.

This isn't necessarily a problem, but we pretty much have to use
mprotect() then.

Maybe the semantics could just be that mmap() on the SGX device gives
natural alignment, but that there is no actual constraint enforced by
the driver as to whether mmap() happens before or after ECREATE.
After all, it's *ugly* for user code to reserve its address range with
an awkward giant mmap(), there's nothing fundamentally wrong with it.

As far as I know from this whole discussion, we still haven't come up
with any credible way to avoid tracking, per enclave page, whether
that page came from unmodified PROT_EXEC memory.

On Wed, May 22, 2019 at 03:42:45PM -0700, Andy Lutomirski wrote:

On Wed, May 22, 2019 at 8:38 AM Sean Christopherson
[off-list ref] wrote:

quoted

And that straight up doesn't work with the v20 driver because mmap() with
the enclave_fd will run through sgx_get_unmapped_area(), which also does
the natural alignment adjustments (the idea being that mmap() is mapping
the entire enclave).  E.g. mmap() will map the wrong address if the offset
of a chunk is less than its size due to the driver adjusting the address.

That presumably needs to change.

If we want to allow mmap() on a subset of the enclave, yes.  I assume it's
a simple matter of respecting MAP_FIXED.

Are we entirely missing an API to allocate a naturally aligned VA
range?  That's kind of annoying.

Yes?

quoted

Eliminating sgx_get_unmapped_area() means userspace is once again on the
hook for naturally aligning the enclave, which is less than desirable.

Looking back at the original API discussions around a builder process[1],
we never fleshed out the end-to-end flow.  While having a builder process
*sounds* reasonable, in practice it adds a lot of complexity without
providing much in the way of added security.  E.g. in addition to the
above mmap() issues, since the order of EADDs affects the enclave
measurement, the enclave owner would need to communicate the exact steps
to build the enclave, or the builder would need a priori knowledge of the
enclave format.

Userspace can still restrict access to /dev/sgx/enclave, e.g. by having a
daemon that requires additional credentials to obtain a new enclave_fd.
So AFAICT, the only benefit to having a dedicated builder is that it can
do its own whitelisting of enclaves, but since we're trending towards
supporting whitelisting enclaves in the kernel, e.g. via sigstruct,
whitelisting in userspace purely in userspace also provides marginal value.

TL;DR: Requiring VMA backing to build an enclave seems reasonable and sane.

This isn't necessarily a problem, but we pretty much have to use
mprotect() then.

You lost me there.  Who needs to mprotect() what?

Maybe the semantics could just be that mmap() on the SGX device gives
natural alignment, but that there is no actual constraint enforced by
the driver as to whether mmap() happens before or after ECREATE.
After all, it's *ugly* for user code to reserve its address range with
an awkward giant mmap(), there's nothing fundamentally wrong with it.

As far as I know from this whole discussion, we still haven't come up
with any credible way to avoid tracking, per enclave page, whether
that page came from unmodified PROT_EXEC memory.

Disallowing mmap() after ECREATE is credible, but apparently not
palatable. :-)

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.

Tracking permissions per enclave page isn't difficult, it's the new SGX
specific LSM hooks and mprotect() interactions that I want to avoid.

Jumping back to mmap(), AIUI the fundamental issue is that we want to
allow building/running an enclave without FILE__WRITE and FILE__EXECUTE,
otherwise FILE__WRITE and FILE__EXECUTE become meaningless.  Assuming I'm
not off in the weeds, that means we really just need to special case
mmap() on enclaves so it can map enclave memory using the verified page
permissions so as not to run afoul of LSM checks.  All other behaviors,
e.g. mprotect(), can reuse the existing LSM checks for shared mappings.

So, what if we snapshot the permissions for each enclave page at EADD,
and then special case mmap() to propagate flags from the snapshot to the
VMA?  More or less the same idea as doing mprotect_fixup() using the
source VMA during EADD.  We could define the EADD semantics to match
this as well, e.g. only propagate the flags from the source VMA to the
enclave VMA if the EADD range is fully mapped with PROT_NONE.  This would
allow the enclave builder concept, albeit with funky semantics, and
wouldn't require new LSM hooks.

E.g. something like this:

static inline void sgx_mmap_update_prot_flags(struct vm_area_struct *vma,
					      struct sgx_encl *encl)
{
	struct radix_tree_iter iter;
	struct sgx_encl_page *entry;
	unsigned long addr;
	vm_flags_t flags;
	void **slot;

	/*
	 * SGX special: if userspace is requesting PROT_NONE and pages have
	 * been added to the enclave, then propagate the flags snapshot from
	 * the enclave to the VMA.  Do this if and only if all overlapped
	 * pages are defined and have identical permissions.  Stuffing the
	 * VMA on PROT_NONE allows userspace to map EPC pages without being
	 * incorrectly rejected by LSMs due to insufficient permissions (the
	 * snapshottted flags have alaredy been vetted).
	 */
	if (vma->vm_flags & (VM_READ|VM_WRITE|VM_EXEC))
		return;

	flags = 0;

	for (addr = vma->vm_start; addr < vma->vm_end; addr += PAGE_SIZE) {
		entry = radix_tree_lookup(&encl->page_tree, addr >> PAGE_SHIFT);

		if (!entry && flags)
			return;
		if (!flags && entry) {
			if (addr == vma->vm_start) {
				flags = entry->vm_flags;
				continue;
			}
			return;
		}
		if (entry && flags && entry->vm_flags != flags)
			return;

	}
	vma->vm_flags |= flags;
}

On Wed, May 22, 2019 at 03:42:45PM -0700, Andy Lutomirski wrote:

As far as I know from this whole discussion, we still haven't come up
with any credible way to avoid tracking, per enclave page, whether
that page came from unmodified PROT_EXEC memory.

So is this in the context that the enclave is read from another VMA
and not through a file descriptor? Is that locked in?

/Jarkko

On Thu, May 23, 2019 at 11:10:48AM +0300, Jarkko Sakkinen wrote:

On Wed, May 22, 2019 at 03:42:45PM -0700, Andy Lutomirski wrote:

quoted

As far as I know from this whole discussion, we still haven't come up
with any credible way to avoid tracking, per enclave page, whether
that page came from unmodified PROT_EXEC memory.

So is this in the context that the enclave is read from another VMA
and not through a file descriptor? Is that locked in?

No need to answer. Got in page from Sean's response.

/Jarkko

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.

We can still support fork() if we take a step back from v20 and require
the mmap(). Given the recent comments, I'd guess that is the best
compromise i.e. multiple processes can still share an enclave within
the limitations of ancestor hierarchy. Is this the constraint we agree
now upon? Some emails are a bit contradicting in this sense.

Tracking permissions per enclave page isn't difficult, it's the new SGX
specific LSM hooks and mprotect() interactions that I want to avoid.

Jumping back to mmap(), AIUI the fundamental issue is that we want to
allow building/running an enclave without FILE__WRITE and FILE__EXECUTE,
otherwise FILE__WRITE and FILE__EXECUTE become meaningless.  Assuming I'm
not off in the weeds, that means we really just need to special case
mmap() on enclaves so it can map enclave memory using the verified page
permissions so as not to run afoul of LSM checks.  All other behaviors,
e.g. mprotect(), can reuse the existing LSM checks for shared mappings.

So, what if we snapshot the permissions for each enclave page at EADD,
and then special case mmap() to propagate flags from the snapshot to the
VMA?  More or less the same idea as doing mprotect_fixup() using the
source VMA during EADD.  We could define the EADD semantics to match
this as well, e.g. only propagate the flags from the source VMA to the
enclave VMA if the EADD range is fully mapped with PROT_NONE.  This would
allow the enclave builder concept, albeit with funky semantics, and
wouldn't require new LSM hooks.

Dropped off here completely. What if the mmap() is done before any of
the EADD operations?

E.g. something like this:

static inline void sgx_mmap_update_prot_flags(struct vm_area_struct *vma,
					      struct sgx_encl *encl)
{
	struct radix_tree_iter iter;
	struct sgx_encl_page *entry;
	unsigned long addr;
	vm_flags_t flags;
	void **slot;

	/*
	 * SGX special: if userspace is requesting PROT_NONE and pages have
	 * been added to the enclave, then propagate the flags snapshot from
	 * the enclave to the VMA.  Do this if and only if all overlapped
	 * pages are defined and have identical permissions.  Stuffing the
	 * VMA on PROT_NONE allows userspace to map EPC pages without being
	 * incorrectly rejected by LSMs due to insufficient permissions (the
	 * snapshottted flags have alaredy been vetted).
	 */
	if (vma->vm_flags & (VM_READ|VM_WRITE|VM_EXEC))
		return;

	flags = 0;

	for (addr = vma->vm_start; addr < vma->vm_end; addr += PAGE_SIZE) {
		entry = radix_tree_lookup(&encl->page_tree, addr >> PAGE_SHIFT);

		if (!entry && flags)
			return;
		if (!flags && entry) {
			if (addr == vma->vm_start) {
				flags = entry->vm_flags;
				continue;
			}
			return;
		}
		if (entry && flags && entry->vm_flags != flags)
			return;

	}
	vma->vm_flags |= flags;
}

This looks flakky and error prone. You'd better have some "shadow VMAs"
and check that you have such matching size of the VMA you try to mmap()
and check flags from that.

Who would call this function anyhow and when?

Would be better to first agree on constraints. I have zero idea within
which kind of enviroment this snippet would live e.g.

- mmap() (before, after?)
- multi process constraint (only fork or full on versatility)

/Jarkko

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:

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.

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.

We can still support fork() if we take a step back from v20 and require
the mmap(). Given the recent comments, I'd guess that is the best
compromise i.e. multiple processes can still share an enclave within
the limitations of ancestor hierarchy. Is this the constraint we agree
now upon? Some emails are a bit contradicting in this sense.

quoted

Tracking permissions per enclave page isn't difficult, it's the new SGX
specific LSM hooks and mprotect() interactions that I want to avoid.

Jumping back to mmap(), AIUI the fundamental issue is that we want to
allow building/running an enclave without FILE__WRITE and FILE__EXECUTE,
otherwise FILE__WRITE and FILE__EXECUTE become meaningless.  Assuming I'm
not off in the weeds, that means we really just need to special case
mmap() on enclaves so it can map enclave memory using the verified page
permissions so as not to run afoul of LSM checks.  All other behaviors,
e.g. mprotect(), can reuse the existing LSM checks for shared mappings.

So, what if we snapshot the permissions for each enclave page at EADD,
and then special case mmap() to propagate flags from the snapshot to the
VMA?  More or less the same idea as doing mprotect_fixup() using the
source VMA during EADD.  We could define the EADD semantics to match
this as well, e.g. only propagate the flags from the source VMA to the
enclave VMA if the EADD range is fully mapped with PROT_NONE.  This would
allow the enclave builder concept, albeit with funky semantics, and
wouldn't require new LSM hooks.

Dropped off here completely. What if the mmap() is done before any of
the EADD operations?

Three options I can think of, in descending order of magic required:

  1. Do nothing.  Userspace would essentially be required to mmap() the
     enclave after EINIT, which is ugly but not breaking since userspace
     could mmap() the enclave with a placeholder VMA prior to building
     the enclave, and then a series of mmap() to establish its "real"
     mapping.

  2. Propagate the permissions from EADD to the VMAs of the current mm
     if the entire EADD range is mapped and the mapping is PROT_NONE.

  3. Propagate the permissions from EADD to the VMAs of all mm structs
     that have mapped some piece of the enclave, following the matching
     rules from #2.

quoted

E.g. something like this:

static inline void sgx_mmap_update_prot_flags(struct vm_area_struct *vma,
					      struct sgx_encl *encl)
{
	struct radix_tree_iter iter;
	struct sgx_encl_page *entry;
	unsigned long addr;
	vm_flags_t flags;
	void **slot;

	/*
	 * SGX special: if userspace is requesting PROT_NONE and pages have
	 * been added to the enclave, then propagate the flags snapshot from
	 * the enclave to the VMA.  Do this if and only if all overlapped
	 * pages are defined and have identical permissions.  Stuffing the
	 * VMA on PROT_NONE allows userspace to map EPC pages without being
	 * incorrectly rejected by LSMs due to insufficient permissions (the
	 * snapshottted flags have alaredy been vetted).
	 */
	if (vma->vm_flags & (VM_READ|VM_WRITE|VM_EXEC))
		return;

	flags = 0;

	for (addr = vma->vm_start; addr < vma->vm_end; addr += PAGE_SIZE) {
		entry = radix_tree_lookup(&encl->page_tree, addr >> PAGE_SHIFT);

		if (!entry && flags)
			return;
		if (!flags && entry) {
			if (addr == vma->vm_start) {
				flags = entry->vm_flags;
				continue;
			}
			return;
		}
		if (entry && flags && entry->vm_flags != flags)
			return;

	}
	vma->vm_flags |= flags;
}

This looks flakky and error prone. You'd better have some "shadow VMAs"
and check that you have such matching size of the VMA you try to mmap()
and check flags from that.

Who would call this function anyhow and when?

Would be better to first agree on constraints. I have zero idea within
which kind of enviroment this snippet would live e.g.

- mmap() (before, after?)
- multi process constraint (only fork or full on versatility)

This would be called from sgx_mmap(), i.e. mmap().  Sorry that wasn't at
all clear.  The idea is to inherit the protections from the enclave pages
if mmap() was passed PROT_NONE, but do so in a paranoid way.

I don't think multi-process contraints would be required.  This would
allow an individual process to inherit the pre-verified protections.
Other process(es) could map the enclave page with different protections,
but doing so would require the appropriate FILE__* permissions for the
other process(es).

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

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.

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
after ECREATE because the alignment isn't known before that.

Associated with the enclave are a bunch (up to 7) "enclave segment
inodes".  These are anon_inodes that are created automagically.  An
enclave segment is a group of pages, not necessary contiguous, with an
upper bound on the memory permissions.  Each enclave page belongs to a
segment.  When you do EADD, you tell the driver what segment you're
adding to. [0]  This means that EADD gets an extra argument that is a
permission mask for the page -- in addition to the initial SECINFO,
you also pass to EADD something to the effect of "I promise never to
map this with permissions greater than RX".

Then we just need some way to mmap a region from an enclave segment.
This could be done by having a way to get an fd for an enclave segment
or it could be done by having a new ioctl SGX_IOC_MAP_SEGMENT.  User
code would use this operation to replace, MAP_FIXED-style, ranges from
the big PROT_NONE mapping with the relevant pages from the enclave
segment.  The resulting vma would only have VM_MAYWRITE if the segment
is W, only have VM_MAYEXEC if the segment is X, and only have
VM_MAYREAD if the segment is R.  Depending on implementation details,
the VMAs might need to restrict mremap() to avoid mapping pages that
aren't part of the segment in question.

It's plausible that this whole thing works without the magic segment
inodes under the hood, but figuring that out would need a careful look
at how all the core mm bits and LSM bits work together.

To get all the LSM stuff to work, SELinux will need some way to
automatically assign an appropriate label to the segment inodes.  I
assume that such a mechanism already exists and gets used for things
like sockets, but I haven't actually confirmed this.

[0] There needs to be some vaguely intelligent semantics if you EADD
the *same* address more than once.  A simple solution would be to
disallow it if the segments don't match.

On Thu, May 23, 2019 at 07:17:52AM -0700, Sean Christopherson wrote:

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.

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.

Argh, as I was working through Andy's latest proposal I realized that I
was subconciously making FILE__READ imply FILE__EXECUTE.

The idea behind inheriting permissions from the source VMA is to exempt
"standard" enclaves from needing FILE__WRITE.  But if we don't add an
exemption for FILE__EXECUTE as well, then all enclaves need FILE__EXECUTE,
which means FILE__EXECUTE can't be used to identify the case where
userspace is mapping an inherited PROT_WRITE page as PROT_EXEC.  And if
the SGX magic exempts FILE__EXECUTE, then FILE__READ implies FILE__EXECUTE.

Yuck.

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

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.

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.

Associated with the enclave are a bunch (up to 7) "enclave segment

I assume 7 = R, W, X, RW, RX, WX and RWX?

inodes".  These are anon_inodes that are created automagically.  An
enclave segment is a group of pages, not necessary contiguous, with an
upper bound on the memory permissions.  Each enclave page belongs to a
segment.  When you do EADD, you tell the driver what segment you're
adding to. [0]  This means that EADD gets an extra argument that is a
permission mask for the page -- in addition to the initial SECINFO,
you also pass to EADD something to the effect of "I promise never to
map this with permissions greater than RX".

Then we just need some way to mmap a region from an enclave segment.
This could be done by having a way to get an fd for an enclave segment
or it could be done by having a new ioctl SGX_IOC_MAP_SEGMENT.  User
code would use this operation to replace, MAP_FIXED-style, ranges from
the big PROT_NONE mapping with the relevant pages from the enclave
segment.  The resulting vma would only have VM_MAYWRITE if the segment
is W, only have VM_MAYEXEC if the segment is X, and only have
VM_MAYREAD if the segment is R.  Depending on implementation details,
the VMAs might need to restrict mremap() to avoid mapping pages that
aren't part of the segment in question.

If my above assumptions regarding VM_MAY* and the "7 segments" are
correct, IIUC you're proposing that an LSM could have policies for each
of the anon inodes, e.g. grant/deny RWX vs. RW vs RX.  Am I in the
ballpark?

It's plausible that this whole thing works without the magic segment
inodes under the hood, but figuring that out would need a careful look
at how all the core mm bits and LSM bits work together.

To get all the LSM stuff to work, SELinux will need some way to
automatically assign an appropriate label to the segment inodes.  I
assume that such a mechanism already exists and gets used for things
like sockets, but I haven't actually confirmed this.

I (obviously) don't fully understand your proposal, but I don't think we
want to hook inodes, e.g. AppArmor doesn't implement inode_permission()
but does implement file_mprotect() and mmap_file(), which feel like the
natural hooks for this sort of thing.  I also think it's overkill, e.g.
AppArmor doesn't have a concept of EXECMOD, EXECMEM, EXECHEAP, etc.., so
I don't think we need to go beyond detecting W+X scenarios.

Starting with your original idea of tracking "safe to execute" and
Cedric's of propagating the permissions from the source VMA, but tweaked
with your new idea of clearing VM_MAY* and a custom MAP_FIXED/mprotect().

Add SGX_IOC_MPROTECT (or SGX_IOC_MAP_REGION?) that works as follows:

  1. Track VM_MAY{READ,WRITE,EXEC} flags for each enclave page.
  2. SGX_IOC_ADD_REGION, i.e. EADD, initializes the VM_MAY* flags for each
     enclave page based on the source VMA.
  3. sgx_mmap() only works with PROT_NONE, skips alignment stuff if
     MAP_FIXED, and clears VM_MAY{READ,WRITE,EXEC}.
  4. mprotect() on /dev/sgx/enclave doesn't work because the VMA doesn't
     have any VM_MAY{READ,WRITE,EXEC} capabilities.
  5. Deny mremap() post-ECREATE as the address and size of the enclave
     are fixed at ECREATE (in hardware).
  6. SGX_IOC_MPROTECT works like normal mprotect(), except the VM_MAY*
     flags are pulled from the enclave pages, and its call to
     security_file_mprotect() is VM_READ|VM_EXEC by default.  The LSM call
     sets VM_WRITE iff the enclave page has both VM_MAYWRITE and
     VM_MAYEXEC.  The idea here is to require READ and EXECUTE to run an
     enclave, and only require WRITE on /dev/sgx/enclave when the enclave
     can execute modified memory.

To support SGX2 down the road, which will want to convert a page to
executable on the fly, we could add:

  7. SGX_IOC_EXTEND_PERMISSIONS enables userspace to extend the VM_MAY*
     flags for an enclave page, e.g. to make a page executable.
     SGX_IOC_MPROTECT is still required to actually map the page.
     Notably, adding a RW page to the enclave, e.g. to grow its heap,
     doesn't require WRITE, whereas adding a RWX page, e.g. for dynamic
     loading, would require WRITE.  This can only extend!  E.g. userspace
     can't circumvent the WRITE requirement by clearing VM_MAYWRITE.

Note, FILE__WRITE on /dev/sgx/enclave is essentially equivalent to
FILE__EXECMOD.  Using FILE__WRITE in this way means there are no changes
to SELinux (triggering FILE__EXECMOD would be awkward), and AppArmor also
picks up extra protections for enclaves.

[0] There needs to be some vaguely intelligent semantics if you EADD
the *same* address more than once.  A simple solution would be to
disallow it if the segments don't match.

I don't see any reason to allow duplicate EADD as it serves no purpose,
e.g. doing so changes the enclave's measurement and that's it.

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

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.

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.

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

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:

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);

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.


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.

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

Hi Andy,

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.

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

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?

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

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

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.)
 *   + 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
 *   + <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);
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);
}

/* Step 2: Create enclave */
int enclave_fd = open("/dev/sgx/enclave", O_RDONLY /* or O_RDWR for SGX2 enclaves */);
void *enclave_base = mmap(NULL, <enclave size>, MAP_SHARED, PROT_READ, enclave_fd, 0); /* Only FILE__READ is required here */
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); 

/* 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);

-Cedric

On 5/23/19 11:38 AM, Andy Lutomirski wrote:

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.

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
after ECREATE because the alignment isn't known before that.

Associated with the enclave are a bunch (up to 7) "enclave segment
inodes".  These are anon_inodes that are created automagically.  An
enclave segment is a group of pages, not necessary contiguous, with an
upper bound on the memory permissions.  Each enclave page belongs to a
segment.  When you do EADD, you tell the driver what segment you're
adding to. [0]  This means that EADD gets an extra argument that is a
permission mask for the page -- in addition to the initial SECINFO,
you also pass to EADD something to the effect of "I promise never to
map this with permissions greater than RX".

Then we just need some way to mmap a region from an enclave segment.
This could be done by having a way to get an fd for an enclave segment
or it could be done by having a new ioctl SGX_IOC_MAP_SEGMENT.  User
code would use this operation to replace, MAP_FIXED-style, ranges from
the big PROT_NONE mapping with the relevant pages from the enclave
segment.  The resulting vma would only have VM_MAYWRITE if the segment
is W, only have VM_MAYEXEC if the segment is X, and only have
VM_MAYREAD if the segment is R.  Depending on implementation details,
the VMAs might need to restrict mremap() to avoid mapping pages that
aren't part of the segment in question.

It's plausible that this whole thing works without the magic segment
inodes under the hood, but figuring that out would need a careful look
at how all the core mm bits and LSM bits work together.

To get all the LSM stuff to work, SELinux will need some way to
automatically assign an appropriate label to the segment inodes.  I
assume that such a mechanism already exists and gets used for things
like sockets, but I haven't actually confirmed this.

I don't follow that.  socket inodes are not anon inodes, and anon inodes have no per-instance data by definition, and typically you're only dealing with a single anon inode for all files, and hence they were long ago marked S_PRIVATE and exempted from all LSM checking except for EXECMEM on mmap/mprotect PROT_EXEC.  We have no way to perform useful security checking on them currently.  socket inodes we can label from their creating process but even that's not going to support multiple labels for different sockets created by the same process unless the process explicitly used setsockcreatecon(3) aka /proc/self/attr/sockcreate

[0] There needs to be some vaguely intelligent semantics if you EADD
the *same* address more than once.  A simple solution would be to
disallow it if the segments don't match.

On 5/24/19 3:24 AM, Xing, Cedric 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.

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

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?

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

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

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

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

}

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

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

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

/* 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?

-Cedric

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.