re: xfrm: policy: add inexact policy search tree infrastructure

re: xfrm: policy: add inexact policy search tree infrastructure

[Colin Ian King]

Hi,

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

commit 6be3b0db6db82cf056a72cc18042048edd27f8ee
Author: Florian Westphal <fw@strlen.de>
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

Re: xfrm: policy: add inexact policy search tree infrastructure

[Florian Westphal]

Colin Ian King <colin.king@canonical.com> wrote:
> Hi,
> 
> Static analysis with CoverityScan found a potential issue with the commit:
> 
> commit 6be3b0db6db82cf056a72cc18042048edd27f8ee
> Author: Florian Westphal <fw@strlen.de>
> 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.

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

> 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;
>
> 
> I suspect this is not intentional and is probably a bug.

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:

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -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 {

[PATCH ipsec-next] xfrm: policy: fix netlink/pf_key policy lookups

[Florian Westphal]

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

Fixes: 6be3b0db6db ("xfrm: policy: add inexact policy search tree infrastructure")
Reported-by: Colin Ian King <colin.king@canonical.com>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/xfrm/xfrm_policy.c                     |  5 ++-
 tools/testing/selftests/net/xfrm_policy.sh | 38 ++++++++++++++++++----
 2 files changed, 36 insertions(+), 7 deletions(-)

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index bd80b8a4322f..cff8c5b720b4 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -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 {
diff --git a/tools/testing/selftests/net/xfrm_policy.sh b/tools/testing/selftests/net/xfrm_policy.sh
index 6ca63a6e50e9..8db35b99457c 100755
--- a/tools/testing/selftests/net/xfrm_policy.sh
+++ b/tools/testing/selftests/net/xfrm_policy.sh
@@ -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

Re: [PATCH ipsec-next] xfrm: policy: fix netlink/pf_key policy lookups

[Steffen Klassert]

On Thu, Nov 15, 2018 at 02:51:57AM +0100, Florian Westphal wrote:
> 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
> 
> Fixes: 6be3b0db6db ("xfrm: policy: add inexact policy search tree infrastructure")
> Reported-by: Colin Ian King <colin.king@canonical.com>
> Signed-off-by: Florian Westphal <fw@strlen.de>

Applied, thanks everyone!