netdev - Re: gro: Make GRO aware of lightweight tunnels.

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:	Wed, 10 Feb 2016 13:29:40 +0100
From:	Jesse Gross <jesse@...nel.org>
To:	Dave Jones <davej@...emonkey.org.uk>
Cc:	Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: gro: Make GRO aware of lightweight tunnels.

On Tue, Feb 9, 2016 at 1:49 AM, Dave Jones <davej@...emonkey.org.uk> wrote:
> On Tue, Feb 02, 2016 at 02:28:58AM +0000, Linux Kernel wrote:
>  > Web:        https://git.kernel.org/torvalds/c/ce87fc6ce3f9f4488546187e3757cf666d9d4a2a
>  > Commit:     ce87fc6ce3f9f4488546187e3757cf666d9d4a2a
>  > Parent:     5f2f3cad8b878b23f17a11dd5af4f4a2cc41c797
>  > Refname:    refs/heads/master
>  > Author:     Jesse Gross <jesse@...nel.org>
>  > AuthorDate: Wed Jan 20 17:59:49 2016 -0800
>  > Committer:  David S. Miller <davem@...emloft.net>
>  > CommitDate: Wed Jan 20 18:48:38 2016 -0800
>  >
>  >     gro: Make GRO aware of lightweight tunnels.
>
> Coverity just started complaining about this commit.
>
>  > diff --git a/include/net/dst_metadata.h b/include/net/dst_metadata.h
>  > index 6816f0f..30a56ab 100644
>  > --- a/include/net/dst_metadata.h
>  > +++ b/include/net/dst_metadata.h
>  > @@ -44,6 +44,24 @@ static inline bool skb_valid_dst(const struct sk_buff *skb)
>  >      return dst && !(dst->flags & DST_METADATA);
>  >  }
>  >
>  > +static inline int skb_metadata_dst_cmp(const struct sk_buff *skb_a,
>  > +                                   const struct sk_buff *skb_b)
>  > +{
>  > +    const struct metadata_dst *a, *b;
>  > +
>  > +    if (!(skb_a->_skb_refdst | skb_b->_skb_refdst))
>  > +            return 0;
>  > +
>  > +    a = (const struct metadata_dst *) skb_dst(skb_a);
>  > +    b = (const struct metadata_dst *) skb_dst(skb_b);
>  > +
>  > +    if (!a != !b || a->u.tun_info.options_len != b->u.tun_info.options_len)
>
> It complains that we could dereference a null 'a' here.
>
> It's possible the checker got confused, but the first part of that
> expression looks odd, is that doing what's intended ?

I don't think that this can actually happen in practice.

We know that the combination of 'a' and 'b' is not NULL from the first
line in the function ('a' is simply 'skb_a->_skb_refdst' with a flag
knocked off and there is an earlier problem if this makes a difference
for the NULL check). The expression you mentioned is seeing if one is
NULL but not the other. If that is not the case then we can safely
dereference the pointers to do further comparisons.

I guess Coverity doesn't understand the relationship between the
pointer and the flag as handled in skb_dst().