* [PATCH net] sctp: fix err handling of stream initialization
@ 2019-12-20 18:03 Marcelo Ricardo Leitner
2019-12-23 12:46 ` Neil Horman
2019-12-25 0:08 ` David Miller
0 siblings, 2 replies; 5+ messages in thread
From: Marcelo Ricardo Leitner @ 2019-12-20 18:03 UTC (permalink / raw)
To: netdev; +Cc: Neil Horman, linux-sctp, Xin Long, syzbot, syzkaller-bugs
The fix on 951c6db954a1 fixed the issued reported there but introduced
another. When the allocation fails within sctp_stream_init() it is
okay/necessary to free the genradix. But it is also called when adding
new streams, from sctp_send_add_streams() and
sctp_process_strreset_addstrm_in() and in those situations it cannot
just free the genradix because by then it is a fully operational
association.
The fix here then is to only free the genradix in sctp_stream_init()
and on those other call sites move on with what it already had and let
the subsequent error handling to handle it.
Tested with the reproducers from this report and the previous one,
with lksctp-tools and sctp-tests.
Reported-by: syzbot+9a1bc632e78a1a98488b@syzkaller.appspotmail.com
Fixes: 951c6db954a1 ("sctp: fix memleak on err handling of stream initialization")
Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
---
net/sctp/stream.c | 30 +++++++++++++++---------------
1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/net/sctp/stream.c b/net/sctp/stream.c
index 6a30392068a04bfcefcb14c3d7f13fc092d59cd3..c1a100d2fed39c2d831487e05fcbf5e8d507d470 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;
}
--
2.23.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH net] sctp: fix err handling of stream initialization 2019-12-20 18:03 [PATCH net] sctp: fix err handling of stream initialization Marcelo Ricardo Leitner @ 2019-12-23 12:46 ` Neil Horman 2019-12-23 12:51 ` Marcelo Ricardo Leitner 2019-12-25 0:08 ` David Miller 1 sibling, 1 reply; 5+ messages in thread From: Neil Horman @ 2019-12-23 12:46 UTC (permalink / raw) To: Marcelo Ricardo Leitner Cc: netdev, linux-sctp, Xin Long, syzbot, syzkaller-bugs On Fri, Dec 20, 2019 at 03:03:44PM -0300, Marcelo Ricardo Leitner wrote: > The fix on 951c6db954a1 fixed the issued reported there but introduced > another. When the allocation fails within sctp_stream_init() it is > okay/necessary to free the genradix. But it is also called when adding > new streams, from sctp_send_add_streams() and > sctp_process_strreset_addstrm_in() and in those situations it cannot > just free the genradix because by then it is a fully operational > association. > > The fix here then is to only free the genradix in sctp_stream_init() > and on those other call sites move on with what it already had and let > the subsequent error handling to handle it. > > Tested with the reproducers from this report and the previous one, > with lksctp-tools and sctp-tests. > > Reported-by: syzbot+9a1bc632e78a1a98488b@syzkaller.appspotmail.com > Fixes: 951c6db954a1 ("sctp: fix memleak on err handling of stream initialization") > Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> > --- > net/sctp/stream.c | 30 +++++++++++++++--------------- > 1 file changed, 15 insertions(+), 15 deletions(-) > > diff --git a/net/sctp/stream.c b/net/sctp/stream.c > index 6a30392068a04bfcefcb14c3d7f13fc092d59cd3..c1a100d2fed39c2d831487e05fcbf5e8d507d470 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); Isn't this effectively a double free in the fall through case? Neil > + stream->outcnt = 0; > out: > return ret; > } > -- > 2.23.0 > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] sctp: fix err handling of stream initialization 2019-12-23 12:46 ` Neil Horman @ 2019-12-23 12:51 ` Marcelo Ricardo Leitner 2019-12-23 22:55 ` Neil Horman 0 siblings, 1 reply; 5+ messages in thread From: Marcelo Ricardo Leitner @ 2019-12-23 12:51 UTC (permalink / raw) To: Neil Horman; +Cc: netdev, linux-sctp, Xin Long, syzbot, syzkaller-bugs On Mon, Dec 23, 2019 at 07:46:09AM -0500, Neil Horman wrote: > On Fri, Dec 20, 2019 at 03:03:44PM -0300, Marcelo Ricardo Leitner wrote: > > The fix on 951c6db954a1 fixed the issued reported there but introduced > > another. When the allocation fails within sctp_stream_init() it is > > okay/necessary to free the genradix. But it is also called when adding > > new streams, from sctp_send_add_streams() and > > sctp_process_strreset_addstrm_in() and in those situations it cannot > > just free the genradix because by then it is a fully operational > > association. > > > > The fix here then is to only free the genradix in sctp_stream_init() > > and on those other call sites move on with what it already had and let > > the subsequent error handling to handle it. > > > > Tested with the reproducers from this report and the previous one, > > with lksctp-tools and sctp-tests. > > > > Reported-by: syzbot+9a1bc632e78a1a98488b@syzkaller.appspotmail.com > > Fixes: 951c6db954a1 ("sctp: fix memleak on err handling of stream initialization") > > Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> > > --- > > net/sctp/stream.c | 30 +++++++++++++++--------------- > > 1 file changed, 15 insertions(+), 15 deletions(-) > > > > diff --git a/net/sctp/stream.c b/net/sctp/stream.c > > index 6a30392068a04bfcefcb14c3d7f13fc092d59cd3..c1a100d2fed39c2d831487e05fcbf5e8d507d470 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); > Isn't this effectively a double free in the fall through case? Hm, if you got you right, the line 3 lines above is freeing '->in' and the right above one, '->out', so no. No other calls to genradix_free() are left in this function, too. Marcelo > Neil > > > + stream->outcnt = 0; > > out: > > return ret; > > } > > -- > > 2.23.0 > > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] sctp: fix err handling of stream initialization 2019-12-23 12:51 ` Marcelo Ricardo Leitner @ 2019-12-23 22:55 ` Neil Horman 0 siblings, 0 replies; 5+ messages in thread From: Neil Horman @ 2019-12-23 22:55 UTC (permalink / raw) To: Marcelo Ricardo Leitner Cc: netdev, linux-sctp, Xin Long, syzbot, syzkaller-bugs On Mon, Dec 23, 2019 at 09:51:08AM -0300, Marcelo Ricardo Leitner wrote: > On Mon, Dec 23, 2019 at 07:46:09AM -0500, Neil Horman wrote: > > On Fri, Dec 20, 2019 at 03:03:44PM -0300, Marcelo Ricardo Leitner wrote: > > > The fix on 951c6db954a1 fixed the issued reported there but introduced > > > another. When the allocation fails within sctp_stream_init() it is > > > okay/necessary to free the genradix. But it is also called when adding > > > new streams, from sctp_send_add_streams() and > > > sctp_process_strreset_addstrm_in() and in those situations it cannot > > > just free the genradix because by then it is a fully operational > > > association. > > > > > > The fix here then is to only free the genradix in sctp_stream_init() > > > and on those other call sites move on with what it already had and let > > > the subsequent error handling to handle it. > > > > > > Tested with the reproducers from this report and the previous one, > > > with lksctp-tools and sctp-tests. > > > > > > Reported-by: syzbot+9a1bc632e78a1a98488b@syzkaller.appspotmail.com > > > Fixes: 951c6db954a1 ("sctp: fix memleak on err handling of stream initialization") > > > Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> > > > --- > > > net/sctp/stream.c | 30 +++++++++++++++--------------- > > > 1 file changed, 15 insertions(+), 15 deletions(-) > > > > > > diff --git a/net/sctp/stream.c b/net/sctp/stream.c > > > index 6a30392068a04bfcefcb14c3d7f13fc092d59cd3..c1a100d2fed39c2d831487e05fcbf5e8d507d470 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); > > Isn't this effectively a double free in the fall through case? > > Hm, if you got you right, the line 3 lines above is freeing '->in' and > the right above one, '->out', so no. > No other calls to genradix_free() are left in this function, too. > you're right, I missed the different streams in the free function Neil > Marcelo > > > Neil > > > > > + stream->outcnt = 0; > > > out: > > > return ret; > > > } > > > -- > > > 2.23.0 > > > > > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] sctp: fix err handling of stream initialization 2019-12-20 18:03 [PATCH net] sctp: fix err handling of stream initialization Marcelo Ricardo Leitner 2019-12-23 12:46 ` Neil Horman @ 2019-12-25 0:08 ` David Miller 1 sibling, 0 replies; 5+ messages in thread From: David Miller @ 2019-12-25 0:08 UTC (permalink / raw) To: marcelo.leitner Cc: netdev, nhorman, linux-sctp, lucien.xin, syzbot+9a1bc632e78a1a98488b, syzkaller-bugs From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> Date: Fri, 20 Dec 2019 15:03:44 -0300 > The fix on 951c6db954a1 fixed the issued reported there but introduced > another. When the allocation fails within sctp_stream_init() it is > okay/necessary to free the genradix. But it is also called when adding > new streams, from sctp_send_add_streams() and > sctp_process_strreset_addstrm_in() and in those situations it cannot > just free the genradix because by then it is a fully operational > association. > > The fix here then is to only free the genradix in sctp_stream_init() > and on those other call sites move on with what it already had and let > the subsequent error handling to handle it. > > Tested with the reproducers from this report and the previous one, > with lksctp-tools and sctp-tests. > > Reported-by: syzbot+9a1bc632e78a1a98488b@syzkaller.appspotmail.com > Fixes: 951c6db954a1 ("sctp: fix memleak on err handling of stream initialization") > Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> Applied and since I backported the commit mentioned in the Fixes: tag to -stable, I queued this up for -stable as well. Thanks. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-12-25 0:08 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-12-20 18:03 [PATCH net] sctp: fix err handling of stream initialization Marcelo Ricardo Leitner 2019-12-23 12:46 ` Neil Horman 2019-12-23 12:51 ` Marcelo Ricardo Leitner 2019-12-23 22:55 ` Neil Horman 2019-12-25 0:08 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).