Re: [RFC PATCH v4 10/12] security/selinux: Add enclave_load() implementation

From: Andy Lutomirski <luto@kernel.org>
To: "Xing, Cedric" <cedric.xing@intel.com>
Cc: Andy Lutomirski <luto@kernel.org>,
	Stephen Smalley <sds@tycho.nsa.gov>,
	"Christopherson, Sean J" <sean.j.christopherson@intel.com>,
	Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>,
	"linux-sgx@vger.kernel.org" <linux-sgx@vger.kernel.org>,
	"linux-security-module@vger.kernel.org" 
	<linux-security-module@vger.kernel.org>,
	"selinux@vger.kernel.org" <selinux@vger.kernel.org>,
	"Roberts, William C" <william.c.roberts@intel.com>,
	"Schaufler, Casey" <casey.schaufler@intel.com>,
	James Morris <jmorris@namei.org>,
	"Hansen, Dave" <dave.hansen@intel.com>,
	Jethro Beekman <jethro@fortanix.com>,
	"Dr . Greg Wettstein" <greg@enjellic.com>
Subject: Re: [RFC PATCH v4 10/12] security/selinux: Add enclave_load() implementation
Date: Mon, 1 Jul 2019 10:53:45 -0700	[thread overview]
Message-ID: <CALCETrXjq9JNjXZo3Va83Ca7fiAJx7ZM9VRWYebzpyH6ug0cKg@mail.gmail.com> (raw)
In-Reply-To: <960B34DE67B9E140824F1DCDEC400C0F6551D558@ORSMSX116.amr.corp.intel.com>

On Mon, Jul 1, 2019 at 10:46 AM Xing, Cedric <cedric.xing@intel.com> wrote:
>
> > From: Andy Lutomirski [mailto:luto@kernel.org]
> > Sent: Saturday, June 29, 2019 4:42 PM
> >
> > On Tue, Jun 25, 2019 at 2:09 PM Stephen Smalley <sds@tycho.nsa.gov>
> > wrote:
> > >
> > > On 6/21/19 5:22 PM, Xing, Cedric wrote:
> > > >> From: Christopherson, Sean J
> > > >> Sent: Wednesday, June 19, 2019 3:24 PM
> > > >>
> > > >> Intended use of each permission:
> > > >>
> > > >>    - SGX_EXECDIRTY: dynamically load code within the enclave itself
> > > >>    - SGX_EXECUNMR: load unmeasured code into the enclave, e.g.
> > > >> Graphene
> > > >
> > > > Why does it matter whether a code page is measured or not?
> > >
> > > It won't be incorporated into an attestation?
> > >
> >
> > Also, if there is, in parallel, a policy that limits the set of enclave
> > SIGSTRUCTs that are accepted, requiring all code be measured makes it
> > harder to subvert by writing incompetent or maliciously incompetent
> > enclaves.
>
> As analyzed in my reply to one of Stephen's comments, no executable page shall be "enclave only" as enclaves have access to host's memory, so if an executable page in regular memory is considered posting a threat to the process, it should be considered posting the same threat inside an enclave as well.

Huh?  The SDM (37.3 in whateve version I'm reading) says "Code fetches
from inside an enclave to a linear address outside that enclave result
in a #GP(0) exception."  Enclaves execute enclave code only.

In any event, I believe we're discussing taking readable memory from
outside the enclave and copying it to an executable code inside the
enclave.

>
> That said, every executable enclave page should have an executable source page (doesn’t have to executable, as long as mprotect(X) would succeed on it, as shown in my patch)

Does Sean's series require this?  I think that, if we can get away
with it, it's a lot nicer to *not* require user code to map the source
pages PROT_EXEC.  Some policy may check that it's VM_MAYEXEC or check
some other attribute of the VMA, but actually requiring PROT_EXEC
seems like we're weakening existing hardening measures to enforce a
policy, which is a mistake.