netdev - Re: [PATCH net-next] net: annotate data-races around sock->ops

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

Message-ID: <dd0c339f4ee4f58a7439589ce6e7766d7ce844ae.camel@redhat.com>
Date: Mon, 28 Aug 2023 15:13:11 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Eric Dumazet <edumazet@...gle.com>
Cc: "David S . Miller" <davem@...emloft.net>, Jakub Kicinski
 <kuba@...nel.org>,  netdev@...r.kernel.org, eric.dumazet@...il.com, syzbot
 <syzkaller@...glegroups.com>
Subject: Re: [PATCH net-next] net: annotate data-races around sock->ops

On Mon, 2023-08-28 at 13:20 +0200, Eric Dumazet wrote:
> On Mon, Aug 28, 2023 at 12:17 PM Paolo Abeni <pabeni@...hat.com> wrote:
> > On Tue, 2023-08-08 at 13:58 +0000, Eric Dumazet wrote:
> > > IPV6_ADDRFORM socket option is evil, because it can change sock->ops
> > > while other threads might read it. Same issue for sk->sk_family
> > > being set to AF_INET.
> > > 
> > > Adding READ_ONCE() over sock->ops reads is needed for sockets
> > > that might be impacted by IPV6_ADDRFORM.
> > > 
> > > Note that mptcp_is_tcpsk() can also overwrite sock->ops.
> > > 
> > > Adding annotations for all sk->sk_family reads will require
> > > more patches :/
> > 
> > I was unable to give the above a proper look before due to OoO on my
> > side.
> > 
> > The mptcp code calls mptcp_is_tcpsk() only before the fd for the newly
> > accepted socket is installed, so we should not have concurrent racing
> > access to sock->ops?!? Do you have any related splat handy?
> > 
> 
> syzbot splat was on another layer. I tried to fix all sites that could
> trigger a similar issue.
> 
> BUG: KCSAN: data-race in ____sys_sendmsg / do_ipv6_setsockopt
> 
> write to 0xffff888109f24ca0 of 8 bytes by task 4470 on cpu 0:
> do_ipv6_setsockopt+0x2c5e/0x2ce0 net/ipv6/ipv6_sockglue.c:491
> ipv6_setsockopt+0x57/0x130 net/ipv6/ipv6_sockglue.c:1012
> udpv6_setsockopt+0x95/0xa0 net/ipv6/udp.c:1690
> sock_common_setsockopt+0x61/0x70 net/core/sock.c:3663
> __sys_setsockopt+0x1c3/0x230 net/socket.c:2273
> __do_sys_setsockopt net/socket.c:2284 [inline]
> __se_sys_setsockopt net/socket.c:2281 [inline]
> __x64_sys_setsockopt+0x66/0x80 net/socket.c:2281
> do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> do_syscall_64+0x41/0xc0 arch/x86/entry/common.c:80
> entry_SYSCALL_64_after_hwframe+0x63/0xcd
> 
> read to 0xffff888109f24ca0 of 8 bytes by task 4469 on cpu 1:
> sock_sendmsg_nosec net/socket.c:724 [inline]
> sock_sendmsg net/socket.c:747 [inline]
> ____sys_sendmsg+0x349/0x4c0 net/socket.c:2503
> ___sys_sendmsg net/socket.c:2557 [inline]
> __sys_sendmmsg+0x263/0x500 net/socket.c:2643
> __do_sys_sendmmsg net/socket.c:2672 [inline]
> __se_sys_sendmmsg net/socket.c:2669 [inline]
> __x64_sys_sendmmsg+0x57/0x60 net/socket.c:2669
> do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> do_syscall_64+0x41/0xc0 arch/x86/entry/common.c:80
> entry_SYSCALL_64_after_hwframe+0x63/0xcd
> 
> value changed: 0xffffffff850e32b8 -> 0xffffffff850da890
> 
> Reported by Kernel Concurrency Sanitizer on:
> CPU: 1 PID: 4469 Comm: syz-executor.1 Not tainted
> 6.4.0-rc5-syzkaller-00313-g4c605260bc60 #0
> Hardware name: Google Google Compute Engine/Google Compute Engine,
> BIOS Google 05/25/2023
> 
> Maybe MPTCP side (mptcp_is_tcpsk()) was ok,
> but  the fact that mptcp_is_tcpsk() was able to write over sock->ops
> was a bit strange to me.
> 
> mptcp_is_tcpsk() should answer a question, with a read-only argument.
> 
> I suggest changing the name of the helper to better reflect what it is doing.

Thanks for double checking. Indeed such helper name is confusing, and
should be renamed; I'll try to take care of it after the merge window.

Cheers,

Paolo