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