RE: [bug][tipc] Too short struct tipc_subscr passed to tipc_subscrb_rcv_cb()

-----Original Message-----
From: Alexander Potapenko [mailto:glider@google.com]
Sent: Wednesday, September 27, 2017 14:16
To: David Miller <davem@davemloft.net>; Ying Xue
[off-list ref]; Jon Maloy [off-list ref]
Cc: Networking <redacted>; syzkaller
[off-list ref]
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