Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1031245AbdIZPGu (ORCPT ); Tue, 26 Sep 2017 11:06:50 -0400 Received: from mail-ua0-f179.google.com ([209.85.217.179]:47160 "EHLO mail-ua0-f179.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1031181AbdIZPGp (ORCPT ); Tue, 26 Sep 2017 11:06:45 -0400 X-Google-Smtp-Source: AOwi7QBaFE63/XCsVdrB9xs7djwoxnbCMgL4wzEvCSRXljOED4v+xQ81VkNvAeWEjj+novdCHXRjVfv7fMPlI7HV6O8= MIME-Version: 1.0 In-Reply-To: References: <20170926145359.28344-1-glider@google.com> From: Alexander Potapenko Date: Tue, 26 Sep 2017 17:06:43 +0200 Message-ID: Subject: Re: [PATCH] tun: bail out from tun_get_user() if the skb is empty To: Eric Dumazet Cc: David Miller , Dmitry Vyukov , syzkaller , netdev , LKML Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by nfs id v8QF6uKk023079 Content-Length: 4678 Lines: 124 On Tue, Sep 26, 2017 at 5:02 PM, 'Eric Dumazet' via syzkaller wrote: > On Tue, Sep 26, 2017 at 7:53 AM, Alexander Potapenko wrote: >> KMSAN (https://github.com/google/kmsan) reported accessing uninitialized >> skb->data[0] in the case the skb is empty (i.e. skb->len is 0): >> >> ================================================ >> BUG: KMSAN: use of uninitialized memory in tun_get_user+0x19ba/0x3770 >> CPU: 0 PID: 3051 Comm: probe Not tainted 4.13.0+ #3140 >> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 >> Call Trace: >> ... >> __msan_warning_32+0x66/0xb0 mm/kmsan/kmsan_instr.c:477 >> tun_get_user+0x19ba/0x3770 drivers/net/tun.c:1301 >> tun_chr_write_iter+0x19f/0x300 drivers/net/tun.c:1365 >> call_write_iter ./include/linux/fs.h:1743 >> new_sync_write fs/read_write.c:457 >> __vfs_write+0x6c3/0x7f0 fs/read_write.c:470 >> vfs_write+0x3e4/0x770 fs/read_write.c:518 >> SYSC_write+0x12f/0x2b0 fs/read_write.c:565 >> SyS_write+0x55/0x80 fs/read_write.c:557 >> do_syscall_64+0x242/0x330 arch/x86/entry/common.c:284 >> entry_SYSCALL64_slow_path+0x25/0x25 arch/x86/entry/entry_64.S:245 >> ... >> origin: >> ... >> kmsan_poison_shadow+0x6e/0xc0 mm/kmsan/kmsan.c:211 >> slab_alloc_node mm/slub.c:2732 >> __kmalloc_node_track_caller+0x351/0x370 mm/slub.c:4351 >> __kmalloc_reserve net/core/skbuff.c:138 >> __alloc_skb+0x26a/0x810 net/core/skbuff.c:231 >> alloc_skb ./include/linux/skbuff.h:903 >> alloc_skb_with_frags+0x1d7/0xc80 net/core/skbuff.c:4756 >> sock_alloc_send_pskb+0xabf/0xfe0 net/core/sock.c:2037 >> tun_alloc_skb drivers/net/tun.c:1144 >> tun_get_user+0x9a8/0x3770 drivers/net/tun.c:1274 >> tun_chr_write_iter+0x19f/0x300 drivers/net/tun.c:1365 >> call_write_iter ./include/linux/fs.h:1743 >> new_sync_write fs/read_write.c:457 >> __vfs_write+0x6c3/0x7f0 fs/read_write.c:470 >> vfs_write+0x3e4/0x770 fs/read_write.c:518 >> SYSC_write+0x12f/0x2b0 fs/read_write.c:565 >> SyS_write+0x55/0x80 fs/read_write.c:557 >> do_syscall_64+0x242/0x330 arch/x86/entry/common.c:284 >> return_from_SYSCALL_64+0x0/0x6a arch/x86/entry/entry_64.S:245 >> ================================================ >> >> Make sure tun_get_user() doesn't touch skb->data[0] unless there is >> actual data. >> >> C reproducer below: >> ========================== >> // autogenerated by syzkaller (http://github.com/google/syzkaller) >> >> #define _GNU_SOURCE >> >> #include >> #include >> #include >> #include >> #include >> #include >> >> int main() >> { >> int sock = socket(PF_INET, SOCK_STREAM, IPPROTO_IP); >> int tun_fd = open("/dev/net/tun", O_RDWR); >> struct ifreq req; >> memset(&req, 0, sizeof(struct ifreq)); >> strcpy((char*)&req.ifr_name, "gre0"); >> req.ifr_flags = IFF_UP | IFF_MULTICAST; >> ioctl(tun_fd, TUNSETIFF, &req); >> ioctl(sock, SIOCSIFFLAGS, "gre0"); >> write(tun_fd, "hi", 0); >> return 0; >> } >> ========================== >> >> Signed-off-by: Alexander Potapenko >> --- >> drivers/net/tun.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/net/tun.c b/drivers/net/tun.c >> index 3c9985f29950..25d96f54a729 100644 >> --- a/drivers/net/tun.c >> +++ b/drivers/net/tun.c >> @@ -1496,6 +1496,8 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile, >> switch (tun->flags & TUN_TYPE_MASK) { >> case IFF_TUN: >> if (tun->flags & IFF_NO_PI) { >> + if (!skb->len) >> + return -EINVAL; >> switch (skb->data[0] & 0xf0) { >> case 0x40: >> pi.proto = htons(ETH_P_IP); >> -- >> 2.14.1.821.g8fa685d3b7-goog >> > > Good catch, but... > > You are replacing an harmless read by a memory leak, which is far more > problematic :/ Ah, you're right, I should have done kfree_skb() and incremented the stats. That's also rx_dropped, correct? > -- > You received this message because you are subscribed to the Google Groups "syzkaller" group. > To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/d/optout. -- 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