netdev - Re: KASAN: use-after-free Read in sctp_outq

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]
Date:   Thu, 14 Feb 2019 22:52:00 +0800
From:   Xin Long <lucien.xin@...il.com>
To:     Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
Cc:     syzbot <syzbot+7823fa3f3e2d69341ea8@...kaller.appspotmail.com>,
        davem <davem@...emloft.net>, LKML <linux-kernel@...r.kernel.org>,
        linux-sctp@...r.kernel.org, network dev <netdev@...r.kernel.org>,
        Neil Horman <nhorman@...driver.com>,
        syzkaller-bugs <syzkaller-bugs@...glegroups.com>,
        Vlad Yasevich <vyasevich@...il.com>
Subject: Re: KASAN: use-after-free Read in sctp_outq_tail

On Thu, Feb 14, 2019 at 10:39 PM Marcelo Ricardo Leitner
<marcelo.leitner@...il.com> wrote:
>
> On Thu, Feb 14, 2019 at 12:17:45PM -0200, Marcelo Ricardo Leitner wrote:
> > On Thu, Feb 14, 2019 at 10:03:30PM +0800, Xin Long wrote:
> > > On Wed, Feb 13, 2019 at 9:52 PM Marcelo Ricardo Leitner
> > > <marcelo.leitner@...il.com> wrote:
> > > >
> > > > On Wed, Feb 13, 2019 at 12:35:56PM +0800, Xin Long wrote:
> > > > > On Wed, Feb 13, 2019 at 4:00 AM syzbot
> > > > > <syzbot+7823fa3f3e2d69341ea8@...kaller.appspotmail.com> wrote:
> > > > > >
> > > > > > Hello,
> > > > > >
> > > > > > syzbot found the following crash on:
> > > > > >
> > > > > > HEAD commit:    d4104460aec1 Add linux-next specific files for 20190211
> > > > > > git tree:       linux-next
> > > > > > console output: https://syzkaller.appspot.com/x/log.txt?x=14140124c00000
> > > > > > kernel config:  https://syzkaller.appspot.com/x/.config?x=c8a112d3b0d6719b
> > > > > > dashboard link: https://syzkaller.appspot.com/bug?extid=7823fa3f3e2d69341ea8
> > > > > > compiler:       gcc (GCC) 9.0.0 20181231 (experimental)
> > > > > >
> > > > > > Unfortunately, I don't have any reproducer for this crash yet.
> > > > > >
> > > > > > IMPORTANT: if you fix the bug, please add the following tag to the commit:
> > > > > > Reported-by: syzbot+7823fa3f3e2d69341ea8@...kaller.appspotmail.com
> > > > > >
> > > > > > ==================================================================
> > > > > > BUG: KASAN: use-after-free in list_add_tail include/linux/list.h:93 [inline]
> > > > > > BUG: KASAN: use-after-free in sctp_outq_tail_data net/sctp/outqueue.c:105
> > > > > > [inline]
> > > > > > BUG: KASAN: use-after-free in sctp_outq_tail+0x816/0x930
> > > > > > net/sctp/outqueue.c:313
> > > > > > Read of size 8 at addr ffff88807b19a7b8 by task syz-executor.0/30745
> > > > > I think https://patchwork.ozlabs.org/patch/1040500/ will fix this.
> > > >
> > > > I don't think so. Seems it will switch from use-after-free to NULL deref
> > > > instead with that patch.
> > > It will allocate ext for the stream if its ext is NULL in
> > > sctp_sendmsg_to_asoc(), the new data will work fine. As for
> >
> > Ehh, right!
> >
> > > the old data on the surplus streams, it has been dropped in
> > > sctp_stream_outq_migrate().
> >
> > Yep.
> >
> > >
> > > >
> > > > > > CPU: 1 PID: 30745 Comm: syz-executor.0 Not tainted 5.0.0-rc5-next-20190211
> > > > > > #32
> > > > > > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> > > > > > Google 01/01/2011
> > > > > > Call Trace:
> > > > > >   __dump_stack lib/dump_stack.c:77 [inline]
> > > > > >   dump_stack+0x172/0x1f0 lib/dump_stack.c:113
> > > > > >   print_address_description.cold+0x7c/0x20d mm/kasan/report.c:187
> > > > > >   kasan_report.cold+0x1b/0x40 mm/kasan/report.c:317
> > > > > >   __asan_report_load8_noabort+0x14/0x20 mm/kasan/generic_report.c:132
> > > > > >   list_add_tail include/linux/list.h:93 [inline]
> > > > > >   sctp_outq_tail_data net/sctp/outqueue.c:105 [inline]
> > > > > >   sctp_outq_tail+0x816/0x930 net/sctp/outqueue.c:313
> > > > > >   sctp_cmd_send_msg net/sctp/sm_sideeffect.c:1109 [inline]
> > > > > >   sctp_cmd_interpreter net/sctp/sm_sideeffect.c:1784 [inline]
> > > > > >   sctp_side_effects net/sctp/sm_sideeffect.c:1220 [inline]
> > > > > >   sctp_do_sm+0x68e/0x5380 net/sctp/sm_sideeffect.c:1191
> > > > > >   sctp_primitive_SEND+0xa0/0xd0 net/sctp/primitive.c:178
> > > > > >   sctp_sendmsg_to_asoc+0xa63/0x17b0 net/sctp/socket.c:1955
> > > >
> > > > sctp_sendmsg_to_asoc()
> > > > ...
> > > >         if (sinfo->sinfo_stream >= asoc->stream.outcnt) {
> > > >                 err = -EINVAL;
> > > >                 goto err;
> > > >         }
> > > >
> > > >         if (unlikely(!SCTP_SO(&asoc->stream, sinfo->sinfo_stream)->ext)) {
> > > > ...
> > > >
> > > > It should have aborted even if an old ->ext was still there because
> > > > outcnt is correctly updated. So somehow outcnt was wrong here.
> > > >
> > > > sctp_stream_init()
> > > > ...
> > > >         /* Filter out chunks queued on streams that won't exist anymore */
> > > >         sched->unsched_all(stream);
> > > >         sctp_stream_outq_migrate(stream, NULL, outcnt);   <--- [A]
> > > >         sched->sched_all(stream);
> > > >
> > > >         ret = sctp_stream_alloc_out(stream, outcnt, gfp); <--- [B]
> > > >         if (ret)
> > > >                 goto out;
> > > >
> > > >         stream->outcnt = outcnt;                          <--- [C]
> > > > ...
> > > >
> > > > We have a problem here because [A] is freeing ->ext, but [B] can fail (ENOMEM),
> > > > which would lead it to not update outcnt in [C] even after the
> > > > changes already performed in [A].
> > > >
> > > > It should handle the freeing of ->ext in sctp_stream_alloc_out()
> > > > instead, or allocate the flexarray earlier (so it can bail out before
> > > > freeing stuff).
> > > Agree that it's better to do:
> > >         sched->unsched_all(stream);
> > >         sctp_stream_outq_migrate(stream, NULL, outcnt);
> > >         sched->sched_all(stream);
> > > after the flexarray allocation.
> > >
> > > Just sctp_stream_alloc_out() can not be used here anymore, as
> > > stream->out will be set in it.
> >
> > What about carving out a sctp_stream_init_out() ? Like
> >
> >        struct flex_array *new_out;
> >
> >       /* just allocate it, nothing more */
> >          new_out = sctp_stream_alloc_out(outcnt, gfp);
> >          if (!new_out)
> >               goto out;
> >
> >          /* Filter out chunks queued on streams that won't exist anymore */
> >          sched->unsched_all(stream);
> >          sctp_stream_outq_migrate(stream, NULL, outcnt);
> >          sched->sched_all(stream);
> >
> >       /* initialization stuff, and it can't fail anymore */
> >          sctp_stream_init_out(stream, outcnt, new_out);
> >
> > This may hurt a bit more with the genradix migration, but then we
> > don't end up dropping data for nothing.
>
> Reviewing calls to this function, if this allocation fails it will
> either cause the asoc to be terminated or sendmsg error.  So data loss
> is not really an issue here.
>
  sctp_stream_init+0xbc/0x410 net/sctp/stream.c:139
  sctp_process_init+0x21c3/0x2b20 net/sctp/sm_make_chunk.c:2466
  sctp_cmd_process_init net/sctp/sm_sideeffect.c:682 [inline]
  sctp_cmd_interpreter net/sctp/sm_sideeffect.c:1410 [inline]

on this path, seems that it won't terminate the asoc nor report a sending error.