On Thu, Feb 14, 2019 at 10:39 PM Marcelo Ricardo Leitner
[off-list ref] wrote:

On Thu, Feb 14, 2019 at 12:17:45PM -0200, Marcelo Ricardo Leitner wrote:

quoted

On Thu, Feb 14, 2019 at 10:03:30PM +0800, Xin Long wrote:

quoted

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

quoted

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

Ehh, right!

quoted

the old data on the surplus streams, it has been dropped in
sctp_stream_outq_migrate().

Yep.

quoted

quoted

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.

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.