[PATCH v2 0/4] i2c-hid cleanup and bug fixes

Hi Jiri, Jean,

This is the v2 of the cleanup of i2c-hid.

Changes since v1:
* patch 1: check if dsize < 4
* patch 2: no changes
* patch 3: fixed irq return path
* patch 4: correct *_count handling (much more tested this time).

Cheers,
Benjamin

Benjamin Tissoires (4):
  HID: i2c-hid: fix memory corruption due to missing hid declaration
  HID: i2c-hid: reorder allocation/free of buffers
  HID: i2c-hid: remove extra .irq field in struct i2c_hid
  HID: i2c-hid: fix i2c_hid_get_raw_report count mismatches

 drivers/hid/i2c-hid/i2c-hid.c | 108 +++++++++++++++++++++---------------------
 1 file changed, 53 insertions(+), 55 deletions(-)

-- 
1.8.0.1

There is no point in keeping the irq in i2c_hid as it's already
there in client.

Signed-off-by: Benjamin Tissoires <redacted>
---
 drivers/hid/i2c-hid/i2c-hid.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index d00f185..c6630d4 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c

@@ -135,8 +135,6 @@ struct i2c_hid {
 
 	unsigned long		flags;		/* device flags */
 
-	int			irq;		/* the interrupt line irq */
-
 	wait_queue_head_t	wait;		/* For waiting the interrupt */
 };
 

@@ -736,8 +734,6 @@ static int __devinit i2c_hid_init_irq(struct i2c_client *client)
 		return ret;
 	}
 
-	ihid->irq = client->irq;
-
 	return 0;
 }
 

@@ -851,7 +847,7 @@ static int __devinit i2c_hid_probe(struct i2c_client *client,
 	hid = hid_allocate_device();
 	if (IS_ERR(hid)) {
 		ret = PTR_ERR(hid);
-		goto err;
+		goto err_irq;
 	}
 
 	ihid->hid = hid;

@@ -881,10 +877,10 @@ static int __devinit i2c_hid_probe(struct i2c_client *client,
 err_mem_free:
 	hid_destroy_device(hid);
 
-err:
-	if (ihid->irq)
-		free_irq(ihid->irq, ihid);
+err_irq:
+	free_irq(client->irq, ihid);
 
+err:
 	i2c_hid_free_buffers(ihid);
 	kfree(ihid);
 	return ret;

@@ -912,10 +908,9 @@ static int __devexit i2c_hid_remove(struct i2c_client *client)
 static int i2c_hid_suspend(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
-	struct i2c_hid *ihid = i2c_get_clientdata(client);
 
 	if (device_may_wakeup(&client->dev))
-		enable_irq_wake(ihid->irq);
+		enable_irq_wake(client->irq);
 
 	/* Save some power */
 	i2c_hid_set_power(client, I2C_HID_PWR_SLEEP);

-- 
1.8.0.1

Simplifies i2c_hid_alloc_buffers tests, and makes this function
responsible of the assignment of ihid->bufsize.
The condition for the reallocation in i2c_hid_start is then simpler.

Signed-off-by: Benjamin Tissoires <redacted>
Reviewed-by: Jean Delvare <redacted>
---
 drivers/hid/i2c-hid/i2c-hid.c | 68 ++++++++++++++++++-------------------------
 1 file changed, 28 insertions(+), 40 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 34cca42..d00f185 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c

@@ -464,48 +464,38 @@ static void i2c_hid_find_max_report(struct hid_device *hid, unsigned int type,
 	}
 }
 
-static int i2c_hid_alloc_buffers(struct i2c_hid *ihid)
+static void i2c_hid_free_buffers(struct i2c_hid *ihid)
+{
+	kfree(ihid->inbuf);
+	kfree(ihid->argsbuf);
+	kfree(ihid->cmdbuf);
+	ihid->inbuf = NULL;
+	ihid->cmdbuf = NULL;
+	ihid->argsbuf = NULL;
+	ihid->bufsize = 0;
+}
+
+static int i2c_hid_alloc_buffers(struct i2c_hid *ihid, size_t report_size)
 {
 	/* 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;
+		       report_size; /* report */
 
+	ihid->inbuf = kzalloc(report_size, GFP_KERNEL);
 	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);
-		ihid->inbuf = NULL;
-		ihid->argsbuf = NULL;
+	if (!ihid->inbuf || !ihid->argsbuf || !ihid->cmdbuf) {
+		i2c_hid_free_buffers(ihid);
 		return -ENOMEM;
 	}
 
-	return 0;
-}
+	ihid->bufsize = report_size;
 
-static void i2c_hid_free_buffers(struct i2c_hid *ihid)
-{
-	kfree(ihid->inbuf);
-	kfree(ihid->argsbuf);
-	kfree(ihid->cmdbuf);
-	ihid->inbuf = NULL;
-	ihid->cmdbuf = NULL;
-	ihid->argsbuf = NULL;
+	return 0;
 }
 
 static int i2c_hid_get_raw_report(struct hid_device *hid,

@@ -610,22 +600,19 @@ static int i2c_hid_start(struct hid_device *hid)
 	struct i2c_client *client = hid->driver_data;
 	struct i2c_hid *ihid = i2c_get_clientdata(client);
 	int ret;
-	int old_bufsize = ihid->bufsize;
+	unsigned int bufsize = HID_MIN_BUFFER_SIZE;
 
-	ihid->bufsize = HID_MIN_BUFFER_SIZE;
-	i2c_hid_find_max_report(hid, HID_INPUT_REPORT, &ihid->bufsize);
-	i2c_hid_find_max_report(hid, HID_OUTPUT_REPORT, &ihid->bufsize);
-	i2c_hid_find_max_report(hid, HID_FEATURE_REPORT, &ihid->bufsize);
+	i2c_hid_find_max_report(hid, HID_INPUT_REPORT, &bufsize);
+	i2c_hid_find_max_report(hid, HID_OUTPUT_REPORT, &bufsize);
+	i2c_hid_find_max_report(hid, HID_FEATURE_REPORT, &bufsize);
 
-	if (ihid->bufsize > old_bufsize || !ihid->inbuf || !ihid->cmdbuf) {
+	if (bufsize > ihid->bufsize) {
 		i2c_hid_free_buffers(ihid);
 
-		ret = i2c_hid_alloc_buffers(ihid);
+		ret = i2c_hid_alloc_buffers(ihid, bufsize);
 
-		if (ret) {
-			ihid->bufsize = old_bufsize;
+		if (ret)
 			return ret;
-		}
 	}
 
 	if (!(hid->quirks & HID_QUIRK_NO_INIT_REPORTS))

@@ -849,8 +836,9 @@ static int __devinit i2c_hid_probe(struct i2c_client *client,
 	/* we need to allocate the command buffer without knowing the maximum
 	 * size of the reports. Let's use HID_MIN_BUFFER_SIZE, then we do the
 	 * real computation later. */
-	ihid->bufsize = HID_MIN_BUFFER_SIZE;
-	i2c_hid_alloc_buffers(ihid);
+	ret = i2c_hid_alloc_buffers(ihid, HID_MIN_BUFFER_SIZE);
+	if (ret < 0)
+		goto err;
 
 	ret = i2c_hid_fetch_hid_descriptor(ihid);
 	if (ret < 0)

-- 
1.8.0.1

The previous memcpy implementation relied on the size advertized by the
device. There were no guarantees that buf was big enough.

Some gymnastic is also required with the +2/-2 to take into account
the first 2 bytes of the returned buffer where the total returned
length is supplied by the device.

Signed-off-by: Benjamin Tissoires <redacted>
---
 drivers/hid/i2c-hid/i2c-hid.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index c6630d4..ce01d59 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c

@@ -502,23 +502,31 @@ static int i2c_hid_get_raw_report(struct hid_device *hid,
 {
 	struct i2c_client *client = hid->driver_data;
 	struct i2c_hid *ihid = i2c_get_clientdata(client);
+	size_t ret_count, ask_count;
 	int ret;
 
 	if (report_type == HID_OUTPUT_REPORT)
 		return -EINVAL;
 
-	if (count > ihid->bufsize)
-		count = ihid->bufsize;
+	/* +2 bytes to include the size of the reply in the query buffer */
+	ask_count = min(count + 2, (size_t)ihid->bufsize);
 
 	ret = i2c_hid_get_report(client,
 			report_type == HID_FEATURE_REPORT ? 0x03 : 0x01,
-			report_number, ihid->inbuf, count);
+			report_number, ihid->inbuf, ask_count);
 
 	if (ret < 0)
 		return ret;
 
-	count = ihid->inbuf[0] | (ihid->inbuf[1] << 8);
+	ret_count = ihid->inbuf[0] | (ihid->inbuf[1] << 8);
 
+	if (!ret_count)
+		return 0;
+
+	ret_count = min(ret_count, ask_count);
+
+	/* The query buffer contains the size, dropping it in the reply */
+	count = min(count, ret_count - 2);
 	memcpy(buf, ihid->inbuf + 2, count);
 
 	return count;

-- 
1.8.0.1

HID descriptors contains 4 bytes of reserved field.
The previous implementation was overriding the next fields in struct i2c_hid.

Signed-off-by: Benjamin Tissoires <redacted>
---
 drivers/hid/i2c-hid/i2c-hid.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 7062df2..34cca42 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c

@@ -69,6 +69,7 @@ struct i2c_hid_desc {
 	__le16 wVendorID;
 	__le16 wProductID;
 	__le16 wVersionID;
+	__le32 reserved;
 } __packed;
 
 struct i2c_hid_cmd {

@@ -776,7 +777,13 @@ static int __devinit i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
 	}
 
 	dsize = le16_to_cpu(hdesc->wHIDDescLength);
-	if (!dsize || dsize > HID_MAX_DESCRIPTOR_SIZE) {
+	/*
+	 * the size of the HID descriptor should at least contain
+	 * its size and the bcdVersion (4 bytes), and should not be greater
+	 * than sizeof(struct i2c_hid_desc) as we directly fill this struct
+	 * through i2c_hid_command.
+	 */
+	if (dsize < 4 || dsize > sizeof(struct i2c_hid_desc)) {
 		dev_err(&client->dev, "weird size of HID descriptor (%u)\n",
 			dsize);
 		return -ENODEV;

-- 
1.8.0.1

On Wed,  5 Dec 2012 15:02:53 +0100, Benjamin Tissoires wrote:

Reviewed-by: Jean Delvare <redacted>

-- 
Jean Delvare

On Wed,  5 Dec 2012 15:02:55 +0100, Benjamin Tissoires wrote:

Nice clean-up.

Reviewed-by: Jean Delvare <redacted>

-- 
Jean Delvare

On Wed,  5 Dec 2012 15:02:56 +0100, Benjamin Tissoires wrote:

I'd make this (ret_count <= 2), as this would let you call memcpy with a
null or even negative length.

Other than that, the new code looks OK and safe.

+		return 0;
+
+	ret_count = min(ret_count, ask_count);
+
+	/* The query buffer contains the size, dropping it in the reply */
+	count = min(count, ret_count - 2);
 	memcpy(buf, ihid->inbuf + 2, count);
 
 	return count;

-- 
Jean Delvare

On Wed, 5 Dec 2012, Jean Delvare wrote:

quoted

HID descriptors contains 4 bytes of reserved field.
The previous implementation was overriding the next fields in struct i2c_hid.

Signed-off-by: Benjamin Tissoires <redacted>
---
 drivers/hid/i2c-hid/i2c-hid.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 7062df2..34cca42 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c

@@ -69,6 +69,7 @@ struct i2c_hid_desc {
 	__le16 wVendorID;
 	__le16 wProductID;
 	__le16 wVersionID;
+	__le32 reserved;
 } __packed;
 
 struct i2c_hid_cmd {

@@ -776,7 +777,13 @@ static int __devinit i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
 	}
 
 	dsize = le16_to_cpu(hdesc->wHIDDescLength);
-	if (!dsize || dsize > HID_MAX_DESCRIPTOR_SIZE) {
+	/*
+	 * the size of the HID descriptor should at least contain
+	 * its size and the bcdVersion (4 bytes), and should not be greater
+	 * than sizeof(struct i2c_hid_desc) as we directly fill this struct
+	 * through i2c_hid_command.
+	 */
+	if (dsize < 4 || dsize > sizeof(struct i2c_hid_desc)) {
 		dev_err(&client->dev, "weird size of HID descriptor (%u)\n",
 			dsize);
 		return -ENODEV;

Reviewed-by: Jean Delvare <redacted>

Applied, thanks.

-- 
Jiri Kosina
SUSE Labs

On Wed, 5 Dec 2012, Benjamin Tissoires wrote:

Applied, thank you.

-- 
Jiri Kosina
SUSE Labs

On Wed, 5 Dec 2012, Jean Delvare wrote:

On Wed,  5 Dec 2012 15:02:55 +0100, Benjamin Tissoires wrote:

quoted

There is no point in keeping the irq in i2c_hid as it's already
there in client.

Signed-off-by: Benjamin Tissoires <redacted>

Applied.

quoted

---
 drivers/hid/i2c-hid/i2c-hid.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index d00f185..c6630d4 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c

@@ -135,8 +135,6 @@ struct i2c_hid {
 
 	unsigned long		flags;		/* device flags */
 
-	int			irq;		/* the interrupt line irq */
-
 	wait_queue_head_t	wait;		/* For waiting the interrupt */
 };
 

@@ -736,8 +734,6 @@ static int __devinit i2c_hid_init_irq(struct i2c_client *client)
 		return ret;
 	}
 
-	ihid->irq = client->irq;
-
 	return 0;
 }
 

@@ -851,7 +847,7 @@ static int __devinit i2c_hid_probe(struct i2c_client *client,
 	hid = hid_allocate_device();
 	if (IS_ERR(hid)) {
 		ret = PTR_ERR(hid);
-		goto err;
+		goto err_irq;
 	}
 
 	ihid->hid = hid;

@@ -881,10 +877,10 @@ static int __devinit i2c_hid_probe(struct i2c_client *client,
 err_mem_free:
 	hid_destroy_device(hid);
 
-err:
-	if (ihid->irq)
-		free_irq(ihid->irq, ihid);
+err_irq:
+	free_irq(client->irq, ihid);
 
+err:
 	i2c_hid_free_buffers(ihid);
 	kfree(ihid);
 	return ret;

@@ -912,10 +908,9 @@ static int __devexit i2c_hid_remove(struct i2c_client *client)
 static int i2c_hid_suspend(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
-	struct i2c_hid *ihid = i2c_get_clientdata(client);
 
 	if (device_may_wakeup(&client->dev))
-		enable_irq_wake(ihid->irq);
+		enable_irq_wake(client->irq);
 
 	/* Save some power */
 	i2c_hid_set_power(client, I2C_HID_PWR_SLEEP);

Nice clean-up.

Reviewed-by: Jean Delvare <redacted>

-- 
Jean Delvare

-- 
Jiri Kosina
SUSE Labs

On Wed, 5 Dec 2012, Jean Delvare wrote:

quoted

The previous memcpy implementation relied on the size advertized by the
device. There were no guarantees that buf was big enough.

Some gymnastic is also required with the +2/-2 to take into account
the first 2 bytes of the returned buffer where the total returned
length is supplied by the device.

Signed-off-by: Benjamin Tissoires <redacted>
---
 drivers/hid/i2c-hid/i2c-hid.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index c6630d4..ce01d59 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c

@@ -502,23 +502,31 @@ static int i2c_hid_get_raw_report(struct hid_device *hid,
 {
 	struct i2c_client *client = hid->driver_data;
 	struct i2c_hid *ihid = i2c_get_clientdata(client);
+	size_t ret_count, ask_count;
 	int ret;
 
 	if (report_type == HID_OUTPUT_REPORT)
 		return -EINVAL;
 
-	if (count > ihid->bufsize)
-		count = ihid->bufsize;
+	/* +2 bytes to include the size of the reply in the query buffer */
+	ask_count = min(count + 2, (size_t)ihid->bufsize);
 
 	ret = i2c_hid_get_report(client,
 			report_type == HID_FEATURE_REPORT ? 0x03 : 0x01,
-			report_number, ihid->inbuf, count);
+			report_number, ihid->inbuf, ask_count);
 
 	if (ret < 0)
 		return ret;
 
-	count = ihid->inbuf[0] | (ihid->inbuf[1] << 8);
+	ret_count = ihid->inbuf[0] | (ihid->inbuf[1] << 8);
 
+	if (!ret_count)

I'd make this (ret_count <= 2), as this would let you call memcpy with a
null or even negative length.

Good catch, it doesn't account for the 2 bytes needed for storing the 
reply size.

I have fixed that and applied the patch, thanks everybody!

-- 
Jiri Kosina
SUSE Labs

On Thu, Dec 6, 2012 at 11:01 AM, Jiri Kosina [off-list ref] wrote:

quoted

quoted

-   count = ihid->inbuf[0] | (ihid->inbuf[1] << 8);
+   ret_count = ihid->inbuf[0] | (ihid->inbuf[1] << 8);

+   if (!ret_count)

I'd make this (ret_count <= 2), as this would let you call memcpy with a
null or even negative length.

Good catch, it doesn't account for the 2 bytes needed for storing the
reply size.

I have fixed that and applied the patch, thanks everybody!

Hi Jiri, Jean,

thank you very much for the work done. I was in a meeting past two
days, so I was not able to answer sooner.

Cheers,
Benjamin