From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932665AbaLAVYx (ORCPT ); Mon, 1 Dec 2014 16:24:53 -0500 Received: from mx1.redhat.com ([209.132.183.28]:45540 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932389AbaLAVYv (ORCPT ); Mon, 1 Dec 2014 16:24:51 -0500 Message-ID: <1417469076.25958.14.camel@bling.home> Subject: Re: Out-of-bounds write in driver_override_show From: Alex Williamson To: Dmitry Vyukov Cc: Greg Kroah-Hartman , kasan-dev , LKML , Kostya Serebryany , Dmitry Chernenkov , Andrey Konovalov , Greg Thelen , Kees Cook , Robert Swiecki , Andrey Ryabinin , Konstantin Khlebnikov , Yury Gribov Date: Mon, 01 Dec 2014 14:24:36 -0700 In-Reply-To: References: <20141129180505.GC32510@kroah.com> <1417461137.25958.4.camel@bling.home> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 2014-12-01 at 23:20 +0400, Dmitry Vyukov wrote: > On Mon, Dec 1, 2014 at 10:12 PM, Alex Williamson > wrote: > > On Sat, 2014-11-29 at 10:05 -0800, Greg Kroah-Hartman wrote: > >> On Sat, Nov 29, 2014 at 01:38:10PM +0400, Dmitry Vyukov wrote: > >> > Hello, > >> > > >> > I am working on Kernel AddressSanitizer, a fast memory error detector > >> > for kernel: > >> > https://code.google.com/p/address-sanitizer/wiki/AddressSanitizerForKernel > >> > > >> > Here is an error report that I got while running trinity: > >> > >> Alex, this looks like the code you added, you need to properly check for > >> the size of the string before writing it back... > > > > Thanks for the report, I'm not sure I entirely understand the issue > > though. The string at pci_dev.driver_override is bounds checked when > > set in driver_override_store() and I think that the buffer for a sysfs > > show is always a page, right? So by checking strlen are we just > > guarding against some in-kernel setting of driver_override other than > > the _store() path? Thanks, > > > Hi! > > driver_override_store checks it against PATH_MAX, and PATH_MAX is > generally unrelated to PAGE_SIZE. It happens so that in my config > PATH_MAX==PAGE_SIZE. Yet, the sprintf writes PAGE_SIZE+1 (+1 for the > trailing 0 terminator). So even if PATH_MAX==PAGE_SIZE it smashes the > first byte of the next memory block. Ok, so we just need to use snprintf(buf, PAGE_SIZE,...) to avoid the smashing, assumptions about PATH_MAX vs PAGE_SIZE, and check anything in the kernel that may set driver_override. We might end-up dropping the newline on the _show(), but I don't know that it's worth blurring accepting a PATH_MAX string to resolve that. I'll post a patch. Thanks, Alex > >> > BUG: AddressSanitizer: out of bounds access in vsnprintf+0xd0/0x890 at > >> > addr ffff880057483261 > >> > Write of size 1 by task trinity-c54/4461 > >> > ============================================================================= > >> > BUG kmalloc-4096 (Tainted: G W ): kasan error > >> > ----------------------------------------------------------------------------- > >> > INFO: Allocated in seq_open+0x5a/0xe0 age=1571449 cpu=0 pid=28869 > >> > __slab_alloc+0x4c4/0x4e0 > >> > __kmalloc+0x18b/0x1b0 > >> > seq_buf_alloc+0x16/0x40 > >> > traverse+0x243/0x350 > >> > seq_read+0x434/0x6b0 > >> > kernfs_fop_read+0x176/0x1f0 > >> > vfs_read+0xd7/0x240 > >> > SyS_read+0x57/0xc0 > >> > system_call_fastpath+0x12/0x17 > >> > INFO: Slab 0xffffea00015d2000 objects=7 used=7 fp=0x (null) > >> > flags=0x100000000004080 > >> > INFO: Object 0xffff880057482260 @offset=8800 fp=0xffffffffffffffff > >> > > >> > CPU: 0 PID: 4461 Comm: trinity-c54 Tainted: G B W 3.18.0-rc1+ #8 > >> > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 > >> > ffffffff81fbdaac ffff88008e3ab8c8 ffffffff81c85c3f 0000000000001130 > >> > ffff88011b003200 ffff88008e3ab8f8 ffffffff811ee018 ffff88011b003200 > >> > ffffea00015d2000 ffff880057482260 ffff8800d748225f ffff88008e3ab928 > >> > Call Trace: > >> > [] __asan_store1+0x75/0xb0 mm/kasan/kasan.c:417 > >> > [] vsnprintf+0xd0/0x890 lib/vsprintf.c:1860 > >> > [] sprintf+0x40/0x50 lib/vsprintf.c:1989 > >> > [] driver_override_show+0x31/0x40 drivers/base/platform.c:755 > >> > [] dev_attr_show+0x39/0x80 drivers/base/core.c:120 > >> > [] sysfs_kf_seq_show+0x152/0x230 fs/sysfs/file.c:63 > >> > [] kernfs_seq_show+0x76/0x90 fs/kernfs/file.c:168 > >> > [] traverse+0x185/0x350 fs/seq_file.c:120 > >> > [] seq_read+0x434/0x6b0 fs/seq_file.c:191 > >> > [] kernfs_fop_read+0x176/0x1f0 fs/kernfs/file.c:244 > >> > [] do_loop_readv_writev+0x71/0xa0 fs/read_write.c:708 > >> > [] do_readv_writev+0x350/0x360 fs/read_write.c:842 > >> > [] vfs_readv+0x4b/0x70 fs/read_write.c:867 > >> > [< inlined >] SyS_preadv+0xca/0xf0 SYSC_preadv fs/read_write.c:945 > >> > [] SyS_preadv+0xca/0xf0 fs/read_write.c:931 > >> > [] system_call_fastpath+0x12/0x17 > >> > arch/x86/kernel/entry_64.S:422 > >> > Memory state around the buggy address: > >> > ffff880057482f80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > >> > ffff880057483000: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > >> > ffff880057483080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > >> > ffff880057483100: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > >> > ffff880057483180: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > >> > >ffff880057483200: 00 00 00 00 00 00 00 00 00 00 00 00 fc fc fc fc > >> > ^ > >> > ffff880057483280: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc > >> > ffff880057483300: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc > >> > ffff880057483380: fc fc 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > >> > ffff880057483400: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > >> > ffff880057483480: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > >> > ================================================================== > >> > dev_attr_show: driver_override_show+0x0/0x40 returned bad count > >> > fill_read_buffer: dev_attr_show+0x0/0x80 returned bad count > >> > > >> > > >> > My source is on revision f114040e3ea6e07372334ade75d1ee0775c355e1. > >> > > >> > I've looked at source code and it seems that driver_override_store > >> > does not do length sanitization, so driver_override_show smashes > >> > subsequent memory blocks in sprinf (with user-provided data?). > > > > > >