Message-Id: <ED98AEC9-FFA3-4DA4-9B86-11D8AADC9151@amacapital.net>
Date:   Fri, 17 May 2019 13:14:00 -0700
From:   Andy Lutomirski <luto@...capital.net>
To:     Stephen Smalley <sds@...ho.nsa.gov>
Cc:     Sean Christopherson <sean.j.christopherson@...el.com>,
        "Xing, Cedric" <cedric.xing@...el.com>,
        Andy Lutomirski <luto@...nel.org>,
        James Morris <jmorris@...ei.org>,
        "Serge E. Hallyn" <serge@...lyn.com>,
        LSM List <linux-security-module@...r.kernel.org>,
        Paul Moore <paul@...l-moore.com>,
        Eric Paris <eparis@...isplace.org>,
        "selinux@...r.kernel.org" <selinux@...r.kernel.org>,
        Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>,
        Jethro Beekman <jethro@...tanix.com>,
        "Hansen, Dave" <dave.hansen@...el.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        "Dr. Greg" <greg@...ellic.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        LKML <linux-kernel@...r.kernel.org>, X86 ML <x86@...nel.org>,
        "linux-sgx@...r.kernel.org" <linux-sgx@...r.kernel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        "nhorman@...hat.com" <nhorman@...hat.com>,
        "npmccallum@...hat.com" <npmccallum@...hat.com>,
        "Ayoun, Serge" <serge.ayoun@...el.com>,
        "Katz-zamir, Shay" <shay.katz-zamir@...el.com>,
        "Huang, Haitao" <haitao.huang@...el.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        "Svahn, Kai" <kai.svahn@...el.com>, Borislav Petkov <bp@...en8.de>,
        Josh Triplett <josh@...htriplett.org>,
        "Huang, Kai" <kai.huang@...el.com>,
        David Rientjes <rientjes@...gle.com>
Subject: Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)


> On May 17, 2019, at 1:09 PM, Stephen Smalley <sds@...ho.nsa.gov> 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.

Powered by blists - more mailing lists