Message-ID: <960B34DE67B9E140824F1DCDEC400C0F654E11D8@ORSMSX116.amr.corp.intel.com>
Date:   Tue, 14 May 2019 21:58:24 +0000
From:   "Xing, Cedric" <cedric.xing@...el.com>
To:     Andy Lutomirski <luto@...nel.org>,
        Haitao Huang <haitao.huang@...ux.intel.com>
CC:     Jethro Beekman <jethro@...tanix.com>,
        "Hansen, Dave" <dave.hansen@...el.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        "Dr. Greg" <greg@...ellic.com>,
        Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.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>,
        "Christopherson, Sean J" <sean.j.christopherson@...el.com>,
        "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: [PATCH v20 00/28] Intel SGX1 support

Hi Everyone,

I think we are talking about 2 different kinds of criteria for determining the sanity of an enclave. 

The first kind determines an enclave's sanity by generally accepted good practices. For example, no executable pages shall ever be writable.

The second kind determines an enclave's sanity by "who stands behind it", such as whether the file containing SIGSTRUCT has the proper SELinux label/type, or whether the signing key is trusted.

I'd say those 2 kinds of criteria should be orthogonal because they don't get into each other's way and it could also be beneficial to enable both at the same time. For example, a user may want to allow launching enclaves signed by himself/herself only, however, as a human being he/she may make mistakes so would also like to ensure no RWX pages even for those enclaves explicitly authorized.

I think those 2 kinds of criteria could be abstracted as 2 new LSM hooks - security_sgx_add_pages() and security_sgx_initialize_enclave().

security_sgx_add_pages() is invoked by SGX driver to determine if a range of source pages are allowed to be EADD'ed with the requested EPCM attributes, and by default it returns 0 (allowed) iff the requested EPCM attributes are a subset of the permissions of the VMA covering that range of source pages. An (SGX-aware) LSM module/policy could employ different criteria, such as making sure the source pages are backed by an enclave file (using SELinux label, for example). 

security_sgx_initialize_enclave() is invoked by SGX driver to determine if a given SIGSTRUCT is valid (hence allowed to be EINIT'ed), and it always returns 0 (allowed) by default. An LSM implementation may enforce custom policies, such as whether the signing public key is trusted by the current user, or whether it was backed (mmap()'ed) by an authorized file (e.g. of an expected type in the case of SELinux, or located in a particular directory in the case of AppArmor).

With regard to SGX2/EDMM (Enclave Dynamic Memory Management) support, RW->RX transitions are inevitable to support certain usages such as dynamic loading/linking, meaning those usages may be blocked by existing policies. But from security perspective, I think they should be allowed by default (i.e. for non-SGX-aware LSM modules/policies) because such permission changes are inherent behaviors of the enclave itself, which is considered "sane" after passing all the checks performed in security_sgx_add_pages()/security_sgx_initialize_enclave(). Of course, an SGX-aware LSM module/policy shall be allowed to override. How about adding a new security_sgx_mprotect() LSM hook?

> From: Andy Lutomirski [mailto:luto@...nel.org]
> 
> > On May 14, 2019, at 8:30 AM, Haitao Huang
> <haitao.huang@...ux.intel.com> wrote:
> >
> >> On Tue, 14 May 2019 10:17:29 -0500, Andy Lutomirski <luto@...nel.org>
> wrote:
> >>
> >> On Tue, May 14, 2019 at 7:33 AM Haitao Huang
> >> <haitao.huang@...ux.intel.com> wrote:
> >>>
> >>> On Fri, 10 May 2019 14:22:34 -0500, Andy Lutomirski
> >>> <luto@...nel.org>
> >>> wrote:
> >>>
> >>> > On Fri, May 10, 2019 at 12:04 PM Jethro Beekman
> >>> > <jethro@...tanix.com>
> >>> > wrote:
> >>> >>
> >>> >> On 2019-05-10 11:56, Xing, Cedric wrote:
> >>> >> > Hi Jethro,
> >>> >> >
> >>> >> >> ELF files are explicitly designed such that you can map them
> >>> >> >> (with
> >>> >> mmap)
> >>> >> >> in 4096-byte chunks. However, sometimes there's overlap and
> >>> >> >> you will sometimes see that a particular offset is mapped
> >>> >> >> twice because the
> >>> >> first
> >>> >> >> half of the page in the file belongs to an RX range and the
> >>> >> >> second
> >>> >> half
> >>> >> >> to an R-only range. Also, ELF files don't (normally) describe
> >>> >> >> stack, heap, etc. which you do need for enclaves.
> >>> >> >
> >>> >> > You have probably misread my email. By mmap(), I meant the
> >>> >> > enclave
> >>> >> file would be mapped via *multiple* mmap() calls, in the same way
> >>> >> as what dlopen() would do in loading regular shared object. The
> >>> >> intention here is to make the enclave file subject to the same
> >>> >> checks as regular shared objects.
> >>> >>
> >>> >> No, I didn't misread your email. My original point still stands:
> >>> >> requiring that an enclave's memory is created from one or more
> >>> >> mmap calls of a file puts significant restrictions on the
> >>> >> enclave's on-disk representation.
> >>> >>
> >>> >
> >>> > For a tiny bit of background, Linux (AFAIK*) makes no effort to
> >>> > ensure the complete integrity of DSOs.  What Linux *does* do (if
> >>> > so
> >>> > configured) is to make sure that only approved data is mapped
> >>> > executable.  So, if you want to have some bytes be executable,
> >>> > those bytes have to come from a file that passes the relevant LSM
> >>> > and IMA checks.
> >>>
> >>> Given this, I just want to step back a little to understand the
> >>> exact issue that SGX is causing here for LSM/IMA. Sorry if I missed
> >>> points discussed earlier.
> >>>
> >>> By the time of EADD, enclave file is opened and should have passed
> >>> IMA and SELinux policy enforcement gates if any. We really don't
> >>> need extra mmaps on the enclave files to be IMA and SELinux
> compliant.
> >>
> >> The problem, as i see it, is that they passed the *wrong* checks,
> >> because, as you noticed:
> >>
> >>> We are loading
> >>> enclave files as RO and copying those into EPC.
> >>
> >> Which is, semantically, a lot like loading a normal file as RO and
> >> then mprotecting() it to RX, which is disallowed under quite a few
> >> LSM policies.
> >>
> >>> An IMA policy can enforce
> >>> RO files (or any file). And SELinux policy can say which processes
> >>> can open the file for what permissions. No extra needed here.
> >>
> >> If SELinux says a process may open a file as RO, that does *not* mean
> >> that it can be opened as RX.
> >>
> >
> > But in this case, file itself is mapped as RO treated like data and it
> is not for execution. SGX enclave pages have EPCM enforced permissions.
> So from SELinux point of view I would think it can treat it as RO and
> that's fine.
> 
> As an example, SELinux has an “execute” permission (via
> security_mmap_file — see file_map_prot_check()) that controls whether
> you can execute code from that file.  If you lack this permission on a
> file, you may still be able to map it PROT_READ, but you may not map it
> PROT_EXEC.  Similarly, if you want to malloc() some memory, write
> *code* to it, and execute it, you need a specific permission.
> 
> So, unless we somehow think it’s okay for SGX to break the existing
> model, we need to respect these restrictions in the SGX driver. In other
> words, we either need to respect execmem, etc or require PROT_EXEC or
> the equivalent. I like the latter a lot more.

-Cedric

Powered by blists - more mailing lists