[RFC PATCH 0/2] selinux: avoid nf hooks overhead when not needed

[RFC PATCH 0/2] selinux: avoid nf hooks overhead when not needed

[Paolo Abeni]

Currently, selinux always registers iptables POSTROUTING hooks regarless of
the running policy needs for any action to be performed by them.

Even the socket_sock_rcv_skb() is always registered, but it can result in a no-op
depending on the current policy configuration.

The above invocations in the kernel datapath are cause of measurable
overhead in networking performance test.

This patch series adds explicit notification for netlabel status change 
(other relevant status change, like xfrm and secmark, are already notified to
LSM) and use this information in selinux to register the above hooks only when
the current status makes them relevant, deregistering them when no-op

Avoiding the LSM hooks overhead, in netperf UDP_STREAM test with small packets,
gives about 5% performance improvement on rx and about 8% on tx.

Paolo Abeni (2):
  security: add hook for netlabel status change notification
  selinux: implement support for dynamic net hook [de-]registration

 include/linux/lsm_hooks.h           |  6 ++++
 include/linux/security.h            |  5 +++
 net/netlabel/netlabel_cipso_v4.c    |  8 +++--
 net/netlabel/netlabel_unlabeled.c   |  5 ++-
 security/security.c                 |  7 ++++
 security/selinux/hooks.c            | 72 +++++++++++++++++++++++++++++++------
 security/selinux/include/security.h |  1 +
 security/selinux/ss/services.c      |  1 +
 security/selinux/xfrm.c             |  4 +++
 9 files changed, 96 insertions(+), 13 deletions(-)

-- 
1.8.3.1

[RFC PATCH 1/2] security: add hook for netlabel status change notification

[Paolo Abeni]

This patch adds a new LSM hook called every time the netlbl usage
count is changed. This allows an LSM to register a set of hooks
on demand according to the netlabel status.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 include/linux/lsm_hooks.h         | 6 ++++++
 include/linux/security.h          | 5 +++++
 net/netlabel/netlabel_cipso_v4.c  | 8 ++++++--
 net/netlabel/netlabel_unlabeled.c | 5 ++++-
 security/security.c               | 7 +++++++
 5 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index cdee11c..9a6b90f 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -875,6 +875,10 @@
  *	This hook can be used by the module to update any security state
  *	associated with the TUN device's security structure.
  *	@security pointer to the TUN devices's security structure.
+ * @netlbl_changed:
+ *	This hook can be used by the module to update any security state
+ *	associated with the netlbl_enable() status (i.e. registering/
+ *	deregistering additional hooks on demands)
  *
  * Security hooks for XFRM operations.
  *
@@ -1576,6 +1580,7 @@ union security_list_options {
 	int (*tun_dev_attach_queue)(void *security);
 	int (*tun_dev_attach)(struct sock *sk, void *security);
 	int (*tun_dev_open)(void *security);
+	void (*netlbl_changed)(void);
 #endif	/* CONFIG_SECURITY_NETWORK */
 
 #ifdef CONFIG_SECURITY_NETWORK_XFRM
@@ -1805,6 +1810,7 @@ struct security_hook_heads {
 	struct list_head tun_dev_attach;
 	struct list_head tun_dev_open;
 	struct list_head skb_owned_by;
+	struct list_head netlbl_changed;
 #endif	/* CONFIG_SECURITY_NETWORK */
 #ifdef CONFIG_SECURITY_NETWORK_XFRM
 	struct list_head xfrm_policy_alloc_security;
diff --git a/include/linux/security.h b/include/linux/security.h
index 157f0cb..8bff4e5 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1162,6 +1162,7 @@ int security_tun_dev_create(void);
 int security_tun_dev_attach_queue(void *security);
 int security_tun_dev_attach(struct sock *sk, void *security);
 int security_tun_dev_open(void *security);
+void security_netlbl_changed(void);
 
 #else	/* CONFIG_SECURITY_NETWORK */
 static inline int security_unix_stream_connect(struct sock *sock,
@@ -1354,6 +1355,10 @@ static inline int security_tun_dev_open(void *security)
 {
 	return 0;
 }
+
+static inline void security_netlbl_changed(void)
+{
+}
 #endif	/* CONFIG_SECURITY_NETWORK */
 
 #ifdef CONFIG_SECURITY_NETWORK_XFRM
diff --git a/net/netlabel/netlabel_cipso_v4.c b/net/netlabel/netlabel_cipso_v4.c
index 7fd1104..1b508ea 100644
--- a/net/netlabel/netlabel_cipso_v4.c
+++ b/net/netlabel/netlabel_cipso_v4.c
@@ -438,8 +438,10 @@ static int netlbl_cipsov4_add(struct sk_buff *skb, struct genl_info *info)
 		ret_val = netlbl_cipsov4_add_local(info, &audit_info);
 		break;
 	}
-	if (ret_val == 0)
+	if (ret_val == 0) {
 		atomic_inc(&netlabel_mgmt_protocount);
+		security_netlbl_changed();
+	}
 
 	return ret_val;
 }
@@ -725,8 +727,10 @@ static int netlbl_cipsov4_remove(struct sk_buff *skb, struct genl_info *info)
 				     netlbl_cipsov4_remove_cb, &cb_arg);
 	if (ret_val == 0 || ret_val == -ENOENT) {
 		ret_val = cipso_v4_doi_remove(cb_arg.doi, &audit_info);
-		if (ret_val == 0)
+		if (ret_val == 0) {
 			atomic_dec(&netlabel_mgmt_protocount);
+			security_netlbl_changed();
+		}
 	}
 
 	return ret_val;
diff --git a/net/netlabel/netlabel_unlabeled.c b/net/netlabel/netlabel_unlabeled.c
index 9eaa9a1..7795700 100644
--- a/net/netlabel/netlabel_unlabeled.c
+++ b/net/netlabel/netlabel_unlabeled.c
@@ -451,8 +451,10 @@ int netlbl_unlhsh_add(struct net *net,
 	default:
 		ret_val = -EINVAL;
 	}
-	if (ret_val == 0)
+	if (ret_val == 0) {
 		atomic_inc(&netlabel_mgmt_protocount);
+		security_netlbl_changed();
+	}
 
 unlhsh_add_return:
 	rcu_read_unlock();
@@ -692,6 +694,7 @@ int netlbl_unlhsh_remove(struct net *net,
 	if (ret_val == 0) {
 		netlbl_unlhsh_condremove_iface(iface);
 		atomic_dec(&netlabel_mgmt_protocount);
+		security_netlbl_changed();
 	}
 
 unlhsh_remove_return:
diff --git a/security/security.c b/security/security.c
index 3644b03..e4d6262 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1396,6 +1396,12 @@ int security_tun_dev_open(void *security)
 }
 EXPORT_SYMBOL(security_tun_dev_open);
 
+void security_netlbl_changed(void)
+{
+	call_void_hook(netlbl_changed);
+}
+EXPORT_SYMBOL(security_netlbl_changed);
+
 #endif	/* CONFIG_SECURITY_NETWORK */
 
 #ifdef CONFIG_SECURITY_NETWORK_XFRM
@@ -1849,6 +1855,7 @@ struct security_hook_heads security_hook_heads = {
 		LIST_HEAD_INIT(security_hook_heads.tun_dev_attach),
 	.tun_dev_open =	LIST_HEAD_INIT(security_hook_heads.tun_dev_open),
 	.skb_owned_by =	LIST_HEAD_INIT(security_hook_heads.skb_owned_by),
+	.netlbl_changed = LIST_HEAD_INIT(security_hook_heads.netlbl_changed),
 #endif	/* CONFIG_SECURITY_NETWORK */
 #ifdef CONFIG_SECURITY_NETWORK_XFRM
 	.xfrm_policy_alloc_security =
-- 
1.8.3.1

[RFC PATCH 2/2] selinux: implement support for dynamic net hook [de-]registration

[Paolo Abeni]

This patch leverage the netlbl_changed() hook to perform on demand
registration and deregistration of the netfilter hooks and the
socket_sock_rcv_skb hook.

With default policy and empty netfilter/netlabel configuration, the
above hooks are not registered and this allows avoiding nf_hook_slow
in the xmit path and socket_sock_rcv_skb() in the rx path.

This gives measurable network performance improvement in both
directions.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 security/selinux/hooks.c            | 72 +++++++++++++++++++++++++++++++------
 security/selinux/include/security.h |  1 +
 security/selinux/ss/services.c      |  1 +
 security/selinux/xfrm.c             |  4 +++
 4 files changed, 68 insertions(+), 10 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 912deee..a3baa69 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4745,11 +4745,13 @@ static int selinux_secmark_relabel_packet(u32 sid)
 static void selinux_secmark_refcount_inc(void)
 {
 	atomic_inc(&selinux_secmark_refcount);
+	selinux_net_update();
 }
 
 static void selinux_secmark_refcount_dec(void)
 {
 	atomic_dec(&selinux_secmark_refcount);
+	selinux_net_update();
 }
 
 static void selinux_req_classify_flow(const struct request_sock *req,
@@ -4836,6 +4838,11 @@ static int selinux_tun_dev_open(void *security)
 	return 0;
 }
 
+static void selinux_netlbl_changed(void)
+{
+	selinux_net_update();
+}
+
 static int selinux_nlmsg_perm(struct sock *sk, struct sk_buff *skb)
 {
 	int err = 0;
@@ -6091,7 +6098,6 @@ static struct security_hook_list selinux_hooks[] = {
 	LSM_HOOK_INIT(socket_getsockopt, selinux_socket_getsockopt),
 	LSM_HOOK_INIT(socket_setsockopt, selinux_socket_setsockopt),
 	LSM_HOOK_INIT(socket_shutdown, selinux_socket_shutdown),
-	LSM_HOOK_INIT(socket_sock_rcv_skb, selinux_socket_sock_rcv_skb),
 	LSM_HOOK_INIT(socket_getpeersec_stream,
 			selinux_socket_getpeersec_stream),
 	LSM_HOOK_INIT(socket_getpeersec_dgram, selinux_socket_getpeersec_dgram),
@@ -6113,6 +6119,7 @@ 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),
+	LSM_HOOK_INIT(netlbl_changed, selinux_netlbl_changed),
 
 #ifdef CONFIG_SECURITY_NETWORK_XFRM
 	LSM_HOOK_INIT(xfrm_policy_alloc_security, selinux_xfrm_policy_alloc),
@@ -6145,6 +6152,11 @@ static struct security_hook_list selinux_hooks[] = {
 #endif
 };
 
+/* dynamically registered/unregisterd */
+static struct security_hook_list selinux_sock_hooks[] = {
+	LSM_HOOK_INIT(socket_sock_rcv_skb, selinux_socket_sock_rcv_skb),
+};
+
 static __init int selinux_init(void)
 {
 	if (!security_module_enable("selinux")) {
@@ -6240,7 +6252,9 @@ static struct nf_hook_ops selinux_nf_ops[] = {
 #endif	/* IPV6 */
 };
 
-static int __init selinux_nf_ip_init(void)
+static bool nf_hooks_registered;
+
+static int selinux_nf_ip_init(void)
 {
 	int err;
 
@@ -6253,25 +6267,21 @@ static int __init selinux_nf_ip_init(void)
 	if (err)
 		panic("SELinux: nf_register_hooks: error %d\n", err);
 
+	nf_hooks_registered = true;
 	return 0;
 }
 
-__initcall(selinux_nf_ip_init);
-
-#ifdef CONFIG_SECURITY_SELINUX_DISABLE
 static void selinux_nf_ip_exit(void)
 {
 	printk(KERN_DEBUG "SELinux:  Unregistering netfilter hooks\n");
 
 	nf_unregister_hooks(selinux_nf_ops, ARRAY_SIZE(selinux_nf_ops));
+	nf_hooks_registered = false;
 }
-#endif
 
 #else /* CONFIG_NETFILTER */
 
-#ifdef CONFIG_SECURITY_SELINUX_DISABLE
 #define selinux_nf_ip_exit()
-#endif
 
 #endif /* CONFIG_NETFILTER */
 
@@ -6300,8 +6310,8 @@ int selinux_disable(void)
 	/* Try to destroy the avc node cache */
 	avc_disable();
 
-	/* Unregister netfilter hooks. */
-	selinux_nf_ip_exit();
+	/* Unregister net hooks. */
+	selinux_net_update();
 
 	/* Unregister selinuxfs. */
 	exit_sel_fs();
@@ -6309,3 +6319,45 @@ int selinux_disable(void)
 	return 0;
 }
 #endif
+
+DEFINE_MUTEX(selinux_net_mutex);
+
+static bool nf_hooks_required(void)
+{
+	return (selinux_secmark_enabled() || selinux_peerlbl_enabled() ||
+		!selinux_policycap_netpeer) && selinux_enabled;
+}
+
+static bool sock_hooks_required(void)
+{
+	return (!selinux_policycap_netpeer || selinux_secmark_enabled() ||
+		selinux_peerlbl_enabled()) && selinux_enabled;
+}
+
+static bool sock_hooks_registered;
+
+void selinux_net_update(void)
+{
+	if ((nf_hooks_required() == nf_hooks_registered) &&
+	    (sock_hooks_required() == sock_hooks_registered))
+		return;
+
+	mutex_lock(&selinux_net_mutex);
+	if (nf_hooks_required() != nf_hooks_registered) {
+		if (!nf_hooks_registered)
+			selinux_nf_ip_init();
+		else
+			selinux_nf_ip_exit();
+	}
+
+	if (sock_hooks_required() != sock_hooks_registered) {
+		if (!sock_hooks_registered)
+			security_add_hooks(selinux_sock_hooks,
+					   ARRAY_SIZE(selinux_sock_hooks));
+		else
+			security_delete_hooks(selinux_sock_hooks,
+					      ARRAY_SIZE(selinux_sock_hooks));
+		sock_hooks_registered = !sock_hooks_registered;
+	}
+	mutex_unlock(&selinux_net_mutex);
+}
diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
index 38feb55..0428ab4 100644
--- a/security/selinux/include/security.h
+++ b/security/selinux/include/security.h
@@ -261,6 +261,7 @@ extern void selinux_status_update_setenforce(int enforcing);
 extern void selinux_status_update_policyload(int seqno);
 extern void selinux_complete_init(void);
 extern int selinux_disable(void);
+extern void selinux_net_update(void);
 extern void exit_sel_fs(void);
 extern struct path selinux_null;
 extern struct vfsmount *selinuxfs_mount;
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index ebda973..c509018 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -2016,6 +2016,7 @@ static void security_load_policycaps(void)
 						  POLICYDB_CAPABILITY_OPENPERM);
 	selinux_policycap_alwaysnetwork = ebitmap_get_bit(&policydb.policycaps,
 						  POLICYDB_CAPABILITY_ALWAYSNETWORK);
+	selinux_net_update();
 }
 
 static int security_preserve_bools(struct policydb *p);
diff --git a/security/selinux/xfrm.c b/security/selinux/xfrm.c
index 56e354f..cc2b0d4 100644
--- a/security/selinux/xfrm.c
+++ b/security/selinux/xfrm.c
@@ -112,6 +112,7 @@ static int selinux_xfrm_alloc_user(struct xfrm_sec_ctx **ctxp,
 
 	*ctxp = ctx;
 	atomic_inc(&selinux_xfrm_refcount);
+	selinux_net_update();
 	return 0;
 
 err:
@@ -128,6 +129,7 @@ static void selinux_xfrm_free(struct xfrm_sec_ctx *ctx)
 		return;
 
 	atomic_dec(&selinux_xfrm_refcount);
+	selinux_net_update();
 	kfree(ctx);
 }
 
@@ -303,6 +305,7 @@ int selinux_xfrm_policy_clone(struct xfrm_sec_ctx *old_ctx,
 	if (!new_ctx)
 		return -ENOMEM;
 	atomic_inc(&selinux_xfrm_refcount);
+	selinux_net_update();
 	*new_ctxp = new_ctx;
 
 	return 0;
@@ -370,6 +373,7 @@ int selinux_xfrm_state_alloc_acquire(struct xfrm_state *x,
 
 	x->security = ctx;
 	atomic_inc(&selinux_xfrm_refcount);
+	selinux_net_update();
 out:
 	kfree(ctx_str);
 	return rc;
-- 
1.8.3.1

Re: [RFC PATCH 0/2] selinux: avoid nf hooks overhead when not needed

[Paul Moore]

On Wed, Apr 6, 2016 at 5:51 AM, Paolo Abeni <pabeni@redhat.com> wrote:
> Currently, selinux always registers iptables POSTROUTING hooks regarless of
> the running policy needs for any action to be performed by them.
>
> Even the socket_sock_rcv_skb() is always registered, but it can result in a no-op
> depending on the current policy configuration.
>
> The above invocations in the kernel datapath are cause of measurable
> overhead in networking performance test.
>
> This patch series adds explicit notification for netlabel status change
> (other relevant status change, like xfrm and secmark, are already notified to
> LSM) and use this information in selinux to register the above hooks only when
> the current status makes them relevant, deregistering them when no-op
>
> Avoiding the LSM hooks overhead, in netperf UDP_STREAM test with small packets,
> gives about 5% performance improvement on rx and about 8% on tx.

[NOTE: added the SELinux mailing list to the CC line, please include
when submitting SELinux patches]

While I appreciate the patch and the work that went into development
and testing, I'm going to reject this patch on the grounds that it
conflicts with work we've just started thinking about which should
bring some tangible security benefit.

The recent addition of post-init read only memory opens up some
interesting possibilities for SELinux and LSMs in general, the thing
which we've just started looking at is marking the LSM hook structure
read only after init.  There are some complicating factors for
SELinux, but I'm confident those can be resolved, and from what I can
tell marking the hooks read only will have no effect on other LSMs.
While marking the LSM hook structure doesn't directly affect the
SELinux netfilter hooks, once we remove the ability to deregister the
LSM hooks we will have no need to support deregistering netfilter
hooks and I expect we will drop that functionality as well to help
decrease the risk of tampering.

-- 
paul moore
www.paul-moore.com

Re: [RFC PATCH 0/2] selinux: avoid nf hooks overhead when not needed

[Paolo Abeni]

On Wed, 2016-04-06 at 08:33 -0400, Paul Moore wrote:
> On Wed, Apr 6, 2016 at 5:51 AM, Paolo Abeni <pabeni@redhat.com> wrote:
> > Currently, selinux always registers iptables POSTROUTING hooks regarless of
> > the running policy needs for any action to be performed by them.
> >
> > Even the socket_sock_rcv_skb() is always registered, but it can result in a no-op
> > depending on the current policy configuration.
> >
> > The above invocations in the kernel datapath are cause of measurable
> > overhead in networking performance test.
> >
> > This patch series adds explicit notification for netlabel status change
> > (other relevant status change, like xfrm and secmark, are already notified to
> > LSM) and use this information in selinux to register the above hooks only when
> > the current status makes them relevant, deregistering them when no-op
> >
> > Avoiding the LSM hooks overhead, in netperf UDP_STREAM test with small packets,
> > gives about 5% performance improvement on rx and about 8% on tx.
> 
> [NOTE: added the SELinux mailing list to the CC line, please include
> when submitting SELinux patches]
> 
> While I appreciate the patch and the work that went into development
> and testing, I'm going to reject this patch on the grounds that it
> conflicts with work we've just started thinking about which should
> bring some tangible security benefit.
> 
> The recent addition of post-init read only memory opens up some
> interesting possibilities for SELinux and LSMs in general, the thing
> which we've just started looking at is marking the LSM hook structure
> read only after init.  There are some complicating factors for
> SELinux, but I'm confident those can be resolved, and from what I can
> tell marking the hooks read only will have no effect on other LSMs.
> While marking the LSM hook structure doesn't directly affect the
> SELinux netfilter hooks, once we remove the ability to deregister the
> LSM hooks we will have no need to support deregistering netfilter
> hooks and I expect we will drop that functionality as well to help
> decrease the risk of tampering.

What if we drops the selinux hook related changes in the second patch
(the on-demand socket_sock_rcv_skb() [de-]registration)?

The patch will not conflict with the LSM hook structure becoming
read-only, we still retain the ability of registering/de-registering the
netfilter hooks, and that will still affect positively the tx network
performance.

Regards,

Paolo

Re: [RFC PATCH 0/2] selinux: avoid nf hooks overhead when not needed

[Paul Moore]

On Wed, Apr 6, 2016 at 10:03 AM, Paolo Abeni <pabeni@redhat.com> wrote:
> On Wed, 2016-04-06 at 08:33 -0400, Paul Moore wrote:
>> On Wed, Apr 6, 2016 at 5:51 AM, Paolo Abeni <pabeni@redhat.com> wrote:
>> > Currently, selinux always registers iptables POSTROUTING hooks regarless of
>> > the running policy needs for any action to be performed by them.
>> >
>> > Even the socket_sock_rcv_skb() is always registered, but it can result in a no-op
>> > depending on the current policy configuration.
>> >
>> > The above invocations in the kernel datapath are cause of measurable
>> > overhead in networking performance test.
>> >
>> > This patch series adds explicit notification for netlabel status change
>> > (other relevant status change, like xfrm and secmark, are already notified to
>> > LSM) and use this information in selinux to register the above hooks only when
>> > the current status makes them relevant, deregistering them when no-op
>> >
>> > Avoiding the LSM hooks overhead, in netperf UDP_STREAM test with small packets,
>> > gives about 5% performance improvement on rx and about 8% on tx.
>>
>> [NOTE: added the SELinux mailing list to the CC line, please include
>> when submitting SELinux patches]
>>
>> While I appreciate the patch and the work that went into development
>> and testing, I'm going to reject this patch on the grounds that it
>> conflicts with work we've just started thinking about which should
>> bring some tangible security benefit.
>>
>> The recent addition of post-init read only memory opens up some
>> interesting possibilities for SELinux and LSMs in general, the thing
>> which we've just started looking at is marking the LSM hook structure
>> read only after init.  There are some complicating factors for
>> SELinux, but I'm confident those can be resolved, and from what I can
>> tell marking the hooks read only will have no effect on other LSMs.
>> While marking the LSM hook structure doesn't directly affect the
>> SELinux netfilter hooks, once we remove the ability to deregister the
>> LSM hooks we will have no need to support deregistering netfilter
>> hooks and I expect we will drop that functionality as well to help
>> decrease the risk of tampering.
>
> What if we drops the selinux hook related changes in the second patch
> (the on-demand socket_sock_rcv_skb() [de-]registration)?
>
> The patch will not conflict with the LSM hook structure becoming
> read-only, we still retain the ability of registering/de-registering the
> netfilter hooks, and that will still affect positively the tx network
> performance.

As I already said above:

"While marking the LSM hook structure doesn't directly affect the
SELinux netfilter hooks, once we remove the ability to deregister the
LSM hooks we will have no need to support deregistering netfilter
hooks and I expect we will drop that functionality as well to help
decrease the risk of tampering."

-- 
paul moore
www.paul-moore.com

Re: [RFC PATCH 0/2] selinux: avoid nf hooks overhead when not needed

[David Miller]

From: Paul Moore <paul@paul-moore.com>
Date: Wed, 6 Apr 2016 10:07:27 -0400

> "While marking the LSM hook structure doesn't directly affect the
> SELinux netfilter hooks, once we remove the ability to deregister the
> LSM hooks we will have no need to support deregistering netfilter
> hooks and I expect we will drop that functionality as well to help
> decrease the risk of tampering."

This is not a reasonable postiion.

The performance implications are non-trivial for using netfilter hooks
when they aren't actually needed.

Re: [RFC PATCH 0/2] selinux: avoid nf hooks overhead when not needed

[Paul Moore]

On Wed, Apr 6, 2016 at 2:23 PM, David Miller <davem@davemloft.net> wrote:
> From: Paul Moore <paul@paul-moore.com>
> Date: Wed, 6 Apr 2016 10:07:27 -0400
>
>> "While marking the LSM hook structure doesn't directly affect the
>> SELinux netfilter hooks, once we remove the ability to deregister the
>> LSM hooks we will have no need to support deregistering netfilter
>> hooks and I expect we will drop that functionality as well to help
>> decrease the risk of tampering."
>
> This is not a reasonable postiion.
>
> The performance implications are non-trivial for using netfilter hooks
> when they aren't actually needed.

With all due respect, I think you've taken what I consider to be some
unreasonable positions when it comes to the network stack and LSMs in
the past.  We have different perspectives and different priorities as
a result, from my perspective the security advantage gained by
eliminating the ability to disable SELinux at runtime is more
important.

-- 
paul moore
www.paul-moore.com

Re: [RFC PATCH 0/2] selinux: avoid nf hooks overhead when not needed

[David Miller]

From: Paul Moore <paul@paul-moore.com>
Date: Wed, 6 Apr 2016 14:36:43 -0400

> On Wed, Apr 6, 2016 at 2:23 PM, David Miller <davem@davemloft.net> wrote:
>> From: Paul Moore <paul@paul-moore.com>
>> Date: Wed, 6 Apr 2016 10:07:27 -0400
>>
>>> "While marking the LSM hook structure doesn't directly affect the
>>> SELinux netfilter hooks, once we remove the ability to deregister the
>>> LSM hooks we will have no need to support deregistering netfilter
>>> hooks and I expect we will drop that functionality as well to help
>>> decrease the risk of tampering."
>>
>> This is not a reasonable postiion.
>>
>> The performance implications are non-trivial for using netfilter hooks
>> when they aren't actually needed.
> 
> With all due respect, I think you've taken what I consider to be some
> unreasonable positions when it comes to the network stack and LSMs in
> the past.  We have different perspectives and different priorities as
> a result, from my perspective the security advantage gained by
> eliminating the ability to disable SELinux at runtime is more
> important.

SELinux folks seem to get rather upset to people outright disabling
the facility, but many users still do exactly that.

In my opinion, it's uncompromising positions like the one you are
having here is part of the reason that issue will continue.

It is not AND, it's an OR, people want choice, and if you don't give
it to them they will find a way to achieve what they want with or
without your help.  And you might not like what they come up with.

If distributions are turning SELinux on by default, then we have to
care about whather netfilter performance should suffer for facilities
which are unused.

Re: [RFC PATCH 0/2] selinux: avoid nf hooks overhead when not needed

[Paul Moore]

On Wed, Apr 6, 2016 at 3:39 PM, David Miller <davem@davemloft.net> wrote:
> From: Paul Moore <paul@paul-moore.com>
> Date: Wed, 6 Apr 2016 14:36:43 -0400
>
>> On Wed, Apr 6, 2016 at 2:23 PM, David Miller <davem@davemloft.net> wrote:
>>> From: Paul Moore <paul@paul-moore.com>
>>> Date: Wed, 6 Apr 2016 10:07:27 -0400
>>>
>>>> "While marking the LSM hook structure doesn't directly affect the
>>>> SELinux netfilter hooks, once we remove the ability to deregister the
>>>> LSM hooks we will have no need to support deregistering netfilter
>>>> hooks and I expect we will drop that functionality as well to help
>>>> decrease the risk of tampering."
>>>
>>> This is not a reasonable postiion.
>>>
>>> The performance implications are non-trivial for using netfilter hooks
>>> when they aren't actually needed.
>>
>> With all due respect, I think you've taken what I consider to be some
>> unreasonable positions when it comes to the network stack and LSMs in
>> the past.  We have different perspectives and different priorities as
>> a result, from my perspective the security advantage gained by
>> eliminating the ability to disable SELinux at runtime is more
>> important.
>
> SELinux folks seem to get rather upset to people outright disabling
> the facility, but many users still do exactly that.

My opinion is that SELinux isn't for everyone; I think it would be
great if everyone enabled it, but I recognize that it isn't the best
fit for everyone's needs.  If users want to disable it in order to
better meet their needs, who am I to argue?

Or perhaps I should be upset?  I dunno, please tell me how I should
feel.  Like most people, I *love* when I'm told how I should react.

> In my opinion, it's uncompromising positions like the one you are
> having here is part of the reason that issue will continue.

Once again, I suspect this all a matter of perspective; from my point
of view the SELinux code has compromised quite a lot, especially in
the case of the networking controls.

> It is not AND, it's an OR, people want choice, and if you don't give
> it to them they will find a way to achieve what they want with or
> without your help.  And you might not like what they come up with.
>
> If distributions are turning SELinux on by default, then we have to
> care about whather netfilter performance should suffer for facilities
> which are unused.

I think you've made your point known, and I believe I've been clear
about the reasoning behind my decision as well.  I would suggest we
leave it at that until/unless someone has something constructive to
add to the conversation.

-- 
paul moore
www.paul-moore.com

Re: [RFC PATCH 0/2] selinux: avoid nf hooks overhead when not needed

[Casey Schaufler]

On 4/6/2016 2:51 AM, Paolo Abeni wrote:
> Currently, selinux always registers iptables POSTROUTING hooks regarless of
> the running policy needs for any action to be performed by them.
>
> Even the socket_sock_rcv_skb() is always registered, but it can result in a no-op
> depending on the current policy configuration.
>
> The above invocations in the kernel datapath are cause of measurable
> overhead in networking performance test.
>
> This patch series adds explicit notification for netlabel status change 
> (other relevant status change, like xfrm and secmark, are already notified to
> LSM) and use this information in selinux to register the above hooks only when
> the current status makes them relevant, deregistering them when no-op
>
> Avoiding the LSM hooks overhead, in netperf UDP_STREAM test with small packets,
> gives about 5% performance improvement on rx and about 8% on tx.
>
> Paolo Abeni (2):
>   security: add hook for netlabel status change notification
>   selinux: implement support for dynamic net hook [de-]registration
>
>  include/linux/lsm_hooks.h           |  6 ++++
>  include/linux/security.h            |  5 +++
>  net/netlabel/netlabel_cipso_v4.c    |  8 +++--
>  net/netlabel/netlabel_unlabeled.c   |  5 ++-
>  security/security.c                 |  7 ++++
>  security/selinux/hooks.c            | 72 +++++++++++++++++++++++++++++++------
>  security/selinux/include/security.h |  1 +
>  security/selinux/ss/services.c      |  1 +
>  security/selinux/xfrm.c             |  4 +++
>  9 files changed, 96 insertions(+), 13 deletions(-)
>
Is there a patch 1/2?

Re: [RFC PATCH 0/2] selinux: avoid nf hooks overhead when not needed

[Casey Schaufler]

On 4/6/2016 2:51 AM, Paolo Abeni wrote:
> Currently, selinux always registers iptables POSTROUTING hooks regarless of
> the running policy needs for any action to be performed by them.
>
> Even the socket_sock_rcv_skb() is always registered, but it can result in a no-op
> depending on the current policy configuration.
>
> The above invocations in the kernel datapath are cause of measurable
> overhead in networking performance test.
>
> This patch series adds explicit notification for netlabel status change 
> (other relevant status change, like xfrm and secmark, are already notified to
> LSM) and use this information in selinux to register the above hooks only when
> the current status makes them relevant, deregistering them when no-op
>
> Avoiding the LSM hooks overhead, in netperf UDP_STREAM test with small packets,
> gives about 5% performance improvement on rx and about 8% on tx.
>
> Paolo Abeni (2):
>   security: add hook for netlabel status change notification
>   selinux: implement support for dynamic net hook [de-]registration

Did you consider the fact that netlabel and the LSM socket
hooks are used by Smack as well as SELinux? Did you measure the
impact that your changes have on Smack? Does Smack even work
with your changes?

>
>  include/linux/lsm_hooks.h           |  6 ++++
>  include/linux/security.h            |  5 +++
>  net/netlabel/netlabel_cipso_v4.c    |  8 +++--
>  net/netlabel/netlabel_unlabeled.c   |  5 ++-
>  security/security.c                 |  7 ++++
>  security/selinux/hooks.c            | 72 +++++++++++++++++++++++++++++++------
>  security/selinux/include/security.h |  1 +
>  security/selinux/ss/services.c      |  1 +
>  security/selinux/xfrm.c             |  4 +++
>  9 files changed, 96 insertions(+), 13 deletions(-)
>

Re: [RFC PATCH 0/2] selinux: avoid nf hooks overhead when not needed

[Paul Moore]

On Wed, Apr 6, 2016 at 5:37 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 4/6/2016 2:51 AM, Paolo Abeni wrote:
>> Currently, selinux always registers iptables POSTROUTING hooks regarless of
>> the running policy needs for any action to be performed by them.
>>
>> Even the socket_sock_rcv_skb() is always registered, but it can result in a no-op
>> depending on the current policy configuration.
>>
>> The above invocations in the kernel datapath are cause of measurable
>> overhead in networking performance test.
>>
>> This patch series adds explicit notification for netlabel status change
>> (other relevant status change, like xfrm and secmark, are already notified to
>> LSM) and use this information in selinux to register the above hooks only when
>> the current status makes them relevant, deregistering them when no-op
>>
>> Avoiding the LSM hooks overhead, in netperf UDP_STREAM test with small packets,
>> gives about 5% performance improvement on rx and about 8% on tx.
>>
>> Paolo Abeni (2):
>>   security: add hook for netlabel status change notification
>>   selinux: implement support for dynamic net hook [de-]registration
>>
>>  include/linux/lsm_hooks.h           |  6 ++++
>>  include/linux/security.h            |  5 +++
>>  net/netlabel/netlabel_cipso_v4.c    |  8 +++--
>>  net/netlabel/netlabel_unlabeled.c   |  5 ++-
>>  security/security.c                 |  7 ++++
>>  security/selinux/hooks.c            | 72 +++++++++++++++++++++++++++++++------
>>  security/selinux/include/security.h |  1 +
>>  security/selinux/ss/services.c      |  1 +
>>  security/selinux/xfrm.c             |  4 +++
>>  9 files changed, 96 insertions(+), 13 deletions(-)
>>
>
> Is there a patch 1/2?

Yes, there was (it was the "security: add hook ..." patch), but for
some reason it hasn't hit the archive that I normally use.  Odd.

I'll fwd the patch to you off-list so as not to spam everyone again.

-- 
paul moore
www.paul-moore.com

Re: [RFC PATCH 0/2] selinux: avoid nf hooks overhead when not needed

[Florian Westphal]

Paul Moore <paul@paul-moore.com> wrote:
> On Wed, Apr 6, 2016 at 5:51 AM, Paolo Abeni <pabeni@redhat.com> wrote:
> > Currently, selinux always registers iptables POSTROUTING hooks regarless of
> > the running policy needs for any action to be performed by them.
> >
> > Even the socket_sock_rcv_skb() is always registered, but it can result in a no-op
> > depending on the current policy configuration.
> >
> > The above invocations in the kernel datapath are cause of measurable
> > overhead in networking performance test.
> >
> > This patch series adds explicit notification for netlabel status change
> > (other relevant status change, like xfrm and secmark, are already notified to
> > LSM) and use this information in selinux to register the above hooks only when
> > the current status makes them relevant, deregistering them when no-op
> >
> > Avoiding the LSM hooks overhead, in netperf UDP_STREAM test with small packets,
> > gives about 5% performance improvement on rx and about 8% on tx.
> 
> [NOTE: added the SELinux mailing list to the CC line, please include
> when submitting SELinux patches]
> 
> While I appreciate the patch and the work that went into development
> and testing, I'm going to reject this patch on the grounds that it
> conflicts with work we've just started thinking about which should
> bring some tangible security benefit.
> 
> The recent addition of post-init read only memory opens up some
> interesting possibilities for SELinux and LSMs in general, the thing
> which we've just started looking at is marking the LSM hook structure
> read only after init.  There are some complicating factors for
> SELinux, but I'm confident those can be resolved, and from what I can
> tell marking the hooks read only will have no effect on other LSMs.
> While marking the LSM hook structure doesn't directly affect the
> SELinux netfilter hooks, once we remove the ability to deregister the
> LSM hooks we will have no need to support deregistering netfilter
> hooks and I expect we will drop that functionality as well to help
> decrease the risk of tampering.

netfilter hooks are per namespace -- so there is hook unregister when
netns is destroyed.

Do you think it makes sense to rework the patch to delay registering
of the netfiler hooks until the system is in a state where they're
needed, without the 'unregister' aspect?

Ideally this would even be per netns -- in perfect world we would
be able to make it so that a new netns are created with an empty
hook list.

Thanks.

Re: [RFC PATCH 2/2] selinux: implement support for dynamic net hook [de-]registration

[Casey Schaufler]

On 4/6/2016 2:51 AM, Paolo Abeni wrote:
> This patch leverage the netlbl_changed() hook to perform on demand
> registration and deregistration of the netfilter hooks and the
> socket_sock_rcv_skb hook.
>
> With default policy and empty netfilter/netlabel configuration, the
> above hooks are not registered and this allows avoiding nf_hook_slow
> in the xmit path and socket_sock_rcv_skb() in the rx path.

There is no reason to assume that there is a relationship between
a netlabel configuration and a netfilter configuration. Smack always
has a netlabel configuration. Security modules (e.g. AppArmor) may
well use netfilter without netlabel.

Please stop assuming that security == SELinux. 

>
> This gives measurable network performance improvement in both
> directions.

In the case where SELinux is enabled and netfilter is not.

> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
>  security/selinux/hooks.c            | 72 +++++++++++++++++++++++++++++++------
>  security/selinux/include/security.h |  1 +
>  security/selinux/ss/services.c      |  1 +
>  security/selinux/xfrm.c             |  4 +++
>  4 files changed, 68 insertions(+), 10 deletions(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 912deee..a3baa69 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -4745,11 +4745,13 @@ static int selinux_secmark_relabel_packet(u32 sid)
>  static void selinux_secmark_refcount_inc(void)
>  {
>  	atomic_inc(&selinux_secmark_refcount);
> +	selinux_net_update();
>  }
>  
>  static void selinux_secmark_refcount_dec(void)
>  {
>  	atomic_dec(&selinux_secmark_refcount);
> +	selinux_net_update();
>  }
>  
>  static void selinux_req_classify_flow(const struct request_sock *req,
> @@ -4836,6 +4838,11 @@ static int selinux_tun_dev_open(void *security)
>  	return 0;
>  }
>  
> +static void selinux_netlbl_changed(void)
> +{
> +	selinux_net_update();
> +}
> +
>  static int selinux_nlmsg_perm(struct sock *sk, struct sk_buff *skb)
>  {
>  	int err = 0;
> @@ -6091,7 +6098,6 @@ static struct security_hook_list selinux_hooks[] = {
>  	LSM_HOOK_INIT(socket_getsockopt, selinux_socket_getsockopt),
>  	LSM_HOOK_INIT(socket_setsockopt, selinux_socket_setsockopt),
>  	LSM_HOOK_INIT(socket_shutdown, selinux_socket_shutdown),
> -	LSM_HOOK_INIT(socket_sock_rcv_skb, selinux_socket_sock_rcv_skb),
>  	LSM_HOOK_INIT(socket_getpeersec_stream,
>  			selinux_socket_getpeersec_stream),
>  	LSM_HOOK_INIT(socket_getpeersec_dgram, selinux_socket_getpeersec_dgram),
> @@ -6113,6 +6119,7 @@ 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),
> +	LSM_HOOK_INIT(netlbl_changed, selinux_netlbl_changed),
>  
>  #ifdef CONFIG_SECURITY_NETWORK_XFRM
>  	LSM_HOOK_INIT(xfrm_policy_alloc_security, selinux_xfrm_policy_alloc),
> @@ -6145,6 +6152,11 @@ static struct security_hook_list selinux_hooks[] = {
>  #endif
>  };
>  
> +/* dynamically registered/unregisterd */
> +static struct security_hook_list selinux_sock_hooks[] = {
> +	LSM_HOOK_INIT(socket_sock_rcv_skb, selinux_socket_sock_rcv_skb),
> +};
> +
>  static __init int selinux_init(void)
>  {
>  	if (!security_module_enable("selinux")) {
> @@ -6240,7 +6252,9 @@ static struct nf_hook_ops selinux_nf_ops[] = {
>  #endif	/* IPV6 */
>  };
>  
> -static int __init selinux_nf_ip_init(void)
> +static bool nf_hooks_registered;
> +
> +static int selinux_nf_ip_init(void)
>  {
>  	int err;
>  
> @@ -6253,25 +6267,21 @@ static int __init selinux_nf_ip_init(void)
>  	if (err)
>  		panic("SELinux: nf_register_hooks: error %d\n", err);
>  
> +	nf_hooks_registered = true;
>  	return 0;
>  }
>  
> -__initcall(selinux_nf_ip_init);
> -
> -#ifdef CONFIG_SECURITY_SELINUX_DISABLE
>  static void selinux_nf_ip_exit(void)
>  {
>  	printk(KERN_DEBUG "SELinux:  Unregistering netfilter hooks\n");
>  
>  	nf_unregister_hooks(selinux_nf_ops, ARRAY_SIZE(selinux_nf_ops));
> +	nf_hooks_registered = false;
>  }
> -#endif
>  
>  #else /* CONFIG_NETFILTER */
>  
> -#ifdef CONFIG_SECURITY_SELINUX_DISABLE
>  #define selinux_nf_ip_exit()
> -#endif
>  
>  #endif /* CONFIG_NETFILTER */
>  
> @@ -6300,8 +6310,8 @@ int selinux_disable(void)
>  	/* Try to destroy the avc node cache */
>  	avc_disable();
>  
> -	/* Unregister netfilter hooks. */
> -	selinux_nf_ip_exit();
> +	/* Unregister net hooks. */
> +	selinux_net_update();
>  
>  	/* Unregister selinuxfs. */
>  	exit_sel_fs();
> @@ -6309,3 +6319,45 @@ int selinux_disable(void)
>  	return 0;
>  }
>  #endif
> +
> +DEFINE_MUTEX(selinux_net_mutex);
> +
> +static bool nf_hooks_required(void)
> +{
> +	return (selinux_secmark_enabled() || selinux_peerlbl_enabled() ||
> +		!selinux_policycap_netpeer) && selinux_enabled;
> +}
> +
> +static bool sock_hooks_required(void)
> +{
> +	return (!selinux_policycap_netpeer || selinux_secmark_enabled() ||
> +		selinux_peerlbl_enabled()) && selinux_enabled;
> +}
> +
> +static bool sock_hooks_registered;
> +
> +void selinux_net_update(void)
> +{
> +	if ((nf_hooks_required() == nf_hooks_registered) &&
> +	    (sock_hooks_required() == sock_hooks_registered))
> +		return;
> +
> +	mutex_lock(&selinux_net_mutex);
> +	if (nf_hooks_required() != nf_hooks_registered) {
> +		if (!nf_hooks_registered)
> +			selinux_nf_ip_init();
> +		else
> +			selinux_nf_ip_exit();
> +	}
> +
> +	if (sock_hooks_required() != sock_hooks_registered) {
> +		if (!sock_hooks_registered)
> +			security_add_hooks(selinux_sock_hooks,
> +					   ARRAY_SIZE(selinux_sock_hooks));
> +		else
> +			security_delete_hooks(selinux_sock_hooks,
> +					      ARRAY_SIZE(selinux_sock_hooks));
> +		sock_hooks_registered = !sock_hooks_registered;
> +	}
> +	mutex_unlock(&selinux_net_mutex);
> +}
> diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
> index 38feb55..0428ab4 100644
> --- a/security/selinux/include/security.h
> +++ b/security/selinux/include/security.h
> @@ -261,6 +261,7 @@ extern void selinux_status_update_setenforce(int enforcing);
>  extern void selinux_status_update_policyload(int seqno);
>  extern void selinux_complete_init(void);
>  extern int selinux_disable(void);
> +extern void selinux_net_update(void);
>  extern void exit_sel_fs(void);
>  extern struct path selinux_null;
>  extern struct vfsmount *selinuxfs_mount;
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index ebda973..c509018 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -2016,6 +2016,7 @@ static void security_load_policycaps(void)
>  						  POLICYDB_CAPABILITY_OPENPERM);
>  	selinux_policycap_alwaysnetwork = ebitmap_get_bit(&policydb.policycaps,
>  						  POLICYDB_CAPABILITY_ALWAYSNETWORK);
> +	selinux_net_update();
>  }
>  
>  static int security_preserve_bools(struct policydb *p);
> diff --git a/security/selinux/xfrm.c b/security/selinux/xfrm.c
> index 56e354f..cc2b0d4 100644
> --- a/security/selinux/xfrm.c
> +++ b/security/selinux/xfrm.c
> @@ -112,6 +112,7 @@ static int selinux_xfrm_alloc_user(struct xfrm_sec_ctx **ctxp,
>  
>  	*ctxp = ctx;
>  	atomic_inc(&selinux_xfrm_refcount);
> +	selinux_net_update();
>  	return 0;
>  
>  err:
> @@ -128,6 +129,7 @@ static void selinux_xfrm_free(struct xfrm_sec_ctx *ctx)
>  		return;
>  
>  	atomic_dec(&selinux_xfrm_refcount);
> +	selinux_net_update();
>  	kfree(ctx);
>  }
>  
> @@ -303,6 +305,7 @@ int selinux_xfrm_policy_clone(struct xfrm_sec_ctx *old_ctx,
>  	if (!new_ctx)
>  		return -ENOMEM;
>  	atomic_inc(&selinux_xfrm_refcount);
> +	selinux_net_update();
>  	*new_ctxp = new_ctx;
>  
>  	return 0;
> @@ -370,6 +373,7 @@ int selinux_xfrm_state_alloc_acquire(struct xfrm_state *x,
>  
>  	x->security = ctx;
>  	atomic_inc(&selinux_xfrm_refcount);
> +	selinux_net_update();
>  out:
>  	kfree(ctx_str);
>  	return rc;

Re: [RFC PATCH 0/2] selinux: avoid nf hooks overhead when not needed

[Paul Moore]

On Wed, Apr 6, 2016 at 6:14 PM, Florian Westphal <fw@strlen.de> wrote:
> netfilter hooks are per namespace -- so there is hook unregister when
> netns is destroyed.

Looking around, I see the global and per-namespace registration
functions (nf_register_hook and nf_register_net_hook, respectively),
but I'm looking to see if/how newly created namespace inherit
netfilter hooks from the init network namespace ... if you can create
a network namespace and dodge the SELinux hooks, that isn't a good
thing from a SELinux point of view, although it might be a plus
depending on where you view Paolo's original patches ;)

> Do you think it makes sense to rework the patch to delay registering
> of the netfiler hooks until the system is in a state where they're
> needed, without the 'unregister' aspect?

I would need to see the patch to say for certain, but in principle
that seems perfectly reasonable and I think would satisfy both the
netdev and SELinux camps - good suggestion.  My main goal is to drop
the selinux_nf_ip_init() entirely so it can't be used as a ROP gadget.

We might even be able to trim the secmark_active and peerlbl_active
checks in the SELinux netfilter hooks (an earlier attempt at
optimization; contrary to popular belief, I do care about SELinux
performance), although that would mean that enabling the network
access controls would be one way ... I guess you can disregard that
last bit, I'm thinking aloud again.

> Ideally this would even be per netns -- in perfect world we would
> be able to make it so that a new netns are created with an empty
> hook list.

In general SELinux doesn't care about namespaces, for reasons that are
sorta beyond the scope of this conversation, so I would like to stick
to a all or nothing approach to enabling the SELinux netfilter hooks
across namespaces.  Perhaps we can revisit this at a later time, but
let's keep it simple right now.

-- 
paul moore
www.paul-moore.com

Re: [RFC PATCH 0/2] selinux: avoid nf hooks overhead when not needed

[Florian Westphal]

Paul Moore <paul@paul-moore.com> wrote:
> On Wed, Apr 6, 2016 at 6:14 PM, Florian Westphal <fw@strlen.de> wrote:
> > netfilter hooks are per namespace -- so there is hook unregister when
> > netns is destroyed.
> 
> Looking around, I see the global and per-namespace registration
> functions (nf_register_hook and nf_register_net_hook, respectively),
> but I'm looking to see if/how newly created namespace inherit
> netfilter hooks from the init network namespace ... if you can create
> a network namespace and dodge the SELinux hooks, that isn't a good
> thing from a SELinux point of view, although it might be a plus
> depending on where you view Paolo's original patches ;)

Heh :-)

If you use nf_register_net_hook, the hook is only registered in the
namespace.

If you use nf_register_hook, the hook is put on a global list and
registed in all existing namespaces.

New namespaces will have the hook added as well (see
netfilter_net_init -> nf_register_hook_list in netfilter/core.c )

Since nf_register_hook is used it should be impossible to get a netns
that doesn't call these hooks.

> > Do you think it makes sense to rework the patch to delay registering
> > of the netfiler hooks until the system is in a state where they're
> > needed, without the 'unregister' aspect?
> 
> I would need to see the patch to say for certain, but in principle
> that seems perfectly reasonable and I think would satisfy both the
> netdev and SELinux camps - good suggestion.  My main goal is to drop
> the selinux_nf_ip_init() entirely so it can't be used as a ROP gadget.
> 
> We might even be able to trim the secmark_active and peerlbl_active
> checks in the SELinux netfilter hooks (an earlier attempt at
> optimization; contrary to popular belief, I do care about SELinux
> performance), although that would mean that enabling the network
> access controls would be one way ... I guess you can disregard that
> last bit, I'm thinking aloud again.

One way is fine I think.

> > Ideally this would even be per netns -- in perfect world we would
> > be able to make it so that a new netns are created with an empty
> > hook list.
> 
> In general SELinux doesn't care about namespaces, for reasons that are
> sorta beyond the scope of this conversation, so I would like to stick
> to a all or nothing approach to enabling the SELinux netfilter hooks
> across namespaces.  Perhaps we can revisit this at a later time, but
> let's keep it simple right now.

Okay, I'd prefer to stick to your recommendation anyway wrt. to selinux
(Casey, I read your comment regarding smack. Noted, we don't want to
break smack either...)

I think that in this case the entire question is:

In your experience, how likely is a config where
selinux is enabled BUT the hooks are not needed (i.e., where we hit the

if (!selinux_policycap_netpeer)
    return NF_ACCEPT;

if (!secmark_active && !peerlbl_active)
   return NF_ACCEPT;

tests inside the hooks)?  If such setups are uncommon we should just
drop this idea or at least put it on the back burner until the more expensive
netfilter hooks (conntrack, cough) are out of the way.

Thanks,
Florian

Re: [RFC PATCH 0/2] selinux: avoid nf hooks overhead when not needed

[Paolo Abeni]

Hi Casey,

On Wed, 2016-04-06 at 14:43 -0700, Casey Schaufler wrote:
> On 4/6/2016 2:51 AM, Paolo Abeni wrote:
> > Currently, selinux always registers iptables POSTROUTING hooks regarless of
> > the running policy needs for any action to be performed by them.
> >
> > Even the socket_sock_rcv_skb() is always registered, but it can result in a no-op
> > depending on the current policy configuration.
> >
> > The above invocations in the kernel datapath are cause of measurable
> > overhead in networking performance test.
> >
> > This patch series adds explicit notification for netlabel status change 
> > (other relevant status change, like xfrm and secmark, are already notified to
> > LSM) and use this information in selinux to register the above hooks only when
> > the current status makes them relevant, deregistering them when no-op
> >
> > Avoiding the LSM hooks overhead, in netperf UDP_STREAM test with small packets,
> > gives about 5% performance improvement on rx and about 8% on tx.
> >
> > Paolo Abeni (2):
> >   security: add hook for netlabel status change notification
> >   selinux: implement support for dynamic net hook [de-]registration

Thank you for the feedback. The patch series is an RFC, so it's still
rough and not yet well tested in all possible scenarios.

> Did you consider the fact that netlabel and the LSM socket
> hooks are used by Smack as well as SELinux? 

Actually yes. The patch series itself is explicitly targeted at reducing
some overhead introduced by selinux in network loads (I'm sorry, now I
see that the last sentence in the cover letter is misleading), and it
tries to achieve that result without affecting others LSM users.

The first patch in the series just introduces an optional LSM hook
(netlbl_changed) that is invoked every time the
'netlabel_mgmt_protocount' values is changed. It do not modify the
behavior nor meaning of any of the existing hooks and/or netlabel APIs.
It's up to the security module to leverage (or not) the new one.

> Did you measure the impact that your changes have on Smack? 

Actually I didn't. This is one of the reasons I posted the patch as RFC.
As per design security modules not implementing 'netlbl_changed' should
not be affected. Am I missing something ?

Regards,

Paolo

Re: [RFC PATCH 0/2] selinux: avoid nf hooks overhead when not needed

[Paul Moore]

On Thursday, April 07, 2016 01:45:32 AM Florian Westphal wrote:
> Paul Moore <paul@paul-moore.com> wrote:
> > On Wed, Apr 6, 2016 at 6:14 PM, Florian Westphal <fw@strlen.de> wrote:
> > > netfilter hooks are per namespace -- so there is hook unregister when
> > > netns is destroyed.
> > 
> > Looking around, I see the global and per-namespace registration
> > functions (nf_register_hook and nf_register_net_hook, respectively),
> > but I'm looking to see if/how newly created namespace inherit
> > netfilter hooks from the init network namespace ... if you can create
> > a network namespace and dodge the SELinux hooks, that isn't a good
> > thing from a SELinux point of view, although it might be a plus
> > depending on where you view Paolo's original patches ;)
> 
> Heh :-)
> 
> If you use nf_register_net_hook, the hook is only registered in the
> namespace.
> 
> If you use nf_register_hook, the hook is put on a global list and
> registed in all existing namespaces.
> 
> New namespaces will have the hook added as well (see
> netfilter_net_init -> nf_register_hook_list in netfilter/core.c )
>
> Since nf_register_hook is used it should be impossible to get a netns
> that doesn't call these hooks.

Great, thanks.
 
> > > Do you think it makes sense to rework the patch to delay registering
> > > of the netfiler hooks until the system is in a state where they're
> > > needed, without the 'unregister' aspect?
> > 
> > I would need to see the patch to say for certain, but in principle
> > that seems perfectly reasonable and I think would satisfy both the
> > netdev and SELinux camps - good suggestion.  My main goal is to drop
> > the selinux_nf_ip_init() entirely so it can't be used as a ROP gadget.
> > 
> > We might even be able to trim the secmark_active and peerlbl_active
> > checks in the SELinux netfilter hooks (an earlier attempt at
> > optimization; contrary to popular belief, I do care about SELinux
> > performance), although that would mean that enabling the network
> > access controls would be one way ... I guess you can disregard that
> > last bit, I'm thinking aloud again.
> 
> One way is fine I think.

Yes, just disregard my second paragraph above.
 
> > > Ideally this would even be per netns -- in perfect world we would
> > > be able to make it so that a new netns are created with an empty
> > > hook list.
> > 
> > In general SELinux doesn't care about namespaces, for reasons that are
> > sorta beyond the scope of this conversation, so I would like to stick
> > to a all or nothing approach to enabling the SELinux netfilter hooks
> > across namespaces.  Perhaps we can revisit this at a later time, but
> > let's keep it simple right now.
> 
> Okay, I'd prefer to stick to your recommendation anyway wrt. to selinux
> (Casey, I read your comment regarding smack. Noted, we don't want to
> break smack either...)
> 
> I think that in this case the entire question is:
> 
> In your experience, how likely is a config where selinux is enabled BUT the
> hooks are not needed (i.e., where we hit the
> 
> if (!selinux_policycap_netpeer)
>     return NF_ACCEPT;
> 
> if (!secmark_active && !peerlbl_active)
>    return NF_ACCEPT;
> 
> tests inside the hooks)?  If such setups are uncommon we should just
> drop this idea or at least put it on the back burner until the more
> expensive netfilter hooks (conntrack, cough) are out of the way.

A few years ago I would have said that it is relatively uncommon for admins to 
enable the SELinux network access controls; it was typically just 
government/intelligence agencies who had very strict access control 
requirements and represented a small portion of SELinux users.  However, over 
the past few years I've been fielding more and more questions from admins/devs 
in the virtualization space who are interested in some of these capabilities; 
it isn't clear to me how many of these people are switching it on, but there 
is definitely more interest than I have seen in the past and the interested is 
centered around some rather common use cases.

So, to summarize, I don't know ;)

If you've got bigger sources of overhead, my opinion would be to go tackle 
those first.  Perhaps I can even find the time to work on the 
SELinux/netfilter stuff while you are off slaying the bigger dragons, no 
promises at the moment.

-- 
paul moore
www.paul-moore.com

Re: [RFC PATCH 0/2] selinux: avoid nf hooks overhead when not needed

[Paolo Abeni]

On Thu, 2016-04-07 at 14:55 -0400, Paul Moore wrote:
> On Thursday, April 07, 2016 01:45:32 AM Florian Westphal wrote:
> > Paul Moore <paul@paul-moore.com> wrote:
> > > On Wed, Apr 6, 2016 at 6:14 PM, Florian Westphal <fw@strlen.de> wrote:
> > > > netfilter hooks are per namespace -- so there is hook unregister when
> > > > netns is destroyed.
> > > 
> > > Looking around, I see the global and per-namespace registration
> > > functions (nf_register_hook and nf_register_net_hook, respectively),
> > > but I'm looking to see if/how newly created namespace inherit
> > > netfilter hooks from the init network namespace ... if you can create
> > > a network namespace and dodge the SELinux hooks, that isn't a good
> > > thing from a SELinux point of view, although it might be a plus
> > > depending on where you view Paolo's original patches ;)
> > 
> > Heh :-)
> > 
> > If you use nf_register_net_hook, the hook is only registered in the
> > namespace.
> > 
> > If you use nf_register_hook, the hook is put on a global list and
> > registed in all existing namespaces.
> > 
> > New namespaces will have the hook added as well (see
> > netfilter_net_init -> nf_register_hook_list in netfilter/core.c )
> >
> > Since nf_register_hook is used it should be impossible to get a netns
> > that doesn't call these hooks.
> 
> Great, thanks.
>  
> > > > Do you think it makes sense to rework the patch to delay registering
> > > > of the netfiler hooks until the system is in a state where they're
> > > > needed, without the 'unregister' aspect?
> > > 
> > > I would need to see the patch to say for certain, but in principle
> > > that seems perfectly reasonable and I think would satisfy both the
> > > netdev and SELinux camps - good suggestion.  My main goal is to drop
> > > the selinux_nf_ip_init() entirely so it can't be used as a ROP gadget.
> > > 
> > > We might even be able to trim the secmark_active and peerlbl_active
> > > checks in the SELinux netfilter hooks (an earlier attempt at
> > > optimization; contrary to popular belief, I do care about SELinux
> > > performance), although that would mean that enabling the network
> > > access controls would be one way ... I guess you can disregard that
> > > last bit, I'm thinking aloud again.
> > 
> > One way is fine I think.
> 
> Yes, just disregard my second paragraph above.
>  
> > > > Ideally this would even be per netns -- in perfect world we would
> > > > be able to make it so that a new netns are created with an empty
> > > > hook list.
> > > 
> > > In general SELinux doesn't care about namespaces, for reasons that are
> > > sorta beyond the scope of this conversation, so I would like to stick
> > > to a all or nothing approach to enabling the SELinux netfilter hooks
> > > across namespaces.  Perhaps we can revisit this at a later time, but
> > > let's keep it simple right now.
> > 
> > Okay, I'd prefer to stick to your recommendation anyway wrt. to selinux
> > (Casey, I read your comment regarding smack. Noted, we don't want to
> > break smack either...)
> > 
> > I think that in this case the entire question is:
> > 
> > In your experience, how likely is a config where selinux is enabled BUT the
> > hooks are not needed (i.e., where we hit the
> > 
> > if (!selinux_policycap_netpeer)
> >     return NF_ACCEPT;
> > 
> > if (!secmark_active && !peerlbl_active)
> >    return NF_ACCEPT;
> > 
> > tests inside the hooks)?  If such setups are uncommon we should just
> > drop this idea or at least put it on the back burner until the more
> > expensive netfilter hooks (conntrack, cough) are out of the way.
> 
> A few years ago I would have said that it is relatively uncommon for admins to 
> enable the SELinux network access controls; it was typically just 
> government/intelligence agencies who had very strict access control 
> requirements and represented a small portion of SELinux users.  However, over 
> the past few years I've been fielding more and more questions from admins/devs 
> in the virtualization space who are interested in some of these capabilities; 
> it isn't clear to me how many of these people are switching it on, but there 
> is definitely more interest than I have seen in the past and the interested is 
> centered around some rather common use cases.
> 
> So, to summarize, I don't know ;)
> 
> If you've got bigger sources of overhead, my opinion would be to go tackle 
> those first.  Perhaps I can even find the time to work on the 
> SELinux/netfilter stuff while you are off slaying the bigger dragons, no 
> promises at the moment.

Double checking if I got the above correctly.

Will be ok if we post a v2 version of this series, removing the hooks
de-registration bits, but preserving the selinux nf-hooks and
socket_sock_rcv_skb() on-demand/delayed registration ? Will that fit
with the post-init read only memory usage that you are planning ?

Regards,

Paolo

Re: [RFC PATCH 0/2] selinux: avoid nf hooks overhead when not needed

[Casey Schaufler]

On 4/12/2016 1:52 AM, Paolo Abeni wrote:
> On Thu, 2016-04-07 at 14:55 -0400, Paul Moore wrote:
>> On Thursday, April 07, 2016 01:45:32 AM Florian Westphal wrote:
>>> Paul Moore <paul@paul-moore.com> wrote:
>>>> On Wed, Apr 6, 2016 at 6:14 PM, Florian Westphal <fw@strlen.de> wrote:
>>>>> netfilter hooks are per namespace -- so there is hook unregister when
>>>>> netns is destroyed.
>>>> Looking around, I see the global and per-namespace registration
>>>> functions (nf_register_hook and nf_register_net_hook, respectively),
>>>> but I'm looking to see if/how newly created namespace inherit
>>>> netfilter hooks from the init network namespace ... if you can create
>>>> a network namespace and dodge the SELinux hooks, that isn't a good
>>>> thing from a SELinux point of view, although it might be a plus
>>>> depending on where you view Paolo's original patches ;)
>>> Heh :-)
>>>
>>> If you use nf_register_net_hook, the hook is only registered in the
>>> namespace.
>>>
>>> If you use nf_register_hook, the hook is put on a global list and
>>> registed in all existing namespaces.
>>>
>>> New namespaces will have the hook added as well (see
>>> netfilter_net_init -> nf_register_hook_list in netfilter/core.c )
>>>
>>> Since nf_register_hook is used it should be impossible to get a netns
>>> that doesn't call these hooks.
>> Great, thanks.
>>  
>>>>> Do you think it makes sense to rework the patch to delay registering
>>>>> of the netfiler hooks until the system is in a state where they're
>>>>> needed, without the 'unregister' aspect?
>>>> I would need to see the patch to say for certain, but in principle
>>>> that seems perfectly reasonable and I think would satisfy both the
>>>> netdev and SELinux camps - good suggestion.  My main goal is to drop
>>>> the selinux_nf_ip_init() entirely so it can't be used as a ROP gadget.
>>>>
>>>> We might even be able to trim the secmark_active and peerlbl_active
>>>> checks in the SELinux netfilter hooks (an earlier attempt at
>>>> optimization; contrary to popular belief, I do care about SELinux
>>>> performance), although that would mean that enabling the network
>>>> access controls would be one way ... I guess you can disregard that
>>>> last bit, I'm thinking aloud again.
>>> One way is fine I think.
>> Yes, just disregard my second paragraph above.
>>  
>>>>> Ideally this would even be per netns -- in perfect world we would
>>>>> be able to make it so that a new netns are created with an empty
>>>>> hook list.
>>>> In general SELinux doesn't care about namespaces, for reasons that are
>>>> sorta beyond the scope of this conversation, so I would like to stick
>>>> to a all or nothing approach to enabling the SELinux netfilter hooks
>>>> across namespaces.  Perhaps we can revisit this at a later time, but
>>>> let's keep it simple right now.
>>> Okay, I'd prefer to stick to your recommendation anyway wrt. to selinux
>>> (Casey, I read your comment regarding smack. Noted, we don't want to
>>> break smack either...)
>>>
>>> I think that in this case the entire question is:
>>>
>>> In your experience, how likely is a config where selinux is enabled BUT the
>>> hooks are not needed (i.e., where we hit the
>>>
>>> if (!selinux_policycap_netpeer)
>>>     return NF_ACCEPT;
>>>
>>> if (!secmark_active && !peerlbl_active)
>>>    return NF_ACCEPT;
>>>
>>> tests inside the hooks)?  If such setups are uncommon we should just
>>> drop this idea or at least put it on the back burner until the more
>>> expensive netfilter hooks (conntrack, cough) are out of the way.
>> A few years ago I would have said that it is relatively uncommon for admins to 
>> enable the SELinux network access controls; it was typically just 
>> government/intelligence agencies who had very strict access control 
>> requirements and represented a small portion of SELinux users.  However, over 
>> the past few years I've been fielding more and more questions from admins/devs 
>> in the virtualization space who are interested in some of these capabilities; 
>> it isn't clear to me how many of these people are switching it on, but there 
>> is definitely more interest than I have seen in the past and the interested is 
>> centered around some rather common use cases.
>>
>> So, to summarize, I don't know ;)
>>
>> If you've got bigger sources of overhead, my opinion would be to go tackle 
>> those first.  Perhaps I can even find the time to work on the 
>> SELinux/netfilter stuff while you are off slaying the bigger dragons, no 
>> promises at the moment.
> Double checking if I got the above correctly.
>
> Will be ok if we post a v2 version of this series, removing the hooks
> de-registration bits, but preserving the selinux nf-hooks and
> socket_sock_rcv_skb() on-demand/delayed registration ?

Imagine that I have two security modules that control sockets.
The work I'm knee deep in will allow this. If adding hooks after
the init phase is allowed you have to face the possibility that
blob sizes (in this case sock->sk_security) may change. That
requires checking on every hook that uses blobs to determine
whether the blob has data for all the modules using it. I know
that that is a simple matter of arithmetic, but it's additional
overhead on every hook call. It also makes creating kmem caches
for security blobs much more difficult. Another performance
optimization that becomes unavailable.

We know of a number of ways we can improve networking performance
in the face of security modules. Many would make the code a whole
lot cleaner. Your proposed change is clever, but targets one case
at the expense of the general case. If there really is concern
about the performance of networking in the presence of security
modules I would suggest that we revisit some of the changes that
have already been proposed.

>  Will that fit
> with the post-init read only memory usage that you are planning ?
>
> Regards,
>
> Paolo
>
> --
> 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
>

Re: [RFC PATCH 0/2] selinux: avoid nf hooks overhead when not needed

[Paolo Abeni]

On Tue, 2016-04-12 at 06:57 -0700, Casey Schaufler wrote:
> On 4/12/2016 1:52 AM, Paolo Abeni wrote:
> > On Thu, 2016-04-07 at 14:55 -0400, Paul Moore wrote:
> >> On Thursday, April 07, 2016 01:45:32 AM Florian Westphal wrote:
> >>> Paul Moore <paul@paul-moore.com> wrote:
> >>>> On Wed, Apr 6, 2016 at 6:14 PM, Florian Westphal <fw@strlen.de> wrote:
> >>>>> netfilter hooks are per namespace -- so there is hook unregister when
> >>>>> netns is destroyed.
> >>>> Looking around, I see the global and per-namespace registration
> >>>> functions (nf_register_hook and nf_register_net_hook, respectively),
> >>>> but I'm looking to see if/how newly created namespace inherit
> >>>> netfilter hooks from the init network namespace ... if you can create
> >>>> a network namespace and dodge the SELinux hooks, that isn't a good
> >>>> thing from a SELinux point of view, although it might be a plus
> >>>> depending on where you view Paolo's original patches ;)
> >>> Heh :-)
> >>>
> >>> If you use nf_register_net_hook, the hook is only registered in the
> >>> namespace.
> >>>
> >>> If you use nf_register_hook, the hook is put on a global list and
> >>> registed in all existing namespaces.
> >>>
> >>> New namespaces will have the hook added as well (see
> >>> netfilter_net_init -> nf_register_hook_list in netfilter/core.c )
> >>>
> >>> Since nf_register_hook is used it should be impossible to get a netns
> >>> that doesn't call these hooks.
> >> Great, thanks.
> >>  
> >>>>> Do you think it makes sense to rework the patch to delay registering
> >>>>> of the netfiler hooks until the system is in a state where they're
> >>>>> needed, without the 'unregister' aspect?
> >>>> I would need to see the patch to say for certain, but in principle
> >>>> that seems perfectly reasonable and I think would satisfy both the
> >>>> netdev and SELinux camps - good suggestion.  My main goal is to drop
> >>>> the selinux_nf_ip_init() entirely so it can't be used as a ROP gadget.
> >>>>
> >>>> We might even be able to trim the secmark_active and peerlbl_active
> >>>> checks in the SELinux netfilter hooks (an earlier attempt at
> >>>> optimization; contrary to popular belief, I do care about SELinux
> >>>> performance), although that would mean that enabling the network
> >>>> access controls would be one way ... I guess you can disregard that
> >>>> last bit, I'm thinking aloud again.
> >>> One way is fine I think.
> >> Yes, just disregard my second paragraph above.
> >>  
> >>>>> Ideally this would even be per netns -- in perfect world we would
> >>>>> be able to make it so that a new netns are created with an empty
> >>>>> hook list.
> >>>> In general SELinux doesn't care about namespaces, for reasons that are
> >>>> sorta beyond the scope of this conversation, so I would like to stick
> >>>> to a all or nothing approach to enabling the SELinux netfilter hooks
> >>>> across namespaces.  Perhaps we can revisit this at a later time, but
> >>>> let's keep it simple right now.
> >>> Okay, I'd prefer to stick to your recommendation anyway wrt. to selinux
> >>> (Casey, I read your comment regarding smack. Noted, we don't want to
> >>> break smack either...)
> >>>
> >>> I think that in this case the entire question is:
> >>>
> >>> In your experience, how likely is a config where selinux is enabled BUT the
> >>> hooks are not needed (i.e., where we hit the
> >>>
> >>> if (!selinux_policycap_netpeer)
> >>>     return NF_ACCEPT;
> >>>
> >>> if (!secmark_active && !peerlbl_active)
> >>>    return NF_ACCEPT;
> >>>
> >>> tests inside the hooks)?  If such setups are uncommon we should just
> >>> drop this idea or at least put it on the back burner until the more
> >>> expensive netfilter hooks (conntrack, cough) are out of the way.
> >> A few years ago I would have said that it is relatively uncommon for admins to 
> >> enable the SELinux network access controls; it was typically just 
> >> government/intelligence agencies who had very strict access control 
> >> requirements and represented a small portion of SELinux users.  However, over 
> >> the past few years I've been fielding more and more questions from admins/devs 
> >> in the virtualization space who are interested in some of these capabilities; 
> >> it isn't clear to me how many of these people are switching it on, but there 
> >> is definitely more interest than I have seen in the past and the interested is 
> >> centered around some rather common use cases.
> >>
> >> So, to summarize, I don't know ;)
> >>
> >> If you've got bigger sources of overhead, my opinion would be to go tackle 
> >> those first.  Perhaps I can even find the time to work on the 
> >> SELinux/netfilter stuff while you are off slaying the bigger dragons, no 
> >> promises at the moment.
> > Double checking if I got the above correctly.
> >
> > Will be ok if we post a v2 version of this series, removing the hooks
> > de-registration bits, but preserving the selinux nf-hooks and
> > socket_sock_rcv_skb() on-demand/delayed registration ?
> 
> Imagine that I have two security modules that control sockets.
> The work I'm knee deep in will allow this. If adding hooks after
> the init phase is allowed you have to face the possibility that
> blob sizes (in this case sock->sk_security) may change. That
> requires checking on every hook that uses blobs to determine
> whether the blob has data for all the modules using it. I know
> that that is a simple matter of arithmetic, but it's additional
> overhead on every hook call. It also makes creating kmem caches
> for security blobs much more difficult. Another performance
> optimization that becomes unavailable.

I got your point.

Without seeing the code, I wonder if the above scenario could be covered
always allocating a blob large enough for all concurrent security
modules, i.e. each security module always declares/requests/allocates
space into the blobs regardless of it does not have registered (yet)
some security hooks, trading memory usage for performance.

Paolo

Re: [RFC PATCH 0/2] selinux: avoid nf hooks overhead when not needed

[Casey Schaufler]

On 4/13/2016 4:57 AM, Paolo Abeni wrote:
> On Tue, 2016-04-12 at 06:57 -0700, Casey Schaufler wrote:
>> On 4/12/2016 1:52 AM, Paolo Abeni wrote:
>>> On Thu, 2016-04-07 at 14:55 -0400, Paul Moore wrote:
>>>> On Thursday, April 07, 2016 01:45:32 AM Florian Westphal wrote:
>>>>> Paul Moore <paul@paul-moore.com> wrote:
>>>>>> On Wed, Apr 6, 2016 at 6:14 PM, Florian Westphal <fw@strlen.de> wrote:
>>>>>>> netfilter hooks are per namespace -- so there is hook unregister when
>>>>>>> netns is destroyed.
>>>>>> Looking around, I see the global and per-namespace registration
>>>>>> functions (nf_register_hook and nf_register_net_hook, respectively),
>>>>>> but I'm looking to see if/how newly created namespace inherit
>>>>>> netfilter hooks from the init network namespace ... if you can create
>>>>>> a network namespace and dodge the SELinux hooks, that isn't a good
>>>>>> thing from a SELinux point of view, although it might be a plus
>>>>>> depending on where you view Paolo's original patches ;)
>>>>> Heh :-)
>>>>>
>>>>> If you use nf_register_net_hook, the hook is only registered in the
>>>>> namespace.
>>>>>
>>>>> If you use nf_register_hook, the hook is put on a global list and
>>>>> registed in all existing namespaces.
>>>>>
>>>>> New namespaces will have the hook added as well (see
>>>>> netfilter_net_init -> nf_register_hook_list in netfilter/core.c )
>>>>>
>>>>> Since nf_register_hook is used it should be impossible to get a netns
>>>>> that doesn't call these hooks.
>>>> Great, thanks.
>>>>  
>>>>>>> Do you think it makes sense to rework the patch to delay registering
>>>>>>> of the netfiler hooks until the system is in a state where they're
>>>>>>> needed, without the 'unregister' aspect?
>>>>>> I would need to see the patch to say for certain, but in principle
>>>>>> that seems perfectly reasonable and I think would satisfy both the
>>>>>> netdev and SELinux camps - good suggestion.  My main goal is to drop
>>>>>> the selinux_nf_ip_init() entirely so it can't be used as a ROP gadget.
>>>>>>
>>>>>> We might even be able to trim the secmark_active and peerlbl_active
>>>>>> checks in the SELinux netfilter hooks (an earlier attempt at
>>>>>> optimization; contrary to popular belief, I do care about SELinux
>>>>>> performance), although that would mean that enabling the network
>>>>>> access controls would be one way ... I guess you can disregard that
>>>>>> last bit, I'm thinking aloud again.
>>>>> One way is fine I think.
>>>> Yes, just disregard my second paragraph above.
>>>>  
>>>>>>> Ideally this would even be per netns -- in perfect world we would
>>>>>>> be able to make it so that a new netns are created with an empty
>>>>>>> hook list.
>>>>>> In general SELinux doesn't care about namespaces, for reasons that are
>>>>>> sorta beyond the scope of this conversation, so I would like to stick
>>>>>> to a all or nothing approach to enabling the SELinux netfilter hooks
>>>>>> across namespaces.  Perhaps we can revisit this at a later time, but
>>>>>> let's keep it simple right now.
>>>>> Okay, I'd prefer to stick to your recommendation anyway wrt. to selinux
>>>>> (Casey, I read your comment regarding smack. Noted, we don't want to
>>>>> break smack either...)
>>>>>
>>>>> I think that in this case the entire question is:
>>>>>
>>>>> In your experience, how likely is a config where selinux is enabled BUT the
>>>>> hooks are not needed (i.e., where we hit the
>>>>>
>>>>> if (!selinux_policycap_netpeer)
>>>>>     return NF_ACCEPT;
>>>>>
>>>>> if (!secmark_active && !peerlbl_active)
>>>>>    return NF_ACCEPT;
>>>>>
>>>>> tests inside the hooks)?  If such setups are uncommon we should just
>>>>> drop this idea or at least put it on the back burner until the more
>>>>> expensive netfilter hooks (conntrack, cough) are out of the way.
>>>> A few years ago I would have said that it is relatively uncommon for admins to 
>>>> enable the SELinux network access controls; it was typically just 
>>>> government/intelligence agencies who had very strict access control 
>>>> requirements and represented a small portion of SELinux users.  However, over 
>>>> the past few years I've been fielding more and more questions from admins/devs 
>>>> in the virtualization space who are interested in some of these capabilities; 
>>>> it isn't clear to me how many of these people are switching it on, but there 
>>>> is definitely more interest than I have seen in the past and the interested is 
>>>> centered around some rather common use cases.
>>>>
>>>> So, to summarize, I don't know ;)
>>>>
>>>> If you've got bigger sources of overhead, my opinion would be to go tackle 
>>>> those first.  Perhaps I can even find the time to work on the 
>>>> SELinux/netfilter stuff while you are off slaying the bigger dragons, no 
>>>> promises at the moment.
>>> Double checking if I got the above correctly.
>>>
>>> Will be ok if we post a v2 version of this series, removing the hooks
>>> de-registration bits, but preserving the selinux nf-hooks and
>>> socket_sock_rcv_skb() on-demand/delayed registration ?
>> Imagine that I have two security modules that control sockets.
>> The work I'm knee deep in will allow this. If adding hooks after
>> the init phase is allowed you have to face the possibility that
>> blob sizes (in this case sock->sk_security) may change. That
>> requires checking on every hook that uses blobs to determine
>> whether the blob has data for all the modules using it. I know
>> that that is a simple matter of arithmetic, but it's additional
>> overhead on every hook call. It also makes creating kmem caches
>> for security blobs much more difficult. Another performance
>> optimization that becomes unavailable.
> I got your point.
>
> Without seeing the code, I wonder if the above scenario could be covered
> always allocating a blob large enough for all concurrent security
> modules, i.e. each security module always declares/requests/allocates
> space into the blobs regardless of it does not have registered (yet)
> some security hooks, trading memory usage for performance.

Yes, that would be possible. Unfortunately, many distros ship
kernels with multiple security modules compiled in, but only one
in use. That would make the common case the worst case. For the
near (1-2 years) future the common case will be one security
module using blobs in use with multiple modules compiled.

The LSM infrastructure was built on the assumption that
most systems would not use additional security modules.
That was true then. It isn't now.

> Paolo
>
> --
> 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
>

Re: [RFC PATCH 0/2] selinux: avoid nf hooks overhead when not needed

[Paul Moore]

On Tue, Apr 12, 2016 at 4:52 AM, Paolo Abeni <pabeni@redhat.com> wrote:
> Will be ok if we post a v2 version of this series, removing the hooks
> de-registration bits, but preserving the selinux nf-hooks and
> socket_sock_rcv_skb() on-demand/delayed registration ? Will that fit
> with the post-init read only memory usage that you are planning ?

The work Florian and and I were talking about would be limited just to
the netfilter hooks, the LSM hooks, e.g. socket_sock_rcv_skb() and
friends, would remain as they are today.  What what we discussing was
defaulting to not registering the netfilter hooks until it became
necessary due to a labeled networking configuration or the
always_check_network policy capability; the registration of the
netfilter hooks would be permanent, you could not unregister the hooks
at that point, you would need to reboot.  Does that make sense?

As far as Casey's concerns, I don't think the work we are talking
about for the v2 patchset would have any effect on the socket/sock
security blobs as you really can't manage those adequately from the
netfilter hooks; you most likely will reference them and perhaps even
update the data within, but not allocate or free the blobs.  Besides,
even in some weird case you were alloc/free'ing security blobs in the
netfilter hooks, we can deal with that on a per-LSM basis if/when the
full fledged stacking patches are merged; everything we are talking
about is a hidden implementation detail so changing it in the future
shouldn't be a problem.

-- 
paul moore
www.paul-moore.com

Re: [RFC PATCH 0/2] selinux: avoid nf hooks overhead when not needed

[Paolo Abeni]

On Thu, 2016-04-14 at 18:53 -0400, Paul Moore wrote:
> On Tue, Apr 12, 2016 at 4:52 AM, Paolo Abeni <pabeni@redhat.com> wrote:
> > Will be ok if we post a v2 version of this series, removing the hooks
> > de-registration bits, but preserving the selinux nf-hooks and
> > socket_sock_rcv_skb() on-demand/delayed registration ? Will that fit
> > with the post-init read only memory usage that you are planning ?
> 
> The work Florian and and I were talking about would be limited just to
> the netfilter hooks, the LSM hooks, e.g. socket_sock_rcv_skb() and
> friends, would remain as they are today.  What what we discussing was
> defaulting to not registering the netfilter hooks until it became
> necessary due to a labeled networking configuration or the
> always_check_network policy capability; the registration of the
> netfilter hooks would be permanent, you could not unregister the hooks
> at that point, you would need to reboot.  Does that make sense?

Yes, AFAIC it makes sense. I'll try to follow this route for an eventual
v2.

> As far as Casey's concerns, I don't think the work we are talking
> about for the v2 patchset would have any effect on the socket/sock
> security blobs as you really can't manage those adequately from the
> netfilter hooks; you most likely will reference them and perhaps even
> update the data within, but not allocate or free the blobs.  Besides,
> even in some weird case you were alloc/free'ing security blobs in the
> netfilter hooks, we can deal with that on a per-LSM basis if/when the
> full fledged stacking patches are merged; everything we are talking
> about is a hidden implementation detail so changing it in the future
> shouldn't be a problem.

Casey, are you ok with the above?

Thank you,

Paolo

Re: [RFC PATCH 0/2] selinux: avoid nf hooks overhead when not needed

[Casey Schaufler]

On 4/15/2016 2:38 AM, Paolo Abeni wrote:
> On Thu, 2016-04-14 at 18:53 -0400, Paul Moore wrote:
>> On Tue, Apr 12, 2016 at 4:52 AM, Paolo Abeni <pabeni@redhat.com> wrote:
>>> Will be ok if we post a v2 version of this series, removing the hooks
>>> de-registration bits, but preserving the selinux nf-hooks and
>>> socket_sock_rcv_skb() on-demand/delayed registration ? Will that fit
>>> with the post-init read only memory usage that you are planning ?
>> The work Florian and and I were talking about would be limited just to
>> the netfilter hooks, the LSM hooks, e.g. socket_sock_rcv_skb() and
>> friends, would remain as they are today.  What what we discussing was
>> defaulting to not registering the netfilter hooks until it became
>> necessary due to a labeled networking configuration or the
>> always_check_network policy capability; the registration of the
>> netfilter hooks would be permanent, you could not unregister the hooks
>> at that point, you would need to reboot.  Does that make sense?
> Yes, AFAIC it makes sense. I'll try to follow this route for an eventual
> v2.
>
>> As far as Casey's concerns, I don't think the work we are talking
>> about for the v2 patchset would have any effect on the socket/sock
>> security blobs as you really can't manage those adequately from the
>> netfilter hooks; you most likely will reference them and perhaps even
>> update the data within, but not allocate or free the blobs.  Besides,
>> even in some weird case you were alloc/free'ing security blobs in the
>> netfilter hooks, we can deal with that on a per-LSM basis if/when the
>> full fledged stacking patches are merged; everything we are talking
>> about is a hidden implementation detail so changing it in the future
>> shouldn't be a problem.
> Casey, are you ok with the above?

Yes. My concern is with the security module hooks. Altering
the netfilter hooks is a separate issue, and I don't have
trouble with that.

I also would not expect to see an LSM doing blob allocation
during socket delivery, but hey, it *is* networking code,
and stranger things happen all the time.

> Thank you,
>
> Paolo
>
>
> --
> 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
>