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

[Stephen Smalley]

On 7/11/19 12:25 PM, Sean Christopherson wrote:
> 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?

Yes, I think that's fine.

> 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
>