netdev - Re: [PATCH v2 net 01/15] af_unix: Set sk->sk_state under unix_state

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]
Date: Sun, 9 Jun 2024 14:03:07 -0700
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <kuniyu@...zon.com>
CC: <cong.wang@...edance.com>, <davem@...emloft.net>, <edumazet@...gle.com>,
	<kuba@...nel.org>, <kuni1840@...il.com>, <mhal@...x.co>,
	<netdev@...r.kernel.org>, <pabeni@...hat.com>
Subject: Re: [PATCH v2 net 01/15] af_unix: Set sk->sk_state under unix_state_lock() for truly disconencted peer.

From: Kuniyuki Iwashima <kuniyu@...zon.com>
Date: Sun, 9 Jun 2024 12:53:20 -0700
> From: Michal Luczaj <mhal@...x.co>
> Date: Sun, 9 Jun 2024 13:28:34 +0200
> > On 6/4/24 18:52, Kuniyuki Iwashima wrote:
> > > When a SOCK_DGRAM socket connect()s to another socket, the both sockets'
> > > sk->sk_state are changed to TCP_ESTABLISHED so that we can register them
> > > to BPF SOCKMAP. (...)
> > 
> > Speaking of af_unix and sockmap, SOCK_STREAM has a tiny window for
> > bpf(BPF_MAP_UPDATE_ELEM) and unix_stream_connect() to race: when
> > sock_map_sk_state_allowed() passes (sk_state == TCP_ESTABLISHED), but
> > unix_peer(sk) in unix_stream_bpf_update_proto() _still_ returns NULL:
> > 
> > 	T0 bpf				T1 connect
> > 	======				==========
> > 
> > 				WRITE_ONCE(sk->sk_state, TCP_ESTABLISHED)
> > sock_map_sk_state_allowed(sk)
> > ...
> > sk_pair = unix_peer(sk)
> > sock_hold(sk_pair)
> > 				sock_hold(newsk)
> > 				smp_mb__after_atomic()
> > 				unix_peer(sk) = newsk
> > 				unix_state_unlock(sk)
> > 
> > With mdelay(1) stuffed in unix_stream_connect():
> > 
> > [  902.277593] BUG: kernel NULL pointer dereference, address: 0000000000000080
> > [  902.277633] #PF: supervisor write access in kernel mode
> > [  902.277661] #PF: error_code(0x0002) - not-present page
> > [  902.277688] PGD 107191067 P4D 107191067 PUD 10f63c067 PMD 0
> > [  902.277716] Oops: Oops: 0002 [#23] PREEMPT SMP NOPTI
> > [  902.277742] CPU: 2 PID: 1505 Comm: a.out Tainted: G      D            6.10.0-rc1+ #130
> > [  902.277769] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Arch Linux 1.16.3-1-1 04/01/2014
> > [  902.277793] RIP: 0010:unix_stream_bpf_update_proto+0xa1/0x150
> > 
> > Setting TCP_ESTABLISHED _after_ unix_peer() fixes the issue, so how about
> > something like
> > 
> > @@ -1631,12 +1631,13 @@ static int unix_stream_connect(struct socket *sock, struct sockaddr *uaddr,
> >         /* Set credentials */
> >         copy_peercred(sk, other);
> > 
> > -       sock->state     = SS_CONNECTED;
> > -       WRITE_ONCE(sk->sk_state, TCP_ESTABLISHED);
> >         sock_hold(newsk);
> > +       smp_mb__after_atomic(); /* sock_hold() does an atomic_inc() */
> > +       WRITE_ONCE(unix_peer(sk), newsk);
> > +       smp_wmb(); /* ensure peer is set before sk_state */
> > 
> > -       smp_mb__after_atomic(); /* sock_hold() does an atomic_inc() */
> > -       unix_peer(sk)   = newsk;
> > +       sock->state = SS_CONNECTED;
> > +       WRITE_ONCE(sk->sk_state, TCP_ESTABLISHED);
> > 
> >         unix_state_unlock(sk);
> > 
> > @@ -180,7 +180,8 @@ int unix_stream_bpf_update_proto(struct sock *sk, struct sk_psock *psock, bool r
> >          * be a single matching destroy operation.
> >          */
> >         if (!psock->sk_pair) {
> > -               sk_pair = unix_peer(sk);
> > +               smp_rmb();
> > +               sk_pair = READ_ONCE(unix_peer(sk));
> >                 sock_hold(sk_pair);
> >                 psock->sk_pair = sk_pair;
> >         }
> > 
> > This should keep things ordered and lockless... I hope.
> 
> sock_map_update_elem() assumes that the socket is protected
> by lock_sock(), but AF_UNIX uses it only for the general path.
> 
> So, I think we should fix sock_map_sk_state_allowed() and
> then use smp_store_release()/smp_load_acquire() rather than
> smp_[rw]mb() for unix_peer(sk).

Sorry, I think I was wrong and we can't use smp_store_release()
and smp_load_acquire(), and smp_[rw]mb() is needed.

Given we avoid adding code in the hotpath in the original fix
8866730aed510 [0], I prefer adding unix_state_lock() in the SOCKMAP
path again.

[0]: https://lore.kernel.org/bpf/6545bc9f7e443_3358c208ae@john.notmuch/

---8<---
diff --git a/net/core/sock_map.c b/net/core/sock_map.c
index d3dbb92153f2..67794d2c7498 100644
--- a/net/core/sock_map.c
+++ b/net/core/sock_map.c
@@ -549,7 +549,7 @@ static bool sock_map_sk_state_allowed(const struct sock *sk)
 	if (sk_is_tcp(sk))
 		return (1 << sk->sk_state) & (TCPF_ESTABLISHED | TCPF_LISTEN);
 	if (sk_is_stream_unix(sk))
-		return (1 << sk->sk_state) & TCPF_ESTABLISHED;
+		return (1 << READ_ONCE(sk->sk_state)) & TCPF_ESTABLISHED;
 	return true;
 }
 
diff --git a/net/unix/unix_bpf.c b/net/unix/unix_bpf.c
index bd84785bf8d6..1db42cfee70d 100644
--- a/net/unix/unix_bpf.c
+++ b/net/unix/unix_bpf.c
@@ -159,8 +159,6 @@ int unix_dgram_bpf_update_proto(struct sock *sk, struct sk_psock *psock, bool re
 
 int unix_stream_bpf_update_proto(struct sock *sk, struct sk_psock *psock, bool restore)
 {
-	struct sock *sk_pair;
-
 	/* Restore does not decrement the sk_pair reference yet because we must
 	 * keep the a reference to the socket until after an RCU grace period
 	 * and any pending sends have completed.
@@ -180,9 +178,9 @@ int unix_stream_bpf_update_proto(struct sock *sk, struct sk_psock *psock, bool r
 	 * be a single matching destroy operation.
 	 */
 	if (!psock->sk_pair) {
-		sk_pair = unix_peer(sk);
-		sock_hold(sk_pair);
-		psock->sk_pair = sk_pair;
+		psock->sk_pair = unix_peer_get(sk);
+		if (WARN_ON_ONCE(!psock->sk_pair))
+			return -EINVAL;
 	}
 
 	unix_stream_bpf_check_needs_rebuild(psock->sk_proto);
---8<---