linux-kernel - [PATCH 3.10 263/319] sctp: assign assoc_id earlier in __sctp

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: <1486322541-8206-164-git-send-email-w@1wt.eu>
Date:   Sun,  5 Feb 2017 20:21:25 +0100
From:   Willy Tarreau <w@....eu>
To:     linux-kernel@...r.kernel.org, stable@...r.kernel.org,
        linux@...ck-us.net
Cc:     Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        "David S . Miller" <davem@...emloft.net>, Willy Tarreau <w@....eu>
Subject: [PATCH 3.10 263/319] sctp: assign assoc_id earlier in __sctp_connect

From: Marcelo Ricardo Leitner <marcelo.leitner@...il.com>

commit 7233bc84a3aeda835d334499dc00448373caf5c0 upstream.

sctp_wait_for_connect() currently already holds the asoc to keep it
alive during the sleep, in case another thread release it. But Andrey
Konovalov and Dmitry Vyukov reported an use-after-free in such
situation.

Problem is that __sctp_connect() doesn't get a ref on the asoc and will
do a read on the asoc after calling sctp_wait_for_connect(), but by then
another thread may have closed it and the _put on sctp_wait_for_connect
will actually release it, causing the use-after-free.

Fix is, instead of doing the read after waiting for the connect, do it
before so, and avoid this issue as the socket is still locked by then.
There should be no issue on returning the asoc id in case of failure as
the application shouldn't trust on that number in such situations
anyway.

This issue doesn't exist in sctp_sendmsg() path.

Reported-by: Dmitry Vyukov <dvyukov@...gle.com>
Reported-by: Andrey Konovalov <andreyknvl@...gle.com>
Tested-by: Andrey Konovalov <andreyknvl@...gle.com>
Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
Reviewed-by: Xin Long <lucien.xin@...il.com>
Acked-by: Neil Horman <nhorman@...driver.com>
Signed-off-by: David S. Miller <davem@...emloft.net>
Signed-off-by: Willy Tarreau <w@....eu>
---
 net/sctp/socket.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 86e7352..ede7c54 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -1231,9 +1231,12 @@ static int __sctp_connect(struct sock* sk,

 	timeo = sock_sndtimeo(sk, f_flags & O_NONBLOCK);

-	err = sctp_wait_for_connect(asoc, &timeo);
-	if ((err == 0 || err == -EINPROGRESS) && assoc_id)
+	if (assoc_id)
 		*assoc_id = asoc->assoc_id;
+	err = sctp_wait_for_connect(asoc, &timeo);
+	/* Note: the asoc may be freed after the return of
+	 * sctp_wait_for_connect.
+	 */

 	/* Don't free association on exit. */
 	asoc = NULL;
-- 
2.8.0.rc2.1.gbe9624a