2014-11-16 14:45:05

by Antonio Borneo

[permalink] [raw]
Subject: [PATCH] HID: i2c-hid: fix race condition reading reports

From: Jean-Baptiste Maneyrol <[email protected]>

From: Jean-Baptiste Maneyrol <[email protected]>

Current driver uses a common buffer for reading reports either
synchronously in i2c_hid_get_raw_report() and asynchronously in
the interrupt handler.
There is race condition if an interrupt arrives immediately after
the report is received in i2c_hid_get_raw_report(); the common
buffer is modified by the interrupt handler with the new report
and then i2c_hid_get_raw_report() proceed using wrong data.

Fix it by using a separate buffers for asynchronous reports.

Signed-off-by: Jean-Baptiste Maneyrol <[email protected]>
[Antonio Borneo: cleanup and rebase to v3.17]
Signed-off-by: Antonio Borneo <[email protected]>
Cc: [email protected]
---

Hi Jiri, Benjamin,

I think this patch should also go through linux-stable.
Confirmation from our side is welcome.

Regards,
Antonio

drivers/hid/i2c-hid/i2c-hid.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 933bf10..7c80e96 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -136,6 +136,7 @@ struct i2c_hid {
* register of the HID
* descriptor. */
unsigned int bufsize; /* i2c buffer size */
+ char *irqinbuf; /* Asynchronous Input buffer */
char *inbuf; /* Input buffer */
char *cmdbuf; /* Command buffer */
char *argsbuf; /* Command arguments buffer */
@@ -371,7 +372,7 @@ static void i2c_hid_get_input(struct i2c_hid *ihid)
int ret, ret_size;
int size = le16_to_cpu(ihid->hdesc.wMaxInputLength);

- ret = i2c_master_recv(ihid->client, ihid->inbuf, size);
+ ret = i2c_master_recv(ihid->client, ihid->irqinbuf, size);
if (ret != size) {
if (ret < 0)
return;
@@ -381,7 +382,7 @@ static void i2c_hid_get_input(struct i2c_hid *ihid)
return;
}

- ret_size = ihid->inbuf[0] | ihid->inbuf[1] << 8;
+ ret_size = ihid->irqinbuf[0] | ihid->irqinbuf[1] << 8;

if (!ret_size) {
/* host or device initiated RESET completed */
@@ -396,11 +397,11 @@ static void i2c_hid_get_input(struct i2c_hid *ihid)
return;
}

- i2c_hid_dbg(ihid, "input: %*ph\n", ret_size, ihid->inbuf);
+ i2c_hid_dbg(ihid, "input: %*ph\n", ret_size, ihid->irqinbuf);

if (test_bit(I2C_HID_STARTED, &ihid->flags))
- hid_input_report(ihid->hid, HID_INPUT_REPORT, ihid->inbuf + 2,
- ret_size - 2, 1);
+ hid_input_report(ihid->hid, HID_INPUT_REPORT,
+ ihid->irqinbuf + 2, ret_size - 2, 1);

return;
}
@@ -503,9 +504,11 @@ static void i2c_hid_find_max_report(struct hid_device *hid, unsigned int type,

static void i2c_hid_free_buffers(struct i2c_hid *ihid)
{
+ kfree(ihid->irqinbuf);
kfree(ihid->inbuf);
kfree(ihid->argsbuf);
kfree(ihid->cmdbuf);
+ ihid->irqinbuf = NULL;
ihid->inbuf = NULL;
ihid->cmdbuf = NULL;
ihid->argsbuf = NULL;
@@ -521,11 +524,13 @@ static int i2c_hid_alloc_buffers(struct i2c_hid *ihid, size_t report_size)
sizeof(__u16) + /* size of the report */
report_size; /* report */

+ ihid->irqinbuf = kzalloc(report_size, GFP_KERNEL);
ihid->inbuf = kzalloc(report_size, GFP_KERNEL);
ihid->argsbuf = kzalloc(args_len, GFP_KERNEL);
ihid->cmdbuf = kzalloc(sizeof(union command) + args_len, GFP_KERNEL);

- if (!ihid->inbuf || !ihid->argsbuf || !ihid->cmdbuf) {
+ if (!ihid->irqinbuf || !ihid->inbuf ||
+ !ihid->argsbuf || !ihid->cmdbuf) {
i2c_hid_free_buffers(ihid);
return -ENOMEM;
}
--
2.1.3


2014-11-17 21:43:18

by Benjamin Tissoires

[permalink] [raw]
Subject: Re: [PATCH] HID: i2c-hid: fix race condition reading reports

Hey Antonio,

On Nov 16 2014 or thereabouts, Antonio Borneo wrote:
> From: Jean-Baptiste Maneyrol <[email protected]>
>
> From: Jean-Baptiste Maneyrol <[email protected]>
>
> Current driver uses a common buffer for reading reports either
> synchronously in i2c_hid_get_raw_report() and asynchronously in
> the interrupt handler.
> There is race condition if an interrupt arrives immediately after
> the report is received in i2c_hid_get_raw_report(); the common
> buffer is modified by the interrupt handler with the new report
> and then i2c_hid_get_raw_report() proceed using wrong data.
>
> Fix it by using a separate buffers for asynchronous reports.
>
> Signed-off-by: Jean-Baptiste Maneyrol <[email protected]>
> [Antonio Borneo: cleanup and rebase to v3.17]
> Signed-off-by: Antonio Borneo <[email protected]>
> Cc: [email protected]

For your next submission, when you want a patch to go in stable, put CC
here, but please do not CC the actual mail to stable@. Stable should receive
either mails which are already in Linus' tree, or which refer a commit
in Linus' tree in case it does not applies smoothly.

[keeping stable@ here to show them that this one should not get picked
right now]

> ---
>
> Hi Jiri, Benjamin,
>
> I think this patch should also go through linux-stable.
> Confirmation from our side is welcome.

I think the patch is definitively valuable. However, my personal taste
would go for having a new .rawbuf buffer and use it in
i2c_hid_get_raw_report(). The rationale would be that it's actually
i2c_hid_get_raw_report() which is problematic, not the generic irq
handling. Also I prefer rawbuf to irqinbuf.

I am perfectly aware that this is just bikeshedding, so if changing the
patch is too cumbersome (looks like Jean-Baptiste is involved), and if
Jiri agree, this can go into the hid tree with my reviewed-by.

I am fine having this version or the rawbuf one in stable BTW.

Cheers,
Benjamin

>
> Regards,
> Antonio
>
> drivers/hid/i2c-hid/i2c-hid.c | 17 +++++++++++------
> 1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index 933bf10..7c80e96 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -136,6 +136,7 @@ struct i2c_hid {
> * register of the HID
> * descriptor. */
> unsigned int bufsize; /* i2c buffer size */
> + char *irqinbuf; /* Asynchronous Input buffer */
> char *inbuf; /* Input buffer */
> char *cmdbuf; /* Command buffer */
> char *argsbuf; /* Command arguments buffer */
> @@ -371,7 +372,7 @@ static void i2c_hid_get_input(struct i2c_hid *ihid)
> int ret, ret_size;
> int size = le16_to_cpu(ihid->hdesc.wMaxInputLength);
>
> - ret = i2c_master_recv(ihid->client, ihid->inbuf, size);
> + ret = i2c_master_recv(ihid->client, ihid->irqinbuf, size);
> if (ret != size) {
> if (ret < 0)
> return;
> @@ -381,7 +382,7 @@ static void i2c_hid_get_input(struct i2c_hid *ihid)
> return;
> }
>
> - ret_size = ihid->inbuf[0] | ihid->inbuf[1] << 8;
> + ret_size = ihid->irqinbuf[0] | ihid->irqinbuf[1] << 8;
>
> if (!ret_size) {
> /* host or device initiated RESET completed */
> @@ -396,11 +397,11 @@ static void i2c_hid_get_input(struct i2c_hid *ihid)
> return;
> }
>
> - i2c_hid_dbg(ihid, "input: %*ph\n", ret_size, ihid->inbuf);
> + i2c_hid_dbg(ihid, "input: %*ph\n", ret_size, ihid->irqinbuf);
>
> if (test_bit(I2C_HID_STARTED, &ihid->flags))
> - hid_input_report(ihid->hid, HID_INPUT_REPORT, ihid->inbuf + 2,
> - ret_size - 2, 1);
> + hid_input_report(ihid->hid, HID_INPUT_REPORT,
> + ihid->irqinbuf + 2, ret_size - 2, 1);
>
> return;
> }
> @@ -503,9 +504,11 @@ static void i2c_hid_find_max_report(struct hid_device *hid, unsigned int type,
>
> static void i2c_hid_free_buffers(struct i2c_hid *ihid)
> {
> + kfree(ihid->irqinbuf);
> kfree(ihid->inbuf);
> kfree(ihid->argsbuf);
> kfree(ihid->cmdbuf);
> + ihid->irqinbuf = NULL;
> ihid->inbuf = NULL;
> ihid->cmdbuf = NULL;
> ihid->argsbuf = NULL;
> @@ -521,11 +524,13 @@ static int i2c_hid_alloc_buffers(struct i2c_hid *ihid, size_t report_size)
> sizeof(__u16) + /* size of the report */
> report_size; /* report */
>
> + ihid->irqinbuf = kzalloc(report_size, GFP_KERNEL);
> ihid->inbuf = kzalloc(report_size, GFP_KERNEL);
> ihid->argsbuf = kzalloc(args_len, GFP_KERNEL);
> ihid->cmdbuf = kzalloc(sizeof(union command) + args_len, GFP_KERNEL);
>
> - if (!ihid->inbuf || !ihid->argsbuf || !ihid->cmdbuf) {
> + if (!ihid->irqinbuf || !ihid->inbuf ||
> + !ihid->argsbuf || !ihid->cmdbuf) {
> i2c_hid_free_buffers(ihid);
> return -ENOMEM;
> }
> --
> 2.1.3
>

2014-11-17 21:51:15

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH] HID: i2c-hid: fix race condition reading reports

On Mon, Nov 17, 2014 at 04:43:05PM -0500, Benjamin Tissoires wrote:
> Hey Antonio,
>
> On Nov 16 2014 or thereabouts, Antonio Borneo wrote:
> > From: Jean-Baptiste Maneyrol <[email protected]>
> >
> > From: Jean-Baptiste Maneyrol <[email protected]>
> >
> > Current driver uses a common buffer for reading reports either
> > synchronously in i2c_hid_get_raw_report() and asynchronously in
> > the interrupt handler.
> > There is race condition if an interrupt arrives immediately after
> > the report is received in i2c_hid_get_raw_report(); the common
> > buffer is modified by the interrupt handler with the new report
> > and then i2c_hid_get_raw_report() proceed using wrong data.
> >
> > Fix it by using a separate buffers for asynchronous reports.
> >
> > Signed-off-by: Jean-Baptiste Maneyrol <[email protected]>
> > [Antonio Borneo: cleanup and rebase to v3.17]
> > Signed-off-by: Antonio Borneo <[email protected]>
> > Cc: [email protected]
>
> For your next submission, when you want a patch to go in stable, put CC
> here, but please do not CC the actual mail to stable@. Stable should receive
> either mails which are already in Linus' tree, or which refer a commit
> in Linus' tree in case it does not applies smoothly.
>
> [keeping stable@ here to show them that this one should not get picked
> right now]

stable@ is smarter than that, I don't mind seeing patches that are
coming in the future like this at all, it's not a problem.

thanks,

greg k-h

2014-11-17 22:07:25

by Benjamin Tissoires

[permalink] [raw]
Subject: Re: [PATCH] HID: i2c-hid: fix race condition reading reports

On Nov 17 2014 or thereabouts, Greg KH wrote:
> On Mon, Nov 17, 2014 at 04:43:05PM -0500, Benjamin Tissoires wrote:
> > Hey Antonio,
> >
> > On Nov 16 2014 or thereabouts, Antonio Borneo wrote:
> > > From: Jean-Baptiste Maneyrol <[email protected]>
> > >
> > > From: Jean-Baptiste Maneyrol <[email protected]>
> > >
> > > Current driver uses a common buffer for reading reports either
> > > synchronously in i2c_hid_get_raw_report() and asynchronously in
> > > the interrupt handler.
> > > There is race condition if an interrupt arrives immediately after
> > > the report is received in i2c_hid_get_raw_report(); the common
> > > buffer is modified by the interrupt handler with the new report
> > > and then i2c_hid_get_raw_report() proceed using wrong data.
> > >
> > > Fix it by using a separate buffers for asynchronous reports.
> > >
> > > Signed-off-by: Jean-Baptiste Maneyrol <[email protected]>
> > > [Antonio Borneo: cleanup and rebase to v3.17]
> > > Signed-off-by: Antonio Borneo <[email protected]>
> > > Cc: [email protected]
> >
> > For your next submission, when you want a patch to go in stable, put CC
> > here, but please do not CC the actual mail to stable@. Stable should receive
> > either mails which are already in Linus' tree, or which refer a commit
> > in Linus' tree in case it does not applies smoothly.
> >
> > [keeping stable@ here to show them that this one should not get picked
> > right now]
>
> stable@ is smarter than that, I don't mind seeing patches that are
> coming in the future like this at all, it's not a problem.
>

OK. Sorry for the noise then. I assumed that if everybody starts sending
potential patches to stable without them being the accepted ones, you
will end up having to deal with a lot more workload that you already have.

And just to be sure that there is no misinterpretation, I did not wanted
to imply that stable was not smart enough to deal with such patches... I
really appreciate the work done and my concern was to not add workload.

Cheers,
Benjamin

> thanks,
>
> greg k-h

2014-11-19 16:37:49

by Antonio Borneo

[permalink] [raw]
Subject: Re: [PATCH] HID: i2c-hid: fix race condition reading reports

Hi Benjamin,

On Tue, Nov 18, 2014 at 5:43 AM, Benjamin Tissoires
<[email protected]> wrote:
> Hey Antonio,
>
> On Nov 16 2014 or thereabouts, Antonio Borneo wrote:
>> From: Jean-Baptiste Maneyrol <[email protected]>
>>
>> From: Jean-Baptiste Maneyrol <[email protected]>
>>
>> Current driver uses a common buffer for reading reports either
>> synchronously in i2c_hid_get_raw_report() and asynchronously in
>> the interrupt handler.
>> There is race condition if an interrupt arrives immediately after
>> the report is received in i2c_hid_get_raw_report(); the common
>> buffer is modified by the interrupt handler with the new report
>> and then i2c_hid_get_raw_report() proceed using wrong data.
>>
>> Fix it by using a separate buffers for asynchronous reports.
>>
>> Signed-off-by: Jean-Baptiste Maneyrol <[email protected]>
>> [Antonio Borneo: cleanup and rebase to v3.17]
>> Signed-off-by: Antonio Borneo <[email protected]>
>> Cc: [email protected]
>
> For your next submission, when you want a patch to go in stable, put CC
> here, but please do not CC the actual mail to stable@. Stable should receive
> either mails which are already in Linus' tree, or which refer a commit
> in Linus' tree in case it does not applies smoothly.
>
> [keeping stable@ here to show them that this one should not get picked
> right now]

I agree with you to lower the noise in -stable, even if Greg does not
feel annoyed.
I'll take care in the future.

>
>> ---
>>
>> Hi Jiri, Benjamin,
>>
>> I think this patch should also go through linux-stable.
>> Confirmation from our side is welcome.
>
> I think the patch is definitively valuable. However, my personal taste
> would go for having a new .rawbuf buffer and use it in
> i2c_hid_get_raw_report(). The rationale would be that it's actually
> i2c_hid_get_raw_report() which is problematic, not the generic irq
> handling. Also I prefer rawbuf to irqinbuf.
>
> I am perfectly aware that this is just bikeshedding, so if changing the
> patch is too cumbersome (looks like Jean-Baptiste is involved), and if
> Jiri agree, this can go into the hid tree with my reviewed-by.
>
> I am fine having this version or the rawbuf one in stable BTW.

No major issue changing the patch in line with your suggestion; just
some offline work to get also Jean-Baptiste check and approve it.
I'll send shortly a V2 that adds rawbuf instead of irqinbuf.
I will not add your "reviewed-by" since the code in V2 is quite
different than V1.

Thanks!
Antonio

2014-11-19 16:46:40

by Antonio Borneo

[permalink] [raw]
Subject: [PATCH V2] HID: i2c-hid: fix race condition reading reports

From: Jean-Baptiste Maneyrol <[email protected]>

Current driver uses a common buffer for reading reports either
synchronously in i2c_hid_get_raw_report() and asynchronously in
the interrupt handler.
There is race condition if an interrupt arrives immediately after
the report is received in i2c_hid_get_raw_report(); the common
buffer is modified by the interrupt handler with the new report
and then i2c_hid_get_raw_report() proceed using wrong data.

Fix it by using a separate buffers for synchronous reports.

Signed-off-by: Jean-Baptiste Maneyrol <[email protected]>
[Antonio Borneo: cleanup, rebase to v3.17, submit mainline]
Signed-off-by: Antonio Borneo <[email protected]>
Cc: [email protected]
---
V1 -> V2
rename the synchronous buffer as rawbuf (instead of the
asynchronous one)

drivers/hid/i2c-hid/i2c-hid.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 933bf10..c66e6ac 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -137,6 +137,7 @@ struct i2c_hid {
* descriptor. */
unsigned int bufsize; /* i2c buffer size */
char *inbuf; /* Input buffer */
+ char *rawbuf; /* Raw Input buffer */
char *cmdbuf; /* Command buffer */
char *argsbuf; /* Command arguments buffer */

@@ -504,9 +505,11 @@ static void i2c_hid_find_max_report(struct hid_device *hid, unsigned int type,
static void i2c_hid_free_buffers(struct i2c_hid *ihid)
{
kfree(ihid->inbuf);
+ kfree(ihid->rawbuf);
kfree(ihid->argsbuf);
kfree(ihid->cmdbuf);
ihid->inbuf = NULL;
+ ihid->rawbuf = NULL;
ihid->cmdbuf = NULL;
ihid->argsbuf = NULL;
ihid->bufsize = 0;
@@ -522,10 +525,11 @@ static int i2c_hid_alloc_buffers(struct i2c_hid *ihid, size_t report_size)
report_size; /* report */

ihid->inbuf = kzalloc(report_size, GFP_KERNEL);
+ ihid->rawbuf = kzalloc(report_size, GFP_KERNEL);
ihid->argsbuf = kzalloc(args_len, GFP_KERNEL);
ihid->cmdbuf = kzalloc(sizeof(union command) + args_len, GFP_KERNEL);

- if (!ihid->inbuf || !ihid->argsbuf || !ihid->cmdbuf) {
+ if (!ihid->inbuf || !ihid->rawbuf || !ihid->argsbuf || !ihid->cmdbuf) {
i2c_hid_free_buffers(ihid);
return -ENOMEM;
}
@@ -552,12 +556,12 @@ static int i2c_hid_get_raw_report(struct hid_device *hid,

ret = i2c_hid_get_report(client,
report_type == HID_FEATURE_REPORT ? 0x03 : 0x01,
- report_number, ihid->inbuf, ask_count);
+ report_number, ihid->rawbuf, ask_count);

if (ret < 0)
return ret;

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

if (ret_count <= 2)
return 0;
@@ -566,7 +570,7 @@ static int i2c_hid_get_raw_report(struct hid_device *hid,

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

return count;
}
--
2.1.3

2014-11-24 16:07:46

by Benjamin Tissoires

[permalink] [raw]
Subject: Re: [PATCH V2] HID: i2c-hid: fix race condition reading reports

On Wed, Nov 19, 2014 at 11:46 AM, Antonio Borneo
<[email protected]> wrote:
> From: Jean-Baptiste Maneyrol <[email protected]>
>
> Current driver uses a common buffer for reading reports either
> synchronously in i2c_hid_get_raw_report() and asynchronously in
> the interrupt handler.
> There is race condition if an interrupt arrives immediately after
> the report is received in i2c_hid_get_raw_report(); the common
> buffer is modified by the interrupt handler with the new report
> and then i2c_hid_get_raw_report() proceed using wrong data.
>
> Fix it by using a separate buffers for synchronous reports.
>
> Signed-off-by: Jean-Baptiste Maneyrol <[email protected]>
> [Antonio Borneo: cleanup, rebase to v3.17, submit mainline]
> Signed-off-by: Antonio Borneo <[email protected]>
> Cc: [email protected]
> ---
> V1 -> V2
> rename the synchronous buffer as rawbuf (instead of the
> asynchronous one)

Sorry for the lag and thanks for resubmitting.

This one is reviewed-by: Benjamin Tissoires <[email protected]>

Cheers,
Benjamin

>
> drivers/hid/i2c-hid/i2c-hid.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index 933bf10..c66e6ac 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -137,6 +137,7 @@ struct i2c_hid {
> * descriptor. */
> unsigned int bufsize; /* i2c buffer size */
> char *inbuf; /* Input buffer */
> + char *rawbuf; /* Raw Input buffer */
> char *cmdbuf; /* Command buffer */
> char *argsbuf; /* Command arguments buffer */
>
> @@ -504,9 +505,11 @@ static void i2c_hid_find_max_report(struct hid_device *hid, unsigned int type,
> static void i2c_hid_free_buffers(struct i2c_hid *ihid)
> {
> kfree(ihid->inbuf);
> + kfree(ihid->rawbuf);
> kfree(ihid->argsbuf);
> kfree(ihid->cmdbuf);
> ihid->inbuf = NULL;
> + ihid->rawbuf = NULL;
> ihid->cmdbuf = NULL;
> ihid->argsbuf = NULL;
> ihid->bufsize = 0;
> @@ -522,10 +525,11 @@ static int i2c_hid_alloc_buffers(struct i2c_hid *ihid, size_t report_size)
> report_size; /* report */
>
> ihid->inbuf = kzalloc(report_size, GFP_KERNEL);
> + ihid->rawbuf = kzalloc(report_size, GFP_KERNEL);
> ihid->argsbuf = kzalloc(args_len, GFP_KERNEL);
> ihid->cmdbuf = kzalloc(sizeof(union command) + args_len, GFP_KERNEL);
>
> - if (!ihid->inbuf || !ihid->argsbuf || !ihid->cmdbuf) {
> + if (!ihid->inbuf || !ihid->rawbuf || !ihid->argsbuf || !ihid->cmdbuf) {
> i2c_hid_free_buffers(ihid);
> return -ENOMEM;
> }
> @@ -552,12 +556,12 @@ static int i2c_hid_get_raw_report(struct hid_device *hid,
>
> ret = i2c_hid_get_report(client,
> report_type == HID_FEATURE_REPORT ? 0x03 : 0x01,
> - report_number, ihid->inbuf, ask_count);
> + report_number, ihid->rawbuf, ask_count);
>
> if (ret < 0)
> return ret;
>
> - ret_count = ihid->inbuf[0] | (ihid->inbuf[1] << 8);
> + ret_count = ihid->rawbuf[0] | (ihid->rawbuf[1] << 8);
>
> if (ret_count <= 2)
> return 0;
> @@ -566,7 +570,7 @@ static int i2c_hid_get_raw_report(struct hid_device *hid,
>
> /* The query buffer contains the size, dropping it in the reply */
> count = min(count, ret_count - 2);
> - memcpy(buf, ihid->inbuf + 2, count);
> + memcpy(buf, ihid->rawbuf + 2, count);
>
> return count;
> }
> --
> 2.1.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2014-11-25 14:26:45

by Jiri Kosina

[permalink] [raw]
Subject: Re: [PATCH V2] HID: i2c-hid: fix race condition reading reports

On Mon, 24 Nov 2014, Benjamin Tissoires wrote:

> > Current driver uses a common buffer for reading reports either
> > synchronously in i2c_hid_get_raw_report() and asynchronously in
> > the interrupt handler.
> > There is race condition if an interrupt arrives immediately after
> > the report is received in i2c_hid_get_raw_report(); the common
> > buffer is modified by the interrupt handler with the new report
> > and then i2c_hid_get_raw_report() proceed using wrong data.
> >
> > Fix it by using a separate buffers for synchronous reports.
> >
> > Signed-off-by: Jean-Baptiste Maneyrol <[email protected]>
> > [Antonio Borneo: cleanup, rebase to v3.17, submit mainline]
> > Signed-off-by: Antonio Borneo <[email protected]>
> > Cc: [email protected]
> > ---
> > V1 -> V2
> > rename the synchronous buffer as rawbuf (instead of the
> > asynchronous one)
>
> Sorry for the lag and thanks for resubmitting.
>
> This one is reviewed-by: Benjamin Tissoires <[email protected]>

Applied, thanks.


--
Jiri Kosina
SUSE Labs