netdev - Re: [PATCH net] unix: Fix race in SOCK_SEQPACKET's unix_dgram

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, 20 Nov 2022 01:09:28 -0800
From:   Kuniyuki Iwashima <kuniyu@...zon.com>
To:     <tkhai@...ru>
CC:     <davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
        <netdev@...r.kernel.org>, <pabeni@...hat.com>, <kuniyu@...zon.com>
Subject: Re: [PATCH net] unix: Fix race in SOCK_SEQPACKET's unix_dgram_sendmsg()

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.

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;


Thank you!


> ---
>  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;