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. Shall we add an integer field (or yet another callback) to tipc_server, maybe? 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. 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
// autogenerated by syzkaller (http://github.com/google/syzkaller) #include <arpa/inet.h> #include <linux/tipc.h> #include <string.h> int main() { int sock = socket(AF_TIPC, SOCK_SEQPACKET, 0); struct sockaddr_tipc addr; memset(&addr, 0, sizeof(addr)); addr.family = AF_TIPC; addr.addrtype = TIPC_ADDR_NAME; addr.addr.name.name.type = 1; addr.addr.name.name.instance = 1; struct tipc_subscr subscr; memset(&subscr, 0, sizeof(subscr)); struct iovec iov; iov.iov_base = &subscr; iov.iov_len = sizeof(subscr) - 12; struct msghdr hdr; memset(&hdr, 0, sizeof(hdr)); hdr.msg_name = &addr; hdr.msg_namelen = sizeof(addr); hdr.msg_iov = &iov; hdr.msg_iovlen = 1; sendmsg(sock, &hdr, 0); return 0; }
