From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756345AbbAZR21 (ORCPT ); Mon, 26 Jan 2015 12:28:27 -0500 Received: from mailout4.w1.samsung.com ([210.118.77.14]:64099 "EHLO mailout4.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753477AbbAZR2Z (ORCPT ); Mon, 26 Jan 2015 12:28:25 -0500 X-AuditID: cbfec7f4-b7f126d000001e9a-cd-54c678a4140a Message-id: <54C67934.2000807@samsung.com> Date: Mon, 26 Jan 2015 20:28:20 +0300 From: Andrey Ryabinin User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.3.0 MIME-version: 1.0 To: David Laight , "linux-kernel@vger.kernel.org" Cc: "Aneesh Kumar K.V" , Eric Van Hensbergen , Ron Minnich , Latchesar Ionkov , "David S. Miller" , "v9fs-developer@lists.sourceforge.net" , "netdev@vger.kernel.org" Subject: Re: [PATCH] net/9p: fix format string in p9_mount_tag_show() References: <1422290896-25042-1-git-send-email-a.ryabinin@samsung.com> <063D6719AE5E284EB5DD2968C1650D6D1CAD302F@AcuExch.aculab.com> In-reply-to: <063D6719AE5E284EB5DD2968C1650D6D1CAD302F@AcuExch.aculab.com> Content-type: text/plain; charset=windows-1252 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFrrKLMWRmVeSWpSXmKPExsVy+t/xa7pLKo6FGGw/b2Xx+PU8Fos551tY LHYs3cxkMXfWJnaLy7vmsFncm36b1eLYAjGLbb83Mlt8/LuKzYHTo3/2FDaPLStvMnnsnHWX 3WPavU1MHg8ObWbx2L3gM5NH79aFbB6fN8kFcERx2aSk5mSWpRbp2yVwZRybvYy94J1Bxccl V5gaGM9odDFyckgImEhcfn+XHcIWk7hwbz1bFyMXh5DAUkaJGeuaGCGcZiaJ/mV9zCBVvAJa EttXrQLrYBFQlThwoZUJxGYT0JP4N2s7G4gtKhAh8WHVVzaIekGJH5PvsYDYIgLZEmvPtjKC 2MwCt5kkWroKQGxhAVeJueunMEMsa2OUWHFzOlgDp4CXxL2Hj4EWcAA16Encv6gF0SsvsXnN W+YJjAKzkKyYhVA1C0nVAkbmVYyiqaXJBcVJ6bmGesWJucWleel6yfm5mxghUfFlB+PiY1aH GAU4GJV4eCdMPBoixJpYVlyZe4hRgoNZSYRXOvdYiBBvSmJlVWpRfnxRaU5q8SFGJg5OqQbG FJvgKx8mHfh2+Myd9k//dhQd01XXmXh0rjOPw5eVd7nWa55I8Dy67oLqVbtDSlY7km5kJL+x vDXBboeTut5Jx0PV+TO5H4b0b021/H+I7au3ndbF3VmVTdkGn5h/SGnkzCpn2Pvjs8ezdy3x mt+PvDf4tXNV12XJpuOnd1wpuOCf+ijRYbfnUSWW4oxEQy3mouJEAOjVmUVoAgAA Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 01/26/2015 08:04 PM, David Laight wrote: > From: Andrey Ryabinin >> Using "%s" for non-NULL terminated string is quite >> dangerous, since this causes reading out of bounds. >> chan->tag is non-NULL terminated, so precision >> must be specified for printing it. >> >> Fixes: 86c8437383ac ("net/9p: Add sysfs mount_tag file for virtio 9P device") >> Signed-off-by: Andrey Ryabinin >> --- >> net/9p/trans_virtio.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c >> index daa749c..f0d5f90 100644 >> --- a/net/9p/trans_virtio.c >> +++ b/net/9p/trans_virtio.c >> @@ -504,7 +504,8 @@ static ssize_t p9_mount_tag_show(struct device *dev, >> vdev = dev_to_virtio(dev); >> chan = vdev->priv; >> >> - return snprintf(buf, chan->tag_len + 1, "%s", chan->tag); > > Note the 'buffer length' passed to snprintf(). Yes, but 'buffer length' is length of buf, not chan->tag. The problem with "%s" is that vsnprintf expects to chan->tag to be NULL-terminated, so it calls strnlen(chan->tag, -1). Thus we read beyond of allocated memory range: ================================================================== BUG: AddressSanitizer: out of bounds access in strnlen+0xa7/0xb0 at addr ffff88006b882d79 Read of size 1 by task cat/669 ============================================================================= BUG kmalloc-16 (Not tainted): kasan: bad access detected ----------------------------------------------------------------------------- Disabling lock debugging due to kernel taint INFO: Allocated in p9_virtio_probe+0x523/0x11d0 age=7711 cpu=1 pid=1 __slab_alloc.constprop.16+0x765/0x1220 __kmalloc+0x380/0x630 p9_virtio_probe+0x523/0x11d0 virtio_dev_probe+0x739/0x11e0 really_probe+0x204/0xd10 __driver_attach+0x2c1/0x470 bus_for_each_dev+0x16c/0x280 driver_attach+0x48/0x80 bus_add_driver+0x490/0x970 driver_register+0x274/0x620 register_virtio_driver+0x97/0x140 p9_virtio_init+0x31/0x33 do_one_initcall+0x1fb/0x3a0 kernel_init_freeable+0x40d/0x4b1 kernel_init+0xe/0xf0 ret_from_fork+0x7c/0xb0 INFO: Slab 0xffffea0001ae2080 objects=23 used=23 fp=0x (null) flags=0x4000000000004080 INFO: Object 0xffff88006b882d70 @offset=3440 fp=0xffff88006b882ec8 Bytes b4 ffff88006b882d60: 00 00 00 00 00 00 00 00 5a 5a 5a 5a 5a 5a 5a 5a ........ZZZZZZZZ Object ffff88006b882d70: 2f 64 65 76 2f 72 6f 6f 74 6b 6b 6b 6b 6b 6b a5 /dev/rootkkkkkk. Redzone ffff88006b882d80: cc cc cc cc cc cc cc cc ........ Padding ffff88006b882ec0: 5a 5a 5a 5a 5a 5a 5a 5a ZZZZZZZZ CPU: 3 PID: 669 Comm: cat Tainted: G B 3.19.0-rc5+ #168 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.7.5-0-ge51488c-20140602_164612-nilsson.home.kraxel.org 04/01/2014 ffff88006b882d70 ffff880069b8f6e8 ffffffff82101090 0000000000000053 ffff88006c807a80 ffff880069b8f748 ffffffff8152b231 ffff880069b8f758 ffff880069b8f708 ffff880069b8f738 ffff88006d19dd60 ffffffff8237bac4 Call Trace: [] dump_stack+0x45/0x57 [] print_trailer+0x221/0x4b0 [] object_err+0x3a/0x50 [] kasan_report_error+0x3a2/0x9c0 [] ? get_ksymbol+0x1b80/0x1b80 [] ? kasan_unpoison_shadow+0x36/0x70 [] ? kasan_unpoison_shadow+0x36/0x70 [] __asan_report_load1_noabort+0x61/0x80 [] ? strnlen+0xa7/0xb0 [] strnlen+0xa7/0xb0 [] ? __kernel_text_address+0x94/0xc0 [] string.isra.2+0x3f/0x2f0 [] vsnprintf+0x392/0x23b0 [] ? __rmqueue+0x24c0/0x24c0 [] ? pointer.isra.17+0xd80/0xd80 [] ? check_bytes_and_report+0x77/0x290 [] ? deactivate_slab+0x4d1/0x1f00 [] ? p9_virtio_close+0xd0/0xd0 [] snprintf+0x85/0xa0 [] ? vsprintf+0x20/0x20 [] ? alloc_debug_processing+0x1e0/0x4a0 [] p9_mount_tag_show+0xe8/0x180 [] dev_attr_show+0x75/0x170 [] ? memset+0x2c/0x40 [] sysfs_kf_seq_show+0x3fe/0xe80 [] ? dump_trace+0x230/0x920 [] ? sysfs_remove_files+0xd0/0xd0 [] ? kasan_unpoison_shadow+0x36/0x70 [] ? kasan_kmalloc+0x73/0x90 [] kernfs_seq_show+0x1b9/0x330 [] seq_read+0x3be/0x1f30 [] ? handle_mm_fault+0xe86/0x2340 [] ? traverse+0x1000/0x1000 [] ? find_vma+0x21/0x210 [] ? __do_page_fault+0x2bb/0xea0 [] kernfs_fop_read+0x38e/0x6f0 [] ? mm_fault_error+0x410/0x410 [] ? kernfs_vma_page_mkwrite+0x390/0x390 [] __vfs_read+0xbb/0x320 [] vfs_read+0x123/0x320 [] SyS_read+0x109/0x270 [] ? vfs_read+0x320/0x320 [] ? do_async_page_fault+0x19/0x80 [] system_call_fastpath+0x12/0x17 Memory state around the buggy address: ffff88006b882c00: fc fc fc 00 00 fc fc fc fc fc fc fc fc fc fc fc ffff88006b882c80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc >ffff88006b882d00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc 00 01 ^ ffff88006b882d80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc ffff88006b882e00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc ================================================================== > >> + return snprintf(buf, chan->tag_len + 1, "%.*s", >> + chan->tag_len, chan->tag); > > Both these are equivalent to: > buf[chan->tag_len]= 0; > memcpy(buf, chan->tag, chan->tag_len); > > using snprintf() is rather extreme. > Indeed, memcpy is much better here.