| 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
| ||
|
Date: Wed, 22 May 2024 17:25:23 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
kuni1840@...il.com, netdev@...r.kernel.org, syzkaller@...glegroups.com
Subject: Re: [PATCH v1 net] af_unix: Annotate data-race around
unix_sk(sk)->addr.
On Wed, 2024-05-22 at 23:53 +0900, Kuniyuki Iwashima wrote:
> From: Paolo Abeni <pabeni@...hat.com>
> Date: Wed, 22 May 2024 10:52:19 +0200
> > On Sat, 2024-05-18 at 09:01 +0900, Kuniyuki Iwashima wrote:
> > > Once unix_sk(sk)->addr is assigned under net->unx.table.locks,
> > > *(unix_sk(sk)->addr) and unix_sk(sk)->path are fully set up, and
> > > unix_sk(sk)->addr is never changed.
> > >
> > > unix_getname() and unix_copy_addr() access the two fields locklessly,
> > > and commit ae3b564179bf ("missing barriers in some of unix_sock ->addr
> > > and ->path accesses") added smp_store_release() and smp_load_acquire()
> > > pairs.
> > >
> > > In other functions, we still read unix_sk(sk)->addr locklessly to check
> > > if the socket is bound, and KCSAN complains about it. [0]
> > >
> > > Given these functions have no dependency for *(unix_sk(sk)->addr) and
> > > unix_sk(sk)->path, READ_ONCE() is enough to annotate the data-race.
> > >
> > > [0]:
> > > BUG: KCSAN: data-race in unix_bind / unix_listen
> > >
> > > write (marked) to 0xffff88805f8d1840 of 8 bytes by task 13723 on cpu 0:
> > > __unix_set_addr_hash net/unix/af_unix.c:329 [inline]
> > > unix_bind_bsd net/unix/af_unix.c:1241 [inline]
> > > unix_bind+0x881/0x1000 net/unix/af_unix.c:1319
> > > __sys_bind+0x194/0x1e0 net/socket.c:1847
> > > __do_sys_bind net/socket.c:1858 [inline]
> > > __se_sys_bind net/socket.c:1856 [inline]
> > > __x64_sys_bind+0x40/0x50 net/socket.c:1856
> > > do_syscall_x64 arch/x86/entry/common.c:52 [inline]
> > > do_syscall_64+0x4f/0x110 arch/x86/entry/common.c:83
> > > entry_SYSCALL_64_after_hwframe+0x46/0x4e
> > >
> > > read to 0xffff88805f8d1840 of 8 bytes by task 13724 on cpu 1:
> > > unix_listen+0x72/0x180 net/unix/af_unix.c:734
> > > __sys_listen+0xdc/0x160 net/socket.c:1881
> > > __do_sys_listen net/socket.c:1890 [inline]
> > > __se_sys_listen net/socket.c:1888 [inline]
> > > __x64_sys_listen+0x2e/0x40 net/socket.c:1888
> > > do_syscall_x64 arch/x86/entry/common.c:52 [inline]
> > > do_syscall_64+0x4f/0x110 arch/x86/entry/common.c:83
> > > entry_SYSCALL_64_after_hwframe+0x46/0x4e
> > >
> > > value changed: 0x0000000000000000 -> 0xffff88807b5b1b40
> > >
> > > Reported by Kernel Concurrency Sanitizer on:
> > > CPU: 1 PID: 13724 Comm: syz-executor.4 Not tainted 6.8.0-12822-gcd51db110a7e #12
> > > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.0-0-gd239552ce722-prebuilt.qemu.org 04/01/2014
> > >
> > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > > Reported-by: syzkaller <syzkaller@...glegroups.com>
> > > Signed-off-by: Kuniyuki Iwashima <kuniyu@...zon.com>
> > > ---
> > > net/unix/af_unix.c | 10 ++++++----
> > > 1 file changed, 6 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> > > index ca101690e740..92a88ac070ca 100644
> > > --- a/net/unix/af_unix.c
> > > +++ b/net/unix/af_unix.c
> > > @@ -731,7 +731,7 @@ static int unix_listen(struct socket *sock, int backlog)
> > > if (sock->type != SOCK_STREAM && sock->type != SOCK_SEQPACKET)
> > > goto out; /* Only stream/seqpacket sockets accept */
> > > err = -EINVAL;
> > > - if (!u->addr)
> > > + if (!READ_ONCE(u->addr))
> > > goto out; /* No listens on an unbound socket */
> > > unix_state_lock(sk);
> > > if (sk->sk_state != TCP_CLOSE && sk->sk_state != TCP_LISTEN)
> > > @@ -1369,7 +1369,7 @@ static int unix_dgram_connect(struct socket *sock, struct sockaddr *addr,
> > >
> > > if ((test_bit(SOCK_PASSCRED, &sock->flags) ||
> > > test_bit(SOCK_PASSPIDFD, &sock->flags)) &&
> > > - !unix_sk(sk)->addr) {
> > > + !READ_ONCE(unix_sk(sk)->addr)) {
> > > err = unix_autobind(sk);
> > > if (err)
> > > goto out;
> > > @@ -1481,7 +1481,8 @@ static int unix_stream_connect(struct socket *sock, struct sockaddr *uaddr,
> > > goto out;
> > >
> > > if ((test_bit(SOCK_PASSCRED, &sock->flags) ||
> > > - test_bit(SOCK_PASSPIDFD, &sock->flags)) && !u->addr) {
> > > + test_bit(SOCK_PASSPIDFD, &sock->flags)) &&
> > > + !READ_ONCE(u->addr)) {
> > > err = unix_autobind(sk);
> > > if (err)
> > > goto out;
> > > @@ -1951,7 +1952,8 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
> > > }
> > >
> > > if ((test_bit(SOCK_PASSCRED, &sock->flags) ||
> > > - test_bit(SOCK_PASSPIDFD, &sock->flags)) && !u->addr) {
> > > + test_bit(SOCK_PASSPIDFD, &sock->flags)) &&
> > > + !READ_ONCE(u->addr)) {
> > > err = unix_autobind(sk);
> > > if (err)
> > > goto out;
> >
> > There are a few other places where ->addr is accessed lockless (under
> > the bindlock, but prior to acquiring the table spinlock, e.g.
> > unix_bind_* and unix_autobind. The latter is suspect as it's called
> > right after the touched code. Why are such spots not relevant here?
>
> When u->addr is set, bindlock and the hash table lock are held.
> unix_bind_(abstract|bsd)() and unix_autobind() are all serialised
> by bindlock, so ->addr check after acquiring bindlock is not
> lockless actually.
>
>
> >
> > Also the newu->addr/otheru->addr handling in unix_stream_connect()
> > looks suspect.
>
> u->addr is set before the socket is put into the hash table, and
> it never changes after bind().
>
> otheru is found by unix_find_other() from the hash table, and then
> we access ->addr in unix_stream_connect(). So, there is no race.
I see.
I think the serialization chain is not trivial, I think it would help
future memory to at least mention it in the commit message or in a code
comment.
Thanks,
Paolo
Powered by blists - more mailing lists