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

[Xing, Cedric]

Hi Andy,

> From: Andy Lutomirski [mailto:luto@xxxxxxxxxx]
> 
> On Wed, May 15, 2019 at 3:46 PM James Morris <jmorris@xxxxxxxxx> wrote:
> >
> > On Wed, 15 May 2019, Andy Lutomirski wrote:
> >
> > > > 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