netdev - Re: [PATCH v2] sctp: Fix double free in sctp_sendmsg_to

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CADvbK_fhW5w4XG2jXMaYSdGLKQPRn9Bd=3=s-+7JH9Tnf38wwQ@mail.gmail.com>
Date:   Tue, 13 Mar 2018 15:03:20 +0800
From:   Xin Long <lucien.xin@...il.com>
To:     Neil Horman <nhorman@...driver.com>
Cc:     linux-sctp@...r.kernel.org, network dev <netdev@...r.kernel.org>,
        davem <davem@...emloft.net>
Subject: Re: [PATCH v2] sctp: Fix double free in sctp_sendmsg_to_asoc

On Tue, Mar 13, 2018 at 2:15 AM, Neil Horman <nhorman@...driver.com> wrote:
> syzbot/kasan detected a double free in sctp_sendmsg_to_asoc:
> BUG: KASAN: use-after-free in sctp_association_free+0x7b7/0x930
> net/sctp/associola.c:332
> Read of size 8 at addr ffff8801d8006ae0 by task syzkaller914861/4202
>
> CPU: 1 PID: 4202 Comm: syzkaller914861 Not tainted 4.16.0-rc4+ #258
> Hardware name: Google Google Compute Engine/Google Compute Engine
> 01/01/2011
> Call Trace:
>  __dump_stack lib/dump_stack.c:17 [inline]
>  dump_stack+0x194/0x24d lib/dump_stack.c:53
>  print_address_description+0x73/0x250 mm/kasan/report.c:256
>  kasan_report_error mm/kasan/report.c:354 [inline]
>  kasan_report+0x23c/0x360 mm/kasan/report.c:412
>  __asan_report_load8_noabort+0x14/0x20 mm/kasan/report.c:433
>  sctp_association_free+0x7b7/0x930 net/sctp/associola.c:332
>  sctp_sendmsg+0xc67/0x1a80 net/sctp/socket.c:2075
>  inet_sendmsg+0x11f/0x5e0 net/ipv4/af_inet.c:763
>  sock_sendmsg_nosec net/socket.c:629 [inline]
>  sock_sendmsg+0xca/0x110 net/socket.c:639
>  SYSC_sendto+0x361/0x5c0 net/socket.c:1748
>  SyS_sendto+0x40/0x50 net/socket.c:1716
>  do_syscall_64+0x281/0x940 arch/x86/entry/common.c:287
>  entry_SYSCALL_64_after_hwframe+0x42/0xb7
>
> This was introduced by commit:
> f84af33 sctp: factor out sctp_sendmsg_to_asoc from sctp_sendmsg
>
> As the newly refactored function moved the wait_for_sndbuf call to a
> point after the association was connected, allowing for peeloff events
> to occur, which in turn caused wait_for_sndbuf to return -EPIPE which
> was not caught by the logic that determines if an association should be
> freed or not.
>
> Fix it the easy way by returning the ordering of
> sctp_primitive_ASSOCIATE and sctp_wait_for_sndbuf to the old order, to
> ensure that EPIPE will not happen.
>
> Tested by myself using the syzbot reproducers with positive results
>
> Signed-off-by: Neil Horman <nhorman@...driver.com>
> CC: davem@...emloft.net
> CC: Xin Long <lucien.xin@...il.com>
> Reported-by: syzbot+a4e4112c3aff00c8cfd8@...kaller.appspotmail.com
>
> ---
> Change notes
> v2)
>  * Moved additional calls to restore origional ordering
>  * add sctp prefix
> ---
>  net/sctp/socket.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 7d3476a4860d..4bbfcf9532c2 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -1876,6 +1876,19 @@ static int sctp_sendmsg_to_asoc(struct sctp_association *asoc,
>                 goto err;
>         }
>
> +       if (asoc->pmtu_pending)
> +               sctp_assoc_pending_pmtu(asoc);
> +
> +       if (sctp_wspace(asoc) < msg_len)
> +               sctp_prsctp_prune(asoc, sinfo, msg_len - sctp_wspace(asoc));
> +
> +       if (!sctp_wspace(asoc)) {
> +               timeo = sock_sndtimeo(sk, msg->msg_flags & MSG_DONTWAIT);
> +               err = sctp_wait_for_sndbuf(asoc, &timeo, msg_len);
> +               if (err)
> +                       goto err;
> +       }
> +
>         if (sctp_state(asoc, CLOSED)) {
>                 err = sctp_primitive_ASSOCIATE(net, asoc, NULL);
>                 if (err)
> @@ -1893,19 +1906,6 @@ static int sctp_sendmsg_to_asoc(struct sctp_association *asoc,
>                 pr_debug("%s: we associated primitively\n", __func__);
>         }
>
> -       if (asoc->pmtu_pending)
> -               sctp_assoc_pending_pmtu(asoc);
> -
> -       if (sctp_wspace(asoc) < msg_len)
> -               sctp_prsctp_prune(asoc, sinfo, msg_len - sctp_wspace(asoc));
> -
> -       if (!sctp_wspace(asoc)) {
> -               timeo = sock_sndtimeo(sk, msg->msg_flags & MSG_DONTWAIT);
> -               err = sctp_wait_for_sndbuf(asoc, &timeo, msg_len);
> -               if (err)
> -                       goto err;
> -       }
> -
>         datamsg = sctp_datamsg_from_user(asoc, sinfo, &msg->msg_iter);
>         if (IS_ERR(datamsg)) {
>                 err = PTR_ERR(datamsg);
> --
> 2.14.3
>
Reviewed-by: Xin Long <lucien.xin@...il.com>