| 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
| ||
|
Message-ID: <20240609210307.2919-1-kuniyu@amazon.com>
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<---
Powered by blists - more mailing lists