[03/12] selinux: Implement Infiniband flush callback
diff mbox

Message ID 1466711578-64398-4-git-send-email-danielj@mellanox.com
State New
Headers show

Commit Message

Daniel Jurgens June 23, 2016, 7:52 p.m. UTC
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
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(-)

Comments

Yuval Shaia June 30, 2016, 3:10 p.m. UTC | #1
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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Daniel Jurgens June 30, 2016, 3:44 p.m. UTC | #2
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Paul Moore June 30, 2016, 7:52 p.m. UTC | #3
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?
Casey Schaufler June 30, 2016, 8:16 p.m. UTC | #4
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?

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Paul Moore June 30, 2016, 8:24 p.m. UTC | #5
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.
Daniel Jurgens June 30, 2016, 8:39 p.m. UTC | #6
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.

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

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)