Skip to contentSkip to conversation

Fix the SELinux dynamic network access controls

PATCH In review Paul Moore 2 participants 5 messages May 29, 2013 – Jun 3, 2013

Subsystems (4)
Networking Network drivers SELinux security module Networking IPsec
lore
Share & tools
lore

Paul Moore

author
#0 lore raw

v3 cover Fix the SELinux dynamic network access controls

Take #3 ... two changes here: the first is a simple reordering of the two patches and the second is stripping the SELinux patch (2/2 in this patchset) of everything but the bare bones changes needed to fix the problem in the 3.10-rcX tree.

Enjoy.

---

Paul Moore (2):
xfrm: force a garbage collection after deleting a policy
selinux: fix the labeled xfrm/IPsec reference count handling

 include/net/xfrm.h      |    5 +++++
 net/key/af_key.c        |    4 ++++
 net/xfrm/xfrm_policy.c  |    3 ++-
 net/xfrm/xfrm_user.c    |    2 ++
 security/selinux/xfrm.c |   34 ++++++++++++++--------------------
 5 files changed, 27 insertions(+), 21 deletions(-)
author
#1 lore raw

PATCH v3 1/2 xfrm: force a garbage collection after deleting a policy

In some cases after deleting a policy from the SPD the policy would remain in the dst/flow/route cache for an extended period of time which caused problems for SELinux as its dynamic network access controls key off of the number of XFRM policy and state entries. This patch corrects this problem by forcing a XFRM garbage collection whenever a policy is sucessfully removed.

Reported-by Ondrej Moris <[email protected]>
Signed-off-by Paul Moore <[email protected]>
---
 include/net/xfrm.h     |    5 +++++
 net/key/af_key.c       |    4 ++++
 net/xfrm/xfrm_policy.c |    3 ++-
 net/xfrm/xfrm_user.c   |    2 ++
 4 files changed, 13 insertions(+), 1 deletion(-)
include/net/xfrm.h +5 -0
index ae16531..94ce082 100644
@@ -1160,6 +1160,8 @@ static inline void xfrm_sk_free_policy(struct sock *sk)
 	}
 }
 
+extern void xfrm_garbage_collect(struct net *net);
+
 #else
 
 static inline void xfrm_sk_free_policy(struct sock *sk) {}

@@ -1194,6 +1196,9 @@ static inline int xfrm6_policy_check_reverse(struct sock *sk, int dir,
 {
 	return 1;
 }
+static inline void xfrm_garbage_collect(struct net *net)
+{
+}
 #endif
 
 static __inline__
net/key/af_key.c +4 -0
index 5b1e5af..c5fbd75 100644
@@ -2366,6 +2366,8 @@ static int pfkey_spddelete(struct sock *sk, struct sk_buff *skb, const struct sa
 
 out:
 	xfrm_pol_put(xp);
+	if (err == 0)
+		xfrm_garbage_collect(net);
 	return err;
 }
 

@@ -2615,6 +2617,8 @@ static int pfkey_spdget(struct sock *sk, struct sk_buff *skb, const struct sadb_
 
 out:
 	xfrm_pol_put(xp);
+	if (delete && err == 0)
+		xfrm_garbage_collect(net);
 	return err;
 }
net/xfrm/xfrm_policy.c +2 -1
index 23cea0f..ea970b8 100644
@@ -2557,11 +2557,12 @@ static void __xfrm_garbage_collect(struct net *net)
 	}
 }
 
-static void xfrm_garbage_collect(struct net *net)
+void xfrm_garbage_collect(struct net *net)
 {
 	flow_cache_flush();
 	__xfrm_garbage_collect(net);
 }
+EXPORT_SYMBOL(xfrm_garbage_collect);
 
 static void xfrm_garbage_collect_deferred(struct net *net)
 {
net/xfrm/xfrm_user.c +2 -0
index aa77874..3f565e4 100644
@@ -1681,6 +1681,8 @@ static int xfrm_get_policy(struct sk_buff *skb, struct nlmsghdr *nlh,
 
 out:
 	xfrm_pol_put(xp);
+	if (delete && err == 0)
+		xfrm_garbage_collect(net);
 	return err;
 }
author
#2 lore raw

PATCH v3 2/2 selinux: fix the labeled xfrm/IPsec reference count handling

In reply to Paul Moore

The SELinux labeled IPsec code was improperly handling its reference counting, dropping a reference on a delete operation instead of on a free/release operation.

Reported-by Ondrej Moris <[email protected]>
Signed-off-by Paul Moore <[email protected]>
---
 security/selinux/xfrm.c |   34 ++++++++++++++--------------------
 1 file changed, 14 insertions(+), 20 deletions(-)
security/selinux/xfrm.c +14 -20
index 8ab2951..d030818 100644
@@ -316,6 +316,7 @@ int selinux_xfrm_policy_clone(struct xfrm_sec_ctx *old_ctx,
 
 		memcpy(new_ctx, old_ctx, sizeof(*new_ctx));
 		memcpy(new_ctx->ctx_str, old_ctx->ctx_str, new_ctx->ctx_len);
+		atomic_inc(&selinux_xfrm_refcount);
 		*new_ctxp = new_ctx;
 	}
 	return 0;

@@ -326,6 +327,7 @@ int selinux_xfrm_policy_clone(struct xfrm_sec_ctx *old_ctx,
  */
 void selinux_xfrm_policy_free(struct xfrm_sec_ctx *ctx)
 {
+	atomic_dec(&selinux_xfrm_refcount);
 	kfree(ctx);
 }
 

@@ -335,17 +337,13 @@ void selinux_xfrm_policy_free(struct xfrm_sec_ctx *ctx)
 int selinux_xfrm_policy_delete(struct xfrm_sec_ctx *ctx)
 {
 	const struct task_security_struct *tsec = current_security();
-	int rc = 0;
 
-	if (ctx) {
-		rc = avc_has_perm(tsec->sid, ctx->ctx_sid,
-				  SECCLASS_ASSOCIATION,
-				  ASSOCIATION__SETCONTEXT, NULL);
-		if (rc == 0)
-			atomic_dec(&selinux_xfrm_refcount);
-	}
+	if (!ctx)
+		return 0;
 
-	return rc;
+	return avc_has_perm(tsec->sid, ctx->ctx_sid,
+			    SECCLASS_ASSOCIATION, ASSOCIATION__SETCONTEXT,
+			    NULL);
 }
 
 /*

@@ -370,8 +368,8 @@ int selinux_xfrm_state_alloc(struct xfrm_state *x, struct xfrm_user_sec_ctx *uct
  */
 void selinux_xfrm_state_free(struct xfrm_state *x)
 {
-	struct xfrm_sec_ctx *ctx = x->security;
-	kfree(ctx);
+	atomic_dec(&selinux_xfrm_refcount);
+	kfree(x->security);
 }
 
  /*

@@ -381,17 +379,13 @@ int selinux_xfrm_state_delete(struct xfrm_state *x)
 {
 	const struct task_security_struct *tsec = current_security();
 	struct xfrm_sec_ctx *ctx = x->security;
-	int rc = 0;
 
-	if (ctx) {
-		rc = avc_has_perm(tsec->sid, ctx->ctx_sid,
-				  SECCLASS_ASSOCIATION,
-				  ASSOCIATION__SETCONTEXT, NULL);
-		if (rc == 0)
-			atomic_dec(&selinux_xfrm_refcount);
-	}
+	if (!ctx)
+		return 0;
 
-	return rc;
+	return avc_has_perm(tsec->sid, ctx->ctx_sid,
+			    SECCLASS_ASSOCIATION, ASSOCIATION__SETCONTEXT,
+			    NULL);
 }
 
 /*
maintainer
#3 lore raw
In reply to Paul Moore

From: Paul Moore <[email protected]>
Date: Wed, 29 May 2013 13:36:18 -0400

Quoted 4 lines

Take #3 ... two changes here: the first is a simple reordering of the two patches and the second is stripping the SELinux patch (2/2 in this patchset) of everything but the bare bones changes needed to fix the problem in the 3.10-rcX tree.

Series applied, but longer term I'd like to see this handled by generation counts or similar during lookup time.

Doing a GC every policy delete hurts policy delete performance, something we worked really hard to make as light weight as possible given the constraints.

Thanks.

author
#4 lore raw
Quoted 8 lines · David Miller

From: Paul Moore <[email protected]>
Date: Wed, 29 May 2013 13:36:18 -0400

Quoted 4 lines

Take #3 ... two changes here: the first is a simple reordering of the two patches and the second is stripping the SELinux patch (2/2 in this patchset) of everything but the bare bones changes needed to fix the problem in the 3.10-rcX tree.

Quoted 6 lines

Series applied, but longer term I'd like to see this handled by generation counts or similar during lookup time.

Doing a GC every policy delete hurts policy delete performance, something we worked really hard to make as light weight as possible given the constraints.

Thanks for pulling in the fixes for 3.10-rcX.

I promise to take a look at changing our approach to detecting an active labeled IPsec configuration for future releases. I need to cleanup our labeled IPsec code anyway, it is in need of some TLC (you saw some of the changes in the earlier version of this patchset).

Signature · 2 lines
paul moore
security and virtualization @ redhat