linux-kernel - Re: Coverity: __sock_gen_cookie(): Error handling issues

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: <CANn89iKgMvhLbTi=SHn41R--rBQ8As=E52Hnecch6nOhXVYGrg@mail.gmail.com>
Date:   Thu, 17 Nov 2022 15:22:22 -0800
From:   Eric Dumazet <edumazet@...gle.com>
To:     Kees Cook <keescook@...omium.org>
Cc:     linux-kernel@...r.kernel.org,
        Marc Kleine-Budde <mkl@...gutronix.de>,
        Paolo Abeni <pabeni@...hat.com>,
        Stefano Garzarella <sgarzare@...hat.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Nikolay Aleksandrov <nikolay@...dia.com>,
        "David S. Miller" <davem@...emloft.net>,
        Florian Fainelli <f.fainelli@...il.com>,
        netdev@...r.kernel.org,
        "Gustavo A. R. Silva" <gustavo@...eddedor.com>,
        linux-next@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: Coverity: __sock_gen_cookie(): Error handling issues

On Thu, Nov 17, 2022 at 3:14 PM Kees Cook <keescook@...omium.org> wrote:
>
> On Thu, Nov 17, 2022 at 02:49:55PM -0800, Eric Dumazet wrote:
> > On Thu, Nov 17, 2022 at 2:22 PM coverity-bot <keescook@...omium.org> wrote:
> > >
> > > Hello!
> > >
> > > This is an experimental semi-automated report about issues detected by
> > > Coverity from a scan of next-20221117 as part of the linux-next scan project:
> > > https://scan.coverity.com/projects/linux-next-weekly-scan
> > >
> > > You're getting this email because you were associated with the identified
> > > lines of code (noted below) that were touched by commits:
> > >
> > >   Wed Nov 16 12:42:01 2022 +0000
> > >     4ebf802cf1c6 ("net: __sock_gen_cookie() cleanup")
> > >
> > > Coverity reported the following:
> > >
> > > *** CID 1527347:  Error handling issues  (CHECKED_RETURN)
> > > net/core/sock_diag.c:33 in __sock_gen_cookie()
> > > 27     {
> > > 28      u64 res = atomic64_read(&sk->sk_cookie);
> > > 29
> > > 30      if (!res) {
> > > 31              u64 new = gen_cookie_next(&sock_cookie);
> > > 32
> > > vvv     CID 1527347:  Error handling issues  (CHECKED_RETURN)
> > > vvv     Calling "atomic64_try_cmpxchg" without checking return value (as is done elsewhere 8 out of 9 times).
> > > 33              atomic64_try_cmpxchg(&sk->sk_cookie, &res, new);
> >
> >
> > Hmmm. for some reason I thought @res was always updated...
> >
> > A fix would be to read sk->sk_cookie, but I guess your tool will still
> > complain we do not care
> > of  atomic64_try_cmpxchg() return value ?
> >
> > diff --git a/net/core/sock_diag.c b/net/core/sock_diag.c
> > index b11593cae5a09b15a10d6ba35bccc22263cb8fc8..58efb9c1c8dd4f8e5a3009a0176e1b96487daaff
> > 100644
> > --- a/net/core/sock_diag.c
> > +++ b/net/core/sock_diag.c
> > @@ -31,6 +31,10 @@ u64 __sock_gen_cookie(struct sock *sk)
> >                 u64 new = gen_cookie_next(&sock_cookie);
> >
> >                 atomic64_try_cmpxchg(&sk->sk_cookie, &res, new);
> > +               /* Another cpu/thread might have won the race,
> > +                * reload the final value.
> > +                */
> > +               res = atomic64_read(&sk->sk_cookie);
> >         }
> >         return res;
> >  }
>
> I think it's saying it was expecting an update loop -- i.e. to make sure
> the value actually got swapped (the "try" part...)?

The value has been updated, either by us or someone else.

We do not particularly care who won the race, since the value is
updated once only.