* Re: [PATCH] HID: usbhid: fix out-of-bounds bug
[not found] <CAAZ5spCffHnU_EtnhKpP5cDqsEKaGMatpaqAJqorvph+vZm1Sw@mail.gmail.com>
@ 2017-09-27 14:29 ` Alan Stern
0 siblings, 0 replies; 9+ messages in thread
From: Alan Stern @ 2017-09-27 14:29 UTC (permalink / raw)
To: Michel Hermier
Cc: Kostya Serebryany, syzkaller, Jiri Kosina, USB list,
Jaejoong Kim, Andrey Konovalov, Benjamin Tissoires,
Dmitry Vyukov, linux-input, LKML
On Wed, 27 Sep 2017, Michel Hermier wrote:
> Le 27 sept. 2017 07:42, "Alan Stern" <stern@rowland.harvard.edu> a écrit :
> > - for (n = 0; n < hdesc->bNumDescriptors; n++)
> > + num_descriptors = min_t(int, hdesc->bNumDescriptors,
> > + (hdesc->bLength - 6) / 3);
> > + for (n = 0; n < num_descriptors; n++)
> > if (hdesc->desc[n].bDescriptorType == HID_DT_REPORT)
> > rsize = le16_to_cpu(hdesc->desc[n].
> wDescriptorLength);
>
> Yes, this is a lot better.
>
>
> Is it possible to explicit the magic number 6 and 3 in the code. Currently,
> it looks like it comes from no where.
Yes, it is possible. The 6 is equal to
offsetof(struct hid_descriptor, desc)
and the 3 is equal to
sizeof(struct hid_class_descriptor)
(at least, I think it is -- the structure is marked as packed so its
size should be 3).
In this case I found the numbers to be more readable, but other people
may have different opinions.
> I'm also wondering if this change will not affect some devices in the wild,
> by rejecting hid descriptors with num descriptors == 0 ?
It's possible, but I doubt it. If such devices do exist, they should
never have worked in the first place. Certainly they would generate
warnings or errors during enumeration because of their invalid
descriptors.
Alan Stern
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] HID: usbhid: fix out-of-bounds bug
@ 2017-09-27 14:29 ` Alan Stern
0 siblings, 0 replies; 9+ messages in thread
From: Alan Stern @ 2017-09-27 14:29 UTC (permalink / raw)
To: Michel Hermier
Cc: Kostya Serebryany, syzkaller, Jiri Kosina, USB list,
Jaejoong Kim, Andrey Konovalov, Benjamin Tissoires,
Dmitry Vyukov, linux-input, LKML
On Wed, 27 Sep 2017, Michel Hermier wrote:
> Le 27 sept. 2017 07:42, "Alan Stern" <stern@rowland.harvard.edu> a écrit :
> > - for (n = 0; n < hdesc->bNumDescriptors; n++)
> > + num_descriptors = min_t(int, hdesc->bNumDescriptors,
> > + (hdesc->bLength - 6) / 3);
> > + for (n = 0; n < num_descriptors; n++)
> > if (hdesc->desc[n].bDescriptorType == HID_DT_REPORT)
> > rsize = le16_to_cpu(hdesc->desc[n].
> wDescriptorLength);
>
> Yes, this is a lot better.
>
>
> Is it possible to explicit the magic number 6 and 3 in the code. Currently,
> it looks like it comes from no where.
Yes, it is possible. The 6 is equal to
offsetof(struct hid_descriptor, desc)
and the 3 is equal to
sizeof(struct hid_class_descriptor)
(at least, I think it is -- the structure is marked as packed so its
size should be 3).
In this case I found the numbers to be more readable, but other people
may have different opinions.
> I'm also wondering if this change will not affect some devices in the wild,
> by rejecting hid descriptors with num descriptors == 0 ?
It's possible, but I doubt it. If such devices do exist, they should
never have worked in the first place. Certainly they would generate
warnings or errors during enumeration because of their invalid
descriptors.
Alan Stern
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] HID: usbhid: fix out-of-bounds bug
2017-09-27 14:29 ` Alan Stern
(?)
@ 2017-09-28 8:44 ` Jaejoong Kim
2017-09-28 10:16 ` [PATCH v2] " Jaejoong Kim
-1 siblings, 1 reply; 9+ messages in thread
From: Jaejoong Kim @ 2017-09-28 8:44 UTC (permalink / raw)
To: Alan Stern
Cc: Michel Hermier, Kostya Serebryany, syzkaller, Jiri Kosina,
USB list, Andrey Konovalov, Benjamin Tissoires, Dmitry Vyukov,
linux-input, LKML
2017-09-27 23:29 GMT+09:00 Alan Stern <stern@rowland.harvard.edu>:
> On Wed, 27 Sep 2017, Michel Hermier wrote:
>
>> Le 27 sept. 2017 07:42, "Alan Stern" <stern@rowland.harvard.edu> a écrit :
>
>> > - for (n = 0; n < hdesc->bNumDescriptors; n++)
>> > + num_descriptors = min_t(int, hdesc->bNumDescriptors,
>> > + (hdesc->bLength - 6) / 3);
>> > + for (n = 0; n < num_descriptors; n++)
>> > if (hdesc->desc[n].bDescriptorType == HID_DT_REPORT)
>> > rsize = le16_to_cpu(hdesc->desc[n].
>> wDescriptorLength);
>>
>> Yes, this is a lot better.
OK.
>>
>>
>> Is it possible to explicit the magic number 6 and 3 in the code. Currently,
>> it looks like it comes from no where.
I gree with you.
>
> Yes, it is possible. The 6 is equal to
>
> offsetof(struct hid_descriptor, desc)
>
> and the 3 is equal to
>
> sizeof(struct hid_class_descriptor)
>
> (at least, I think it is -- the structure is marked as packed so its
> size should be 3).
>
> In this case I found the numbers to be more readable, but other people
> may have different opinions.
I will post V2 shortly.
>
>> I'm also wondering if this change will not affect some devices in the wild,
>> by rejecting hid descriptors with num descriptors == 0 ?
>
> It's possible, but I doubt it. If such devices do exist, they should
> never have worked in the first place. Certainly they would generate
> warnings or errors during enumeration because of their invalid
> descriptors.
>
> Alan Stern
>
Jaejoong
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] HID: usbhid: fix out-of-bounds bug
2017-09-28 8:44 ` Jaejoong Kim
@ 2017-09-28 10:16 ` Jaejoong Kim
2017-10-10 7:25 ` Jaejoong Kim
2017-10-11 13:06 ` Jiri Kosina
0 siblings, 2 replies; 9+ messages in thread
From: Jaejoong Kim @ 2017-09-28 10:16 UTC (permalink / raw)
To: Alan Stern, Jiri Kosina
Cc: Andrey Konovalov, Benjamin Tissoires, USB list, linux-input,
LKML, syzkaller, Jaejoong Kim
The hid descriptor identifies the length and type of subordinate
descriptors for a device. If the received hid descriptor is smaller than
the size of the struct hid_descriptor, it is possible to cause
out-of-bounds.
In addition, if bNumDescriptors of the hid descriptor have an incorrect
value, this can also cause out-of-bounds while approaching hdesc->desc[n].
So check the size of hid descriptor and bNumDescriptors.
BUG: KASAN: slab-out-of-bounds in usbhid_parse+0x9b1/0xa20
Read of size 1 at addr ffff88006c5f8edf by task kworker/1:2/1261
CPU: 1 PID: 1261 Comm: kworker/1:2 Not tainted
4.14.0-rc1-42251-gebb2c2437d80 #169
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
Workqueue: usb_hub_wq hub_event
Call Trace:
__dump_stack lib/dump_stack.c:16
dump_stack+0x292/0x395 lib/dump_stack.c:52
print_address_description+0x78/0x280 mm/kasan/report.c:252
kasan_report_error mm/kasan/report.c:351
kasan_report+0x22f/0x340 mm/kasan/report.c:409
__asan_report_load1_noabort+0x19/0x20 mm/kasan/report.c:427
usbhid_parse+0x9b1/0xa20 drivers/hid/usbhid/hid-core.c:1004
hid_add_device+0x16b/0xb30 drivers/hid/hid-core.c:2944
usbhid_probe+0xc28/0x1100 drivers/hid/usbhid/hid-core.c:1369
usb_probe_interface+0x35d/0x8e0 drivers/usb/core/driver.c:361
really_probe drivers/base/dd.c:413
driver_probe_device+0x610/0xa00 drivers/base/dd.c:557
__device_attach_driver+0x230/0x290 drivers/base/dd.c:653
bus_for_each_drv+0x161/0x210 drivers/base/bus.c:463
__device_attach+0x26e/0x3d0 drivers/base/dd.c:710
device_initial_probe+0x1f/0x30 drivers/base/dd.c:757
bus_probe_device+0x1eb/0x290 drivers/base/bus.c:523
device_add+0xd0b/0x1660 drivers/base/core.c:1835
usb_set_configuration+0x104e/0x1870 drivers/usb/core/message.c:1932
generic_probe+0x73/0xe0 drivers/usb/core/generic.c:174
usb_probe_device+0xaf/0xe0 drivers/usb/core/driver.c:266
really_probe drivers/base/dd.c:413
driver_probe_device+0x610/0xa00 drivers/base/dd.c:557
__device_attach_driver+0x230/0x290 drivers/base/dd.c:653
bus_for_each_drv+0x161/0x210 drivers/base/bus.c:463
__device_attach+0x26e/0x3d0 drivers/base/dd.c:710
device_initial_probe+0x1f/0x30 drivers/base/dd.c:757
bus_probe_device+0x1eb/0x290 drivers/base/bus.c:523
device_add+0xd0b/0x1660 drivers/base/core.c:1835
usb_new_device+0x7b8/0x1020 drivers/usb/core/hub.c:2457
hub_port_connect drivers/usb/core/hub.c:4903
hub_port_connect_change drivers/usb/core/hub.c:5009
port_event drivers/usb/core/hub.c:5115
hub_event+0x194d/0x3740 drivers/usb/core/hub.c:5195
process_one_work+0xc7f/0x1db0 kernel/workqueue.c:2119
worker_thread+0x221/0x1850 kernel/workqueue.c:2253
kthread+0x3a1/0x470 kernel/kthread.c:231
ret_from_fork+0x2a/0x40 arch/x86/entry/entry_64.S:431
Reported-by: Andrey Konovalov <andreyknvl@google.com>
Signed-off-by: Jaejoong Kim <climbbb.kim@gmail.com>
---
Changes in v2:
- write a new commit message because orginal version is wrong approach
- add check hid descriptor size
- get proper value for bNumDescriptors as suggested by Alan Stern
- fix the Reported-by
drivers/hid/usbhid/hid-core.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 089bad8..045b5da 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -975,6 +975,8 @@ static int usbhid_parse(struct hid_device *hid)
unsigned int rsize = 0;
char *rdesc;
int ret, n;
+ int num_descriptors;
+ size_t offset = offsetof(struct hid_descriptor, desc);
quirks = usbhid_lookup_quirk(le16_to_cpu(dev->descriptor.idVendor),
le16_to_cpu(dev->descriptor.idProduct));
@@ -997,10 +999,18 @@ static int usbhid_parse(struct hid_device *hid)
return -ENODEV;
}
+ if (hdesc->bLength < sizeof(struct hid_descriptor)) {
+ dbg_hid("hid descriptor is too short\n");
+ return -EINVAL;
+ }
+
hid->version = le16_to_cpu(hdesc->bcdHID);
hid->country = hdesc->bCountryCode;
- for (n = 0; n < hdesc->bNumDescriptors; n++)
+ num_descriptors = min_t(int, hdesc->bNumDescriptors,
+ (hdesc->bLength - offset) / sizeof(struct hid_class_descriptor));
+
+ for (n = 0; n < num_descriptors; n++)
if (hdesc->desc[n].bDescriptorType == HID_DT_REPORT)
rsize = le16_to_cpu(hdesc->desc[n].wDescriptorLength);
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] HID: usbhid: fix out-of-bounds bug
2017-09-28 10:16 ` [PATCH v2] " Jaejoong Kim
@ 2017-10-10 7:25 ` Jaejoong Kim
2017-10-10 11:23 ` Andrey Konovalov
2017-10-10 14:22 ` Alan Stern
2017-10-11 13:06 ` Jiri Kosina
1 sibling, 2 replies; 9+ messages in thread
From: Jaejoong Kim @ 2017-10-10 7:25 UTC (permalink / raw)
To: Alan Stern, Jiri Kosina, Andrey Konovalov
Cc: Benjamin Tissoires, USB list, linux-input, LKML, syzkaller, Jaejoong Kim
Hi,
To. Jiri, Alan,
Could you please review this patch?
To. Andey,
Could you please test with this patch for KASAN OOB error?
Thanks, jaejoong
2017-09-28 19:16 GMT+09:00 Jaejoong Kim <climbbb.kim@gmail.com>:
> The hid descriptor identifies the length and type of subordinate
> descriptors for a device. If the received hid descriptor is smaller than
> the size of the struct hid_descriptor, it is possible to cause
> out-of-bounds.
>
> In addition, if bNumDescriptors of the hid descriptor have an incorrect
> value, this can also cause out-of-bounds while approaching hdesc->desc[n].
>
> So check the size of hid descriptor and bNumDescriptors.
>
> BUG: KASAN: slab-out-of-bounds in usbhid_parse+0x9b1/0xa20
> Read of size 1 at addr ffff88006c5f8edf by task kworker/1:2/1261
>
> CPU: 1 PID: 1261 Comm: kworker/1:2 Not tainted
> 4.14.0-rc1-42251-gebb2c2437d80 #169
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> Workqueue: usb_hub_wq hub_event
> Call Trace:
> __dump_stack lib/dump_stack.c:16
> dump_stack+0x292/0x395 lib/dump_stack.c:52
> print_address_description+0x78/0x280 mm/kasan/report.c:252
> kasan_report_error mm/kasan/report.c:351
> kasan_report+0x22f/0x340 mm/kasan/report.c:409
> __asan_report_load1_noabort+0x19/0x20 mm/kasan/report.c:427
> usbhid_parse+0x9b1/0xa20 drivers/hid/usbhid/hid-core.c:1004
> hid_add_device+0x16b/0xb30 drivers/hid/hid-core.c:2944
> usbhid_probe+0xc28/0x1100 drivers/hid/usbhid/hid-core.c:1369
> usb_probe_interface+0x35d/0x8e0 drivers/usb/core/driver.c:361
> really_probe drivers/base/dd.c:413
> driver_probe_device+0x610/0xa00 drivers/base/dd.c:557
> __device_attach_driver+0x230/0x290 drivers/base/dd.c:653
> bus_for_each_drv+0x161/0x210 drivers/base/bus.c:463
> __device_attach+0x26e/0x3d0 drivers/base/dd.c:710
> device_initial_probe+0x1f/0x30 drivers/base/dd.c:757
> bus_probe_device+0x1eb/0x290 drivers/base/bus.c:523
> device_add+0xd0b/0x1660 drivers/base/core.c:1835
> usb_set_configuration+0x104e/0x1870 drivers/usb/core/message.c:1932
> generic_probe+0x73/0xe0 drivers/usb/core/generic.c:174
> usb_probe_device+0xaf/0xe0 drivers/usb/core/driver.c:266
> really_probe drivers/base/dd.c:413
> driver_probe_device+0x610/0xa00 drivers/base/dd.c:557
> __device_attach_driver+0x230/0x290 drivers/base/dd.c:653
> bus_for_each_drv+0x161/0x210 drivers/base/bus.c:463
> __device_attach+0x26e/0x3d0 drivers/base/dd.c:710
> device_initial_probe+0x1f/0x30 drivers/base/dd.c:757
> bus_probe_device+0x1eb/0x290 drivers/base/bus.c:523
> device_add+0xd0b/0x1660 drivers/base/core.c:1835
> usb_new_device+0x7b8/0x1020 drivers/usb/core/hub.c:2457
> hub_port_connect drivers/usb/core/hub.c:4903
> hub_port_connect_change drivers/usb/core/hub.c:5009
> port_event drivers/usb/core/hub.c:5115
> hub_event+0x194d/0x3740 drivers/usb/core/hub.c:5195
> process_one_work+0xc7f/0x1db0 kernel/workqueue.c:2119
> worker_thread+0x221/0x1850 kernel/workqueue.c:2253
> kthread+0x3a1/0x470 kernel/kthread.c:231
> ret_from_fork+0x2a/0x40 arch/x86/entry/entry_64.S:431
>
> Reported-by: Andrey Konovalov <andreyknvl@google.com>
> Signed-off-by: Jaejoong Kim <climbbb.kim@gmail.com>
> ---
>
> Changes in v2:
> - write a new commit message because orginal version is wrong approach
> - add check hid descriptor size
> - get proper value for bNumDescriptors as suggested by Alan Stern
> - fix the Reported-by
>
> drivers/hid/usbhid/hid-core.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index 089bad8..045b5da 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -975,6 +975,8 @@ static int usbhid_parse(struct hid_device *hid)
> unsigned int rsize = 0;
> char *rdesc;
> int ret, n;
> + int num_descriptors;
> + size_t offset = offsetof(struct hid_descriptor, desc);
>
> quirks = usbhid_lookup_quirk(le16_to_cpu(dev->descriptor.idVendor),
> le16_to_cpu(dev->descriptor.idProduct));
> @@ -997,10 +999,18 @@ static int usbhid_parse(struct hid_device *hid)
> return -ENODEV;
> }
>
> + if (hdesc->bLength < sizeof(struct hid_descriptor)) {
> + dbg_hid("hid descriptor is too short\n");
> + return -EINVAL;
> + }
> +
> hid->version = le16_to_cpu(hdesc->bcdHID);
> hid->country = hdesc->bCountryCode;
>
> - for (n = 0; n < hdesc->bNumDescriptors; n++)
> + num_descriptors = min_t(int, hdesc->bNumDescriptors,
> + (hdesc->bLength - offset) / sizeof(struct hid_class_descriptor));
> +
> + for (n = 0; n < num_descriptors; n++)
> if (hdesc->desc[n].bDescriptorType == HID_DT_REPORT)
> rsize = le16_to_cpu(hdesc->desc[n].wDescriptorLength);
>
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] HID: usbhid: fix out-of-bounds bug
2017-10-10 7:25 ` Jaejoong Kim
@ 2017-10-10 11:23 ` Andrey Konovalov
2017-10-10 14:22 ` Alan Stern
1 sibling, 0 replies; 9+ messages in thread
From: Andrey Konovalov @ 2017-10-10 11:23 UTC (permalink / raw)
To: Jaejoong Kim
Cc: Alan Stern, Jiri Kosina, Benjamin Tissoires, USB list,
linux-input, LKML, syzkaller
On Tue, Oct 10, 2017 at 9:25 AM, Jaejoong Kim <climbbb.kim@gmail.com> wrote:
> Hi,
>
> To. Jiri, Alan,
>
> Could you please review this patch?
>
> To. Andey,
>
> Could you please test with this patch for KASAN OOB error?
Hi!
Yes, your patch fixes the issue.
Thanks!
Tested-by: Andrey Konovalov <andreyknvl@google.com>
>
> Thanks, jaejoong
>
> 2017-09-28 19:16 GMT+09:00 Jaejoong Kim <climbbb.kim@gmail.com>:
>> The hid descriptor identifies the length and type of subordinate
>> descriptors for a device. If the received hid descriptor is smaller than
>> the size of the struct hid_descriptor, it is possible to cause
>> out-of-bounds.
>>
>> In addition, if bNumDescriptors of the hid descriptor have an incorrect
>> value, this can also cause out-of-bounds while approaching hdesc->desc[n].
>>
>> So check the size of hid descriptor and bNumDescriptors.
>>
>> BUG: KASAN: slab-out-of-bounds in usbhid_parse+0x9b1/0xa20
>> Read of size 1 at addr ffff88006c5f8edf by task kworker/1:2/1261
>>
>> CPU: 1 PID: 1261 Comm: kworker/1:2 Not tainted
>> 4.14.0-rc1-42251-gebb2c2437d80 #169
>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
>> Workqueue: usb_hub_wq hub_event
>> Call Trace:
>> __dump_stack lib/dump_stack.c:16
>> dump_stack+0x292/0x395 lib/dump_stack.c:52
>> print_address_description+0x78/0x280 mm/kasan/report.c:252
>> kasan_report_error mm/kasan/report.c:351
>> kasan_report+0x22f/0x340 mm/kasan/report.c:409
>> __asan_report_load1_noabort+0x19/0x20 mm/kasan/report.c:427
>> usbhid_parse+0x9b1/0xa20 drivers/hid/usbhid/hid-core.c:1004
>> hid_add_device+0x16b/0xb30 drivers/hid/hid-core.c:2944
>> usbhid_probe+0xc28/0x1100 drivers/hid/usbhid/hid-core.c:1369
>> usb_probe_interface+0x35d/0x8e0 drivers/usb/core/driver.c:361
>> really_probe drivers/base/dd.c:413
>> driver_probe_device+0x610/0xa00 drivers/base/dd.c:557
>> __device_attach_driver+0x230/0x290 drivers/base/dd.c:653
>> bus_for_each_drv+0x161/0x210 drivers/base/bus.c:463
>> __device_attach+0x26e/0x3d0 drivers/base/dd.c:710
>> device_initial_probe+0x1f/0x30 drivers/base/dd.c:757
>> bus_probe_device+0x1eb/0x290 drivers/base/bus.c:523
>> device_add+0xd0b/0x1660 drivers/base/core.c:1835
>> usb_set_configuration+0x104e/0x1870 drivers/usb/core/message.c:1932
>> generic_probe+0x73/0xe0 drivers/usb/core/generic.c:174
>> usb_probe_device+0xaf/0xe0 drivers/usb/core/driver.c:266
>> really_probe drivers/base/dd.c:413
>> driver_probe_device+0x610/0xa00 drivers/base/dd.c:557
>> __device_attach_driver+0x230/0x290 drivers/base/dd.c:653
>> bus_for_each_drv+0x161/0x210 drivers/base/bus.c:463
>> __device_attach+0x26e/0x3d0 drivers/base/dd.c:710
>> device_initial_probe+0x1f/0x30 drivers/base/dd.c:757
>> bus_probe_device+0x1eb/0x290 drivers/base/bus.c:523
>> device_add+0xd0b/0x1660 drivers/base/core.c:1835
>> usb_new_device+0x7b8/0x1020 drivers/usb/core/hub.c:2457
>> hub_port_connect drivers/usb/core/hub.c:4903
>> hub_port_connect_change drivers/usb/core/hub.c:5009
>> port_event drivers/usb/core/hub.c:5115
>> hub_event+0x194d/0x3740 drivers/usb/core/hub.c:5195
>> process_one_work+0xc7f/0x1db0 kernel/workqueue.c:2119
>> worker_thread+0x221/0x1850 kernel/workqueue.c:2253
>> kthread+0x3a1/0x470 kernel/kthread.c:231
>> ret_from_fork+0x2a/0x40 arch/x86/entry/entry_64.S:431
>>
>> Reported-by: Andrey Konovalov <andreyknvl@google.com>
>> Signed-off-by: Jaejoong Kim <climbbb.kim@gmail.com>
>> ---
>>
>> Changes in v2:
>> - write a new commit message because orginal version is wrong approach
>> - add check hid descriptor size
>> - get proper value for bNumDescriptors as suggested by Alan Stern
>> - fix the Reported-by
>>
>> drivers/hid/usbhid/hid-core.c | 12 +++++++++++-
>> 1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
>> index 089bad8..045b5da 100644
>> --- a/drivers/hid/usbhid/hid-core.c
>> +++ b/drivers/hid/usbhid/hid-core.c
>> @@ -975,6 +975,8 @@ static int usbhid_parse(struct hid_device *hid)
>> unsigned int rsize = 0;
>> char *rdesc;
>> int ret, n;
>> + int num_descriptors;
>> + size_t offset = offsetof(struct hid_descriptor, desc);
>>
>> quirks = usbhid_lookup_quirk(le16_to_cpu(dev->descriptor.idVendor),
>> le16_to_cpu(dev->descriptor.idProduct));
>> @@ -997,10 +999,18 @@ static int usbhid_parse(struct hid_device *hid)
>> return -ENODEV;
>> }
>>
>> + if (hdesc->bLength < sizeof(struct hid_descriptor)) {
>> + dbg_hid("hid descriptor is too short\n");
>> + return -EINVAL;
>> + }
>> +
>> hid->version = le16_to_cpu(hdesc->bcdHID);
>> hid->country = hdesc->bCountryCode;
>>
>> - for (n = 0; n < hdesc->bNumDescriptors; n++)
>> + num_descriptors = min_t(int, hdesc->bNumDescriptors,
>> + (hdesc->bLength - offset) / sizeof(struct hid_class_descriptor));
>> +
>> + for (n = 0; n < num_descriptors; n++)
>> if (hdesc->desc[n].bDescriptorType == HID_DT_REPORT)
>> rsize = le16_to_cpu(hdesc->desc[n].wDescriptorLength);
>>
>> --
>> 2.7.4
>>
>
> --
> You received this message because you are subscribed to the Google Groups "syzkaller" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] HID: usbhid: fix out-of-bounds bug
2017-10-10 7:25 ` Jaejoong Kim
@ 2017-10-10 14:22 ` Alan Stern
2017-10-10 14:22 ` Alan Stern
1 sibling, 0 replies; 9+ messages in thread
From: Alan Stern @ 2017-10-10 14:22 UTC (permalink / raw)
To: Jaejoong Kim
Cc: Jiri Kosina, Andrey Konovalov, Benjamin Tissoires, USB list,
linux-input, LKML, syzkaller
On Tue, 10 Oct 2017, Jaejoong Kim wrote:
> Hi,
>
> To. Jiri, Alan,
>
> Could you please review this patch?
>
> To. Andey,
>
> Could you please test with this patch for KASAN OOB error?
>
> Thanks, jaejoong
>
> 2017-09-28 19:16 GMT+09:00 Jaejoong Kim <climbbb.kim@gmail.com>:
> > The hid descriptor identifies the length and type of subordinate
> > descriptors for a device. If the received hid descriptor is smaller than
> > the size of the struct hid_descriptor, it is possible to cause
> > out-of-bounds.
> >
> > In addition, if bNumDescriptors of the hid descriptor have an incorrect
> > value, this can also cause out-of-bounds while approaching hdesc->desc[n].
> >
> > So check the size of hid descriptor and bNumDescriptors.
> >
> > BUG: KASAN: slab-out-of-bounds in usbhid_parse+0x9b1/0xa20
> > Read of size 1 at addr ffff88006c5f8edf by task kworker/1:2/1261
> >
> > CPU: 1 PID: 1261 Comm: kworker/1:2 Not tainted
> > 4.14.0-rc1-42251-gebb2c2437d80 #169
> > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> > Workqueue: usb_hub_wq hub_event
> > Call Trace:
> > __dump_stack lib/dump_stack.c:16
> > dump_stack+0x292/0x395 lib/dump_stack.c:52
> > print_address_description+0x78/0x280 mm/kasan/report.c:252
> > kasan_report_error mm/kasan/report.c:351
> > kasan_report+0x22f/0x340 mm/kasan/report.c:409
> > __asan_report_load1_noabort+0x19/0x20 mm/kasan/report.c:427
> > usbhid_parse+0x9b1/0xa20 drivers/hid/usbhid/hid-core.c:1004
> > hid_add_device+0x16b/0xb30 drivers/hid/hid-core.c:2944
> > usbhid_probe+0xc28/0x1100 drivers/hid/usbhid/hid-core.c:1369
> > usb_probe_interface+0x35d/0x8e0 drivers/usb/core/driver.c:361
> > really_probe drivers/base/dd.c:413
> > driver_probe_device+0x610/0xa00 drivers/base/dd.c:557
> > __device_attach_driver+0x230/0x290 drivers/base/dd.c:653
> > bus_for_each_drv+0x161/0x210 drivers/base/bus.c:463
> > __device_attach+0x26e/0x3d0 drivers/base/dd.c:710
> > device_initial_probe+0x1f/0x30 drivers/base/dd.c:757
> > bus_probe_device+0x1eb/0x290 drivers/base/bus.c:523
> > device_add+0xd0b/0x1660 drivers/base/core.c:1835
> > usb_set_configuration+0x104e/0x1870 drivers/usb/core/message.c:1932
> > generic_probe+0x73/0xe0 drivers/usb/core/generic.c:174
> > usb_probe_device+0xaf/0xe0 drivers/usb/core/driver.c:266
> > really_probe drivers/base/dd.c:413
> > driver_probe_device+0x610/0xa00 drivers/base/dd.c:557
> > __device_attach_driver+0x230/0x290 drivers/base/dd.c:653
> > bus_for_each_drv+0x161/0x210 drivers/base/bus.c:463
> > __device_attach+0x26e/0x3d0 drivers/base/dd.c:710
> > device_initial_probe+0x1f/0x30 drivers/base/dd.c:757
> > bus_probe_device+0x1eb/0x290 drivers/base/bus.c:523
> > device_add+0xd0b/0x1660 drivers/base/core.c:1835
> > usb_new_device+0x7b8/0x1020 drivers/usb/core/hub.c:2457
> > hub_port_connect drivers/usb/core/hub.c:4903
> > hub_port_connect_change drivers/usb/core/hub.c:5009
> > port_event drivers/usb/core/hub.c:5115
> > hub_event+0x194d/0x3740 drivers/usb/core/hub.c:5195
> > process_one_work+0xc7f/0x1db0 kernel/workqueue.c:2119
> > worker_thread+0x221/0x1850 kernel/workqueue.c:2253
> > kthread+0x3a1/0x470 kernel/kthread.c:231
> > ret_from_fork+0x2a/0x40 arch/x86/entry/entry_64.S:431
> >
> > Reported-by: Andrey Konovalov <andreyknvl@google.com>
> > Signed-off-by: Jaejoong Kim <climbbb.kim@gmail.com>
> > ---
> >
> > Changes in v2:
> > - write a new commit message because orginal version is wrong approach
> > - add check hid descriptor size
> > - get proper value for bNumDescriptors as suggested by Alan Stern
> > - fix the Reported-by
> >
> > drivers/hid/usbhid/hid-core.c | 12 +++++++++++-
> > 1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> > index 089bad8..045b5da 100644
> > --- a/drivers/hid/usbhid/hid-core.c
> > +++ b/drivers/hid/usbhid/hid-core.c
> > @@ -975,6 +975,8 @@ static int usbhid_parse(struct hid_device *hid)
> > unsigned int rsize = 0;
> > char *rdesc;
> > int ret, n;
> > + int num_descriptors;
> > + size_t offset = offsetof(struct hid_descriptor, desc);
> >
> > quirks = usbhid_lookup_quirk(le16_to_cpu(dev->descriptor.idVendor),
> > le16_to_cpu(dev->descriptor.idProduct));
> > @@ -997,10 +999,18 @@ static int usbhid_parse(struct hid_device *hid)
> > return -ENODEV;
> > }
> >
> > + if (hdesc->bLength < sizeof(struct hid_descriptor)) {
> > + dbg_hid("hid descriptor is too short\n");
> > + return -EINVAL;
> > + }
> > +
> > hid->version = le16_to_cpu(hdesc->bcdHID);
> > hid->country = hdesc->bCountryCode;
> >
> > - for (n = 0; n < hdesc->bNumDescriptors; n++)
> > + num_descriptors = min_t(int, hdesc->bNumDescriptors,
> > + (hdesc->bLength - offset) / sizeof(struct hid_class_descriptor));
> > +
> > + for (n = 0; n < num_descriptors; n++)
> > if (hdesc->desc[n].bDescriptorType == HID_DT_REPORT)
> > rsize = le16_to_cpu(hdesc->desc[n].wDescriptorLength);
Acked-by: Alan Stern <stern@rowland.harvard.edu>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] HID: usbhid: fix out-of-bounds bug
@ 2017-10-10 14:22 ` Alan Stern
0 siblings, 0 replies; 9+ messages in thread
From: Alan Stern @ 2017-10-10 14:22 UTC (permalink / raw)
To: Jaejoong Kim
Cc: Jiri Kosina, Andrey Konovalov, Benjamin Tissoires, USB list,
linux-input, LKML, syzkaller
On Tue, 10 Oct 2017, Jaejoong Kim wrote:
> Hi,
>
> To. Jiri, Alan,
>
> Could you please review this patch?
>
> To. Andey,
>
> Could you please test with this patch for KASAN OOB error?
>
> Thanks, jaejoong
>
> 2017-09-28 19:16 GMT+09:00 Jaejoong Kim <climbbb.kim@gmail.com>:
> > The hid descriptor identifies the length and type of subordinate
> > descriptors for a device. If the received hid descriptor is smaller than
> > the size of the struct hid_descriptor, it is possible to cause
> > out-of-bounds.
> >
> > In addition, if bNumDescriptors of the hid descriptor have an incorrect
> > value, this can also cause out-of-bounds while approaching hdesc->desc[n].
> >
> > So check the size of hid descriptor and bNumDescriptors.
> >
> > BUG: KASAN: slab-out-of-bounds in usbhid_parse+0x9b1/0xa20
> > Read of size 1 at addr ffff88006c5f8edf by task kworker/1:2/1261
> >
> > CPU: 1 PID: 1261 Comm: kworker/1:2 Not tainted
> > 4.14.0-rc1-42251-gebb2c2437d80 #169
> > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> > Workqueue: usb_hub_wq hub_event
> > Call Trace:
> > __dump_stack lib/dump_stack.c:16
> > dump_stack+0x292/0x395 lib/dump_stack.c:52
> > print_address_description+0x78/0x280 mm/kasan/report.c:252
> > kasan_report_error mm/kasan/report.c:351
> > kasan_report+0x22f/0x340 mm/kasan/report.c:409
> > __asan_report_load1_noabort+0x19/0x20 mm/kasan/report.c:427
> > usbhid_parse+0x9b1/0xa20 drivers/hid/usbhid/hid-core.c:1004
> > hid_add_device+0x16b/0xb30 drivers/hid/hid-core.c:2944
> > usbhid_probe+0xc28/0x1100 drivers/hid/usbhid/hid-core.c:1369
> > usb_probe_interface+0x35d/0x8e0 drivers/usb/core/driver.c:361
> > really_probe drivers/base/dd.c:413
> > driver_probe_device+0x610/0xa00 drivers/base/dd.c:557
> > __device_attach_driver+0x230/0x290 drivers/base/dd.c:653
> > bus_for_each_drv+0x161/0x210 drivers/base/bus.c:463
> > __device_attach+0x26e/0x3d0 drivers/base/dd.c:710
> > device_initial_probe+0x1f/0x30 drivers/base/dd.c:757
> > bus_probe_device+0x1eb/0x290 drivers/base/bus.c:523
> > device_add+0xd0b/0x1660 drivers/base/core.c:1835
> > usb_set_configuration+0x104e/0x1870 drivers/usb/core/message.c:1932
> > generic_probe+0x73/0xe0 drivers/usb/core/generic.c:174
> > usb_probe_device+0xaf/0xe0 drivers/usb/core/driver.c:266
> > really_probe drivers/base/dd.c:413
> > driver_probe_device+0x610/0xa00 drivers/base/dd.c:557
> > __device_attach_driver+0x230/0x290 drivers/base/dd.c:653
> > bus_for_each_drv+0x161/0x210 drivers/base/bus.c:463
> > __device_attach+0x26e/0x3d0 drivers/base/dd.c:710
> > device_initial_probe+0x1f/0x30 drivers/base/dd.c:757
> > bus_probe_device+0x1eb/0x290 drivers/base/bus.c:523
> > device_add+0xd0b/0x1660 drivers/base/core.c:1835
> > usb_new_device+0x7b8/0x1020 drivers/usb/core/hub.c:2457
> > hub_port_connect drivers/usb/core/hub.c:4903
> > hub_port_connect_change drivers/usb/core/hub.c:5009
> > port_event drivers/usb/core/hub.c:5115
> > hub_event+0x194d/0x3740 drivers/usb/core/hub.c:5195
> > process_one_work+0xc7f/0x1db0 kernel/workqueue.c:2119
> > worker_thread+0x221/0x1850 kernel/workqueue.c:2253
> > kthread+0x3a1/0x470 kernel/kthread.c:231
> > ret_from_fork+0x2a/0x40 arch/x86/entry/entry_64.S:431
> >
> > Reported-by: Andrey Konovalov <andreyknvl@google.com>
> > Signed-off-by: Jaejoong Kim <climbbb.kim@gmail.com>
> > ---
> >
> > Changes in v2:
> > - write a new commit message because orginal version is wrong approach
> > - add check hid descriptor size
> > - get proper value for bNumDescriptors as suggested by Alan Stern
> > - fix the Reported-by
> >
> > drivers/hid/usbhid/hid-core.c | 12 +++++++++++-
> > 1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> > index 089bad8..045b5da 100644
> > --- a/drivers/hid/usbhid/hid-core.c
> > +++ b/drivers/hid/usbhid/hid-core.c
> > @@ -975,6 +975,8 @@ static int usbhid_parse(struct hid_device *hid)
> > unsigned int rsize = 0;
> > char *rdesc;
> > int ret, n;
> > + int num_descriptors;
> > + size_t offset = offsetof(struct hid_descriptor, desc);
> >
> > quirks = usbhid_lookup_quirk(le16_to_cpu(dev->descriptor.idVendor),
> > le16_to_cpu(dev->descriptor.idProduct));
> > @@ -997,10 +999,18 @@ static int usbhid_parse(struct hid_device *hid)
> > return -ENODEV;
> > }
> >
> > + if (hdesc->bLength < sizeof(struct hid_descriptor)) {
> > + dbg_hid("hid descriptor is too short\n");
> > + return -EINVAL;
> > + }
> > +
> > hid->version = le16_to_cpu(hdesc->bcdHID);
> > hid->country = hdesc->bCountryCode;
> >
> > - for (n = 0; n < hdesc->bNumDescriptors; n++)
> > + num_descriptors = min_t(int, hdesc->bNumDescriptors,
> > + (hdesc->bLength - offset) / sizeof(struct hid_class_descriptor));
> > +
> > + for (n = 0; n < num_descriptors; n++)
> > if (hdesc->desc[n].bDescriptorType == HID_DT_REPORT)
> > rsize = le16_to_cpu(hdesc->desc[n].wDescriptorLength);
Acked-by: Alan Stern <stern@rowland.harvard.edu>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] HID: usbhid: fix out-of-bounds bug
2017-09-28 10:16 ` [PATCH v2] " Jaejoong Kim
2017-10-10 7:25 ` Jaejoong Kim
@ 2017-10-11 13:06 ` Jiri Kosina
1 sibling, 0 replies; 9+ messages in thread
From: Jiri Kosina @ 2017-10-11 13:06 UTC (permalink / raw)
To: Jaejoong Kim
Cc: Alan Stern, Andrey Konovalov, Benjamin Tissoires, USB list,
linux-input, LKML, syzkaller
On Thu, 28 Sep 2017, Jaejoong Kim wrote:
> The hid descriptor identifies the length and type of subordinate
> descriptors for a device. If the received hid descriptor is smaller than
> the size of the struct hid_descriptor, it is possible to cause
> out-of-bounds.
>
> In addition, if bNumDescriptors of the hid descriptor have an incorrect
> value, this can also cause out-of-bounds while approaching hdesc->desc[n].
>
> So check the size of hid descriptor and bNumDescriptors.
Applied to for-4.14/upstream-fixes. Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-10-11 13:06 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CAAZ5spCffHnU_EtnhKpP5cDqsEKaGMatpaqAJqorvph+vZm1Sw@mail.gmail.com>
2017-09-27 14:29 ` [PATCH] HID: usbhid: fix out-of-bounds bug Alan Stern
2017-09-27 14:29 ` Alan Stern
2017-09-28 8:44 ` Jaejoong Kim
2017-09-28 10:16 ` [PATCH v2] " Jaejoong Kim
2017-10-10 7:25 ` Jaejoong Kim
2017-10-10 11:23 ` Andrey Konovalov
2017-10-10 14:22 ` Alan Stern
2017-10-10 14:22 ` Alan Stern
2017-10-11 13:06 ` Jiri Kosina
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.