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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Sun, 20 Nov 2022 12:46:55 +0300 From: Kirill Tkhai <tkhai@...ru> To: Kuniyuki Iwashima <kuniyu@...zon.com> Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, netdev@...r.kernel.org, pabeni@...hat.com Subject: Re: [PATCH net] unix: Fix race in SOCK_SEQPACKET's unix_dgram_sendmsg() On 20.11.2022 12:09, Kuniyuki Iwashima wrote: > From: Kirill Tkhai <tkhai@...ru> > Date: Sun, 20 Nov 2022 02:16:47 +0300 >> There is a race resulting in alive SOCK_SEQPACKET socket >> may change its state from TCP_ESTABLISHED to TCP_CLOSE: >> >> unix_release_sock(peer) unix_dgram_sendmsg(sk) >> sock_orphan(peer) >> sock_set_flag(peer, SOCK_DEAD) >> sock_alloc_send_pskb() >> if !(sk->sk_shutdown & SEND_SHUTDOWN) >> OK >> if sock_flag(peer, SOCK_DEAD) >> sk->sk_state = TCP_CLOSE >> sk->sk_shutdown = SHUTDOWN_MASK >> >> >> After that socket sk remains almost normal: it is able to connect, listen, accept >> and recvmsg, while it can't sendmsg. > > nit: Then, also recvmsg() fails with -ENOTCONN. And after connect(), even > both of recvmsg() and sendmsg() does not fail. Possible, I wrote not clear. I mean sendmsg fails after connect, while recvmsg does not fail :) Here is sendmsg abort path: unix_dgram_sendmsg() sock_alloc_send_pskb() if (sk->sk_shutdown & SEND_SHUTDOWN) FAIL > static int unix_seqpacket_recvmsg(struct socket *sock, struct msghdr *msg, > size_t size, int flags) > { > struct sock *sk = sock->sk; > > if (sk->sk_state != TCP_ESTABLISHED) > return -ENOTCONN; > > return unix_dgram_recvmsg(sock, msg, size, flags); > } > > >> >> Since this is the only possibility for alive SOCK_SEQPACKET to change >> the state in such way, we should better fix this strange and potentially >> danger corner case. >> >> Also, move TCP_CLOSE assignment for SOCK_DGRAM sockets under state lock. >> >> Signed-off-by: Kirill Tkhai <tkhai@...ru> > > Fixes tag is needed: > > Fixes: 83301b5367a9 ("af_unix: Set TCP_ESTABLISHED for datagram sockets too") >> Before this commit, there was no state change and SEQPACKET sk also went > through the same path. The bug was introduced because the commit did not > consider SEAPACKET. > > So, I think the fix should be like below, then we can free the peer faster. > Note unix_dgram_peer_wake_disconnect_wakeup() is dgram-specific too. > > ---8<--- > diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c > index b3545fc68097..be40023a61fb 100644 > --- a/net/unix/af_unix.c > +++ b/net/unix/af_unix.c > @@ -2001,11 +2001,14 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg, > err = 0; > if (unix_peer(sk) == other) { > unix_peer(sk) = NULL; > - unix_dgram_peer_wake_disconnect_wakeup(sk, other); > + > + if (sk->sk_type == SOCK_DGRAM) { > + unix_dgram_peer_wake_disconnect_wakeup(sk, other); > + sk->sk_state = TCP_CLOSE; > + } > > unix_state_unlock(sk); > > - sk->sk_state = TCP_CLOSE; > unix_dgram_disconnected(sk, other); > sock_put(other); > err = -ECONNREFUSED; > ---8<--- > > Also, it's better to mention that moving TCP_CLOSE under the lock resolves > another rare race with unix_dgram_connect() for DGRAM sk: > > unix_state_unlock(sk); > <--------------------------> connect() could set TCP_ESTABLISHED here. > sk->sk_state = TCP_CLOSE; Sounds good, I'll test this variant. Thanks! Kirill >> --- >> net/unix/af_unix.c | 11 +++++++++-- >> 1 file changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c >> index b3545fc68097..6fd745cfc492 100644 >> --- a/net/unix/af_unix.c >> +++ b/net/unix/af_unix.c >> @@ -1999,13 +1999,20 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg, >> unix_state_lock(sk); >> >> err = 0; >> - if (unix_peer(sk) == other) { >> + if (sk->sk_type == SOCK_SEQPACKET) { >> + /* We are here only when racing with unix_release_sock() >> + * is clearing @other. Never change state to TCP_CLOSE >> + * unlike SOCK_DGRAM wants. >> + */ >> + unix_state_unlock(sk); >> + err = -EPIPE; >> + } else if (unix_peer(sk) == other) { >> unix_peer(sk) = NULL; >> unix_dgram_peer_wake_disconnect_wakeup(sk, other); >> >> + sk->sk_state = TCP_CLOSE; >> unix_state_unlock(sk); >> >> - sk->sk_state = TCP_CLOSE; >> unix_dgram_disconnected(sk, other); >> sock_put(other); >> err = -ECONNREFUSED;
Powered by blists - more mailing lists