[RFC PATCH v4 08/12] security/selinux: Require SGX_MAPWX to map enclave page WX

[Xing, Cedric]

> From: linux-sgx-owner at vger.kernel.org [mailto:linux-sgx-
> owner at vger.kernel.org] On Behalf Of Stephen Smalley
> Sent: Tuesday, June 25, 2019 2:06 PM
> 
> On 6/21/19 1:09 PM, Xing, Cedric wrote:
> >> From: Christopherson, Sean J
> >> Sent: Wednesday, June 19, 2019 3:24 PM
> >>
> >> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> >> index 3ec702cf46ca..fc239e541b62 100644
> >> --- a/security/selinux/hooks.c
> >> +++ b/security/selinux/hooks.c
> >> @@ -6726,6 +6726,23 @@ static void selinux_bpf_prog_free(struct
> bpf_prog_aux *aux)
> >>   }
> >>   #endif
> >>
> >> +#ifdef CONFIG_INTEL_SGX
> >> +static int selinux_enclave_map(unsigned long prot) {
> >> +	const struct cred *cred = current_cred();
> >> +	u32 sid = cred_sid(cred);
> >> +
> >> +	/* SGX is supported only in 64-bit kernels. */
> >> +	WARN_ON_ONCE(!default_noexec);
> >> +
> >> +	if ((prot & PROT_EXEC) && (prot & PROT_WRITE))
> >> +		return avc_has_perm(&selinux_state, sid, sid,
> >> +				    SECCLASS_PROCESS2, PROCESS2__SGX_MAPWX,
> >> +				    NULL);
> >
> > Why isn't SGX_MAPWX enclave specific but process wide?
> 
> How would you tie it to a specific enclave?  What's the object/target
> SID?  The SID of the enclave inode?  Which one?  The source vma file,
> the /dev/sgx/enclave open instance, the sigstruct file, ...?  If a
> process can map one enclave WX, what's the benefit of preventing it from
> doing likewise for any other enclave it can load?

I wasn't saying we should. Rather, I think we can reuse EXECMEM. After all, under what circumstances are WX necessary? IMHO, WX shall be strongly discouraged and this SGX_MAPWX is kind of trying to give the bearing enclave a dirty look. And if that's the sole purpose, let's make it even dirtier by requiring EXECMEM on the host process. After all, WX is never a good thing in security so I doubt any ISVs would have a practical reason to require WX in their enclaves.