[v4,net] sctp: Free cookie before we memdup a new one
diff mbox series

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

Commit Message

Neil Horman June 12, 2019, 12:38 a.m. UTC
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(+)

Comments

Xin Long June 12, 2019, 5:58 p.m. UTC | #1
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>
Marcelo Ricardo Leitner June 12, 2019, 6:07 p.m. UTC | #2
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>
Neil Horman June 12, 2019, 8:32 p.m. UTC | #3
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>
>
David Miller June 12, 2019, 8:41 p.m. UTC | #4
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

Patch
diff mbox series

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)