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

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

[Antonio Borneo]

From: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>

From: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>

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 <jmaneyrol@invensense.com>
[Antonio Borneo: cleanup and rebase to v3.17]
Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com>
Cc: stable@vger.kernel.org
---

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

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

[Benjamin Tissoires]

Hey Antonio,

On Nov 16 2014 or thereabouts, Antonio Borneo wrote:
> From: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
> 
> From: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
> 
> 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 <jmaneyrol@invensense.com>
> [Antonio Borneo: cleanup and rebase to v3.17]
> Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com>
> Cc: stable@vger.kernel.org

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
> 

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

[Greg KH]

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 <jmaneyrol@invensense.com>
> > 
> > From: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
> > 
> > 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 <jmaneyrol@invensense.com>
> > [Antonio Borneo: cleanup and rebase to v3.17]
> > Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com>
> > Cc: stable@vger.kernel.org
> 
> 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

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

[Benjamin Tissoires]

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 <jmaneyrol@invensense.com>
> > > 
> > > From: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
> > > 
> > > 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 <jmaneyrol@invensense.com>
> > > [Antonio Borneo: cleanup and rebase to v3.17]
> > > Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com>
> > > Cc: stable@vger.kernel.org
> > 
> > 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

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

[Antonio Borneo]

Hi Benjamin,

On Tue, Nov 18, 2014 at 5:43 AM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> Hey Antonio,
>
> On Nov 16 2014 or thereabouts, Antonio Borneo wrote:
>> From: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
>>
>> From: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
>>
>> 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 <jmaneyrol@invensense.com>
>> [Antonio Borneo: cleanup and rebase to v3.17]
>> Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com>
>> Cc: stable@vger.kernel.org
>
> 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

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

[Antonio Borneo]

From: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>

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 <jmaneyrol@invensense.com>
[Antonio Borneo: cleanup, rebase to v3.17, submit mainline]
Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com>
Cc: stable@vger.kernel.org
---
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

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

[Benjamin Tissoires]

On Wed, Nov 19, 2014 at 11:46 AM, Antonio Borneo
<borneo.antonio@gmail.com> wrote:
> From: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
>
> 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 <jmaneyrol@invensense.com>
> [Antonio Borneo: cleanup, rebase to v3.17, submit mainline]
> Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com>
> Cc: stable@vger.kernel.org
> ---
> 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 <benjamin.tissoires@redhat.com>

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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jiri Kosina]

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 <jmaneyrol@invensense.com>
> > [Antonio Borneo: cleanup, rebase to v3.17, submit mainline]
> > Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com>
> > Cc: stable@vger.kernel.org
> > ---
> > 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 <benjamin.tissoires@redhat.com>

Applied, thanks.


-- 
Jiri Kosina
SUSE Labs