netdev - Re: [PATCH v1 net 1/4] af_unix: Fix data-races around user->unix

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: <20230902054522.GA22948@1wt.eu>
Date: Sat, 2 Sep 2023 07:45:22 +0200
From: Willy Tarreau <w@....eu>
To: Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Kuniyuki Iwashima <kuni1840@...il.com>, netdev@...r.kernel.org,
        syzkaller <syzkaller@...glegroups.com>
Subject: Re: [PATCH v1 net 1/4] af_unix: Fix data-races around
 user->unix_inflight.

Hi,

On Fri, Sep 01, 2023 at 05:27:05PM -0700, Kuniyuki Iwashima wrote:
> user->unix_inflight is changed under spin_lock(unix_gc_lock),
> but too_many_unix_fds() reads it locklessly.
> 
> Let's annotate the write/read accesses to user->unix_inflight.
> 
> BUG: KCSAN: data-race in unix_attach_fds / unix_inflight
> 
> write to 0xffffffff8546f2d0 of 8 bytes by task 44798 on cpu 1:
>  unix_inflight+0x157/0x180 net/unix/scm.c:66
>  unix_attach_fds+0x147/0x1e0 net/unix/scm.c:123
>  unix_scm_to_skb net/unix/af_unix.c:1827 [inline]
>  unix_dgram_sendmsg+0x46a/0x14f0 net/unix/af_unix.c:1950
>  unix_seqpacket_sendmsg net/unix/af_unix.c:2308 [inline]
>  unix_seqpacket_sendmsg+0xba/0x130 net/unix/af_unix.c:2292
>  sock_sendmsg_nosec net/socket.c:725 [inline]
>  sock_sendmsg+0x148/0x160 net/socket.c:748
>  ____sys_sendmsg+0x4e4/0x610 net/socket.c:2494
>  ___sys_sendmsg+0xc6/0x140 net/socket.c:2548
>  __sys_sendmsg+0x94/0x140 net/socket.c:2577
>  __do_sys_sendmsg net/socket.c:2586 [inline]
>  __se_sys_sendmsg net/socket.c:2584 [inline]
>  __x64_sys_sendmsg+0x45/0x50 net/socket.c:2584
>  do_syscall_x64 arch/x86/entry/common.c:50 [inline]
>  do_syscall_64+0x3b/0x90 arch/x86/entry/common.c:80
>  entry_SYSCALL_64_after_hwframe+0x6e/0xd8
> 
> read to 0xffffffff8546f2d0 of 8 bytes by task 44814 on cpu 0:
>  too_many_unix_fds net/unix/scm.c:101 [inline]
>  unix_attach_fds+0x54/0x1e0 net/unix/scm.c:110
>  unix_scm_to_skb net/unix/af_unix.c:1827 [inline]
>  unix_dgram_sendmsg+0x46a/0x14f0 net/unix/af_unix.c:1950
>  unix_seqpacket_sendmsg net/unix/af_unix.c:2308 [inline]
>  unix_seqpacket_sendmsg+0xba/0x130 net/unix/af_unix.c:2292
>  sock_sendmsg_nosec net/socket.c:725 [inline]
>  sock_sendmsg+0x148/0x160 net/socket.c:748
>  ____sys_sendmsg+0x4e4/0x610 net/socket.c:2494
>  ___sys_sendmsg+0xc6/0x140 net/socket.c:2548
>  __sys_sendmsg+0x94/0x140 net/socket.c:2577
>  __do_sys_sendmsg net/socket.c:2586 [inline]
>  __se_sys_sendmsg net/socket.c:2584 [inline]
>  __x64_sys_sendmsg+0x45/0x50 net/socket.c:2584
>  do_syscall_x64 arch/x86/entry/common.c:50 [inline]
>  do_syscall_64+0x3b/0x90 arch/x86/entry/common.c:80
>  entry_SYSCALL_64_after_hwframe+0x6e/0xd8
> 
> value changed: 0x000000000000000c -> 0x000000000000000d
> 
> Reported by Kernel Concurrency Sanitizer on:
> CPU: 0 PID: 44814 Comm: systemd-coredum Not tainted 6.4.0-11989-g6843306689af #6
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.0-0-gd239552ce722-prebuilt.qemu.org 04/01/2014
> 
> Fixes: 712f4aad406b ("unix: properly account for FDs passed over unix sockets")
> Reported-by: syzkaller <syzkaller@...glegroups.com>
> Signed-off-by: Kuniyuki Iwashima <kuniyu@...zon.com>
> ---
> Cc: Willy Tarreau <w@....eu>
> ---
>  net/unix/scm.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/net/unix/scm.c b/net/unix/scm.c
> index e9dde7176c8a..6ff628f2349f 100644
> --- a/net/unix/scm.c
> +++ b/net/unix/scm.c
> @@ -64,7 +64,7 @@ void unix_inflight(struct user_struct *user, struct file *fp)
>  		/* Paired with READ_ONCE() in wait_for_unix_gc() */
>  		WRITE_ONCE(unix_tot_inflight, unix_tot_inflight + 1);
>  	}
> -	user->unix_inflight++;
> +	WRITE_ONCE(user->unix_inflight, user->unix_inflight + 1);
>  	spin_unlock(&unix_gc_lock);
>  }
>  
> @@ -85,7 +85,7 @@ void unix_notinflight(struct user_struct *user, struct file *fp)
>  		/* Paired with READ_ONCE() in wait_for_unix_gc() */
>  		WRITE_ONCE(unix_tot_inflight, unix_tot_inflight - 1);
>  	}
> -	user->unix_inflight--;
> +	WRITE_ONCE(user->unix_inflight, user->unix_inflight - 1);
>  	spin_unlock(&unix_gc_lock);
>  }
>  
> @@ -99,7 +99,7 @@ static inline bool too_many_unix_fds(struct task_struct *p)
>  {
>  	struct user_struct *user = current_user();
>  
> -	if (unlikely(user->unix_inflight > task_rlimit(p, RLIMIT_NOFILE)))
> +	if (unlikely(READ_ONCE(user->unix_inflight) > task_rlimit(p, RLIMIT_NOFILE)))
>  		return !capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN);
>  	return false;
>  }

Looks good to me, thanks!
Acked-by: Willy Tarreau <w@....eu>

Willy