Re: general protection fault in sctp_stream_free (2)
From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Date: 2019-12-20 16:06:06
Also in:
linux-sctp, lkml
Subsystem:
networking [general], sctp protocol, the rest · Maintainers:
"David S. Miller", Eric Dumazet, Jakub Kicinski, Paolo Abeni, Marcelo Ricardo Leitner, Xin Long, Linus Torvalds
On Fri, Dec 20, 2019 at 12:28:10PM -0300, Marcelo Ricardo Leitner wrote:
On Thu, Dec 19, 2019 at 07:45:09PM -0800, syzbot wrote:quoted
Hello, syzbot found the following crash on: HEAD commit: 6fa9a115 Merge branch 'stmmac-fixes' git tree: net console output: https://syzkaller.appspot.com/x/log.txt?x=10c4fe99e00000 kernel config: https://syzkaller.appspot.com/x/.config?x=216dca5e1758db87 dashboard link: https://syzkaller.appspot.com/bug?extid=9a1bc632e78a1a98488b compiler: gcc (GCC) 9.0.0 20181231 (experimental) syz repro: https://syzkaller.appspot.com/x/repro.syz?x=178ada71e00000 C reproducer: https://syzkaller.appspot.com/x/repro.c?x=144f23a6e00000 The bug was bisected to: commit 951c6db954a1adefab492f6da805decacabbd1a7 Author: Marcelo Ricardo Leitner [off-list ref] Date: Tue Dec 17 01:01:16 2019 +0000 sctp: fix memleak on err handling of stream initializationOuch... this wasn't a good fix. When called from sctp_stream_init(), it is doing the right thing. But when called from sctp_send_add_streams(), it can't free the genradix. Ditto from sctp_process_strreset_addstrm_in().
Tentative fix. I'll post after additional tests. --8<--
diff --git a/net/sctp/stream.c b/net/sctp/stream.c
index 6a30392068a0..c1a100d2fed3 100644
--- a/net/sctp/stream.c
+++ b/net/sctp/stream.c@@ -84,10 +84,8 @@ static int sctp_stream_alloc_out(struct sctp_stream *stream, __u16 outcnt, return 0; ret = genradix_prealloc(&stream->out, outcnt, gfp); - if (ret) { - genradix_free(&stream->out); + if (ret) return ret; - } stream->outcnt = outcnt; return 0;
@@ -102,10 +100,8 @@ static int sctp_stream_alloc_in(struct sctp_stream *stream, __u16 incnt, return 0; ret = genradix_prealloc(&stream->in, incnt, gfp); - if (ret) { - genradix_free(&stream->in); + if (ret) return ret; - } stream->incnt = incnt; return 0;
@@ -123,7 +119,7 @@ int sctp_stream_init(struct sctp_stream *stream, __u16 outcnt, __u16 incnt, * a new one with new outcnt to save memory if needed. */ if (outcnt == stream->outcnt) - goto in; + goto handle_in; /* Filter out chunks queued on streams that won't exist anymore */ sched->unsched_all(stream);
@@ -132,24 +128,28 @@ int sctp_stream_init(struct sctp_stream *stream, __u16 outcnt, __u16 incnt, ret = sctp_stream_alloc_out(stream, outcnt, gfp); if (ret) - goto out; + goto out_err; for (i = 0; i < stream->outcnt; i++) SCTP_SO(stream, i)->state = SCTP_STREAM_OPEN; -in: +handle_in: sctp_stream_interleave_init(stream); if (!incnt) goto out; ret = sctp_stream_alloc_in(stream, incnt, gfp); - if (ret) { - sched->free(stream); - genradix_free(&stream->out); - stream->outcnt = 0; - goto out; - } + if (ret) + goto in_err; + + goto out; +in_err: + sched->free(stream); + genradix_free(&stream->in); +out_err: + genradix_free(&stream->out); + stream->outcnt = 0; out: return ret; }