From patchwork Sun Aug 30 21:38:34 2009 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff Smith X-Patchwork-Id: 44781 X-Patchwork-Delegate: jikos@jikos.cz Received: from vger.kernel.org (vger.kernel.org [209.132.176.167]) by demeter.kernel.org (8.14.2/8.14.2) with ESMTP id n7ULiADk032337 for ; Sun, 30 Aug 2009 21:44:10 GMT Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754112AbZH3VoG (ORCPT ); Sun, 30 Aug 2009 17:44:06 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1754117AbZH3VoG (ORCPT ); Sun, 30 Aug 2009 17:44:06 -0400 Received: from thinkula.hx3.notconnected.net ([192.207.141.44]:60976 "EHLO sardonic.codimension.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754112AbZH3VoF (ORCPT ); Sun, 30 Aug 2009 17:44:05 -0400 X-Greylist: delayed 308 seconds by postgrey-1.27 at vger.kernel.org; Sun, 30 Aug 2009 17:44:04 EDT Received: from [192.168.52.17] (92.154.241.212.adsl.cix.gxn.net [212.241.154.92]) (authenticated bits=0) by sardonic.codimension.net (8.14.3/8.14.3/Debian-5) with ESMTP id n7ULcYRr025678 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NOT); Sun, 30 Aug 2009 22:38:43 +0100 Message-ID: <4A9AF15A.5080402@code.wastedcycles.net> Date: Sun, 30 Aug 2009 22:38:34 +0100 From: Jeff Smith Reply-To: lkml.sepix@code.wastedcycles.net User-Agent: Mozilla-Thunderbird 2.0.0.22 (X11/20090706) MIME-Version: 1.0 To: linux-input@vger.kernel.org CC: Jiri Kosina , Vegard Nossum Subject: Re: [kmemcheck] WARNING: kmemcheck: Caught 32-bit read from uninitialized memory, in hid_output_report References: <4A9833D3.3040202@code.wastedcycles.net> In-Reply-To: <4A9833D3.3040202@code.wastedcycles.net> Sender: linux-input-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-input@vger.kernel.org Jeff Smith wrote (on lkml): > A kmemcheck warning on booting my 2.6.31-rc8 (I haven't tried previous versions -- this was > actually not the problem I set out to fix). > > I can send Jiri (Bcc'ed) or the list a .config if it will help. I looked at this a bit more and it seems that hid-core.c:implement() does a 64-bit read even when it is not necessary. Admittedly it writes back the same bits (modulo the ones it is trying to change), so in theory no harm should be done provided the "extra" read neither runs off allocated memory nor the end of the current page boundary, nor ... A patch follows. It corrects a typo in the comment, changes the function name "implement" to "set_into_le_bitstream", changes the parameter name "n" to "bitfield_size", printk's unsigned using %u not %d, and only does a 32-bit read-modify-write when a 64-bit one is not necessary. A patch that writes only the bytes that need to be changed, rather than 32 or 64-bit quantities that potentially access irrelevant memory locations -- and that therefore require more complicated verification logic -- would be a better approach. However, as we are at -rc8 already and I don't fully understand the structures that are being changed, or the reasons the code is as it is, I didn't feel confident about presenting such a restructuring here. Could someone from the list give the attached patch a quick look please, and check I have not made any elementary endian errors or otherwise (although it does appear to work for me). Thanks Jeff PS. I am not on the list, so please Bcc: me direct on lkml.sepix code.wastedcycles.net > Jeff > > WARNING: kmemcheck: Caught 32-bit read from uninitialized memory (f6626624) > 0100000001000000000000000000000000000000000000000000000000000000 > i u u u u u u u u u u u u u u u u u u u u u u u u u u u u u u u > ^ > > Pid: 1, comm: swapper Not tainted (2.6.31-rc8-MOAT51 #2) ProLiant ML110 G5 > EIP: 0060:[] EFLAGS: 00010046 CPU: 0 > EIP is at hid_output_report+0x181/0x310 > EAX: 00000001 EBX: ffffffff ECX: 00000000 EDX: 00000001 > ESI: f6626620 EDI: 00000000 EBP: f711fc4c ESP: c1db160c > DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068 > CR0: 8005003b CR2: f676d4fc CR3: 01da4000 CR4: 00002650 > DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000 > DR6: ffff4ff0 DR7: 00000400 > [] __usbhid_submit_report+0x12d/0x290 > [] usbhid_submit_report+0x38/0x50 > [] usbhid_set_leds+0x9c/0xd0 > [] usbhid_start+0x627/0x670 > [] hid_device_probe+0x82/0xd0 > [] driver_probe_device+0x6d/0x180 > [] __device_attach+0x41/0x50 > [] bus_for_each_drv+0x5b/0x80 > [] device_attach+0x63/0x70 > [] bus_attach_device+0x47/0x70 > [] device_add+0x539/0x680 > [] hid_add_device+0x165/0x1d0 > [] hid_probe+0x269/0x3d0 > [] usb_probe_interface+0x86/0x140 > [] driver_probe_device+0x6d/0x180 > [] __driver_attach+0x91/0xa0 > [] bus_for_each_dev+0x5b/0x80 > [] driver_attach+0x19/0x20 > [] bus_add_driver+0x247/0x300 > [] driver_register+0x75/0x170 > [] usb_register_driver+0x87/0x110 > [] hid_init+0x9f/0xc2 > [] do_one_initcall+0x23/0x190 > [] kernel_init+0x144/0x19d > [] kernel_thread_helper+0x7/0x10 > [] 0xffffffff --- hid-core.c 2009-08-30 16:48:32.000000000 +0100 +++ hid-core.c-dist 2009-08-28 01:59:04.000000000 +0100 @@ -767,14 +767,19 @@ * The data mangled in the bit stream remains in little endian * order the whole time. It make more sense to talk about * endianness of register values by considering a register - * a "cached" copy of the little endian bit stream. + * a "cached" copy of the little endiad bit stream. */ -static __inline__ void set_into_le_bitstream(__u8 *report, unsigned offset, unsigned bitfield_size, __u32 value) +static __inline__ void implement(__u8 *report, unsigned offset, unsigned n, __u32 value) { - u64 m = (1ULL << bitfield_size) - 1; + u64 x; + u64 m = (1ULL << n) - 1; + + if (n > 32) + printk(KERN_WARNING "HID: implement() called with n (%d) > 32! (%s)\n", + n, current->comm); if (value > m) - printk(KERN_WARNING "HID: set_into_le_bitstream() value (%u) too big for bitfield for %s\n", + printk(KERN_WARNING "HID: implement() called with too large value %d! (%s)\n", value, current->comm); WARN_ON(value > m); value &= m; @@ -782,23 +787,10 @@ report += offset >> 3; offset &= 7; - if (bitfield_size > 32) { - u64 x; - - printk(KERN_WARNING "HID: set_into_le_bitstream() called with bitfield_size %u > 32 for %s\n", - bitfield_size, current->comm); - x = get_unaligned_le64(report); - x &= ~(m << offset); - x |= ((u64)value) << offset; - put_unaligned_le64(x, report); - } else { - u32 x; - - x = get_unaligned_le32(report); - x &= ~(m << offset); - x |= value << offset; - put_unaligned_le32(x, report); - } + x = get_unaligned_le64(report); + x &= ~(m << offset); + x |= ((u64)value) << offset; + put_unaligned_le64(x, report); } /* @@ -959,9 +951,9 @@ for (n = 0; n < count; n++) { if (field->logical_minimum < 0) /* signed values */ - set_into_le_bitstream(data, offset + n * size, size, s32ton(field->value[n], size)); + implement(data, offset + n * size, size, s32ton(field->value[n], size)); else /* unsigned values */ - set_into_le_bitstream(data, offset + n * size, size, field->value[n]); + implement(data, offset + n * size, size, field->value[n]); } }