On Wed, Feb 13, 2019 at 9:52 PM Marcelo Ricardo Leitner
[off-list ref] wrote:

On Wed, Feb 13, 2019 at 12:35:56PM +0800, Xin Long wrote:

quoted

On Wed, Feb 13, 2019 at 4:00 AM syzbot
[off-list ref] wrote:

quoted

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@syzkaller.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
the old data on the surplus streams, it has been dropped in
sctp_stream_outq_migrate().

quoted

quoted

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.