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

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

[Benjamin Tissoires]

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

[PATCH v2 1/4] HID: i2c-hid: fix memory corruption due to missing hid declaration

[Benjamin Tissoires]

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 <benjamin.tissoires@gmail.com>
---
 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

Re: [PATCH v2 1/4] HID: i2c-hid: fix memory corruption due to missing hid declaration

[Jean Delvare]

On Wed,  5 Dec 2012 15:02:53 +0100, Benjamin Tissoires wrote:
> 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 <benjamin.tissoires@gmail.com>
> ---
>  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 <khali@linux-fr.org>

-- 
Jean Delvare

Re: [PATCH v2 1/4] HID: i2c-hid: fix memory corruption due to missing hid declaration

[Jiri Kosina]

On Wed, 5 Dec 2012, Jean Delvare wrote:

> > 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 <benjamin.tissoires@gmail.com>
> > ---
> >  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 <khali@linux-fr.org>

Applied, thanks.

-- 
Jiri Kosina
SUSE Labs

[PATCH v2 2/4] HID: i2c-hid: reorder allocation/free of buffers

[Benjamin Tissoires]

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 <benjamin.tissoires@gmail.com>
Reviewed-by: Jean Delvare <khali@linux-fr.org>
---
 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

Re: [PATCH v2 2/4] HID: i2c-hid: reorder allocation/free of buffers

[Jiri Kosina]

On Wed, 5 Dec 2012, Benjamin Tissoires wrote:

> 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 <benjamin.tissoires@gmail.com>
> Reviewed-by: Jean Delvare <khali@linux-fr.org>
> ---
>  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)

Applied, thank you.

-- 
Jiri Kosina
SUSE Labs

[PATCH v2 3/4] HID: i2c-hid: remove extra .irq field in struct i2c_hid

[Benjamin Tissoires]

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

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
---
 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

Re: [PATCH v2 3/4] HID: i2c-hid: remove extra .irq field in struct i2c_hid

[Jean Delvare]

On Wed,  5 Dec 2012 15:02:55 +0100, Benjamin Tissoires wrote:
> There is no point in keeping the irq in i2c_hid as it's already
> there in client.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> ---
>  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 <khali@linux-fr.org>

-- 
Jean Delvare

Re: [PATCH v2 3/4] HID: i2c-hid: remove extra .irq field in struct i2c_hid

[Jiri Kosina]

On Wed, 5 Dec 2012, Jean Delvare wrote:

> On Wed,  5 Dec 2012 15:02:55 +0100, Benjamin Tissoires wrote:
> > There is no point in keeping the irq in i2c_hid as it's already
> > there in client.
> > 
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>

Applied.

> > ---
> >  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 <khali@linux-fr.org>
> 
> -- 
> Jean Delvare
> 

-- 
Jiri Kosina
SUSE Labs

[PATCH v2 4/4] HID: i2c-hid: fix i2c_hid_get_raw_report count mismatches

[Benjamin Tissoires]

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 <benjamin.tissoires@gmail.com>
---
 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

Re: [PATCH v2 4/4] HID: i2c-hid: fix i2c_hid_get_raw_report count mismatches

[Jean Delvare]

On Wed,  5 Dec 2012 15:02:56 +0100, Benjamin Tissoires wrote:
> 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 <benjamin.tissoires@gmail.com>
> ---
>  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.

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

Re: [PATCH v2 4/4] HID: i2c-hid: fix i2c_hid_get_raw_report count mismatches

[Jiri Kosina]

On Wed, 5 Dec 2012, Jean Delvare wrote:

> > 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 <benjamin.tissoires@gmail.com>
> > ---
> >  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

Re: [PATCH v2 4/4] HID: i2c-hid: fix i2c_hid_get_raw_report count mismatches

[Benjamin Tissoires]

On Thu, Dec 6, 2012 at 11:01 AM, Jiri Kosina <jkosina@suse.cz> wrote:
>> > -   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