| Message ID | 1466711578-64398-4-git-send-email-danielj@mellanox.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Thu, Jun 23, 2016 at 10:52:49PM +0300, Dan Jurgens wrote: > From: Daniel Jurgens <danielj@mellanox.com> > > Because access for infiniband is enforced in the setup phase of a > connection there must be a way to notify ib_core if the policy or > enforcement setting have changed. > > Added register and unregister_ib_flush_callback hooks so infiniband can > setup notification of policy and enforment changes. In the AVC reset Extra space before " In" > callback function call the ib_flush callback if it's registered. Also > renamed the callback function, the previous name was 'net' specific. > > Signed-off-by: Daniel Jurgens <danielj@mellanox.com> > Reviewed-by: Eli Cohen <eli@mellanox.com> > --- > security/selinux/hooks.c | 36 ++++++++++++++++++++++++++++++++++-- > 1 file changed, 34 insertions(+), 2 deletions(-) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index a86d537..6a8841d 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -94,6 +94,9 @@ > #include "audit.h" > #include "avc_ss.h" > > +static void (*ib_flush_callback)(void); Do we really want to have such ib_ prefix in security/ directory? > +static DEFINE_MUTEX(ib_flush_mutex); > + > /* SECMARK reference count */ > static atomic_t selinux_secmark_refcount = ATOMIC_INIT(0); > > @@ -159,13 +162,17 @@ static int selinux_peerlbl_enabled(void) > return (selinux_policycap_alwaysnetwork || netlbl_enabled() || selinux_xfrm_enabled()); > } > > -static int selinux_netcache_avc_callback(u32 event) > +static int selinux_cache_avc_callback(u32 event) > { > if (event == AVC_CALLBACK_RESET) { > sel_netif_flush(); > sel_netnode_flush(); > sel_netport_flush(); > synchronize_net(); > + mutex_lock(&ib_flush_mutex); Suggesting to have the lock inside the callback (unless you accept my suggestion below) > + if (ib_flush_callback) > + ib_flush_callback(); How about some generic mechanism (such as a list) in case more modules/drivers would like to register callbacks? ( assuming this is no longer RFC :) ) > + mutex_unlock(&ib_flush_mutex); > } > return 0; > } > @@ -5993,6 +6000,23 @@ static int selinux_key_getsecurity(struct key *key, char **_buffer) > > #endif > > +#ifdef CONFIG_SECURITY_INFINIBAND > +static void selinux_register_ib_flush_callback(void (*callback)(void)) > +{ > + mutex_lock(&ib_flush_mutex); > + ib_flush_callback = callback; > + mutex_unlock(&ib_flush_mutex); > +} > + > +static void selinux_unregister_ib_flush_callback(void) > +{ > + mutex_lock(&ib_flush_mutex); > + ib_flush_callback = NULL; > + mutex_unlock(&ib_flush_mutex); > +} > + > +#endif > + > static struct security_hook_list selinux_hooks[] = { > LSM_HOOK_INIT(binder_set_context_mgr, selinux_binder_set_context_mgr), > LSM_HOOK_INIT(binder_transaction, selinux_binder_transaction), > @@ -6174,6 +6198,12 @@ static struct security_hook_list selinux_hooks[] = { > LSM_HOOK_INIT(tun_dev_attach_queue, selinux_tun_dev_attach_queue), > LSM_HOOK_INIT(tun_dev_attach, selinux_tun_dev_attach), > LSM_HOOK_INIT(tun_dev_open, selinux_tun_dev_open), > +#ifdef CONFIG_SECURITY_INFINIBAND > + LSM_HOOK_INIT(register_ib_flush_callback, > + selinux_register_ib_flush_callback), > + LSM_HOOK_INIT(unregister_ib_flush_callback, > + selinux_unregister_ib_flush_callback), > +#endif > > #ifdef CONFIG_SECURITY_NETWORK_XFRM > LSM_HOOK_INIT(xfrm_policy_alloc_security, selinux_xfrm_policy_alloc), > @@ -6233,9 +6263,11 @@ static __init int selinux_init(void) > 0, SLAB_PANIC, NULL); > avc_init(); > > + ib_flush_callback = NULL; > + > security_add_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks)); > > - if (avc_add_callback(selinux_netcache_avc_callback, AVC_CALLBACK_RESET)) > + if (avc_add_callback(selinux_cache_avc_callback, AVC_CALLBACK_RESET)) > panic("SELinux: Unable to register AVC netcache callback\n"); > > if (selinux_enforcing) > -- > 1.8.3.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On 6/30/2016 10:10 AM, Yuval Shaia wrote: > On Thu, Jun 23, 2016 at 10:52:49PM +0300, Dan Jurgens wrote: > >> +static void (*ib_flush_callback)(void); > Do we really want to have such ib_ prefix in security/ directory? > >> + if (ib_flush_callback) >> + ib_flush_callback(); > How about some generic mechanism (such as a list) in case more > modules/drivers would like to register callbacks? > ( assuming this is no longer RFC :) ) > Paul Moore and I were having a higher level discussion about this in the 00/12 thread. I think your suggestion makes sense, perhaps Paul will weigh in when he reaches this patch.
On Thu, Jun 30, 2016 at 11:44 AM, Daniel Jurgens <danielj@mellanox.com> wrote: > On 6/30/2016 10:10 AM, Yuval Shaia wrote: >> On Thu, Jun 23, 2016 at 10:52:49PM +0300, Dan Jurgens wrote: >> >>> +static void (*ib_flush_callback)(void); >> Do we really want to have such ib_ prefix in security/ directory? >> >>> + if (ib_flush_callback) >>> + ib_flush_callback(); >> How about some generic mechanism (such as a list) in case more >> modules/drivers would like to register callbacks? >> ( assuming this is no longer RFC :) ) >> > Paul Moore and I were having a higher level discussion about this in the 00/12 thread. I think your suggestion makes sense, perhaps Paul will weigh in when he reaches this patch. I'm still working on understanding IB, but my current thinking is very similar to Yuval's suggestions. There is a risk of creating a general purpose mechanism to solve a specific, isolated problem, but adding a LSM notification mechanism does seem like a reasonable thing to do. My current thinking is to have the LSM framework itself, e.g. security/security.c, maintain a list of callbacks (BTW, please make it a RCU protected list) with other non-LSM subsystems registering callbacks, and specific LSMs making notification calls into the LSM framework itself which would handle iterating through the registered callbacks. Since we're going down the general purpose solution route, I might add an event field and a void pointer to the callback, for example: void lsm_notifier_callback(unsigned int event, void *ptr); ... I would expect at first we would only have a POLICY_CHANGE event (ptr set to NULL), but we may want/need to add other events in the future. Make sense?
On 6/30/2016 12:52 PM, Paul Moore wrote: > On Thu, Jun 30, 2016 at 11:44 AM, Daniel Jurgens <danielj@mellanox.com> wrote: >> On 6/30/2016 10:10 AM, Yuval Shaia wrote: >>> On Thu, Jun 23, 2016 at 10:52:49PM +0300, Dan Jurgens wrote: >>> >>>> +static void (*ib_flush_callback)(void); >>> Do we really want to have such ib_ prefix in security/ directory? >>> >>>> + if (ib_flush_callback) >>>> + ib_flush_callback(); >>> How about some generic mechanism (such as a list) in case more >>> modules/drivers would like to register callbacks? >>> ( assuming this is no longer RFC :) ) >>> >> Paul Moore and I were having a higher level discussion about this in the 00/12 thread. I think your suggestion makes sense, perhaps Paul will weigh in when he reaches this patch. > I'm still working on understanding IB, but my current thinking is very > similar to Yuval's suggestions. There is a risk of creating a general > purpose mechanism to solve a specific, isolated problem, but adding a > LSM notification mechanism does seem like a reasonable thing to do. > > My current thinking is to have the LSM framework itself, e.g. > security/security.c, maintain a list of callbacks (BTW, please make it > a RCU protected list) with other non-LSM subsystems registering > callbacks, and specific LSMs making notification calls into the LSM > framework itself which would handle iterating through the registered > callbacks. Since we're going down the general purpose solution route, > I might add an event field and a void pointer to the callback, for > example: > > void lsm_notifier_callback(unsigned int event, void *ptr); > > ... I would expect at first we would only have a POLICY_CHANGE event > (ptr set to NULL), but we may want/need to add other events in the > future. > > Make sense? Hmm. Do you think that we'd want to rewhack the audit code so that it used this new, general mechanism for its callbacks?
On Thu, Jun 30, 2016 at 4:16 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: > On 6/30/2016 12:52 PM, Paul Moore wrote: >> On Thu, Jun 30, 2016 at 11:44 AM, Daniel Jurgens <danielj@mellanox.com> wrote: >>> On 6/30/2016 10:10 AM, Yuval Shaia wrote: >>>> On Thu, Jun 23, 2016 at 10:52:49PM +0300, Dan Jurgens wrote: >>>> >>>>> +static void (*ib_flush_callback)(void); >>>> Do we really want to have such ib_ prefix in security/ directory? >>>> >>>>> + if (ib_flush_callback) >>>>> + ib_flush_callback(); >>>> How about some generic mechanism (such as a list) in case more >>>> modules/drivers would like to register callbacks? >>>> ( assuming this is no longer RFC :) ) >>>> >>> Paul Moore and I were having a higher level discussion about this in the 00/12 thread. I think your suggestion makes sense, perhaps Paul will weigh in when he reaches this patch. >> I'm still working on understanding IB, but my current thinking is very >> similar to Yuval's suggestions. There is a risk of creating a general >> purpose mechanism to solve a specific, isolated problem, but adding a >> LSM notification mechanism does seem like a reasonable thing to do. >> >> My current thinking is to have the LSM framework itself, e.g. >> security/security.c, maintain a list of callbacks (BTW, please make it >> a RCU protected list) with other non-LSM subsystems registering >> callbacks, and specific LSMs making notification calls into the LSM >> framework itself which would handle iterating through the registered >> callbacks. Since we're going down the general purpose solution route, >> I might add an event field and a void pointer to the callback, for >> example: >> >> void lsm_notifier_callback(unsigned int event, void *ptr); >> >> ... I would expect at first we would only have a POLICY_CHANGE event >> (ptr set to NULL), but we may want/need to add other events in the >> future. >> >> Make sense? > > Hmm. Do you think that we'd want to rewhack the audit code > so that it used this new, general mechanism for its callbacks? You aren't talking about the callbacks in common_lsm_audit() are you? I think that's a completely different beast from a LSM notifier callback. I'm not opposed to changes to common_lsm_audit(), but I think that's a different topic and a different thread.
On 6/30/2016 2:52 PM, Paul Moore wrote: > I'm still working on understanding IB, but my current thinking is very > similar to Yuval's suggestions. There is a risk of creating a general > purpose mechanism to solve a specific, isolated problem, but adding a > LSM notification mechanism does seem like a reasonable thing to do. > > My current thinking is to have the LSM framework itself, e.g. > security/security.c, maintain a list of callbacks (BTW, please make it > a RCU protected list) with other non-LSM subsystems registering > callbacks, and specific LSMs making notification calls into the LSM > framework itself which would handle iterating through the registered > callbacks. Since we're going down the general purpose solution route, > I might add an event field and a void pointer to the callback, for > example: > > void lsm_notifier_callback(unsigned int event, void *ptr); > > ... I would expect at first we would only have a POLICY_CHANGE event > (ptr set to NULL), but we may want/need to add other events in the > future. > > Make sense? Yes, I think so. I'll make this change.
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index a86d537..6a8841d 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -94,6 +94,9 @@ #include "audit.h" #include "avc_ss.h" +static void (*ib_flush_callback)(void); +static DEFINE_MUTEX(ib_flush_mutex); + /* SECMARK reference count */ static atomic_t selinux_secmark_refcount = ATOMIC_INIT(0); @@ -159,13 +162,17 @@ static int selinux_peerlbl_enabled(void) return (selinux_policycap_alwaysnetwork || netlbl_enabled() || selinux_xfrm_enabled()); } -static int selinux_netcache_avc_callback(u32 event) +static int selinux_cache_avc_callback(u32 event) { if (event == AVC_CALLBACK_RESET) { sel_netif_flush(); sel_netnode_flush(); sel_netport_flush(); synchronize_net(); + mutex_lock(&ib_flush_mutex); + if (ib_flush_callback) + ib_flush_callback(); + mutex_unlock(&ib_flush_mutex); } return 0; } @@ -5993,6 +6000,23 @@ static int selinux_key_getsecurity(struct key *key, char **_buffer) #endif +#ifdef CONFIG_SECURITY_INFINIBAND +static void selinux_register_ib_flush_callback(void (*callback)(void)) +{ + mutex_lock(&ib_flush_mutex); + ib_flush_callback = callback; + mutex_unlock(&ib_flush_mutex); +} + +static void selinux_unregister_ib_flush_callback(void) +{ + mutex_lock(&ib_flush_mutex); + ib_flush_callback = NULL; + mutex_unlock(&ib_flush_mutex); +} + +#endif + static struct security_hook_list selinux_hooks[] = { LSM_HOOK_INIT(binder_set_context_mgr, selinux_binder_set_context_mgr), LSM_HOOK_INIT(binder_transaction, selinux_binder_transaction), @@ -6174,6 +6198,12 @@ static struct security_hook_list selinux_hooks[] = { LSM_HOOK_INIT(tun_dev_attach_queue, selinux_tun_dev_attach_queue), LSM_HOOK_INIT(tun_dev_attach, selinux_tun_dev_attach), LSM_HOOK_INIT(tun_dev_open, selinux_tun_dev_open), +#ifdef CONFIG_SECURITY_INFINIBAND + LSM_HOOK_INIT(register_ib_flush_callback, + selinux_register_ib_flush_callback), + LSM_HOOK_INIT(unregister_ib_flush_callback, + selinux_unregister_ib_flush_callback), +#endif #ifdef CONFIG_SECURITY_NETWORK_XFRM LSM_HOOK_INIT(xfrm_policy_alloc_security, selinux_xfrm_policy_alloc), @@ -6233,9 +6263,11 @@ static __init int selinux_init(void) 0, SLAB_PANIC, NULL); avc_init(); + ib_flush_callback = NULL; + security_add_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks)); - if (avc_add_callback(selinux_netcache_avc_callback, AVC_CALLBACK_RESET)) + if (avc_add_callback(selinux_cache_avc_callback, AVC_CALLBACK_RESET)) panic("SELinux: Unable to register AVC netcache callback\n"); if (selinux_enforcing)