Message-ID: <20190607124813.GA3436@localhost.localdomain>
Date:   Fri, 7 Jun 2019 09:48:13 -0300
From:   Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
To:     Neil Horman <nhorman@...driver.com>
Cc:     Xin Long <lucien.xin@...il.com>, linux-sctp@...r.kernel.org,
        syzbot+f7e9153b037eac9b1df8@...kaller.appspotmail.com,
        "David S. Miller" <davem@...emloft.net>,
        network dev <netdev@...r.kernel.org>
Subject: Re: [PATCH V2] Fix memory leak in sctp_process_init

On Fri, Jun 07, 2019 at 06:56:39AM -0400, Neil Horman wrote:
> On Thu, Jun 06, 2019 at 12:47:55PM -0300, Marcelo Ricardo Leitner wrote:
> > On Wed, Jun 05, 2019 at 07:20:10AM -0400, Neil Horman wrote:
> > > On Wed, Jun 05, 2019 at 04:16:24AM +0800, Xin Long wrote:
> > > > On Tue, Jun 4, 2019 at 4:34 AM Neil Horman <nhorman@...driver.com> 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.
> > > > This's not gonna happen, as after processing INIT, the temp asoc will be
> > > > deleted on the server side. Besides, from the reproducer:
> > > > 
> > > >   https://syzkaller.appspot.com/x/repro.syz?x=10e32f8ca00000
> > > > 
> > > > Packet(INIT|COOKIE_ECHO) can't be made in here.
> > > > 
> > > > The call trace says the leak happened when processing INIT_ACK on the
> > > > client side, as Marcelo noticed.  Then it can be triggered by:
> > > > 
> > > > 1. sctp_sf_do_5_1C_ack() -> SCTP_CMD_PEER_INIT -> sctp_process_init():
> > > >    where it "goto clean_up" after sctp_process_param(), but in 'cleanup'
> > > >    path, it doesn't do any freeup for the memdups of sctp_process_param().
> > > >    then the client T1 retrans INIT, and later get INIT_ACK again from the
> > > >    peer, and go to sctp_process_init() to memdup().
> > > > 
> > > > 2. sctp_sf_cookie_echoed_err() -> sctp_sf_do_5_2_6_stale():
> > > >    where the asoc state will go from COOKIE_ECHOED back to COOKIE_WAIT,
> > > >    and T1 starts to retrans INIT, and later it will get INIT_ACK again
> > > >    to sctp_process_init() and memdup().
> > > > 
> > > ok, you may well be right as to the path that we take to get here, but the root
> > > cause is the same, multiple passes through sctp_process_init without freeing the
> > > previously memduped memory.  Thats why I would think my patch is fixing the
> > > issue, because now we're always duping the cookie memory and always freeing it
> > > when we transition to the ESTABLISHED state.
> > > 
> > > As for the other variables (peer_[random|chunks|hmacs]), I'm not sure why we're
> > > not seeing leak reports of those variables.
> > 
> > This actually also works for peer.cookie as well, as now they are
> > allocated and freed in the same places.
> > 
> > I talked with Xin and I agree that this patch didn't really fix the
> > leak. It moved the allocation from sctp_process_init() to
> > sctp_process_param() which is called by sctp_process_init and there is
> > nothing avoiding multiple allocations on these vars once INIT_ACK is
> > retransmited.
> > 
> > The reproducer works by having the client to re-request cookies by
> > setting a short lifetime. So
> > Client sends INIT
> >                        Server replies INIT_ACK with cookie
> > Client echoes the cookie
> >                        Server rejects because the cookie expired
> > Client sends INIT again, to ask for another cookie
> >   (sctp_sf_do_5_2_6_stale)
> >                        Server sends INIT_ACK again with a new cookie
> > Client calls sctp_process_init again, from
> >   -> sctp_sf_do_5_1C_ack()
> >    -> sctp_cmd_process_init()
> >     -> sctp_process_init()
> >      -> sctp_process_param()
> >         ^-- leak happens
> > 	               Server may or may not accept the new cookie,
> > 		       leading to further leaks if not
> If this is the case, it seems like the most reasonable fix then is to just
> check to see if peer.cookie is non-null prior to calling kmemdup, and freeing
> the old cookie first, no?

That probably would be clearer and easier to maintain later, yes.
Xin?

  Marcelo

> 
> Neil
> 
> > (this is his 2nd case above)
> > 
> > Yet somehow the patch changed the dynamics and made the test pass (4x
> > 10mins each run) here. Uff.
> > 
> >   Marcelo
> > 
> > > 
> > > Neil
> > > 
> > > > As on either above, asoc's never been to ESTABLISHED state,
> > > > asoc->peer.cookie can be not freed, and this patch won't work.
> > > > But yes, it's nice to have this patch, just not to fix this memleak.
> > 
> > +1 (read: won't hurt -stable and no need to revert)
> > 
> > > > 
> > > > I tracked the code, this memleak was triggered by case 2, so I think
> > > > you also need to add something like:
> > > > 
> > > > @@ -881,6 +893,18 @@ static void sctp_cmd_new_state(struct sctp_cmd_seq *cmds,
> > > >                                                 asoc->rto_initial;
> > > >                 asoc->timeouts[SCTP_EVENT_TIMEOUT_T1_COOKIE] =
> > > >                                                 asoc->rto_initial;
> > > > +
> > > > +               if (asoc->peer.cookie) {
> > > > +                       kfree(asoc->peer.cookie);
> > > > +                       kfree(asoc->peer.peer_random);
> > > > +                       kfree(asoc->peer.peer_chunks);
> > > > +                       kfree(asoc->peer.peer_hmacs);
> > > > +
> > > > +                       asoc->peer.cookie = NULL;
> > > > +                       asoc->peer.peer_random = NULL;
> > > > +                       asoc->peer.peer_random = NULL;
> > > > +                       asoc->peer.peer_hmacs = NULL;
> > > > +               }
> > > >         }
> > > > 
> > > > and the same thing may also be needed in sctp_cmd_process_init() on the
> > > > err path for case 1.
> > > > 
> > > > >
> > > > > Fix is to always allocate the cookie value, and free it when we are done
> > > > > using it.
> > > > >
> > > > > Signed-off-by: Neil Horman <nhorman@...driver.com>
> > > > > Reported-by: syzbot+f7e9153b037eac9b1df8@...kaller.appspotmail.com
> > > > > CC: Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
> > > > > CC: "David S. Miller" <davem@...emloft.net>
> > > > > CC: netdev@...r.kernel.org
> > > > >
> > > > > ---
> > > > > 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
> > > > >
> > > > 
> > > 
> > 

Powered by blists - more mailing lists