linux-kernel - Re: [PATCH v2] i2c-hid: introduce HID over i2c specification implementation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <507D182E.9010902@suse.cz>
Date:	Tue, 16 Oct 2012 10:17:50 +0200
From:	Jiri Slaby <jslaby@...e.cz>
To:	Benjamin Tissoires <benjamin.tissoires@...il.com>
CC:	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Jiri Kosina <jkosina@...e.cz>,
	Stephane Chatty <chatty@...c.fr>, fabien.andre@...il.com,
	scott.liu@....com.tw, Jean Delvare <khali@...ux-fr.org>,
	JJ Ding <jj_ding@....com.tw>,
	Shubhrajyoti Datta <omaplinuxkernel@...il.com>,
	linux-i2c@...r.kernel.org, linux-input@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] i2c-hid: introduce HID over i2c specification implementation

On 10/15/2012 10:38 PM, Benjamin Tissoires wrote:
> Notes:
> {1}: I don't have all the informations in the beginning of the probe function to
> get the real size I need to allocate. So the behavior is to allocate first a
> buffer by using HID_MIN_BUFFER_SIZE and once I got the information, I can
> reallocate the buffer to the right size (in i2c_hid_start).

And there is a bug in this. See below.

> --- /dev/null
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
...
> +static int i2c_hid_alloc_buffers(struct i2c_hid *ihid)
> +{
> +	/* the worst case is computed from the set_report command with a
> +	 * reportID > 15 and the maximum report length */
> +	int args_len = sizeof(__u8) + /* optional ReportID byte */
> +		       sizeof(__u16) + /* data register */
> +		       sizeof(__u16) + /* size of the report */
> +		       ihid->bufsize; /* report */
> +
> +	ihid->inbuf = kzalloc(ihid->bufsize, GFP_KERNEL);
> +
> +	if (!ihid->inbuf)
> +		return -ENOMEM;
> +
> +	ihid->argsbuf = kzalloc(args_len, GFP_KERNEL);
> +
> +	if (!ihid->argsbuf) {
> +		kfree(ihid->inbuf);
> +		return -ENOMEM;
> +	}
> +
> +	ihid->cmdbuf = kzalloc(sizeof(union command) + args_len, GFP_KERNEL);
> +
> +	if (!ihid->cmdbuf) {
> +		kfree(ihid->inbuf);
> +		kfree(ihid->argsbuf);
> +		return -ENOMEM;
> +	}
> +
> +	return 0;

If this is called from hid_start and some of the latter allocation fails
here, you free the buffers appropriately. However if another start
occurs (e.g. by loading another module for that particular device), it
will crash, as the buffers will remain unallocated because at this point
ihid->bufsize == old_bufsize. You should set ihid->bufsize back to
old_bufsize if i2c_hid_alloc_buffers fails and also set the pointers to
NULL here.

regards,
-- 
js
suse labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/