netdev - [PATCH 2/3] xfrm: do unconditional template resolution before pcpu cache check

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 9 Nov 2017 02:40:46 +0100
From:   Steffen Klassert <steffen.klassert@...unet.com>
To:     David Miller <davem@...emloft.net>
CC:     Herbert Xu <herbert@...dor.apana.org.au>,
        Steffen Klassert <steffen.klassert@...unet.com>,
        <netdev@...r.kernel.org>
Subject: [PATCH 2/3] xfrm: do unconditional template resolution before pcpu cache check

From: Florian Westphal <fw@...len.de>

Stephen Smalley says:
 Since 4.14-rc1, the selinux-testsuite has been encountering sporadic
 failures during testing of labeled IPSEC. git bisect pointed to
 commit ec30d ("xfrm: add xdst pcpu cache").
 The xdst pcpu cache is only checking that the policies are the same,
 but does not validate that the policy, state, and flow match with respect
 to security context labeling.
 As a result, the wrong SA could be used and the receiver could end up
 performing permission checking and providing SO_PEERSEC or SCM_SECURITY
 values for the wrong security context.

This fix makes it so that we always do the template resolution, and
then checks that the found states match those in the pcpu bundle.

This has the disadvantage of doing a bit more work (lookup in state hash
table) if we can reuse the xdst entry (we only avoid xdst alloc/free)
but we don't add a lot of extra work in case we can't reuse.

xfrm_pol_dead() check is removed, reasoning is that
xfrm_tmpl_resolve does all needed checks.

Cc: Paul Moore <paul@...l-moore.com>
Fixes: ec30d78c14a813db39a647b6a348b428 ("xfrm: add xdst pcpu cache")
Reported-by: Stephen Smalley <sds@...ho.nsa.gov>
Tested-by: Stephen Smalley <sds@...ho.nsa.gov>
Signed-off-by: Florian Westphal <fw@...len.de>
Acked-by: Paul Moore <paul@...l-moore.com>
Signed-off-by: Steffen Klassert <steffen.klassert@...unet.com>
---
 net/xfrm/xfrm_policy.c | 42 ++++++++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 18 deletions(-)

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 8cafb3c..a2e531b 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -1787,19 +1787,23 @@ void xfrm_policy_cache_flush(void)
 	put_online_cpus();
 }
 
-static bool xfrm_pol_dead(struct xfrm_dst *xdst)
+static bool xfrm_xdst_can_reuse(struct xfrm_dst *xdst,
+				struct xfrm_state * const xfrm[],
+				int num)
 {
-	unsigned int num_pols = xdst->num_pols;
-	unsigned int pol_dead = 0, i;
+	const struct dst_entry *dst = &xdst->u.dst;
+	int i;
 
-	for (i = 0; i < num_pols; i++)
-		pol_dead |= xdst->pols[i]->walk.dead;
+	if (xdst->num_xfrms != num)
+		return false;
 
-	/* Mark DST_OBSOLETE_DEAD to fail the next xfrm_dst_check() */
-	if (pol_dead)
-		xdst->u.dst.obsolete = DST_OBSOLETE_DEAD;
+	for (i = 0; i < num; i++) {
+		if (!dst || dst->xfrm != xfrm[i])
+			return false;
+		dst = dst->child;
+	}
 
-	return pol_dead;
+	return xfrm_bundle_ok(xdst);
 }
 
 static struct xfrm_dst *
@@ -1813,26 +1817,28 @@ xfrm_resolve_and_create_bundle(struct xfrm_policy **pols, int num_pols,
 	struct dst_entry *dst;
 	int err;
 
+	/* Try to instantiate a bundle */
+	err = xfrm_tmpl_resolve(pols, num_pols, fl, xfrm, family);
+	if (err <= 0) {
+		if (err != 0 && err != -EAGAIN)
+			XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTPOLERROR);
+		return ERR_PTR(err);
+	}
+
 	xdst = this_cpu_read(xfrm_last_dst);
 	if (xdst &&
 	    xdst->u.dst.dev == dst_orig->dev &&
 	    xdst->num_pols == num_pols &&
-	    !xfrm_pol_dead(xdst) &&
 	    memcmp(xdst->pols, pols,
 		   sizeof(struct xfrm_policy *) * num_pols) == 0 &&
-	    xfrm_bundle_ok(xdst)) {
+	    xfrm_xdst_can_reuse(xdst, xfrm, err)) {
 		dst_hold(&xdst->u.dst);
+		while (err > 0)
+			xfrm_state_put(xfrm[--err]);
 		return xdst;
 	}
 
 	old = xdst;
-	/* Try to instantiate a bundle */
-	err = xfrm_tmpl_resolve(pols, num_pols, fl, xfrm, family);
-	if (err <= 0) {
-		if (err != 0 && err != -EAGAIN)
-			XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTPOLERROR);
-		return ERR_PTR(err);
-	}
 
 	dst = xfrm_bundle_create(pols[0], xfrm, err, fl, dst_orig);
 	if (IS_ERR(dst)) {
-- 
2.7.4