Skip to contentSkip to conversation

HID: i2c-hid: allocate hid buffers for real worst case

PATCH In review Dmitry Torokhov 3 participants 6 messages Sep 8, 2017 – Sep 13, 2017 HID

lore
Share & tools
lore

Dmitry Torokhov

author
#0 lore raw

PATCH 1/1 HID: i2c-hid: allocate hid buffers for real worst case

From: Adrian Salido <[email protected]>

The buffer allocation is not currently accounting for an extra byte for the report id. This can cause an out of bounds access in function i2c_hid_set_or_send_report() with reportID > 15.

Signed-off-by Guenter Roeck <[email protected]>
Signed-off-by Dmitry Torokhov <[email protected]>
---
 drivers/hid/i2c-hid/i2c-hid.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/hid/i2c-hid/i2c-hid.c +2 -1
index 77396145d2d0..9145c2129a96 100644
@@ -543,7 +543,8 @@ 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 */
+	int args_len = sizeof(__u8) + /* ReportID */
+		       sizeof(__u8) + /* optional ReportID byte */
 		       sizeof(__u16) + /* data register */
 		       sizeof(__u16) + /* size of the report */
 		       report_size; /* report */
Signature · 5 lines
2.14.1.581.gf28d330327-goog


-- 
Dmitry
maintainer
#1 lore raw
Quoted 8 lines · Dmitry Torokhov

From: Adrian Salido <[email protected]>

The buffer allocation is not currently accounting for an extra byte for the report id. This can cause an out of bounds access in function i2c_hid_set_or_send_report() with reportID > 15.

Signed-off-by Guenter Roeck <[email protected]>
Signed-off-by Dmitry Torokhov <[email protected]>

Missing signoff from the patch author?

Also, I think this should have Cc: stable, right?

Thanks,

Signature · 2 lines
Jiri Kosina
SUSE Labs
author
#2 lore raw
Quoted 4 lines · Jiri Kosina
Quoted 2 lines · Dmitry Torokhov

From: Adrian Salido <[email protected]>

Quoted 8 lines
Quoted 6 lines

The buffer allocation is not currently accounting for an extra byte for the report id. This can cause an out of bounds access in function i2c_hid_set_or_send_report() with reportID > 15.

Signed-off-by Guenter Roeck <[email protected]>
Signed-off-by Dmitry Torokhov <[email protected]>

Missing signoff from the patch author?

Oops, I must have cut it off on accident while removing ChromeOS specific tags, the original commit is here:

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/475212

Quoted 2 lines

Also, I think this should have Cc: stable, right?

I usually let maintainers decide, but yes.

Thanks.

Signature · 1 line
Dmitry
maintainer Reviewed-by Benson Leung
#3 lore raw
Quoted 9 lines · Dmitry Torokhov
Quoted 9 lines
Quoted 8 lines

From: Adrian Salido <[email protected]>

The buffer allocation is not currently accounting for an extra byte for the report id. This can cause an out of bounds access in function i2c_hid_set_or_send_report() with reportID > 15.

Signed-off-by Guenter Roeck <[email protected]>
Signed-off-by Dmitry Torokhov <[email protected]>
Quoted 6 lines
Quoted 1 line

Missing signoff from the patch author?

Oops, I must have cut it off on accident while removing ChromeOS specific tags, the original commit is here:

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/475212

Ok, thanks, will use that one. How about

Reviewed-by Benson Leung <[email protected]>

which is missing in the mail you've sent, but is there in the above reference commit?

Quoted 3 lines
Quoted 1 line

Also, I think this should have Cc: stable, right?

I usually let maintainers decide, but yes.

I'll be adding it. Thanks,

Signature · 2 lines
Jiri Kosina
SUSE Labs
Reviewed-by
#4 lore raw

Hi Jiri,

Quoted 11 lines · Jiri Kosina
Quoted 4 lines

Oops, I must have cut it off on accident while removing ChromeOS specific tags, the original commit is here:

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/475212

Ok, thanks, will use that one. How about

Reviewed-by Benson Leung <[email protected]>

which is missing in the mail you've sent, but is there in the above reference commit?

Submission looks good to me. Go ahead and add.

Reviewed-by Benson Leung <[email protected]>

Thanks,
Benson

Signature · 7 lines
Benson Leung
Staff Software Engineer
Chrome OS Kernel
Google Inc.
[email protected]
Chromium OS Project
[email protected]
maintainer
#5 lore raw
In reply to Dmitry Torokhov
Quoted 8 lines · Dmitry Torokhov

From: Adrian Salido <[email protected]>

The buffer allocation is not currently accounting for an extra byte for the report id. This can cause an out of bounds access in function i2c_hid_set_or_send_report() with reportID > 15.

Signed-off-by Guenter Roeck <[email protected]>
Signed-off-by Dmitry Torokhov <[email protected]>

I've added the missing tags and applied to for-4.14/upstream-fixes

Signature · 2 lines
Jiri Kosina
SUSE Labs