From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.4 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_MUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 31A12C04AB5 for ; Mon, 3 Jun 2019 21:42:31 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 0589C242DA for ; Mon, 3 Jun 2019 21:42:30 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="kAICzCmY" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726674AbfFCVma (ORCPT ); Mon, 3 Jun 2019 17:42:30 -0400 Received: from mail-qk1-f196.google.com ([209.85.222.196]:41833 "EHLO mail-qk1-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726281AbfFCVm3 (ORCPT ); Mon, 3 Jun 2019 17:42:29 -0400 Received: by mail-qk1-f196.google.com with SMTP id c11so1481565qkk.8; Mon, 03 Jun 2019 14:42:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=9zHfCDG+/L/gkbHgBJ3EZBLamkjNE+UcwXD9DXDZax4=; b=kAICzCmYbmlls9VUAj/D+LE6FKaiRuiddmESmrhT6gWp2+3U76WDWH2Zsq6GcvT/zz GegFfnUaWvqF1TXiamrvxzKNN2lc5Fq97e7fU0cv8g453F7LDIur/1V5asmLIaK4EuBl DnxZD96efqjGvKD/VWGPPkauAI9FRm0ZR2U7tLQVeuszsLtJZJprDprsQli9PvlJlOQZ 4u4MUbYkJuryF1TlkHHLcQ/z7IzaBZSy5uPiy8CxYNWDTR6ZJfHrCXLfeiaNuiTaIjI9 8s97Hgl8fEQjyRZa4o6G5YBPcVZWD8pWqfFtptF18dq1mgWDCaTeiblXwgeEJhiFQBie os1A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=9zHfCDG+/L/gkbHgBJ3EZBLamkjNE+UcwXD9DXDZax4=; b=rYBZPa3wKhk2FLG+7YoUlo5kPM0alN6B+ynFYtzEPu+HIl/si527HCcXKol3wSzuxz o7OVJmyr+60of7wsrhgeSpSrgtCPE7QKOwCarm8X3CFTwT4MysYmbXadgDy6WMBmh4Wo vFK484h3C0J/Egj1NQDBg4ylvLGecsGT9nFETvLooC3rcjytbkloOTBPApQ6jrx7lmFm DRl5qSF4hSv53gRae4KSR0nnK1IcD9mGhyyrVm5SJSzv6aOkr1N7sHrP7zgbADRdVjVo fkdAu9qSrDLzH9yla5OVbOC9l920zSHA6iTik1DnN1sB+rcSaN8R7KgEvQQmmXTJLDwm 1rGw== X-Gm-Message-State: APjAAAVVh5KiOv3eARtLYftc9AIVnlgypAIHZ1x3gQv50/wVeVTo6AZV KvbUuOhYqeahyJSvR3uXvIU= X-Google-Smtp-Source: APXvYqyS3TkiYdcqeV6bp9SqATDtig1mTsCE6xb7S8AK+yJRmhfRrfHxizyH7Onkx+9mj7FjH7vPYw== X-Received: by 2002:a37:30c:: with SMTP id 12mr23686682qkd.175.1559598147743; Mon, 03 Jun 2019 14:42:27 -0700 (PDT) Received: from localhost.localdomain ([2001:1284:f013:b5ff:e40:8c89:ad5c:4ccc]) by smtp.gmail.com with ESMTPSA id d23sm12270534qta.26.2019.06.03.14.42.25 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 03 Jun 2019 14:42:26 -0700 (PDT) Received: by localhost.localdomain (Postfix, from userid 1000) id 5E7C0C1C0C; Mon, 3 Jun 2019 18:42:23 -0300 (-03) Date: Mon, 3 Jun 2019 18:42:23 -0300 From: Marcelo Ricardo Leitner To: Neil Horman Cc: linux-sctp@vger.kernel.org, syzbot+f7e9153b037eac9b1df8@syzkaller.appspotmail.com, "David S. Miller" , netdev@vger.kernel.org Subject: Re: [PATCH V2] Fix memory leak in sctp_process_init Message-ID: <20190603214223.GJ3713@localhost.localdomain> References: <00000000000097abb90589e804fd@google.com> <20190603203259.21508-1-nhorman@tuxdriver.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190603203259.21508-1-nhorman@tuxdriver.com> User-Agent: Mutt/1.11.4 (2019-03-13) Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Mon, Jun 03, 2019 at 04:32:59PM -0400, Neil Horman wrote: > syzbot found the following leak in sctp_process_init > BUG: memory leak > unreferenced object 0xffff88810ef68400 (size 1024): > comm "syz-executor273", pid 7046, jiffies 4294945598 (age 28.770s) > hex dump (first 32 bytes): > 1d de 28 8d de 0b 1b e3 b5 c2 f9 68 fd 1a 97 25 ..(........h...% > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > backtrace: > [<00000000a02cebbd>] kmemleak_alloc_recursive include/linux/kmemleak.h:55 > [inline] > [<00000000a02cebbd>] slab_post_alloc_hook mm/slab.h:439 [inline] > [<00000000a02cebbd>] slab_alloc mm/slab.c:3326 [inline] > [<00000000a02cebbd>] __do_kmalloc mm/slab.c:3658 [inline] > [<00000000a02cebbd>] __kmalloc_track_caller+0x15d/0x2c0 mm/slab.c:3675 > [<000000009e6245e6>] kmemdup+0x27/0x60 mm/util.c:119 > [<00000000dfdc5d2d>] kmemdup include/linux/string.h:432 [inline] > [<00000000dfdc5d2d>] sctp_process_init+0xa7e/0xc20 > net/sctp/sm_make_chunk.c:2437 > [<00000000b58b62f8>] sctp_cmd_process_init net/sctp/sm_sideeffect.c:682 > [inline] > [<00000000b58b62f8>] sctp_cmd_interpreter net/sctp/sm_sideeffect.c:1384 > [inline] > [<00000000b58b62f8>] sctp_side_effects net/sctp/sm_sideeffect.c:1194 > [inline] > [<00000000b58b62f8>] sctp_do_sm+0xbdc/0x1d60 net/sctp/sm_sideeffect.c:1165 > [<0000000044e11f96>] sctp_assoc_bh_rcv+0x13c/0x200 > net/sctp/associola.c:1074 > [<00000000ec43804d>] sctp_inq_push+0x7f/0xb0 net/sctp/inqueue.c:95 > [<00000000726aa954>] sctp_backlog_rcv+0x5e/0x2a0 net/sctp/input.c:354 > [<00000000d9e249a8>] sk_backlog_rcv include/net/sock.h:950 [inline] > [<00000000d9e249a8>] __release_sock+0xab/0x110 net/core/sock.c:2418 > [<00000000acae44fa>] release_sock+0x37/0xd0 net/core/sock.c:2934 > [<00000000963cc9ae>] sctp_sendmsg+0x2c0/0x990 net/sctp/socket.c:2122 > [<00000000a7fc7565>] inet_sendmsg+0x64/0x120 net/ipv4/af_inet.c:802 > [<00000000b732cbd3>] sock_sendmsg_nosec net/socket.c:652 [inline] > [<00000000b732cbd3>] sock_sendmsg+0x54/0x70 net/socket.c:671 > [<00000000274c57ab>] ___sys_sendmsg+0x393/0x3c0 net/socket.c:2292 > [<000000008252aedb>] __sys_sendmsg+0x80/0xf0 net/socket.c:2330 > [<00000000f7bf23d1>] __do_sys_sendmsg net/socket.c:2339 [inline] > [<00000000f7bf23d1>] __se_sys_sendmsg net/socket.c:2337 [inline] > [<00000000f7bf23d1>] __x64_sys_sendmsg+0x23/0x30 net/socket.c:2337 > [<00000000a8b4131f>] do_syscall_64+0x76/0x1a0 arch/x86/entry/common.c:3 > > The problem was that the peer.cookie value points to an skb allocated > area on the first pass through this function, at which point it is > overwritten with a heap allocated value, but in certain cases, where a > COOKIE_ECHO chunk is included in the packet, a second pass through > sctp_process_init is made, where the cookie value is re-allocated, > leaking the first allocation. > > Fix is to always allocate the cookie value, and free it when we are done > using it. > > Signed-off-by: Neil Horman > Reported-by: syzbot+f7e9153b037eac9b1df8@syzkaller.appspotmail.com > CC: Marcelo Ricardo Leitner > CC: "David S. Miller" > CC: netdev@vger.kernel.org Acked-by: Marcelo Ricardo Leitner > > --- > Change notes > > V1->V2: > * Removed an accidental double free I let slip in in > sctp_association_free > * Removed now unused cookie variable > --- > net/sctp/sm_make_chunk.c | 13 +++---------- > net/sctp/sm_sideeffect.c | 5 +++++ > 2 files changed, 8 insertions(+), 10 deletions(-) > > diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c > index 72e74503f9fc..8e12e19fe42d 100644 > --- a/net/sctp/sm_make_chunk.c > +++ b/net/sctp/sm_make_chunk.c > @@ -2327,7 +2327,6 @@ int sctp_process_init(struct sctp_association *asoc, struct sctp_chunk *chunk, > union sctp_addr addr; > struct sctp_af *af; > int src_match = 0; > - char *cookie; > > /* We must include the address that the INIT packet came from. > * This is the only address that matters for an INIT packet. > @@ -2431,14 +2430,6 @@ int sctp_process_init(struct sctp_association *asoc, struct sctp_chunk *chunk, > /* Peer Rwnd : Current calculated value of the peer's rwnd. */ > asoc->peer.rwnd = asoc->peer.i.a_rwnd; > > - /* Copy cookie in case we need to resend COOKIE-ECHO. */ > - cookie = asoc->peer.cookie; > - if (cookie) { > - asoc->peer.cookie = kmemdup(cookie, asoc->peer.cookie_len, gfp); > - if (!asoc->peer.cookie) > - goto clean_up; > - } > - > /* RFC 2960 7.2.1 The initial value of ssthresh MAY be arbitrarily > * high (for example, implementations MAY use the size of the receiver > * advertised window). > @@ -2607,7 +2598,9 @@ 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); > - asoc->peer.cookie = param.cookie->body; > + asoc->peer.cookie = kmemdup(param.cookie->body, asoc->peer.cookie_len, gfp); > + if (!asoc->peer.cookie) > + retval = 0; > break; > > case SCTP_PARAM_HEARTBEAT_INFO: > diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c > index 4aa03588f87b..27ddf2d8f001 100644 > --- a/net/sctp/sm_sideeffect.c > +++ b/net/sctp/sm_sideeffect.c > @@ -898,6 +898,11 @@ static void sctp_cmd_new_state(struct sctp_cmd_seq *cmds, > asoc->rto_initial; > } > > + if (sctp_state(asoc, ESTABLISHED)) { > + kfree(asoc->peer.cookie); > + asoc->peer.cookie = NULL; > + } > + > if (sctp_state(asoc, ESTABLISHED) || > sctp_state(asoc, CLOSED) || > sctp_state(asoc, SHUTDOWN_RECEIVED)) { > -- > 2.20.1 > From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcelo Ricardo Leitner Date: Mon, 03 Jun 2019 21:42:23 +0000 Subject: Re: [PATCH V2] Fix memory leak in sctp_process_init Message-Id: <20190603214223.GJ3713@localhost.localdomain> List-Id: References: <00000000000097abb90589e804fd@google.com> <20190603203259.21508-1-nhorman@tuxdriver.com> In-Reply-To: <20190603203259.21508-1-nhorman@tuxdriver.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Neil Horman Cc: linux-sctp@vger.kernel.org, syzbot+f7e9153b037eac9b1df8@syzkaller.appspotmail.com, "David S. Miller" , netdev@vger.kernel.org On Mon, Jun 03, 2019 at 04:32:59PM -0400, Neil Horman wrote: > syzbot found the following leak in sctp_process_init > BUG: memory leak > unreferenced object 0xffff88810ef68400 (size 1024): > comm "syz-executor273", pid 7046, jiffies 4294945598 (age 28.770s) > hex dump (first 32 bytes): > 1d de 28 8d de 0b 1b e3 b5 c2 f9 68 fd 1a 97 25 ..(........h...% > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > backtrace: > [<00000000a02cebbd>] kmemleak_alloc_recursive include/linux/kmemleak.h:55 > [inline] > [<00000000a02cebbd>] slab_post_alloc_hook mm/slab.h:439 [inline] > [<00000000a02cebbd>] slab_alloc mm/slab.c:3326 [inline] > [<00000000a02cebbd>] __do_kmalloc mm/slab.c:3658 [inline] > [<00000000a02cebbd>] __kmalloc_track_caller+0x15d/0x2c0 mm/slab.c:3675 > [<000000009e6245e6>] kmemdup+0x27/0x60 mm/util.c:119 > [<00000000dfdc5d2d>] kmemdup include/linux/string.h:432 [inline] > [<00000000dfdc5d2d>] sctp_process_init+0xa7e/0xc20 > net/sctp/sm_make_chunk.c:2437 > [<00000000b58b62f8>] sctp_cmd_process_init net/sctp/sm_sideeffect.c:682 > [inline] > [<00000000b58b62f8>] sctp_cmd_interpreter net/sctp/sm_sideeffect.c:1384 > [inline] > [<00000000b58b62f8>] sctp_side_effects net/sctp/sm_sideeffect.c:1194 > [inline] > [<00000000b58b62f8>] sctp_do_sm+0xbdc/0x1d60 net/sctp/sm_sideeffect.c:1165 > [<0000000044e11f96>] sctp_assoc_bh_rcv+0x13c/0x200 > net/sctp/associola.c:1074 > [<00000000ec43804d>] sctp_inq_push+0x7f/0xb0 net/sctp/inqueue.c:95 > [<00000000726aa954>] sctp_backlog_rcv+0x5e/0x2a0 net/sctp/input.c:354 > [<00000000d9e249a8>] sk_backlog_rcv include/net/sock.h:950 [inline] > [<00000000d9e249a8>] __release_sock+0xab/0x110 net/core/sock.c:2418 > [<00000000acae44fa>] release_sock+0x37/0xd0 net/core/sock.c:2934 > [<00000000963cc9ae>] sctp_sendmsg+0x2c0/0x990 net/sctp/socket.c:2122 > [<00000000a7fc7565>] inet_sendmsg+0x64/0x120 net/ipv4/af_inet.c:802 > [<00000000b732cbd3>] sock_sendmsg_nosec net/socket.c:652 [inline] > [<00000000b732cbd3>] sock_sendmsg+0x54/0x70 net/socket.c:671 > [<00000000274c57ab>] ___sys_sendmsg+0x393/0x3c0 net/socket.c:2292 > [<000000008252aedb>] __sys_sendmsg+0x80/0xf0 net/socket.c:2330 > [<00000000f7bf23d1>] __do_sys_sendmsg net/socket.c:2339 [inline] > [<00000000f7bf23d1>] __se_sys_sendmsg net/socket.c:2337 [inline] > [<00000000f7bf23d1>] __x64_sys_sendmsg+0x23/0x30 net/socket.c:2337 > [<00000000a8b4131f>] do_syscall_64+0x76/0x1a0 arch/x86/entry/common.c:3 > > The problem was that the peer.cookie value points to an skb allocated > area on the first pass through this function, at which point it is > overwritten with a heap allocated value, but in certain cases, where a > COOKIE_ECHO chunk is included in the packet, a second pass through > sctp_process_init is made, where the cookie value is re-allocated, > leaking the first allocation. > > Fix is to always allocate the cookie value, and free it when we are done > using it. > > Signed-off-by: Neil Horman > Reported-by: syzbot+f7e9153b037eac9b1df8@syzkaller.appspotmail.com > CC: Marcelo Ricardo Leitner > CC: "David S. Miller" > CC: netdev@vger.kernel.org Acked-by: Marcelo Ricardo Leitner > > --- > Change notes > > V1->V2: > * Removed an accidental double free I let slip in in > sctp_association_free > * Removed now unused cookie variable > --- > net/sctp/sm_make_chunk.c | 13 +++---------- > net/sctp/sm_sideeffect.c | 5 +++++ > 2 files changed, 8 insertions(+), 10 deletions(-) > > diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c > index 72e74503f9fc..8e12e19fe42d 100644 > --- a/net/sctp/sm_make_chunk.c > +++ b/net/sctp/sm_make_chunk.c > @@ -2327,7 +2327,6 @@ int sctp_process_init(struct sctp_association *asoc, struct sctp_chunk *chunk, > union sctp_addr addr; > struct sctp_af *af; > int src_match = 0; > - char *cookie; > > /* We must include the address that the INIT packet came from. > * This is the only address that matters for an INIT packet. > @@ -2431,14 +2430,6 @@ int sctp_process_init(struct sctp_association *asoc, struct sctp_chunk *chunk, > /* Peer Rwnd : Current calculated value of the peer's rwnd. */ > asoc->peer.rwnd = asoc->peer.i.a_rwnd; > > - /* Copy cookie in case we need to resend COOKIE-ECHO. */ > - cookie = asoc->peer.cookie; > - if (cookie) { > - asoc->peer.cookie = kmemdup(cookie, asoc->peer.cookie_len, gfp); > - if (!asoc->peer.cookie) > - goto clean_up; > - } > - > /* RFC 2960 7.2.1 The initial value of ssthresh MAY be arbitrarily > * high (for example, implementations MAY use the size of the receiver > * advertised window). > @@ -2607,7 +2598,9 @@ 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); > - asoc->peer.cookie = param.cookie->body; > + asoc->peer.cookie = kmemdup(param.cookie->body, asoc->peer.cookie_len, gfp); > + if (!asoc->peer.cookie) > + retval = 0; > break; > > case SCTP_PARAM_HEARTBEAT_INFO: > diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c > index 4aa03588f87b..27ddf2d8f001 100644 > --- a/net/sctp/sm_sideeffect.c > +++ b/net/sctp/sm_sideeffect.c > @@ -898,6 +898,11 @@ static void sctp_cmd_new_state(struct sctp_cmd_seq *cmds, > asoc->rto_initial; > } > > + if (sctp_state(asoc, ESTABLISHED)) { > + kfree(asoc->peer.cookie); > + asoc->peer.cookie = NULL; > + } > + > if (sctp_state(asoc, ESTABLISHED) || > sctp_state(asoc, CLOSED) || > sctp_state(asoc, SHUTDOWN_RECEIVED)) { > -- > 2.20.1 >