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@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.
>
you're right, I missed the different streams in the free function
Neil
> Marcelo
>
> > Neil
> >
> > > + stream->outcnt = 0;
> > > out:
> > > return ret;
> > > }
> > > --
> > > 2.23.0
> > >
> > >
>
