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

[Andy Lutomirski]

> On May 17, 2019, at 1:09 PM, Stephen Smalley <sds@xxxxxxxxxxxxx> wrote:
> 
>> On 5/17/19 3:28 PM, Sean Christopherson wrote:
>>> On Fri, May 17, 2019 at 02:05:39PM -0400, Stephen Smalley wrote:
>>>> 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.
>>> 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.