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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ