xfrm: policy: add inexact policy search tree infrastructure

Hi,

Static analysis with CoverityScan found a potential issue with the commit:

commit 6be3b0db6db82cf056a72cc18042048edd27f8ee

Author: Florian Westphal <[email protected]>
Date:   Wed Nov 7 23:00:37 2018 +0100

xfrm: policy: add inexact policy search tree infrastructure

It seems that pointer pol is set to NULL and then a check to see if it is non-null is used to set pol to tmp; howeverm this check is always going to be false because pol is always NULL.

The issue is reported by CoverityScan as follows:

Line
1658
assignment: Assigning: pol = NULL.
1659 pol = NULL;
1660 for (i = 0; i < ARRAY_SIZE(cand.res); i++) {
1661 struct xfrm_policy *tmp;
1662
1663 tmp = __xfrm_policy_bysel_ctx(cand.res[i], mark,
1664 if_id, type, dir,
1665 sel, ctx);

null: At condition pol, the value of pol must be NULL.
dead_error_condition: The condition pol cannot be true.

CID 1475480 (#1 of 1): Logically dead code

(DEADCODE) dead_error_line: Execution cannot reach the expression tmp->pos < pol->pos inside this statement: if (tmp && pol && tmp->pos ....

1666 if (tmp && pol && tmp->pos < pol->pos)
1667 pol = tmp;
1668 }

I suspect this is not intentional and is probably a bug.

Colin

Right. This is in the control-plane code to retrieve a policy via netlink or PF_KEY.

Right, bug. Would like to just break after first 'tmp != NULL' but that might make us return a different policy than old linear search. So we should update pol in case its NULL as well.

Steffen, let me know if you want an incremental fix or if you prefer to squash this:

@@ -1663,7 +1663,10 @@ struct xfrm_policy *xfrm_policy_bysel_ctx(struct net *net, u32 mark, u32 if_id,
 			tmp = __xfrm_policy_bysel_ctx(cand.res[i], mark,
 						      if_id, type, dir,
 						      sel, ctx);
-			if (tmp && pol && tmp->pos < pol->pos)
+			if (!tmp)
+				continue;
+
+			if (!pol || tmp->pos < pol->pol)
 				pol = tmp;
 		}
 	} else {

Colin Ian King says:
Static analysis with CoverityScan found a potential issue [..] It seems that pointer pol is set to NULL and then a check to see if it is non-null is used to set pol to tmp; howeverm this check is always going to be false because pol is always NULL.

Fix this and update test script to catch this. Updated script only:
./xfrm_policy.sh ; echo $?
RTNETLINK answers: No such file or directory
FAIL: ip -net ns3 xfrm policy get src 10.0.1.0/24 dst 10.0.2.0/24 dir out RTNETLINK answers: No such file or directory
[..]
PASS: policy before exception matches
PASS: ping to .254 bypassed ipsec tunnel
PASS: direct policy matches
PASS: policy matches
1

---
 net/xfrm/xfrm_policy.c                     |  5 ++-
 tools/testing/selftests/net/xfrm_policy.sh | 38 ++++++++++++++++++----
 2 files changed, 36 insertions(+), 7 deletions(-)

index bd80b8a4322f..cff8c5b720b4 100644
@@ -1663,7 +1663,10 @@ struct xfrm_policy *xfrm_policy_bysel_ctx(struct net *net, u32 mark, u32 if_id,
 			tmp = __xfrm_policy_bysel_ctx(cand.res[i], mark,
 						      if_id, type, dir,
 						      sel, ctx);
-			if (tmp && pol && tmp->pos < pol->pos)
+			if (!tmp)
+				continue;
+
+			if (!pol || tmp->pos < pol->pos)
 				pol = tmp;
 		}
 	} else {

index 6ca63a6e50e9..8db35b99457c 100755
@@ -21,6 +21,7 @@
 # Kselftest framework requirement - SKIP code is 4.
 ksft_skip=4
 ret=0
+policy_checks_ok=1
 
 KEY_SHA=0xdeadbeef1234567890abcdefabcdefabcdefabcd
 KEY_AES=0x0123456789abcdef0123456789012345

@@ -45,6 +46,26 @@ do_esp() {
     ip -net $ns xfrm policy add src $rnet dst $lnet dir fwd tmpl src $remote dst $me proto esp mode tunnel priority 100 action allow
 }
 
+do_esp_policy_get_check() {
+    local ns=$1
+    local lnet=$2
+    local rnet=$3
+
+    ip -net $ns xfrm policy get src $lnet dst $rnet dir out > /dev/null
+    if [ $? -ne 0 ] && [ $policy_checks_ok -eq 1 ] ;then
+        policy_checks_ok=0
+        echo "FAIL: ip -net $ns xfrm policy get src $lnet dst $rnet dir out"
+        ret=1
+    fi
+
+    ip -net $ns xfrm policy get src $rnet dst $lnet dir fwd > /dev/null
+    if [ $? -ne 0 ] && [ $policy_checks_ok -eq 1 ] ;then
+        policy_checks_ok=0
+        echo "FAIL: ip -net $ns xfrm policy get src $rnet dst $lnet dir fwd"
+        ret=1
+    fi
+}
+
 do_exception() {
     local ns=$1
     local me=$2

@@ -112,31 +133,31 @@ check_xfrm() {
 	# 1: iptables -m policy rule count != 0
 	rval=$1
 	ip=$2
-	ret=0
+	lret=0
 
 	ip netns exec ns1 ping -q -c 1 10.0.2.$ip > /dev/null
 
 	check_ipt_policy_count ns3
 	if [ $? -ne $rval ] ; then
-		ret=1
+		lret=1
 	fi
 	check_ipt_policy_count ns4
 	if [ $? -ne $rval ] ; then
-		ret=1
+		lret=1
 	fi
 
 	ip netns exec ns2 ping -q -c 1 10.0.1.$ip > /dev/null
 
 	check_ipt_policy_count ns3
 	if [ $? -ne $rval ] ; then
-		ret=1
+		lret=1
 	fi
 	check_ipt_policy_count ns4
 	if [ $? -ne $rval ] ; then
-		ret=1
+		lret=1
 	fi
 
-	return $ret
+	return $lret
 }
 
 #check for needed privileges

@@ -227,6 +248,11 @@ do_esp ns4 dead:3::10 dead:3::1 dead:2::/64 dead:1::/64 $SPI2 $SPI1
 do_dummies4 ns3
 do_dummies6 ns4
 
+do_esp_policy_get_check ns3 10.0.1.0/24 10.0.2.0/24
+do_esp_policy_get_check ns4 10.0.2.0/24 10.0.1.0/24
+do_esp_policy_get_check ns3 dead:1::/64 dead:2::/64
+do_esp_policy_get_check ns4 dead:2::/64 dead:1::/64
+
 # ping to .254 should use ipsec, exception is not installed.
 check_xfrm 1 254
 if [ $? -ne 0 ]; then

2.18.1

Applied, thanks everyone!