Re: general protection fault in sctp_stream_free (2)

[Marcelo Ricardo Leitner]

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:
> > 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 <marcelo.leitner@xxxxxxxxx>
> > Date:   Tue Dec 17 01:01:16 2019 +0000
> > 
> >     sctp: fix memleak on err handling of stream initialization
> 
> Ouch... 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;
 }