netdev - Re: [RFC] nasty corner case in 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]

Message-ID: <20190226063804.GI2217@ZenIV.linux.org.uk>
Date:   Tue, 26 Feb 2019 06:38:04 +0000
From:   Al Viro <viro@...iv.linux.org.uk>
To:     Rainer Weikusat <rweikusat@...ileactivedefense.com>,
        Jason Baron <jbaron@...mai.com>
Cc:     netdev@...r.kernel.org
Subject: Re: [RFC] nasty corner case in unix_dgram_sendmsg()

On Tue, Feb 26, 2019 at 06:28:17AM +0000, Al Viro wrote:
> 2) the logics in sendmsg is very odd:
> 	* if we'd detected n:1 send *and* found that we need to
> wait, do so (not using the peer_wake - other's peer_wait is not
> going away).  No questions there.
> 	* if we'd detected n:1 send *and* found that we need to
> wait, but don't have any remaining timeout, we lock both ends
> and check if unix_peer(sk) is (now) not equal to other, either
> because it never had or because we'd been hit by connect(2) while
> we'd been doing the locking.  In that case we fail with -EAGAIN.
> Fair enough, but
> 	* if after relocking we see that unix_peer(sk) now
> is equal to other, we arrange for wakeup forwarding from other's
> peer_wait *and* if that has (likely) succeeded we fail with -EAGAIN.
> Huh?  What's the point?  The only thing that depends upon that
> wakeup forwarding is poll, and _that_ will set the forwarding up
> on its own.
> 	* if we'd failed (either because other is dead now or
> because its queue is not full), we go back to restart_locked.
> If it's dead, we'll sod off with ECONNREFUSED, if it's not
> full anymore, we'll send the damn thing.
> 
> All of that comes at the cost of considerable headache in
> unix_dgram_sendmsg() - flag-conditional locking, etc.  Why do
> we bother?  What's wrong with simple "n:1 case detected, queue
> full, no timeout left, return -EAGAIN and be done with that"?
> 
> IDGI...  Am I missing something subtle going on here?
> 
> I understand what peer_wake is for, and the poll side of things
> is fine; it's sendmsg one that looks weird.  What's going on
> there?

What I have in mind is something like this (for that part of the
issues):

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 74d1eed7cbd4..85d72ea79924 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -1635,7 +1635,6 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 	long timeo;
 	struct scm_cookie scm;
 	int data_len = 0;
-	int sk_locked;
 
 	wait_for_unix_gc();
 	err = scm_send(sock, msg, &scm, false);
@@ -1713,9 +1712,8 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 		goto out_free;
 	}
 
-	sk_locked = 0;
 	unix_state_lock(other);
-restart_locked:
+
 	err = -EPERM;
 	if (!unix_may_send(sk, other))
 		goto out_unlock;
@@ -1728,8 +1726,7 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 		unix_state_unlock(other);
 		sock_put(other);
 
-		if (!sk_locked)
-			unix_state_lock(sk);
+		unix_state_lock(sk);
 
 		err = 0;
 		if (unix_peer(sk) == other) {
@@ -1767,36 +1764,19 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 	 */
 	if (other != sk &&
 	    unlikely(unix_peer(other) != sk && unix_recvq_full(other))) {
-		if (timeo) {
-			timeo = unix_wait_for_peer(other, timeo);
-
-			err = sock_intr_errno(timeo);
-			if (signal_pending(current))
-				goto out_free;
-
-			goto restart;
-		}
-
-		if (!sk_locked) {
-			unix_state_unlock(other);
-			unix_state_double_lock(sk, other);
-		}
-
-		if (unix_peer(sk) != other ||
-		    unix_dgram_peer_wake_me(sk, other)) {
+		if (!timeo) {
 			err = -EAGAIN;
-			sk_locked = 1;
 			goto out_unlock;
 		}
 
-		if (!sk_locked) {
-			sk_locked = 1;
-			goto restart_locked;
-		}
-	}
+		timeo = unix_wait_for_peer(other, timeo);
 
-	if (unlikely(sk_locked))
-		unix_state_unlock(sk);
+		err = sock_intr_errno(timeo);
+		if (signal_pending(current))
+			goto out_free;
+
+		goto restart;
+	}
 
 	if (sock_flag(other, SOCK_RCVTSTAMP))
 		__net_timestamp(skb);
@@ -1809,8 +1789,6 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 	return len;
 
 out_unlock:
-	if (sk_locked)
-		unix_state_unlock(sk);
 	unix_state_unlock(other);
 out_free:
 	kfree_skb(skb);