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@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Fixes: 951c6db954a1 ("sctp: fix memleak on err handling of stream initialization")
> > Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@xxxxxxxxx>
> > ---
> > 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
> >
> >
