linux-sgx.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Sean Christopherson <sean.j.christopherson@intel.com>
To: Stephen Smalley <sds@tycho.nsa.gov>
Cc: Cedric Xing <cedric.xing@intel.com>,
	linux-security-module@vger.kernel.org, selinux@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-sgx@vger.kernel.org,
	jarkko.sakkinen@linux.intel.com, luto@kernel.org,
	jmorris@namei.org, serge@hallyn.com, paul@paul-moore.com,
	eparis@parisplace.org, jethro@fortanix.com,
	dave.hansen@intel.com, tglx@linutronix.de,
	torvalds@linux-foundation.org, akpm@linux-foundation.org,
	nhorman@redhat.com, pmccallum@redhat.com, serge.ayoun@intel.com,
	shay.katz-zamir@intel.com, haitao.huang@intel.com,
	andriy.shevchenko@linux.intel.com, kai.svahn@intel.com,
	bp@alien8.de, josh@joshtriplett.org, kai.huang@intel.com,
	rientjes@google.com, william.c.roberts@intel.com,
	philip.b.tricca@intel.com
Subject: Re: [RFC PATCH v1 2/3] LSM/x86/sgx: Implement SGX specific hooks in SELinux
Date: Fri, 14 Jun 2019 08:38:40 -0700	[thread overview]
Message-ID: <20190614153840.GC12191@linux.intel.com> (raw)
In-Reply-To: <20190614004600.GF18385@linux.intel.com>

On Thu, Jun 13, 2019 at 05:46:00PM -0700, Sean Christopherson wrote:
> On Thu, Jun 13, 2019 at 01:02:17PM -0400, Stephen Smalley wrote:
> > On 6/11/19 6:02 PM, Sean Christopherson wrote:
> > >On Tue, Jun 11, 2019 at 09:40:25AM -0400, Stephen Smalley wrote:
> > >>I haven't looked at this code closely, but it feels like a lot of
> > >>SGX-specific logic embedded into SELinux that will have to be repeated or
> > >>reused for every security module.  Does SGX not track this state itself?
> > >
> > >SGX does track equivalent state.
> > >
> > >There are three proposals on the table (I think):
> > >
> > >   1. Require userspace to explicitly specificy (maximal) enclave page
> > >      permissions at build time.  The enclave page permissions are provided
> > >      to, and checked by, LSMs at enclave build time.
> > >
> > >      Pros: Low-complexity kernel implementation, straightforward auditing
> > >      Cons: Sullies the SGX UAPI to some extent, may increase complexity of
> > >            SGX2 enclave loaders.
> > >
> > >   2. Pre-check LSM permissions and dynamically track mappings to enclave
> > >      pages, e.g. add an SGX mprotect() hook to restrict W->X and WX
> > >      based on the pre-checked permissions.
> > >
> > >      Pros: Does not impact SGX UAPI, medium kernel complexity
> > >      Cons: Auditing is complex/weird, requires taking enclave-specific
> > >            lock during mprotect() to query/update tracking.
> > >
> > >   3. Implement LSM hooks in SGX to allow LSMs to track enclave regions
> > >      from cradle to grave, but otherwise defer everything to LSMs.
> > >
> > >      Pros: Does not impact SGX UAPI, maximum flexibility, precise auditing
> > >      Cons: Most complex and "heaviest" kernel implementation of the three,
> > >            pushes more SGX details into LSMs.
> > >
> > >My RFC series[1] implements #1.  My understanding is that Andy (Lutomirski)
> > >prefers #2.  Cedric's RFC series implements #3.
> > >
> > >Perhaps the easiest way to make forward progress is to rule out the
> > >options we absolutely *don't* want by focusing on the potentially blocking
> > >issue with each option:
> > >
> > >   #1 - SGX UAPI funkiness
> > >
> > >   #2 - Auditing complexity, potential enclave lock contention
> > >
> > >   #3 - Pushing SGX details into LSMs and complexity of kernel implementation
> > >
> > >
> > >[1] https://lkml.kernel.org/r/20190606021145.12604-1-sean.j.christopherson@intel.com
> > 
> > Given the complexity tradeoff, what is the clear motivating example for why
> > #1 isn't the obvious choice? That the enclave loader has no way of knowing a
> > priori whether the enclave will require W->X or WX?  But aren't we better
> > off requiring enclaves to be explicitly marked as needing such so that we
> > can make a more informed decision about whether to load them in the first
> > place?
> 
> Andy and/or Cedric, can you please weigh in with a concrete (and practical)
> use case that will break if we go with #1?  The auditing issues for #2/#3
> are complex to say the least...

Follow-up question, is #1 any more palatable if SELinux adds SGX specific
permissions and ties them to the process (instead of the vma or sigstruct)?

Something like this for SELinux, where the absolute worst case scenario is
that SGX2 enclave loaders need SGXEXECMEM.  Graphene would need SGXEXECUNMR
and probably SGXEXECANON.

static inline int sgx_has_perm(u32 sid, u32 requested)
{
        return avc_has_perm(&selinux_state, sid, sid,
			    SECCLASS_PROCESS2, requested, NULL);
}

static int selinux_enclave_load(struct vm_area_struct *vma, unsigned long prot,
				bool measured)
{
	const struct cred *cred = current_cred();
	u32 sid = cred_sid(cred);
	int ret;

	/* SGX is supported only in 64-bit kernels. */
	WARN_ON_ONCE(!default_noexec);

	/* Only executable enclave pages are restricted in any way. */
	if (!(prot & PROT_EXEC))
		return 0;

	/*
	 * Private mappings to enclave pages are impossible, ergo we don't
	 * differentiate between W->X and WX, either case requires EXECMEM.
	 */
	if (prot & PROT_WRITE) {
		ret = sgx_has_perm(sid, PROCESS2__SGXEXECMEM);
		if (ret)
			goto out;
	}
	if (!measured) {
		ret = sgx_has_perm(sid, PROCESS2__SGXEXECUNMR);
		if (ret)
			goto out;
	}

	if (!vma->vm_file || !IS_PRIVATE(file_inode(vma->vm_file)) ||
	    vma->anon_vma) {
		/*
		 * Loading enclave code from an anonymous mapping or from a
		 * modified private file mapping.
		 */
		ret = sgx_has_perm(sid, PROCESS2__SGXEXECANON);
		if (ret)
			goto out;
	} else {
		/* Loading from a shared or unmodified private file mapping. */
		ret = sgx_has_perm(sid, PROCESS2__SGXEXECFILE);
		if (ret)
			goto out;

		/* The source file must be executable in this case. */
		ret = file_has_perm(cred, vma->vm_file, FILE__EXECUTE);
		if (ret)
			goto out;
	}

out:
	return ret;
}


Given that AppArmor generally only cares about accessing files, its
enclave_load() implementation would be something like:

static int apparmor_enclave_load(struct vm_area_struct *vma, unsigned long prot,
				bool measured)
{
	if (!(prot & PROT_EXEC))
		return 0;

	return common_file_perm(OP_ENCL_LOAD, vma->vm_file, AA_EXEC_MMAP);
}

  reply	other threads:[~2019-06-14 15:38 UTC|newest]

Thread overview: 67+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-06  2:11 [RFC PATCH v2 0/5] security: x86/sgx: SGX vs. LSM Sean Christopherson
2019-06-06  2:11 ` [RFC PATCH v2 1/5] mm: Introduce vm_ops->may_mprotect() Sean Christopherson
2019-06-10 15:06   ` Jarkko Sakkinen
2019-06-10 15:55     ` Sean Christopherson
2019-06-10 17:47       ` Xing, Cedric
2019-06-10 19:49         ` Sean Christopherson
2019-06-10 22:06           ` Xing, Cedric
2019-06-06  2:11 ` [RFC PATCH v2 2/5] x86/sgx: Require userspace to define enclave pages' protection bits Sean Christopherson
2019-06-10 15:27   ` Jarkko Sakkinen
2019-06-10 16:15     ` Sean Christopherson
2019-06-10 17:45       ` Jarkko Sakkinen
2019-06-10 18:17         ` Sean Christopherson
2019-06-12 19:26           ` Jarkko Sakkinen
2019-06-10 18:29   ` Xing, Cedric
2019-06-10 19:15     ` Andy Lutomirski
2019-06-10 22:28       ` Xing, Cedric
2019-06-12  0:09         ` Andy Lutomirski
2019-06-12 14:34           ` Sean Christopherson
2019-06-12 18:20             ` Xing, Cedric
2019-06-06  2:11 ` [RFC PATCH v2 3/5] x86/sgx: Enforce noexec filesystem restriction for enclaves Sean Christopherson
2019-06-10 16:00   ` Jarkko Sakkinen
2019-06-10 16:44     ` Andy Lutomirski
2019-06-11 17:21       ` Stephen Smalley
2019-06-06  2:11 ` [RFC PATCH v2 4/5] LSM: x86/sgx: Introduce ->enclave_load() hook for Intel SGX Sean Christopherson
2019-06-07 19:58   ` Stephen Smalley
2019-06-10 16:21     ` Sean Christopherson
2019-06-10 16:05   ` Jarkko Sakkinen
2019-06-06  2:11 ` [RFC PATCH v2 5/5] security/selinux: Add enclave_load() implementation Sean Christopherson
2019-06-07 21:16   ` Stephen Smalley
2019-06-10 16:46     ` Sean Christopherson
2019-06-17 16:38   ` Jarkko Sakkinen
2019-06-10  7:03 ` [RFC PATCH v1 0/3] security/x86/sgx: SGX specific LSM hooks Cedric Xing
2019-06-10  7:03   ` [RFC PATCH v1 1/3] LSM/x86/sgx: Add " Cedric Xing
2019-06-10  7:03   ` [RFC PATCH v1 2/3] LSM/x86/sgx: Implement SGX specific hooks in SELinux Cedric Xing
2019-06-11 13:40     ` Stephen Smalley
2019-06-11 22:02       ` Sean Christopherson
2019-06-12  9:32         ` Dr. Greg
2019-06-12 14:25           ` Sean Christopherson
2019-06-13  7:25             ` Dr. Greg
2019-06-12 19:30         ` Andy Lutomirski
2019-06-12 22:02           ` Sean Christopherson
2019-06-13  0:10             ` Xing, Cedric
2019-06-13  1:02             ` Xing, Cedric
2019-06-13 17:02         ` Stephen Smalley
2019-06-13 23:03           ` Xing, Cedric
2019-06-13 23:17             ` Sean Christopherson
2019-06-14  0:31               ` Xing, Cedric
2019-06-14  0:46           ` Sean Christopherson
2019-06-14 15:38             ` Sean Christopherson [this message]
2019-06-16 22:14               ` Andy Lutomirski
2019-06-17 16:49                 ` Sean Christopherson
2019-06-17 17:08                   ` Andy Lutomirski
2019-06-18 15:40                   ` Dr. Greg
2019-06-14 17:16             ` Xing, Cedric
2019-06-14 17:45               ` Sean Christopherson
2019-06-14 17:53                 ` Sean Christopherson
2019-06-14 20:01                   ` Sean Christopherson
2019-06-16 22:16               ` Andy Lutomirski
2019-06-14 23:19             ` Dr. Greg
2019-06-11 22:55       ` Xing, Cedric
2019-06-13 18:00         ` Stephen Smalley
2019-06-13 19:48           ` Sean Christopherson
2019-06-13 21:09             ` Xing, Cedric
2019-06-13 21:02           ` Xing, Cedric
2019-06-14  0:37           ` Sean Christopherson
2019-06-10  7:03   ` [RFC PATCH v1 3/3] LSM/x86/sgx: Call new LSM hooks from SGX subsystem Cedric Xing
2019-06-10 17:36   ` [RFC PATCH v1 0/3] security/x86/sgx: SGX specific LSM hooks Jarkko Sakkinen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190614153840.GC12191@linux.intel.com \
    --to=sean.j.christopherson@intel.com \
    --cc=akpm@linux-foundation.org \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=bp@alien8.de \
    --cc=cedric.xing@intel.com \
    --cc=dave.hansen@intel.com \
    --cc=eparis@parisplace.org \
    --cc=haitao.huang@intel.com \
    --cc=jarkko.sakkinen@linux.intel.com \
    --cc=jethro@fortanix.com \
    --cc=jmorris@namei.org \
    --cc=josh@joshtriplett.org \
    --cc=kai.huang@intel.com \
    --cc=kai.svahn@intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=linux-sgx@vger.kernel.org \
    --cc=luto@kernel.org \
    --cc=nhorman@redhat.com \
    --cc=paul@paul-moore.com \
    --cc=philip.b.tricca@intel.com \
    --cc=pmccallum@redhat.com \
    --cc=rientjes@google.com \
    --cc=sds@tycho.nsa.gov \
    --cc=selinux@vger.kernel.org \
    --cc=serge.ayoun@intel.com \
    --cc=serge@hallyn.com \
    --cc=shay.katz-zamir@intel.com \
    --cc=tglx@linutronix.de \
    --cc=torvalds@linux-foundation.org \
    --cc=william.c.roberts@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).