* [PATCH v2 0/4] i2c-hid cleanup and bug fixes
@ 2012-12-05 14:02 Benjamin Tissoires
2012-12-05 14:02 ` [PATCH v2 1/4] HID: i2c-hid: fix memory corruption due to missing hid declaration Benjamin Tissoires
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Benjamin Tissoires @ 2012-12-05 14:02 UTC (permalink / raw)
To: Benjamin Tissoires, Jiri Kosina, Jean Delvare, linux-input,
linux-i2c, linux-kernel
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 1/4] HID: i2c-hid: fix memory corruption due to missing hid declaration
2012-12-05 14:02 [PATCH v2 0/4] i2c-hid cleanup and bug fixes Benjamin Tissoires
@ 2012-12-05 14:02 ` Benjamin Tissoires
2012-12-05 17:25 ` Jean Delvare
2012-12-05 14:02 ` [PATCH v2 2/4] HID: i2c-hid: reorder allocation/free of buffers Benjamin Tissoires
` (2 subsequent siblings)
3 siblings, 1 reply; 13+ messages in thread
From: Benjamin Tissoires @ 2012-12-05 14:02 UTC (permalink / raw)
To: Benjamin Tissoires, Jiri Kosina, Jean Delvare, linux-input,
linux-i2c, linux-kernel
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 2/4] HID: i2c-hid: reorder allocation/free of buffers
2012-12-05 14:02 [PATCH v2 0/4] i2c-hid cleanup and bug fixes Benjamin Tissoires
2012-12-05 14:02 ` [PATCH v2 1/4] HID: i2c-hid: fix memory corruption due to missing hid declaration Benjamin Tissoires
@ 2012-12-05 14:02 ` Benjamin Tissoires
2012-12-06 9:56 ` Jiri Kosina
2012-12-05 14:02 ` [PATCH v2 3/4] HID: i2c-hid: remove extra .irq field in struct i2c_hid Benjamin Tissoires
2012-12-05 14:02 ` [PATCH v2 4/4] HID: i2c-hid: fix i2c_hid_get_raw_report count mismatches Benjamin Tissoires
3 siblings, 1 reply; 13+ messages in thread
From: Benjamin Tissoires @ 2012-12-05 14:02 UTC (permalink / raw)
To: Benjamin Tissoires, Jiri Kosina, Jean Delvare, linux-input,
linux-i2c, linux-kernel
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 3/4] HID: i2c-hid: remove extra .irq field in struct i2c_hid
2012-12-05 14:02 [PATCH v2 0/4] i2c-hid cleanup and bug fixes Benjamin Tissoires
2012-12-05 14:02 ` [PATCH v2 1/4] HID: i2c-hid: fix memory corruption due to missing hid declaration Benjamin Tissoires
2012-12-05 14:02 ` [PATCH v2 2/4] HID: i2c-hid: reorder allocation/free of buffers Benjamin Tissoires
@ 2012-12-05 14:02 ` Benjamin Tissoires
2012-12-05 17:28 ` Jean Delvare
2012-12-05 14:02 ` [PATCH v2 4/4] HID: i2c-hid: fix i2c_hid_get_raw_report count mismatches Benjamin Tissoires
3 siblings, 1 reply; 13+ messages in thread
From: Benjamin Tissoires @ 2012-12-05 14:02 UTC (permalink / raw)
To: Benjamin Tissoires, Jiri Kosina, Jean Delvare, linux-input,
linux-i2c, linux-kernel
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 4/4] HID: i2c-hid: fix i2c_hid_get_raw_report count mismatches
2012-12-05 14:02 [PATCH v2 0/4] i2c-hid cleanup and bug fixes Benjamin Tissoires
` (2 preceding siblings ...)
2012-12-05 14:02 ` [PATCH v2 3/4] HID: i2c-hid: remove extra .irq field in struct i2c_hid Benjamin Tissoires
@ 2012-12-05 14:02 ` Benjamin Tissoires
2012-12-05 19:59 ` Jean Delvare
3 siblings, 1 reply; 13+ messages in thread
From: Benjamin Tissoires @ 2012-12-05 14:02 UTC (permalink / raw)
To: Benjamin Tissoires, Jiri Kosina, Jean Delvare, linux-input,
linux-i2c, linux-kernel
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/4] HID: i2c-hid: fix memory corruption due to missing hid declaration
2012-12-05 14:02 ` [PATCH v2 1/4] HID: i2c-hid: fix memory corruption due to missing hid declaration Benjamin Tissoires
@ 2012-12-05 17:25 ` Jean Delvare
2012-12-06 9:54 ` Jiri Kosina
0 siblings, 1 reply; 13+ messages in thread
From: Jean Delvare @ 2012-12-05 17:25 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: Jiri Kosina, linux-input, linux-i2c, linux-kernel
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/4] HID: i2c-hid: remove extra .irq field in struct i2c_hid
2012-12-05 14:02 ` [PATCH v2 3/4] HID: i2c-hid: remove extra .irq field in struct i2c_hid Benjamin Tissoires
@ 2012-12-05 17:28 ` Jean Delvare
2012-12-06 9:56 ` Jiri Kosina
0 siblings, 1 reply; 13+ messages in thread
From: Jean Delvare @ 2012-12-05 17:28 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: Jiri Kosina, linux-input, linux-i2c, linux-kernel
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 4/4] HID: i2c-hid: fix i2c_hid_get_raw_report count mismatches
2012-12-05 14:02 ` [PATCH v2 4/4] HID: i2c-hid: fix i2c_hid_get_raw_report count mismatches Benjamin Tissoires
@ 2012-12-05 19:59 ` Jean Delvare
2012-12-06 10:01 ` Jiri Kosina
0 siblings, 1 reply; 13+ messages in thread
From: Jean Delvare @ 2012-12-05 19:59 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: Jiri Kosina, linux-input, linux-i2c, linux-kernel
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/4] HID: i2c-hid: fix memory corruption due to missing hid declaration
2012-12-05 17:25 ` Jean Delvare
@ 2012-12-06 9:54 ` Jiri Kosina
0 siblings, 0 replies; 13+ messages in thread
From: Jiri Kosina @ 2012-12-06 9:54 UTC (permalink / raw)
To: Jean Delvare; +Cc: Benjamin Tissoires, linux-input, linux-i2c, linux-kernel
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/4] HID: i2c-hid: reorder allocation/free of buffers
2012-12-05 14:02 ` [PATCH v2 2/4] HID: i2c-hid: reorder allocation/free of buffers Benjamin Tissoires
@ 2012-12-06 9:56 ` Jiri Kosina
0 siblings, 0 replies; 13+ messages in thread
From: Jiri Kosina @ 2012-12-06 9:56 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: Jean Delvare, linux-input, linux-i2c, linux-kernel
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/4] HID: i2c-hid: remove extra .irq field in struct i2c_hid
2012-12-05 17:28 ` Jean Delvare
@ 2012-12-06 9:56 ` Jiri Kosina
0 siblings, 0 replies; 13+ messages in thread
From: Jiri Kosina @ 2012-12-06 9:56 UTC (permalink / raw)
To: Jean Delvare; +Cc: Benjamin Tissoires, linux-input, linux-i2c, linux-kernel
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 4/4] HID: i2c-hid: fix i2c_hid_get_raw_report count mismatches
2012-12-05 19:59 ` Jean Delvare
@ 2012-12-06 10:01 ` Jiri Kosina
2012-12-07 15:13 ` Benjamin Tissoires
0 siblings, 1 reply; 13+ messages in thread
From: Jiri Kosina @ 2012-12-06 10:01 UTC (permalink / raw)
To: Jean Delvare; +Cc: Benjamin Tissoires, linux-input, linux-i2c, linux-kernel
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 4/4] HID: i2c-hid: fix i2c_hid_get_raw_report count mismatches
2012-12-06 10:01 ` Jiri Kosina
@ 2012-12-07 15:13 ` Benjamin Tissoires
0 siblings, 0 replies; 13+ messages in thread
From: Benjamin Tissoires @ 2012-12-07 15:13 UTC (permalink / raw)
To: Jiri Kosina; +Cc: Jean Delvare, linux-input, linux-i2c, linux-kernel
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
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2012-12-07 15:13 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-12-05 14:02 [PATCH v2 0/4] i2c-hid cleanup and bug fixes Benjamin Tissoires
2012-12-05 14:02 ` [PATCH v2 1/4] HID: i2c-hid: fix memory corruption due to missing hid declaration Benjamin Tissoires
2012-12-05 17:25 ` Jean Delvare
2012-12-06 9:54 ` Jiri Kosina
2012-12-05 14:02 ` [PATCH v2 2/4] HID: i2c-hid: reorder allocation/free of buffers Benjamin Tissoires
2012-12-06 9:56 ` Jiri Kosina
2012-12-05 14:02 ` [PATCH v2 3/4] HID: i2c-hid: remove extra .irq field in struct i2c_hid Benjamin Tissoires
2012-12-05 17:28 ` Jean Delvare
2012-12-06 9:56 ` Jiri Kosina
2012-12-05 14:02 ` [PATCH v2 4/4] HID: i2c-hid: fix i2c_hid_get_raw_report count mismatches Benjamin Tissoires
2012-12-05 19:59 ` Jean Delvare
2012-12-06 10:01 ` Jiri Kosina
2012-12-07 15:13 ` Benjamin Tissoires
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).