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: <CANn89iJAZ+OYVebm-x4pJgjYTdj8RiXc8iLnQC8X6JC3RUywuA@mail.gmail.com>
Date: Mon, 8 May 2023 19:31:12 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: davem@...emloft.net, kuba@...nel.org, kuni1840@...il.com, 
	netdev@...r.kernel.org, pabeni@...hat.com, syzkaller@...glegroups.com
Subject: Re: [PATCH v2 net] net: Fix sk->sk_stamp race in sock_recv_cmsgs().

On Mon, May 8, 2023 at 7:20 PM Kuniyuki Iwashima <kuniyu@...zon.com> wrote:
>
> From: Eric Dumazet <edumazet@...gle.com>
> Date: Mon, 8 May 2023 19:08:58 +0200
> > On Mon, May 8, 2023 at 6:58 PM Kuniyuki Iwashima <kuniyu@...zon.com> wrote:
> > >
> > > KCSAN found a data race in sock_recv_cmsgs() [0] where the read access
> > > to sk->sk_stamp needs READ_ONCE().
> > >
> > > Also, there is another race like below.  If the torn load of the high
> > > 32-bits precedes WRITE_ONCE(sk, skb->tstamp) and later the written
> > > lower 32-bits happens to match with SK_DEFAULT_STAMP, the final result
> > > of sk->sk_stamp could be 0.
> > >
> > >   sock_recv_cmsgs()  ioctl(SIOCGSTAMP)      sock_recv_cmsgs()
> > >   |                  |                      |
> > >   |- if (sock_flag(sk, SOCK_TIMESTAMP))     |
> > >   |                  |                      |
> > >   |                  `- sock_set_flag(sk, SOCK_TIMESTAMP)
> > >   |                                         |
> > >   |                                          `- if (sock_flag(sk, SOCK_TIMESTAMP))
> > >   `- if (sk->sk_stamp == SK_DEFAULT_STAMP)      `- sock_write_timestamp(sk, skb->tstamp)
> > >       `- sock_write_timestamp(sk, 0)
> > >
> > > Even with READ_ONCE(), we could get the same result if READ_ONCE() precedes
> > > WRITE_ONCE() because the SK_DEFAULT_STAMP check and WRITE_ONCE(sk_stamp, 0)
> > > are not atomic.
> > >
> > > Let's avoid the race by cmpxchg() on 64-bits architecture or seqlock on
> > > 32-bits machines.
> > >
> >
> > I disagree. Please use WRITE_ONCE(), even if we know it is racy on 32bit.
> >
> > sock_read_timestamp() and sock_write_timestamp() already are racy, and
> > we do not care.
>
> I think it's not racy since commit 3a0ed3e96197 ("sock: Make sock->sk_stamp
> thread-safe"), which introduced seqlock in sock_read_timestamp() and
> sock_write_timestamp().

Please note I do not care of 32bit.

It is definitely racy on 64bit.

Please look at

commit f75359f3ac855940c5718af10ba089b8977bf339
Author: Eric Dumazet <edumazet@...gle.com>
Date:   Mon Nov 4 21:38:43 2019 -0800

    net: prevent load/store tearing on sk->sk_stamp


We can not use cmpxchg() only in one place and not the others.

cmpxchg() is expensive, we do not want it here on our fast path.

Thanks.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ