Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 24 May 2019 12:13:44 -0700
From:   Sean Christopherson <sean.j.christopherson@intel.com>
To:     "Xing, Cedric" <cedric.xing@intel.com>
Cc:     Stephen Smalley <sds@tycho.nsa.gov>,
        Andy Lutomirski <luto@kernel.org>,
        Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>,
        James Morris <jmorris@namei.org>,
        "Serge E. Hallyn" <serge@hallyn.com>,
        LSM List <linux-security-module@vger.kernel.org>,
        Paul Moore <paul@paul-moore.com>,
        Eric Paris <eparis@parisplace.org>,
        "selinux@vger.kernel.org" <selinux@vger.kernel.org>,
        Jethro Beekman <jethro@fortanix.com>,
        "Hansen, Dave" <dave.hansen@intel.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        "Dr. Greg" <greg@enjellic.com>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        LKML <linux-kernel@vger.kernel.org>, X86 ML <x86@kernel.org>,
        "linux-sgx@vger.kernel.org" <linux-sgx@vger.kernel.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        "nhorman@redhat.com" <nhorman@redhat.com>,
        "npmccallum@redhat.com" <npmccallum@redhat.com>,
        "Ayoun, Serge" <serge.ayoun@intel.com>,
        "Katz-zamir, Shay" <shay.katz-zamir@intel.com>,
        "Huang, Haitao" <haitao.huang@intel.com>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        "Svahn, Kai" <kai.svahn@intel.com>, Borislav Petkov <bp@alien8.de>,
        Josh Triplett <josh@joshtriplett.org>,
        "Huang, Kai" <kai.huang@intel.com>,
        David Rientjes <rientjes@google.com>
Subject: Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
Message-ID: <20190524191344.GD365@linux.intel.com>
References: <20190523102628.GC10955@linux.intel.com>
 <20190523141752.GA12078@linux.intel.com>
 <CALCETrUzx3LPAKCLFf75P-XshAkRcr+JLET3LA_kHDs9MA11FA@mail.gmail.com>
 <20190523234044.GC12078@linux.intel.com>
 <CALCETrV4DVEfW6EJ6DnQGGYDJAiA5M1QcuYJTiroumOM+D6Jjg@mail.gmail.com>
 <960B34DE67B9E140824F1DCDEC400C0F654E8956@ORSMSX116.amr.corp.intel.com>
 <dda0912b-cb15-3c07-d368-345159e995f7@tycho.nsa.gov>
 <20190524174243.GA365@linux.intel.com>
 <20190524175458.GB365@linux.intel.com>
 <960B34DE67B9E140824F1DCDEC400C0F654E8E1D@ORSMSX116.amr.corp.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <960B34DE67B9E140824F1DCDEC400C0F654E8E1D@ORSMSX116.amr.corp.intel.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, May 24, 2019 at 11:34:32AM -0700, Xing, Cedric wrote:
> > From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx-
> > owner@vger.kernel.org] On Behalf Of Sean Christopherson
> > Sent: Friday, May 24, 2019 10:55 AM
> > 
> > On Fri, May 24, 2019 at 10:42:43AM -0700, Sean Christopherson wrote:
> > > Hmm, I've been thinking more about pulling permissions from the source
> > > page.  Conceptually I'm not sure we need to meet the same requirements as
> > > non-enclave DSOs while the enclave is being built, i.e. do we really need
> > > to force userspace to fully map the enclave in normal memory?
> > >
> > > Consider the Graphene scenario where it's building an enclave on the fly.
> > > Pulling permissions from the source VMAs means Graphene has to map the
> > > code pages of the enclave with X.  This means Graphene will need EXEDMOD
> > > (or EXECMEM if Graphene isn't careful).  In a non-SGX scenario this makes
> > > perfect sense since there is no way to verify the end result of RW->RX.
> > >
> > > But for SGX, assuming enclaves are whitelisted by their sigstruct (checked
> > > at EINIT) and because page permissions affect sigstruct.MRENCLAVE, it *is*
> > > possible to verify the resulting RX contents.  E.g. for the purposes of
> > > LSMs, can't we use the .sigstruct file as a proxy for the enclave and
> > > require FILE__EXECUTE on the .sigstruct inode to map/run the enclave?
> > >
> > > Stephen, is my logic sound?
> > >
> > >
> > > If so...
> > >
> > >   - Require FILE__READ+FILE__EXECUTE on .sigstruct to mmap() the enclave.
> > >
> > >   - Prevent userspace from mapping the enclave with permissions beyond the
> > >     original permissions of the enclave.  This can be done by populating
> > >     VM_MAY{READ,WRITE,EXEC} from the SECINFO (same basic concept as Andy's
> > >     proposals).  E.g. pre-EINIT, mmap() and mprotect() can only succeed
> > >     with PROT_NONE.
> > >
> > >   - Require FILE__{READ,WRITE,EXECUTE} on /dev/sgx/enclave for simplicity,
> > >     or provide an alternate SGX_IOC_MPROTECT if we want to sidestep the
> > >     FILE__WRITE requirement.
> > 
> > One more thought.  EADD (and the equivalent SGX2 flow) could do
> > security_mmap_file() with a NULL file on the SECINFO permissions, which
> > would trigger PROCESS_EXECMEM if an enclave attempts to map a page RWX.
> 
> If "initial permissions" for enclaves are less restrictive than shared
> objects, then it'd become a backdoor for circumventing LSM when enclave
> whitelisting is *not* in place. For example, an adversary may load a page,
> which would otherwise never be executable, as an executable page in EPC.

My point is that enclaves have different properties than shared objects.

Normal LSM behavior with regard to executing files is to label files with
e.g. FILE__EXECUTE.  Because an enclave must be built to the exact
specifications of .sigstruct, requring FILE__EXECUTE on the .sigstruct is
effectively the same as requiring FILE__EXECUTE on the enclave itself.

Addressing your scenario of loading an executable page in EPC, doing so
would require one of the following:

  - Ability to install a .sigstruct with FILE__EXECUTE

  - PROCESS__EXECMEM

  - FILE__EXECMOD and SGX2 support

> In the case a RWX page is needed, the calling process has to have a RWX page
> serving as the source for EADD so PROCESS__EXECMEM will have been checked.
> For SGX2, changing an EPC page to RWX is subject to FILE__EXECMEM on
> /dev/sgx/enclave, which I see as a security benefit because it only affects
> the enclave but not the whole process hosting it.

There is no FILE__EXECMEM check, only PROCESS__EXECMEM and FILE__EXECMOD.
I assume you're referring to the latter?

I don't see a fundamental difference between having RWX in an enclave and
RWX in normal memory, either way the process can execute arbitrary code,
i.e. PROCESS__EXECMEM is appropriate.  Yes, an enclave will #UD on certain
instructions, but that's easily sidestepped by having a trampoline in the
host (marked RX) and piping arbitrary code into the enclave.  Or using
EEXIT to do a bit of ROP.

> > > No changes are required to LSMs, SGX1 has a single LSM touchpoint in
> > its
> > > mmap(), and I *think* the only required userspace change is to mmap()
> > > PROT_NONE when allocating the enclave's virtual address range.
> 
> I'm not sure I understand the motivation behind this proposal to decouple
> initial EPC permissions from source pages.

Pulling permissions from source pages means userspace needs to fully map
the in normal memory, including marking pages executable.  That exposes
the loader to having executable pages in its address space that it has no
intention of executing (outside of the enclave).  And for Graphene, it
means having to actively avoid PROCESS__EXECMEM, e.g. by using a dummy
backing file to build the enclave instead of anon memory.

> I don't think it a big deal to fully mmap() enclave files, which have to be
> parsed by user mode anyway to determine various things including but not
> limited to the size of heap(s), size and number of TCSs/stacks/TLS areas, and
> the overall enclave size. So with PHDRs parsed, it's trivial to mmap() each
> segment with permissions from its PHDR.
>
> > > As for Graphene, it doesn't need extra permissions to run its enclaves,
> > > it just needs a way to install .sigstruct, which is a generic permissions
> > > problem and not SGX specific.
> > >
> > >
> > > For SGX2 maybe:
> > >
> > >   - No additional requirements to map an EAUG'd page as RW page.  Not
> > >     aligned with standard MAP_SHARED behavior, but we really don't want
> > >     to require FILE__WRITE, and thus allow writes to .sigstruct.
> > >
> > >   - Require FILE__EXECMOD on the .sigstruct to map previously writable
> > >     page as executable (which indirectly includes all EAUG'd pages).
> > >     Wiring this up will be a little funky, but we again we don't want
> > >     to require FILE__WRITE on .sigstruct.
> > >
> 
> I'm lost. Why is EAUG tied to permissions on .sigstruct? 

Because for the purposes of LSM checks, .sigstruct is the enclave's
backing file, and mapping a previously writable enclave page as exectuable
is roughly equivalent to mapping a CoW'd page as exectuable.