netdev - Re: [PATCH net] ipv4: fix data-races around inet->inet

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: <CANn89iLv+zCrP28NKi9fpS7JELTQvAf=c-w1OCb6jA6mVQiKJA@mail.gmail.com>
Date: Sat, 19 Aug 2023 05:13:12 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: David Ahern <dsahern@...nel.org>
Cc: "David S . Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org, eric.dumazet@...il.com, 
	syzbot <syzkaller@...glegroups.com>
Subject: Re: [PATCH net] ipv4: fix data-races around inet->inet_id

On Sat, Aug 19, 2023 at 3:19 AM David Ahern <dsahern@...nel.org> wrote:
>
> On 8/18/23 4:40 AM, Eric Dumazet wrote:
> > UDP sendmsg() is lockless, so ip_select_ident_segs()
> > can very well be run from multiple cpus [1]
> >
> > Convert inet->inet_id to an atomic_t, but implement
> > a dedicated path for TCP, avoiding cost of a locked
> > instruction (atomic_add_return())
> >
> > Note that this patch will cause a trivial merge conflict
> > because we added inet->flags in net-next tree.
> >
> > [1]
> >
> > BUG: KCSAN: data-race in __ip_make_skb / __ip_make_skb
> >
> > read-write to 0xffff888145af952a of 2 bytes by task 7803 on cpu 1:
> > ip_select_ident_segs include/net/ip.h:542 [inline]
> > ip_select_ident include/net/ip.h:556 [inline]
> > __ip_make_skb+0x844/0xc70 net/ipv4/ip_output.c:1446
> > ip_make_skb+0x233/0x2c0 net/ipv4/ip_output.c:1560
> > udp_sendmsg+0x1199/0x1250 net/ipv4/udp.c:1260
> > inet_sendmsg+0x63/0x80 net/ipv4/af_inet.c:830
> > sock_sendmsg_nosec net/socket.c:725 [inline]
> > sock_sendmsg net/socket.c:748 [inline]
> > ____sys_sendmsg+0x37c/0x4d0 net/socket.c:2494
> > ___sys_sendmsg net/socket.c:2548 [inline]
> > __sys_sendmmsg+0x269/0x500 net/socket.c:2634
> > __do_sys_sendmmsg net/socket.c:2663 [inline]
> > __se_sys_sendmmsg net/socket.c:2660 [inline]
> > __x64_sys_sendmmsg+0x57/0x60 net/socket.c:2660
> > 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 0xffff888145af952a of 2 bytes by task 7804 on cpu 0:
> > ip_select_ident_segs include/net/ip.h:541 [inline]
> > ip_select_ident include/net/ip.h:556 [inline]
> > __ip_make_skb+0x817/0xc70 net/ipv4/ip_output.c:1446
> > ip_make_skb+0x233/0x2c0 net/ipv4/ip_output.c:1560
> > udp_sendmsg+0x1199/0x1250 net/ipv4/udp.c:1260
> > inet_sendmsg+0x63/0x80 net/ipv4/af_inet.c:830
> > sock_sendmsg_nosec net/socket.c:725 [inline]
> > sock_sendmsg net/socket.c:748 [inline]
> > ____sys_sendmsg+0x37c/0x4d0 net/socket.c:2494
> > ___sys_sendmsg net/socket.c:2548 [inline]
> > __sys_sendmmsg+0x269/0x500 net/socket.c:2634
> > __do_sys_sendmmsg net/socket.c:2663 [inline]
> > __se_sys_sendmmsg net/socket.c:2660 [inline]
> > __x64_sys_sendmmsg+0x57/0x60 net/socket.c:2660
> > 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: 0x184d -> 0x184e
> >
> > Reported by Kernel Concurrency Sanitizer on:
> > CPU: 0 PID: 7804 Comm: syz-executor.1 Not tainted 6.5.0-rc6-syzkaller #0
> > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 07/26/2023
> > ==================================================================
> >
> > Fixes: 23f57406b82d ("ipv4: avoid using shared IP generator for connected sockets")
> > Reported-by: syzbot <syzkaller@...glegroups.com>
> > Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> > ---
> >  include/net/inet_sock.h |  2 +-
> >  include/net/ip.h        | 15 +++++++++++++--
> >  net/dccp/ipv4.c         |  4 ++--
> >  net/ipv4/af_inet.c      |  2 +-
> >  net/ipv4/datagram.c     |  2 +-
> >  net/ipv4/tcp_ipv4.c     |  4 ++--
> >  net/sctp/socket.c       |  2 +-
> >  7 files changed, 21 insertions(+), 10 deletions(-)
> >
>
> drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c also
> references inet_id.

Oh well, I really wonder how we accepted such a big mess of layering violations.

I will send a v2, thanks !