Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752254AbcLFVDz (ORCPT ); Tue, 6 Dec 2016 16:03:55 -0500 Received: from mx1.redhat.com ([209.132.183.28]:35450 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751802AbcLFVDy (ORCPT ); Tue, 6 Dec 2016 16:03:54 -0500 Date: Tue, 6 Dec 2016 22:01:39 +0100 From: Benjamin Tissoires To: =?utf-8?B?Sm/Do28=?= Paulo Rechi Vita Cc: Jiri Kosina , linux@endlessm.com, =?utf-8?B?Sm/Do28=?= Paulo Rechi Vita , linux-input@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] i2c-hid: Disable IRQ before freeing buffers Message-ID: <20161206210139.GC20872@mail.corp.redhat.com> References: <20161206204835.7665-1-jprvita@endlessm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20161206204835.7665-1-jprvita@endlessm.com> X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Tue, 06 Dec 2016 21:01:43 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6026 Lines: 131 On Dec 06 2016 or thereabouts, João Paulo Rechi Vita wrote: > The HID report buffers that are initially allocated on i2c_hid_probe() > might not be big enough to hold the HID reports from a specific device, > in which case they will be freed and new ones will be allocated in > i2c_hid_start(), at point which the device's report size is known. But > at this point ihid->irq is already running, and may call > i2c_hid_get_input() which passes ihid->inbuf to i2c_master_recv(). Since > this handler runs in a separate thread, ihid->inbuf may be freed at this > very moment, and i2c_master_recv() will write on memory which may be > already owned by a different part of the kernel, corrupting its data. > > This problem has been observed on an Asus UX360UA laptop which has an > I2C touchpad, and results in a complete system freeze or an unusable > slowness with a lof of "BUG: unable to handle kernel paging request at >

" warnings. Enabling SLUB debugging shows a use-after-free > warning on memory allocated in i2c_hid_alloc_buffers() and freed in > i2c_hid_free_buffers(): > > ============================================================================= > BUG kmalloc-64 (Not tainted): Poison overwritten > ----------------------------------------------------------------------------- > Disabling lock debugging due to kernel taint > INFO: 0xffff880264083273-0xffff88026408329e. first byte 0x0 instead of 0x6b > INFO: Allocated in i2c_hid_alloc_buffers+0x25/0xa0 [i2c_hid] age=35793 cpu=2 pid=430 > ___slab_alloc+0x41e/0x460 > __slab_alloc+0x20/0x40 > __kmalloc+0x210/0x280 > i2c_hid_alloc_buffers+0x25/0xa0 [i2c_hid] > i2c_hid_probe+0x12f/0x5e0 [i2c_hid] > i2c_device_probe+0x10a/0x1b0 > driver_probe_device+0x220/0x4a0 > __device_attach_driver+0x71/0xa0 > bus_for_each_drv+0x67/0xb0 > __device_attach+0xdc/0x170 > device_initial_probe+0x13/0x20 > bus_probe_device+0x92/0xa0 > device_add+0x4aa/0x670 > device_register+0x1a/0x20 > i2c_new_device+0x18e/0x230 > acpi_i2c_add_device+0x1a0/0x210 > INFO: Freed in i2c_hid_free_buffers+0x16/0x60 [i2c_hid] age=7552 cpu=1 pid=1473 > __slab_free+0x221/0x330 > kfree+0x139/0x160 > i2c_hid_free_buffers+0x16/0x60 [i2c_hid] > i2c_hid_start+0x2a9/0x2df [i2c_hid] > mt_probe+0x160/0x22e [hid_multitouch] > hid_device_probe+0xd7/0x150 [hid] > driver_probe_device+0x220/0x4a0 > __driver_attach+0x84/0x90 > bus_for_each_dev+0x6c/0xc0 > driver_attach+0x1e/0x20 > bus_add_driver+0x1c3/0x280 > driver_register+0x60/0xe0 > __hid_register_driver+0x53/0x90 [hid] > 0xffffffffc004f01e > do_one_initcall+0xb3/0x1f0 > do_init_module+0x5f/0x1d0 > INFO: Slab 0xffffea0009902080 objects=20 used=20 fp=0x (null) flags=0x17fff8000004080 > INFO: Object 0xffff880264083260 @offset=4704 fp=0x (null) > Bytes b4 ffff880264083250: 8d e6 fe ff 00 00 00 00 5a 5a 5a 5a 5a 5a 5a 5a ........ZZZZZZZZ > Object ffff880264083260: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk > Object ffff880264083270: 6b 6b 6b 00 00 00 00 00 00 00 00 00 00 00 00 00 kkk............. > Object ffff880264083280: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > Object ffff880264083290: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > Redzone ffff8802640832a0: bb bb bb bb bb bb bb bb ........ > Padding ffff8802640833e0: 5a 5a 5a 5a 5a 5a 5a 5a ZZZZZZZZ > CPU: 1 PID: 1503 Comm: python3 Tainted: G B 4.4.21+ #10 > Hardware name: ASUSTeK COMPUTER INC. UX360UA/UX360UA, BIOS UX360UA.200 05/05/2016 > 0000000000000086 00000000622d48a2 ffff88026061ba38 ffffffff813f6044 > ffff880264082010 ffff880264083260 ffff88026061ba78 ffffffff811e8eab > 0000000000000008 ffff880200000001 ffff88026408329f ffff88026a007700 > Call Trace: > [] dump_stack+0x63/0x8f > [] print_trailer+0x14b/0x1f0 > [] check_bytes_and_report+0xc1/0x100 > [] check_object+0x1c4/0x240 > [] ? ext4_htree_store_dirent+0x3e/0x120 > [] alloc_debug_processing+0x104/0x180 > [] ___slab_alloc+0x41e/0x460 > [] ? ext4_htree_store_dirent+0x3e/0x120 > [] ? __getblk_gfp+0x2b/0x60 > [] ? ext4_getblk+0xa9/0x190 > [] __slab_alloc+0x20/0x40 > [] __kmalloc+0x210/0x280 > [] ? ext4_htree_store_dirent+0x3e/0x120 > [] ? ext4fs_dirhash+0xc2/0x2a0 > [] ext4_htree_store_dirent+0x3e/0x120 > [] htree_dirblock_to_tree+0x187/0x1b0 > [] ext4_htree_fill_tree+0xb2/0x2e0 > [] ? kmem_cache_alloc_trace+0x1fa/0x220 > [] ? ext4_readdir+0x775/0x8b0 > [] ext4_readdir+0x5e1/0x8b0 > [] iterate_dir+0x92/0x120 > [] SyS_getdents+0x98/0x110 > [] ? iterate_dir+0x120/0x120 > [] entry_SYSCALL_64_fastpath+0x16/0x71 > FIX kmalloc-64: Restoring 0xffff880264083273-0xffff88026408329e=0x6b > FIX kmalloc-64: Marking all objects used > > Signed-off-by: João Paulo Rechi Vita > --- Good catch (and very well documented). Reviewed-by: Benjamin Tissoires Cheers, Benjamin > drivers/hid/i2c-hid/i2c-hid.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c > index b3ec4f2..d4c2f2d 100644 > --- a/drivers/hid/i2c-hid/i2c-hid.c > +++ b/drivers/hid/i2c-hid/i2c-hid.c > @@ -716,9 +716,11 @@ static int i2c_hid_start(struct hid_device *hid) > i2c_hid_find_max_report(hid, HID_FEATURE_REPORT, &bufsize); > > if (bufsize > ihid->bufsize) { > + disable_irq(ihid->irq); > i2c_hid_free_buffers(ihid); > > ret = i2c_hid_alloc_buffers(ihid, bufsize); > + enable_irq(ihid->irq); > > if (ret) > return ret; > -- > 2.10.2 >