Re: [RFC PATCH v3 3/4] X86/sgx: Introduce EMA as a new LSM module

From: Sean Christopherson <sean.j.christopherson@intel.com>
To: Stephen Smalley <sds@tycho.nsa.gov>
Cc: "Xing, Cedric" <cedric.xing@intel.com>,
	Casey Schaufler <casey@schaufler-ca.com>,
	linux-sgx@vger.kernel.org, linux-security-module@vger.kernel.org,
	selinux@vger.kernel.org, James Morris <jmorris@namei.org>,
	Paul Moore <paul@paul-moore.com>
Subject: Re: [RFC PATCH v3 3/4] X86/sgx: Introduce EMA as a new LSM module
Date: Thu, 11 Jul 2019 09:25:06 -0700	[thread overview]
Message-ID: <20190711162506.GF15067@linux.intel.com> (raw)
In-Reply-To: <27e55a96-d5c4-69ed-a88d-7a3c26fb7f75@tycho.nsa.gov>

On Thu, Jul 11, 2019 at 12:11:06PM -0400, Stephen Smalley wrote:
> On 7/11/19 11:12 AM, Sean Christopherson wrote:
> >On Thu, Jul 11, 2019 at 09:51:19AM -0400, Stephen Smalley wrote:
> >>I'd also feel better if there was clear consensus among all of the
> >>@intel.com participants that this is the right approach. To date that has
> >>seemed elusive.
> >
> >That's a very kind way to phrase things :-)
> >
> >For initial upstreaming, we've agreed that there is no need to extend the
> >uapi, i.e. we can punt on deciding between on-the-fly tracking and having
> >userspace specify maximal permissions until we add SGX2 support.
> >
> >The last open (knock on wood) for initial upstreaming is whether SELinux
> >would prefer to have new enclave specific permissions or reuse the
> >existing PROCESS__EXECMEM, FILE__EXECUTE and FILE__EXECMOD permissions.
> >My understanding is that enclave specific permissions are preferred.
> 
> I was left unclear on this topic after the email exchanges with Cedric.
> There are at least three options:
> 
> 1) Reuse the existing EXECMEM, EXECUTE, and EXECMOD permissions.  Pros:
> Existing distro policies will be applied in the expected manner with respect
> to the introduction of executable code into the system, consistent control
> will be provided over the enclave and the host process, no change for
> users/documentation wrt policy.  Cons: Existing permissions don't map
> exactly to SGX semantics, no ability to distinguish executable content
> within the enclave versus the host process at the LSM level (argued earlier
> by Cedric to be unnecessary and perhaps meaningless), need to allow
> FILE__EXECUTE or other checks on sigstruct files that may not actually
> contain code.
> 
> 2) Define new permissions within existing security classes (e.g. process2,
> file). Pros: Can tailor permission names and definitions to SGX semantics,
> ability to distinguish enclave versus host process execute access, no need
> to grant FILE__EXECUTE to sigstruct files, class matches the target object,
> permissions computed and cached upon existing checks (i.e. when a process
> accesses a file, all of the permissions to that file are computed and then
> cached at once, including the enclave-related ones).  Cons: Typical distro
> policies (unlike Android) allow unknown permissions by default for forward
> kernel compatibility reasons, so existing policies will permit these new
> permissions by default and enforcement will only truly take effect once
> policies are updated, adding new permissions to existing classes requires an
> update to the base policy (so they can't be shipped as a third party policy
> module alongside the SGX driver or installed as a local module by an admin,
> for example), documentation/user education required for new permissions.
> 
> 3) Define new permissions in new security classes (e.g. enclave). Pros
> relative to #2: New classes and permissions can be defined and installed in
> third party or local policy module without requiring a change to the base
> policy.  Cons relative to #2: Class won't correspond to the target object,
> permissions won't be computed and cached upon existing checks (only when
> performing the checks against the new classes).
> 
> Combinations are also possible, of course.

What's the impact on distros/ecosystems if we go with #1 for now and later
decide to switch to #2 after upstreaming?  I.e. can we take a minimal-ish
approach now without painting ourselves into a corner?

We can map quite closely to the existing intent of EXECUTE, EXECMOD and
EXECMEM via a combination of checking protections at enclave load time and
again at mmap()/mprotect(), e.g.:

#ifdef CONFIG_INTEL_SGX
static inline int enclave_has_perm(u32 sid, u32 requested)
{
	return avc_has_perm(&selinux_state, sid, sid,
			    SECCLASS_PROCESS, requested, NULL);
}

static int selinux_enclave_map(unsigned long prot)
{
	const struct cred *cred = current_cred();
	u32 sid = cred_sid(cred);

	if ((prot & PROT_EXEC) && (prot & PROT_WRITE))
		return enclave_has_perm(sid, PROCESS__EXECMEM);

	return 0;
}

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

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

	if (!vma->vm_file || IS_PRIVATE(file_inode(vma->vm_file))) {
		ret = enclave_has_perm(sid, PROCESS__EXECMEM);
	} else {
		ret = file_has_perm(cred, vma->vm_file, FILE__EXECUTE);
		if (ret)
			goto out;

		/*
		 * Load code from a modified private mapping or from a file
		 * with the ability to do W->X within the enclave.
		 */
		if (vma->anon_vma || (prot & PROT_WRITE))
			ret = file_has_perm(cred, vma->vm_file,
					    FILE__EXECMOD);
	}

out:
	return ret;
}
#endif