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

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]

Message-ID: <20181114224417.qwki54ozgoywfiap@breakpoint.cc>
Date:   Wed, 14 Nov 2018 23:44:17 +0100
From:   Florian Westphal <fw@...len.de>
To:     Colin Ian King <colin.king@...onical.com>
Cc:     Florian Westphal <fw@...len.de>,
        Steffen Klassert <steffen.klassert@...unet.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        "David S. Miller" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: xfrm: policy: add inexact policy search tree infrastructure

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