* [PATCH net] sctp: fix memleak on err handling of stream initialization
@ 2019-12-17 1:01 Marcelo Ricardo Leitner
2019-12-17 8:30 ` Xin Long
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Marcelo Ricardo Leitner @ 2019-12-17 1:01 UTC (permalink / raw)
To: netdev; +Cc: Xin Long, Kent Overstreet, Neil Horman, linux-sctp
syzbot reported a memory leak when an allocation fails within
genradix_prealloc() for output streams. That's because
genradix_prealloc() leaves initialized members initialized when the
issue happens and SCTP stack will abort the current initialization but
without cleaning up such members.
The fix here is to always call genradix_free() when genradix_prealloc()
fails, for output and also input streams, as it suffers from the same
issue.
Reported-by: syzbot+772d9e36c490b18d51d1@syzkaller.appspotmail.com
Fixes: 2075e50caf5e ("sctp: convert to genradix")
Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
---
net/sctp/stream.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/net/sctp/stream.c b/net/sctp/stream.c
index df60b5ef24cbf5c6f628ab8ed88a6faaaa422b6d..e0b01bf912b3f3cdbc3f713bcfa50868e4802929 100644
--- a/net/sctp/stream.c
+++ b/net/sctp/stream.c
@@ -84,8 +84,10 @@ static int sctp_stream_alloc_out(struct sctp_stream *stream, __u16 outcnt,
return 0;
ret = genradix_prealloc(&stream->out, outcnt, gfp);
- if (ret)
+ if (ret) {
+ genradix_free(&stream->out);
return ret;
+ }
stream->outcnt = outcnt;
return 0;
@@ -100,8 +102,10 @@ static int sctp_stream_alloc_in(struct sctp_stream *stream, __u16 incnt,
return 0;
ret = genradix_prealloc(&stream->in, incnt, gfp);
- if (ret)
+ if (ret) {
+ genradix_free(&stream->in);
return ret;
+ }
stream->incnt = incnt;
return 0;
--
2.23.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH net] sctp: fix memleak on err handling of stream initialization
2019-12-17 1:01 [PATCH net] sctp: fix memleak on err handling of stream initialization Marcelo Ricardo Leitner
@ 2019-12-17 8:30 ` Xin Long
2019-12-17 11:55 ` Neil Horman
2019-12-18 5:59 ` David Miller
2 siblings, 0 replies; 5+ messages in thread
From: Xin Long @ 2019-12-17 8:30 UTC (permalink / raw)
To: Marcelo Ricardo Leitner
Cc: network dev, Kent Overstreet, Neil Horman, linux-sctp
On Tue, Dec 17, 2019 at 9:01 AM Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
>
> syzbot reported a memory leak when an allocation fails within
> genradix_prealloc() for output streams. That's because
> genradix_prealloc() leaves initialized members initialized when the
> issue happens and SCTP stack will abort the current initialization but
> without cleaning up such members.
>
> The fix here is to always call genradix_free() when genradix_prealloc()
> fails, for output and also input streams, as it suffers from the same
> issue.
>
> Reported-by: syzbot+772d9e36c490b18d51d1@syzkaller.appspotmail.com
> Fixes: 2075e50caf5e ("sctp: convert to genradix")
> Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> ---
> net/sctp/stream.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/net/sctp/stream.c b/net/sctp/stream.c
> index df60b5ef24cbf5c6f628ab8ed88a6faaaa422b6d..e0b01bf912b3f3cdbc3f713bcfa50868e4802929 100644
> --- a/net/sctp/stream.c
> +++ b/net/sctp/stream.c
> @@ -84,8 +84,10 @@ static int sctp_stream_alloc_out(struct sctp_stream *stream, __u16 outcnt,
> return 0;
>
> ret = genradix_prealloc(&stream->out, outcnt, gfp);
> - if (ret)
> + if (ret) {
> + genradix_free(&stream->out);
> return ret;
> + }
>
> stream->outcnt = outcnt;
> return 0;
> @@ -100,8 +102,10 @@ static int sctp_stream_alloc_in(struct sctp_stream *stream, __u16 incnt,
> return 0;
>
> ret = genradix_prealloc(&stream->in, incnt, gfp);
> - if (ret)
> + if (ret) {
> + genradix_free(&stream->in);
> return ret;
> + }
>
> stream->incnt = incnt;
> return 0;
> --
> 2.23.0
>
Tested-by: Xin Long <lucien.xin@gmail.com>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH net] sctp: fix memleak on err handling of stream initialization
2019-12-17 1:01 [PATCH net] sctp: fix memleak on err handling of stream initialization Marcelo Ricardo Leitner
2019-12-17 8:30 ` Xin Long
@ 2019-12-17 11:55 ` Neil Horman
2019-12-17 12:33 ` Marcelo Ricardo Leitner
2019-12-18 5:59 ` David Miller
2 siblings, 1 reply; 5+ messages in thread
From: Neil Horman @ 2019-12-17 11:55 UTC (permalink / raw)
To: Marcelo Ricardo Leitner; +Cc: netdev, Xin Long, Kent Overstreet, linux-sctp
On Mon, Dec 16, 2019 at 10:01:16PM -0300, Marcelo Ricardo Leitner wrote:
> syzbot reported a memory leak when an allocation fails within
> genradix_prealloc() for output streams. That's because
> genradix_prealloc() leaves initialized members initialized when the
> issue happens and SCTP stack will abort the current initialization but
> without cleaning up such members.
>
> The fix here is to always call genradix_free() when genradix_prealloc()
> fails, for output and also input streams, as it suffers from the same
> issue.
>
> Reported-by: syzbot+772d9e36c490b18d51d1@syzkaller.appspotmail.com
> Fixes: 2075e50caf5e ("sctp: convert to genradix")
> Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> ---
> net/sctp/stream.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/net/sctp/stream.c b/net/sctp/stream.c
> index df60b5ef24cbf5c6f628ab8ed88a6faaaa422b6d..e0b01bf912b3f3cdbc3f713bcfa50868e4802929 100644
> --- a/net/sctp/stream.c
> +++ b/net/sctp/stream.c
> @@ -84,8 +84,10 @@ static int sctp_stream_alloc_out(struct sctp_stream *stream, __u16 outcnt,
> return 0;
>
> ret = genradix_prealloc(&stream->out, outcnt, gfp);
> - if (ret)
> + if (ret) {
> + genradix_free(&stream->out);
> return ret;
> + }
>
> stream->outcnt = outcnt;
> return 0;
> @@ -100,8 +102,10 @@ static int sctp_stream_alloc_in(struct sctp_stream *stream, __u16 incnt,
> return 0;
>
> ret = genradix_prealloc(&stream->in, incnt, gfp);
> - if (ret)
> + if (ret) {
> + genradix_free(&stream->in);
> return ret;
> + }
>
> stream->incnt = incnt;
> return 0;
> --
> 2.23.0
>
>
As mentioned in the other thread, shouldn't genradix_prealloc clean this up
internal to its function. It seems odd having to free memory allocated on
error.
Neil
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH net] sctp: fix memleak on err handling of stream initialization
2019-12-17 11:55 ` Neil Horman
@ 2019-12-17 12:33 ` Marcelo Ricardo Leitner
0 siblings, 0 replies; 5+ messages in thread
From: Marcelo Ricardo Leitner @ 2019-12-17 12:33 UTC (permalink / raw)
To: Neil Horman; +Cc: netdev, Xin Long, Kent Overstreet, linux-sctp
On Tue, Dec 17, 2019 at 06:55:13AM -0500, Neil Horman wrote:
> On Mon, Dec 16, 2019 at 10:01:16PM -0300, Marcelo Ricardo Leitner wrote:
> > syzbot reported a memory leak when an allocation fails within
> > genradix_prealloc() for output streams. That's because
> > genradix_prealloc() leaves initialized members initialized when the
> > issue happens and SCTP stack will abort the current initialization but
> > without cleaning up such members.
> >
> > The fix here is to always call genradix_free() when genradix_prealloc()
> > fails, for output and also input streams, as it suffers from the same
> > issue.
> >
> > Reported-by: syzbot+772d9e36c490b18d51d1@syzkaller.appspotmail.com
> > Fixes: 2075e50caf5e ("sctp: convert to genradix")
> > Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> > ---
> > net/sctp/stream.c | 8 ++++++--
> > 1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/sctp/stream.c b/net/sctp/stream.c
> > index df60b5ef24cbf5c6f628ab8ed88a6faaaa422b6d..e0b01bf912b3f3cdbc3f713bcfa50868e4802929 100644
> > --- a/net/sctp/stream.c
> > +++ b/net/sctp/stream.c
> > @@ -84,8 +84,10 @@ static int sctp_stream_alloc_out(struct sctp_stream *stream, __u16 outcnt,
> > return 0;
> >
> > ret = genradix_prealloc(&stream->out, outcnt, gfp);
> > - if (ret)
> > + if (ret) {
> > + genradix_free(&stream->out);
> > return ret;
> > + }
> >
> > stream->outcnt = outcnt;
> > return 0;
> > @@ -100,8 +102,10 @@ static int sctp_stream_alloc_in(struct sctp_stream *stream, __u16 incnt,
> > return 0;
> >
> > ret = genradix_prealloc(&stream->in, incnt, gfp);
> > - if (ret)
> > + if (ret) {
> > + genradix_free(&stream->in);
> > return ret;
> > + }
> >
> > stream->incnt = incnt;
> > return 0;
> > --
> > 2.23.0
> >
> >
> As mentioned in the other thread, shouldn't genradix_prealloc clean this up
> internal to its function. It seems odd having to free memory allocated on
> error.
Depends on how fatal you're considering that it is. With the current
version of genradix_prealloc() there could be a place that attempted
to pre-allocate it, it failed, and then resumed later on. SCTP can't
do that, because right after this prealloc it will write to each
and every element in there, but someone could.
Marcelo
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] sctp: fix memleak on err handling of stream initialization
2019-12-17 1:01 [PATCH net] sctp: fix memleak on err handling of stream initialization Marcelo Ricardo Leitner
2019-12-17 8:30 ` Xin Long
2019-12-17 11:55 ` Neil Horman
@ 2019-12-18 5:59 ` David Miller
2 siblings, 0 replies; 5+ messages in thread
From: David Miller @ 2019-12-18 5:59 UTC (permalink / raw)
To: marcelo.leitner; +Cc: netdev, lucien.xin, kent.overstreet, nhorman, linux-sctp
From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Date: Mon, 16 Dec 2019 22:01:16 -0300
> syzbot reported a memory leak when an allocation fails within
> genradix_prealloc() for output streams. That's because
> genradix_prealloc() leaves initialized members initialized when the
> issue happens and SCTP stack will abort the current initialization but
> without cleaning up such members.
>
> The fix here is to always call genradix_free() when genradix_prealloc()
> fails, for output and also input streams, as it suffers from the same
> issue.
>
> Reported-by: syzbot+772d9e36c490b18d51d1@syzkaller.appspotmail.com
> Fixes: 2075e50caf5e ("sctp: convert to genradix")
> Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Applied and queued up for -stable.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-12-18 5:59 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-12-17 1:01 [PATCH net] sctp: fix memleak on err handling of stream initialization Marcelo Ricardo Leitner
2019-12-17 8:30 ` Xin Long
2019-12-17 11:55 ` Neil Horman
2019-12-17 12:33 ` Marcelo Ricardo Leitner
2019-12-18 5:59 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox