Re: net/sctp: use-after-free in __sctp_connect
From: Andrey Konovalov <hidden>
Date: 2016-11-03 18:45:08
Also in:
linux-sctp, lkml
On Thu, Nov 3, 2016 at 7:35 PM, Marcelo Ricardo Leitner [off-list ref] wrote:
On Thu, Nov 03, 2016 at 07:02:47PM +0100, Andrey Konovalov wrote:quoted
On Thu, Nov 3, 2016 at 6:52 PM, Marcelo Ricardo Leitner [off-list ref] wrote:quoted
On Thu, Nov 03, 2016 at 06:11:01PM +0100, Andrey Konovalov wrote:quoted
On Wed, Nov 2, 2016 at 11:42 PM, Andrey Konovalov [off-list ref] wrote:quoted
On Wed, Oct 19, 2016 at 6:57 PM, Marcelo Ricardo Leitner [off-list ref] wrote:quoted
On Wed, Oct 19, 2016 at 02:25:24PM +0200, Andrey Konovalov wrote:quoted
Hi, I've got the following error report while running the syzkaller fuzzer: ================================================================== BUG: KASAN: use-after-free in __sctp_connect+0xabe/0xbf0 at addr ffff88006b1dc610Seems this is the same that Dmitry Vyukov had reported back in Jan 13th. So far I couldn't identify the reason. "Good" to know it's still there, thanks for reporting it.Hi Marcelo,Hiquoted
So I've looked at the code. As far as I understand, the problem is a race condition between setsockopt(SCTP_SOCKOPT_CONNECTX) and shutdown on an sctp socket. setsockopt() calls sctp_wait_for_connect(), which exits the for loop on the sk->sk_shutdown & RCV_SHUTDOWN if clause, and then frees asoc with sctp_association_put() and returns err = 0. Then __sctp_connect() checks that err == 0 and reads asoc->assoc_id from the freed asoc.Suddenly this seems familiar. Your description makes sense, thanks for looking deeper into this, Andrey. This fix should do it, can you please try it? I'll post it properly if it works.Yes, it fixes the issue. Tested-by: Andrey Konovalov <redacted> Thanks for the fix!Ahm this other fix looks better: do the read before calling sctp_wait_for_connect() as that id won't change in this call and the application shouldn't trust this number if an error is returned, so there should be no issues by returning it in such situation. Can you please confirm this one also works? Thanks!
Sure! This one also works. Tested-by: Andrey Konovalov <redacted>
quoted hunk ↗ jump to hunk
---8<---diff --git a/net/sctp/socket.c b/net/sctp/socket.c index 6cdc61c21438..be1d9bb98230 100644 --- a/net/sctp/socket.c +++ b/net/sctp/socket.c@@ -1214,9 +1214,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;