| Message ID | 20190612003814.7219-1-nhorman@tuxdriver.com |
|---|---|
| State | Superseded |
| Delegated to: | David Miller |
| Headers | show |
| Series | [v4,net] sctp: Free cookie before we memdup a new one | expand |
On Wed, Jun 12, 2019 at 8:38 AM Neil Horman <nhorman@tuxdriver.com> wrote: > > Based on comments from Xin, even after fixes for our recent syzbot > report of cookie memory leaks, its possible to get a resend of an INIT > chunk which would lead to us leaking cookie memory. > > To ensure that we don't leak cookie memory, free any previously > allocated cookie first. > > --- > Change notes > v1->v2 > update subsystem tag in subject (davem) > repeat kfree check for peer_random and peer_hmacs (xin) > > v2->v3 > net->sctp > also free peer_chunks > > v3->v4 > fix subject tags > > Signed-off-by: Neil Horman <nhorman@tuxdriver.com> > Reported-by: syzbot+f7e9153b037eac9b1df8@syzkaller.appspotmail.com > CC: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> > CC: Xin Long <lucien.xin@gmail.com> > CC: "David S. Miller" <davem@davemloft.net> > CC: netdev@vger.kernel.org > --- > net/sctp/sm_make_chunk.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c > index f17908f5c4f3..9b0e5b0d701a 100644 > --- a/net/sctp/sm_make_chunk.c > +++ b/net/sctp/sm_make_chunk.c > @@ -2583,6 +2583,8 @@ static int sctp_process_param(struct sctp_association *asoc, > case SCTP_PARAM_STATE_COOKIE: > asoc->peer.cookie_len = > ntohs(param.p->length) - sizeof(struct sctp_paramhdr); > + if (asoc->peer.cookie) > + kfree(asoc->peer.cookie); > asoc->peer.cookie = kmemdup(param.cookie->body, asoc->peer.cookie_len, gfp); > if (!asoc->peer.cookie) > retval = 0; > @@ -2647,6 +2649,8 @@ static int sctp_process_param(struct sctp_association *asoc, > goto fall_through; > > /* Save peer's random parameter */ > + if (asoc->peer.peer_random) > + kfree(asoc->peer.peer_random); > asoc->peer.peer_random = kmemdup(param.p, > ntohs(param.p->length), gfp); > if (!asoc->peer.peer_random) { > @@ -2660,6 +2664,8 @@ static int sctp_process_param(struct sctp_association *asoc, > goto fall_through; > > /* Save peer's HMAC list */ > + if (asoc->peer.peer_hmacs) > + kfree(asoc->peer.peer_hmacs); > asoc->peer.peer_hmacs = kmemdup(param.p, > ntohs(param.p->length), gfp); > if (!asoc->peer.peer_hmacs) { > @@ -2675,6 +2681,8 @@ static int sctp_process_param(struct sctp_association *asoc, > if (!ep->auth_enable) > goto fall_through; > > + if (asoc->peer.peer_chunks) > + kfree(asoc->peer.peer_chunks); > asoc->peer.peer_chunks = kmemdup(param.p, > ntohs(param.p->length), gfp); > if (!asoc->peer.peer_chunks) > -- > 2.20.1 > Reviewed-by: Xin Long <lucien.xin@gmail.com>
On Tue, Jun 11, 2019 at 08:38:14PM -0400, Neil Horman wrote: > Based on comments from Xin, even after fixes for our recent syzbot > report of cookie memory leaks, its possible to get a resend of an INIT > chunk which would lead to us leaking cookie memory. > > To ensure that we don't leak cookie memory, free any previously > allocated cookie first. > > --- This marker can't be here, as it causes git to loose everything below. > Change notes > v1->v2 > update subsystem tag in subject (davem) > repeat kfree check for peer_random and peer_hmacs (xin) > > v2->v3 > net->sctp > also free peer_chunks > > v3->v4 > fix subject tags > > Signed-off-by: Neil Horman <nhorman@tuxdriver.com> > Reported-by: syzbot+f7e9153b037eac9b1df8@syzkaller.appspotmail.com > CC: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> > CC: Xin Long <lucien.xin@gmail.com> > CC: "David S. Miller" <davem@davemloft.net> > CC: netdev@vger.kernel.org Anyhow, LGTM and reproducer didn't give any hits in 2 runs of 50mins. Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
On Wed, Jun 12, 2019 at 03:07:15PM -0300, Marcelo Ricardo Leitner wrote: > On Tue, Jun 11, 2019 at 08:38:14PM -0400, Neil Horman wrote: > > Based on comments from Xin, even after fixes for our recent syzbot > > report of cookie memory leaks, its possible to get a resend of an INIT > > chunk which would lead to us leaking cookie memory. > > > > To ensure that we don't leak cookie memory, free any previously > > allocated cookie first. > > > > --- > > This marker can't be here, as it causes git to loose everything below. > thats intentional so that, when Dave commits it, the change notes arent carried into the changelog (I.e. the change notes are useful for email review, but not especially useful in the permanent commit history). Neil > > Change notes > > v1->v2 > > update subsystem tag in subject (davem) > > repeat kfree check for peer_random and peer_hmacs (xin) > > > > v2->v3 > > net->sctp > > also free peer_chunks > > > > v3->v4 > > fix subject tags > > > > Signed-off-by: Neil Horman <nhorman@tuxdriver.com> > > Reported-by: syzbot+f7e9153b037eac9b1df8@syzkaller.appspotmail.com > > CC: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> > > CC: Xin Long <lucien.xin@gmail.com> > > CC: "David S. Miller" <davem@davemloft.net> > > CC: netdev@vger.kernel.org > > Anyhow, LGTM and reproducer didn't give any hits in 2 runs of 50mins. > Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> >
From: Neil Horman <nhorman@tuxdriver.com> Date: Wed, 12 Jun 2019 16:32:30 -0400 > On Wed, Jun 12, 2019 at 03:07:15PM -0300, Marcelo Ricardo Leitner wrote: >> On Tue, Jun 11, 2019 at 08:38:14PM -0400, Neil Horman wrote: >> > Based on comments from Xin, even after fixes for our recent syzbot >> > report of cookie memory leaks, its possible to get a resend of an INIT >> > chunk which would lead to us leaking cookie memory. >> > >> > To ensure that we don't leak cookie memory, free any previously >> > allocated cookie first. >> > >> > --- >> >> This marker can't be here, as it causes git to loose everything below. >> > thats intentional so that, when Dave commits it, the change notes arent carried > into the changelog (I.e. the change notes are useful for email review, but not > especially useful in the permanent commit history). 1) I like the changelog notes to be included 2) Your signoff etc. was in that area too so would have been lost as well
diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c index f17908f5c4f3..9b0e5b0d701a 100644 --- a/net/sctp/sm_make_chunk.c +++ b/net/sctp/sm_make_chunk.c @@ -2583,6 +2583,8 @@ static int sctp_process_param(struct sctp_association *asoc, case SCTP_PARAM_STATE_COOKIE: asoc->peer.cookie_len = ntohs(param.p->length) - sizeof(struct sctp_paramhdr); + if (asoc->peer.cookie) + kfree(asoc->peer.cookie); asoc->peer.cookie = kmemdup(param.cookie->body, asoc->peer.cookie_len, gfp); if (!asoc->peer.cookie) retval = 0; @@ -2647,6 +2649,8 @@ static int sctp_process_param(struct sctp_association *asoc, goto fall_through; /* Save peer's random parameter */ + if (asoc->peer.peer_random) + kfree(asoc->peer.peer_random); asoc->peer.peer_random = kmemdup(param.p, ntohs(param.p->length), gfp); if (!asoc->peer.peer_random) { @@ -2660,6 +2664,8 @@ static int sctp_process_param(struct sctp_association *asoc, goto fall_through; /* Save peer's HMAC list */ + if (asoc->peer.peer_hmacs) + kfree(asoc->peer.peer_hmacs); asoc->peer.peer_hmacs = kmemdup(param.p, ntohs(param.p->length), gfp); if (!asoc->peer.peer_hmacs) { @@ -2675,6 +2681,8 @@ static int sctp_process_param(struct sctp_association *asoc, if (!ep->auth_enable) goto fall_through; + if (asoc->peer.peer_chunks) + kfree(asoc->peer.peer_chunks); asoc->peer.peer_chunks = kmemdup(param.p, ntohs(param.p->length), gfp); if (!asoc->peer.peer_chunks)
Based on comments from Xin, even after fixes for our recent syzbot report of cookie memory leaks, its possible to get a resend of an INIT chunk which would lead to us leaking cookie memory. To ensure that we don't leak cookie memory, free any previously allocated cookie first. --- Change notes v1->v2 update subsystem tag in subject (davem) repeat kfree check for peer_random and peer_hmacs (xin) v2->v3 net->sctp also free peer_chunks v3->v4 fix subject tags Signed-off-by: Neil Horman <nhorman@tuxdriver.com> Reported-by: syzbot+f7e9153b037eac9b1df8@syzkaller.appspotmail.com CC: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> CC: Xin Long <lucien.xin@gmail.com> CC: "David S. Miller" <davem@davemloft.net> CC: netdev@vger.kernel.org --- net/sctp/sm_make_chunk.c | 8 ++++++++ 1 file changed, 8 insertions(+)