Re: [PATCH net] sctp: fix err handling of stream initialization

[Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>]

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
> > 
> >