[RFC,v4,08/12] security/selinux: Require SGX_MAPWX to map enclave page WX
diff mbox series

Message ID 20190619222401.14942-9-sean.j.christopherson@intel.com
State Superseded
Headers show
Series
  • security: x86/sgx: SGX vs. LSM
Related show

Commit Message

Sean Christopherson June 19, 2019, 10:23 p.m. UTC
Hook enclave_map() to require a new per-process capability, SGX_MAPWX,
when mapping an enclave as simultaneously writable and executable.
Note, @prot contains the actual protection bits that will be set by the
kernel, not the maximal protection bits specified by userspace when the
page was first loaded into the enclave.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 security/selinux/hooks.c            | 21 +++++++++++++++++++++
 security/selinux/include/classmap.h |  3 ++-
 2 files changed, 23 insertions(+), 1 deletion(-)

Comments

Xing, Cedric June 21, 2019, 5:09 p.m. UTC | #1
> 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?
Stephen Smalley June 25, 2019, 8:19 p.m. UTC | #2
On 6/19/19 6:23 PM, Sean Christopherson wrote:
> Hook enclave_map() to require a new per-process capability, SGX_MAPWX,
> when mapping an enclave as simultaneously writable and executable.
> Note, @prot contains the actual protection bits that will be set by the
> kernel, not the maximal protection bits specified by userspace when the
> page was first loaded into the enclave.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>   security/selinux/hooks.c            | 21 +++++++++++++++++++++
>   security/selinux/include/classmap.h |  3 ++-
>   2 files changed, 23 insertions(+), 1 deletion(-)
> 
> 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);

Possibly we should use a slightly more general name for the permission 
to allow reusing it in the future if/when another architecture 
introduces a similar construct under a different branding?  ENCLAVE_* 
seems slightly more generic than SGX_*.

I was interested in testing this code but sadly the driver reports the 
following on my development workstation:

[    1.644191] sgx: The launch control MSRs are not writable
[    1.695477] sgx: EPC section 0x70200000-0x75f7ffff
[    1.771760] sgx: The public key MSRs are not writable

I guess I'm out of luck until/unless I get a NUC or server class 
hardware that supports flexible launch control?  Seems developer unfriendly.

> +	return 0;
> +}
> +#endif
> +
>   struct lsm_blob_sizes selinux_blob_sizes __lsm_ro_after_init = {
>   	.lbs_cred = sizeof(struct task_security_struct),
>   	.lbs_file = sizeof(struct file_security_struct),
> @@ -6968,6 +6985,10 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
>   	LSM_HOOK_INIT(bpf_map_free_security, selinux_bpf_map_free),
>   	LSM_HOOK_INIT(bpf_prog_free_security, selinux_bpf_prog_free),
>   #endif
> +
> +#ifdef CONFIG_INTEL_SGX
> +	LSM_HOOK_INIT(enclave_map, selinux_enclave_map),
> +#endif
>   };
>   
>   static __init int selinux_init(void)
> diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
> index 201f7e588a29..cfd91e879bdf 100644
> --- a/security/selinux/include/classmap.h
> +++ b/security/selinux/include/classmap.h
> @@ -51,7 +51,8 @@ struct security_class_mapping secclass_map[] = {
>   	    "execmem", "execstack", "execheap", "setkeycreate",
>   	    "setsockcreate", "getrlimit", NULL } },
>   	{ "process2",
> -	  { "nnp_transition", "nosuid_transition", NULL } },
> +	  { "nnp_transition", "nosuid_transition",
> +	    "sgx_mapwx", NULL } },
>   	{ "system",
>   	  { "ipc_info", "syslog_read", "syslog_mod",
>   	    "syslog_console", "module_request", "module_load", NULL } },
>
Stephen Smalley June 25, 2019, 9:05 p.m. UTC | #3
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?
Dr. Greg June 26, 2019, 12:49 p.m. UTC | #4
On Tue, Jun 25, 2019 at 04:19:35PM -0400, Stephen Smalley wrote:

Good morning, I hope the week is going well for everyone.

> On 6/19/19 6:23 PM, Sean Christopherson wrote:
> >Hook enclave_map() to require a new per-process capability, SGX_MAPWX,
> >when mapping an enclave as simultaneously writable and executable.
> >Note, @prot contains the actual protection bits that will be set by the
> >kernel, not the maximal protection bits specified by userspace when the
> >page was first loaded into the enclave.
> >
> >Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> >---
> >  security/selinux/hooks.c            | 21 +++++++++++++++++++++
> >  security/selinux/include/classmap.h |  3 ++-
> >  2 files changed, 23 insertions(+), 1 deletion(-)
> >
> >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);

> Possibly we should use a slightly more general name for the
> permission to allow reusing it in the future if/when another
> architecture introduces a similar construct under a different
> branding?  ENCLAVE_* seems slightly more generic than SGX_*.

Perhaps TEE_*, since it is generic and expresses the notion of
privileges specific to an alternate execution environment.

> I was interested in testing this code but sadly the driver reports
> the following on my development workstation:
>
> [    1.644191] sgx: The launch control MSRs are not writable
> [    1.695477] sgx: EPC section 0x70200000-0x75f7ffff
> [    1.771760] sgx: The public key MSRs are not writable
>
> I guess I'm out of luck until/unless I get a NUC or server class
> hardware that supports flexible launch control?  Seems developer
> unfriendly.

Indeed.

Most importantly, it is decidedly unfriendly to the future of the
technology on Linux.

More problematically, from a development perspective, the driver is
incompatible with the current Intel runtime, which makes testing at a
level beyond the one page test harness that is included with the
patchset impossible.

As I noted previously, before the LSM discussion, we have a patch that
addresses the compatibility, security and launch control issues the
original version of the driver had.  If you missed the thread, it is
available from the following URL:

ftp://ftp.idfusion.net/pub/idfusion/jarkko-master-SFLC.patch

It will be a bit dated by now and doesn't address the API change
needed to set page permissions.  It is a pretty solid starting point
if you want to use the existing runtime to do more then trivial
testing.

We have an extension to the existing driver that we will be releasing,
so users of our SRDE will be able to use both the out-of-tree and
in-tree drivers.  It also re-establishes launch control and provides a
very simplistic interface to implement ring-0 security for launch
control on flexible launch control platforms.

I'm in Israel right now but we should have a GIT tree against the
current development branches by the weekend.  We will be testing the
driver with our SRDE against real world enclaves as we advance the
driver forward.

Have a good day.

Dr. Greg

As always,
Dr. Greg Wettstein, Ph.D, Worker
IDfusion, LLC
4206 N. 19th Ave.           Implementing measured information privacy
Fargo, ND  58102            and integrity architectures.
PH: 701-281-1686
FAX: 701-281-3949           EMAIL: greg@idfusion.net
------------------------------------------------------------------------------
"This place is so screwed up.  It's just like the Titanic, only
 we don't even have a band playing.
                                -- Terrance George Wieland
                                   Resurrection.
Xing, Cedric June 27, 2019, 8:26 p.m. UTC | #5
> From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx-
> owner@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.

Patch
diff mbox series

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);
+	return 0;
+}
+#endif
+
 struct lsm_blob_sizes selinux_blob_sizes __lsm_ro_after_init = {
 	.lbs_cred = sizeof(struct task_security_struct),
 	.lbs_file = sizeof(struct file_security_struct),
@@ -6968,6 +6985,10 @@  static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(bpf_map_free_security, selinux_bpf_map_free),
 	LSM_HOOK_INIT(bpf_prog_free_security, selinux_bpf_prog_free),
 #endif
+
+#ifdef CONFIG_INTEL_SGX
+	LSM_HOOK_INIT(enclave_map, selinux_enclave_map),
+#endif
 };
 
 static __init int selinux_init(void)
diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
index 201f7e588a29..cfd91e879bdf 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -51,7 +51,8 @@  struct security_class_mapping secclass_map[] = {
 	    "execmem", "execstack", "execheap", "setkeycreate",
 	    "setsockcreate", "getrlimit", NULL } },
 	{ "process2",
-	  { "nnp_transition", "nosuid_transition", NULL } },
+	  { "nnp_transition", "nosuid_transition",
+	    "sgx_mapwx", NULL } },
 	{ "system",
 	  { "ipc_info", "syslog_read", "syslog_mod",
 	    "syslog_console", "module_request", "module_load", NULL } },