netdev - RE: -Wsometimes-uninitialized Clang warning in net/tipc/node.c

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <BL0PR1501MB2003762D0D1CB08622C9244F9A420@BL0PR1501MB2003.namprd15.prod.outlook.com>
Date:   Thu, 21 Mar 2019 14:57:21 +0000
From:   Jon Maloy <jon.maloy@...csson.com>
To:     Arnd Bergmann <arnd@...db.de>,
        Nick Desaulniers <ndesaulniers@...gle.com>
CC:     Nathan Chancellor <natechancellor@...il.com>,
        Ying Xue <ying.xue@...driver.com>,
        "David S. Miller" <davem@...emloft.net>,
        "tipc-discussion@...ts.sourceforge.net" 
        <tipc-discussion@...ts.sourceforge.net>,
        Networking <netdev@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        "clang-built-linux@...glegroups.com" 
        <clang-built-linux@...glegroups.com>
Subject: RE: -Wsometimes-uninitialized Clang warning in net/tipc/node.c



> -----Original Message-----
> From: Arnd Bergmann <arnd@...db.de>
> Sent: 21-Mar-19 12:45
> To: Nick Desaulniers <ndesaulniers@...gle.com>
> Cc: Nathan Chancellor <natechancellor@...il.com>; Jon Maloy
> <jon.maloy@...csson.com>; Ying Xue <ying.xue@...driver.com>; David S.
> Miller <davem@...emloft.net>; tipc-discussion@...ts.sourceforge.net;
> Networking <netdev@...r.kernel.org>; LKML <linux-
> kernel@...r.kernel.org>; clang-built-linux@...glegroups.com
> Subject: Re: -Wsometimes-uninitialized Clang warning in net/tipc/node.c
> 
> On Wed, Mar 20, 2019 at 9:51 PM 'Nick Desaulniers' via Clang Built Linux
> <clang-built-linux@...glegroups.com> wrote:
> > On Wed, Mar 20, 2019 at 12:07 PM Nathan Chancellor
> > <natechancellor@...il.com> wrote:
> >
> > The use in tipc_bearer_xmit() isn't even guaranteed to set the in/out
> > parameter, so even if the if is taken doesn't guarantee that maddr is
> > always initialized before calling tipc_bearer_xmit().
> 
> Right, it is only initialized in certain states. It was always initialized until
> commit 598411d70f85 ("tipc: make resetting of links non-atomic"),
> afterwards only if the link was not reset, and as of commit 73f646cec354
> ("tipc: delay ESTABLISH state event when link is established") only if it's not
> 'establishing' or 'reset'.
> 
> > At the minimum, we should initialize maddr to NULL.  I think we'd
> > prefer to risk the possibility of a null pointer dereference to the
> > possibility of working with uninitialized memory.  To be clear, both
> > are bad, but one is easier to spot/debug later than the other.
> 
> I disagree with setting it to NULL, given that it is still an obviously incorrect
> value. We could add a if(maddr) check before calling tipc_bearer_xmit(), but
> I think it would be clearer to check
> skb_queue_empty(xmitq)) if that avoids the warning:

I may be wrong, but I would be surprised if that would eliminate the warning.
To me, setting maddr to NULL and then testing for it looks both more comprehensible and is guaranteed to fix the warning.

> 
> diff --git a/net/tipc/node.c b/net/tipc/node.c index
> 2dc4919ab23c..147786795e48 100644
> --- a/net/tipc/node.c
> +++ b/net/tipc/node.c
> @@ -844,7 +844,8 @@ static void tipc_node_link_down(struct tipc_node *n,
> int bearer_id, bool delete)
>         tipc_node_write_unlock(n);
>         if (delete)
>                 tipc_mon_remove_peer(n->net, n->addr, old_bearer_id);
> -       tipc_bearer_xmit(n->net, bearer_id, &xmitq, maddr);
> +       if (skb_queue_empty(xmitq))
> +               tipc_bearer_xmit(n->net, bearer_id, &xmitq, maddr);
>         tipc_sk_rcv(n->net, &le->inputq);  }
> 
> This duplicates the check inside of skb_queue_empty(), but I don't know if
> the compiler can see through the logic behind the inlined function calls.

Probably not, but this is not in any way time critical.

///jon

> 
>       Arnd