| Message ID | 20190619222401.14942-9-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | security: x86/sgx: SGX vs. LSM | expand |
> 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?
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 } }, >
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?
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.
> 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.
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 } },
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(-)