Message-ID: <CALCETrVufjQSJ1nq2sL4tmetFA_Kj+o9xW0YQ1Z2w6Oz+2Y0wA@mail.gmail.com>
Date:   Thu, 16 May 2019 18:21:19 -0700
From:   Andy Lutomirski <luto@...nel.org>
To:     "Xing, Cedric" <cedric.xing@...el.com>
Cc:     Andy Lutomirski <luto@...nel.org>,
        James Morris <jmorris@...ei.org>,
        "Christopherson, Sean J" <sean.j.christopherson@...el.com>,
        "Serge E. Hallyn" <serge@...lyn.com>,
        LSM List <linux-security-module@...r.kernel.org>,
        Paul Moore <paul@...l-moore.com>,
        Stephen Smalley <sds@...ho.nsa.gov>,
        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 Thu, May 16, 2019 at 6:06 PM Xing, Cedric <cedric.xing@...el.com> wrote:
>
> > From: Andy Lutomirski [mailto:luto@...nel.org]
> >
> > On Thu, May 16, 2019 at 3:23 PM Xing, Cedric <cedric.xing@...el.com>
> > wrote:
> > >
> > > Hi Andy,
> > >
> > > > > 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.
> > >
> > > 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.
>
> 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.

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.

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

Powered by blists - more mailing lists