netdev - RE: [bug][tipc] Too short struct tipc_subscr passed to tipc_subscrb_rcv

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] [day] [month] [year] [list]
Message-ID: <DB5PR0701MB1958F230F839CD73BDA3950F9A790@DB5PR0701MB1958.eurprd07.prod.outlook.com>
Date:   Thu, 28 Sep 2017 13:23:57 +0000
From:   Jon Maloy <jon.maloy@...csson.com>
To:     Alexander Potapenko <glider@...gle.com>,
        David Miller <davem@...emloft.net>,
        Ying Xue <ying.xue@...driver.com>
CC:     Networking <netdev@...r.kernel.org>,
        syzkaller <syzkaller@...glegroups.com>
Subject: RE: [bug][tipc] Too short struct tipc_subscr passed to
 tipc_subscrb_rcv_cb()



> -----Original Message-----
> From: Alexander Potapenko [mailto:glider@...gle.com]
> Sent: Wednesday, September 27, 2017 14:16
> To: David Miller <davem@...emloft.net>; Ying Xue
> <ying.xue@...driver.com>; Jon Maloy <jon.maloy@...csson.com>
> Cc: Networking <netdev@...r.kernel.org>; syzkaller
> <syzkaller@...glegroups.com>
> Subject: [bug][tipc] Too short struct tipc_subscr passed to
> tipc_subscrb_rcv_cb()
> 
> Hi all,
> 
> I've spotted the following bug in TIPC server code while fuzzing it with
> KMSAN and syzkaller:
> 
> ==========================================================
> ========
> BUG: KMSAN: use of uninitialized memory in
> tipc_subscrb_rcv_cb+0x14a/0xc20
> CPU: 2 PID: 29 Comm: kworker/u8:1 Not tainted 4.13.0+ #3150 Hardware
> name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> Workqueue: tipc_rcv tipc_recv_work
> Call Trace:
>  __dump_stack lib/dump_stack.c:16
>  dump_stack+0x172/0x1c0 lib/dump_stack.c:52
>  kmsan_report+0x145/0x3d0 mm/kmsan/kmsan.c:943
>  __msan_warning_32+0x66/0xb0 mm/kmsan/kmsan_instr.c:477  htohl
> net/tipc/subscr.c:66
>  tipc_subscrb_rcv_cb+0x14a/0xc20 net/tipc/subscr.c:333
>  tipc_receive_from_sock+0x369/0x540 net/tipc/server.c:273
>  tipc_recv_work+0xbe/0x1c0 net/tipc/server.c:546
>  process_one_work+0xf9e/0x1ad0 kernel/workqueue.c:2097
>  worker_thread+0xefd/0x2090 kernel/workqueue.c:2231
>  kthread+0x499/0x620 kernel/kthread.c:232
>  ret_from_fork+0x22/0x30 arch/x86/entry/entry_64.S:425
> origin:
>  save_stack_trace+0x16/0x20 arch/x86/kernel/stacktrace.c:59
> kmsan_save_stack_with_flags mm/kmsan/kmsan.c:302
>  kmsan_internal_poison_shadow+0xb3/0x1b0 mm/kmsan/kmsan.c:198
>  kmsan_kmalloc+0x80/0xe0 mm/kmsan/kmsan.c:337
>  kmem_cache_alloc+0x1e2/0x220 mm/slub.c:2756
>  tipc_receive_from_sock+0xff/0x540 net/tipc/server.c:257
>  tipc_recv_work+0xbe/0x1c0 net/tipc/server.c:546
>  process_one_work+0xf9e/0x1ad0 kernel/workqueue.c:2097
>  worker_thread+0xefd/0x2090 kernel/workqueue.c:2231
>  kthread+0x499/0x620 kernel/kthread.c:232
>  ret_from_fork+0x22/0x30 arch/x86/entry/entry_64.S:425
> ==========================================================
> ========
> (see the reproducer attached).
> 
> Here the user sends 16 bytes via sendmsg(), which the kernel attempts to
> treat as a struct tipc_subscr. Because the actual size of a struct is bigger, we
> end up touching uninitialized fields.
> 
> What is the best way to fix this?
> 
> I think it's too late to check for |len| in tipc_subscrb_rcv_cb()
> (http://elixir.free-
> electrons.com/linux/latest/source/net/tipc/subscr.c#L320),
> because we'll probably need to call tipc_subscrp_cancel(s, subscriber) in
> order to cancel the subscription, which is a bad idea since |s| is invalid.
> We could check that ret >= sizeof(struct tipc_subscr) in
> tipc_receive_from_sock()
> (http://elixir.free-electrons.com/linux/v4.8/source/net/tipc/server.c#L273),
> but this function is agnostic to the type of the data received by the callback,
> so we'd better not hardcode the size there.

I don't think that is a problem. This server was built to be more generic than what
we really need (maybe because we also had a configuration server at some moment)
but you can safely assume that an incoming message should have the size of a 
tipc_subscr, and nothing else.

> 
> On a related note, something is wrong with the tipc_receive_from_sock()'s
> return value.
> First, the user appears to receive a positive value (16 in our case) regardless
> of what tipc_receive_from_sock() returns.
> Second, the branch at
> http://elixir.free-electrons.com/linux/v4.8/source/net/tipc/server.c#L288
> is never taken.

I believe the test at the beginning of the loop

   while (test_bit(CF_CONNECTED, &con->flags)) {
                e = list_entry(con->outqueue.next, struct outqueue_entry,
                               list);
                if ((struct list_head *) e == &con->outqueue)
                        break;
                 .....

is incorrect somehow. I was troubleshooting this code myself the last day, and noted that the loop always seems to take an extra loop after the queue is already empty, of course attempting to send crazy values.

I myself am changing this to

   while (test_bit(CF_CONNECTED, &con->flags) && !list_empty(&con->outqueue)) {
                e = list_first_entry(con->outqueue, struct outqueue_entry,    list);
                 .....

I am also doing some more changes to this, which will be sent out to the tipc-discussion list in the near future.

BR
///jon

> 
> WBR,
> 
> Alexander Potapenko
> Software Engineer
> 
> Google Germany GmbH
> Erika-Mann-Straße, 33
> 80636 München
> 
> Geschäftsführer: Paul Manicle, Halimah DeLaine Prado Registergericht und -
> nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg