usb/hid: slab-out-of-bounds read in usbhid_parse

usb/hid: slab-out-of-bounds read in usbhid_parse

[Andrey Konovalov]

Hi!

I've got the following crash while fuzzing the kernel with syzkaller.

On commit ebb2c2437d8008d46796902ff390653822af6cc4 (Sep 18).

It seems that there's no proper check on the hdesc->bNumDescriptors
value in usbhid_parse(). it iterates over hdesc->desc and accesses
hdesc->desc[n] fields, which might be out-of-bounds.

==================================================================
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

Allocated by task 1261:
 save_stack_trace+0x1b/0x20 arch/x86/kernel/stacktrace.c:59
 save_stack+0x43/0xd0 mm/kasan/kasan.c:447
 set_track mm/kasan/kasan.c:459
 kasan_kmalloc+0xad/0xe0 mm/kasan/kasan.c:551
 __kmalloc+0x14e/0x310 mm/slub.c:3782
 kmalloc ./include/linux/slab.h:498
 usb_get_configuration+0x372/0x2a60 drivers/usb/core/config.c:848
 usb_enumerate_device drivers/usb/core/hub.c:2290
 usb_new_device+0xaae/0x1020 drivers/usb/core/hub.c:2426
 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

Freed by task 2927:
 save_stack_trace+0x1b/0x20 arch/x86/kernel/stacktrace.c:59
 save_stack+0x43/0xd0 mm/kasan/kasan.c:447
 set_track mm/kasan/kasan.c:459
 kasan_slab_free+0x72/0xc0 mm/kasan/kasan.c:524
 slab_free_hook mm/slub.c:1390
 slab_free_freelist_hook mm/slub.c:1412
 slab_free mm/slub.c:2988
 kfree+0xf6/0x2f0 mm/slub.c:3919
 free_rb_tree_fname+0x8a/0xe0 fs/ext4/dir.c:402
 ext4_htree_free_dir_info fs/ext4/dir.c:424
 ext4_release_dir+0x49/0x70 fs/ext4/dir.c:622
 __fput+0x33e/0x800 fs/file_table.c:210
 ____fput+0x1a/0x20 fs/file_table.c:244
 task_work_run+0x1af/0x280 kernel/task_work.c:112
 tracehook_notify_resume ./include/linux/tracehook.h:191
 exit_to_usermode_loop+0x1e1/0x220 arch/x86/entry/common.c:162
 prepare_exit_to_usermode arch/x86/entry/common.c:197
 syscall_return_slowpath+0x414/0x480 arch/x86/entry/common.c:266
 entry_SYSCALL_64_fastpath+0xc0/0xc2 arch/x86/entry/entry_64.S:238

The buggy address belongs to the object at ffff88006c5f8ea0
 which belongs to the cache kmalloc-64 of size 64
The buggy address is located 63 bytes inside of
 64-byte region [ffff88006c5f8ea0, ffff88006c5f8ee0)
The buggy address belongs to the page:
page:ffffea0001b17e00 count:1 mapcount:0 mapping:          (null) index:0x0
flags: 0x100000000000100(slab)
raw: 0100000000000100 0000000000000000 0000000000000000 00000001002a002a
raw: ffffea0001a83000 0000001500000015 ffff88006c403800 0000000000000000
page dumped because: kasan: bad access detected

Memory state around the buggy address:
 ffff88006c5f8d80: fb fb fb fb fb fb fb fb fc fc fc fc 00 00 00 00
 ffff88006c5f8e00: 00 fc fc fc fc fc fc fc fb fb fb fb fb fb fb fb
>ffff88006c5f8e80: fc fc fc fc 00 00 00 00 00 00 00 07 fc fc fc fc
                                                    ^
 ffff88006c5f8f00: fb fb fb fb fb fb fb fb fc fc fc fc fb fb fb fb
 ffff88006c5f8f80: fb fb fb fb fc fc fc fc fc fc fc fc fc fc fc fc
==================================================================

Re: usb/hid: slab-out-of-bounds read in usbhid_parse

[Kim Jaejoong]

[-- Attachment #1: Type: text/plain, Size: 7655 bytes --]

Hi, Andrey Konovalov

Thanks for the report.

2017-09-19 2:33 GMT+09:00 Andrey Konovalov <andreyknvl@google.com>:
> Hi!
>
> I've got the following crash while fuzzing the kernel with syzkaller.
>
> On commit ebb2c2437d8008d46796902ff390653822af6cc4 (Sep 18).
>
> It seems that there's no proper check on the hdesc->bNumDescriptors
> value in usbhid_parse(). it iterates over hdesc->desc and accesses
> hdesc->desc[n] fields, which might be out-of-bounds.

The bNumDescriptors in hid descriptor means 'numeric expression
specifying the number of class descriptors'.
The value bNumberDescriptors identifies the number of additional class
specific descriptors present.
(refer to the 6.1.2 HID Descriptor in hid documents :
http://www.usb.org/developers/hidpage/HID1_11.pdf)

So, it can be out-of-bounds in hdesc->desc[n] if there is an
additional class descriptor.

Does the patch below fix this?

Thanks, jaejoong
------

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 089bad8..7b6a0b6 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -974,7 +974,7 @@ static int usbhid_parse(struct hid_device *hid)
        u32 quirks = 0;
        unsigned int rsize = 0;
        char *rdesc;
-       int ret, n;
+       int ret;

        quirks = usbhid_lookup_quirk(le16_to_cpu(dev->descriptor.idVendor),
                        le16_to_cpu(dev->descriptor.idProduct));
@@ -1000,9 +1000,8 @@ static int usbhid_parse(struct hid_device *hid)
        hid->version = le16_to_cpu(hdesc->bcdHID);
        hid->country = hdesc->bCountryCode;

-       for (n = 0; n < hdesc->bNumDescriptors; n++)
-               if (hdesc->desc[n].bDescriptorType == HID_DT_REPORT)
-                       rsize = le16_to_cpu(hdesc->desc[n].wDescriptorLength);
+       if (hdesc->desc[0].bDescriptorType == HID_DT_REPORT)
+               rsize = le16_to_cpu(hdesc->desc[0].wDescriptorLength);

        if (!rsize || rsize > HID_MAX_DESCRIPTOR_SIZE) {
                dbg_hid("weird size of report descriptor (%u)\n", rsize);


>
> ==================================================================
> 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
>
> Allocated by task 1261:
>  save_stack_trace+0x1b/0x20 arch/x86/kernel/stacktrace.c:59
>  save_stack+0x43/0xd0 mm/kasan/kasan.c:447
>  set_track mm/kasan/kasan.c:459
>  kasan_kmalloc+0xad/0xe0 mm/kasan/kasan.c:551
>  __kmalloc+0x14e/0x310 mm/slub.c:3782
>  kmalloc ./include/linux/slab.h:498
>  usb_get_configuration+0x372/0x2a60 drivers/usb/core/config.c:848
>  usb_enumerate_device drivers/usb/core/hub.c:2290
>  usb_new_device+0xaae/0x1020 drivers/usb/core/hub.c:2426
>  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
>
> Freed by task 2927:
>  save_stack_trace+0x1b/0x20 arch/x86/kernel/stacktrace.c:59
>  save_stack+0x43/0xd0 mm/kasan/kasan.c:447
>  set_track mm/kasan/kasan.c:459
>  kasan_slab_free+0x72/0xc0 mm/kasan/kasan.c:524
>  slab_free_hook mm/slub.c:1390
>  slab_free_freelist_hook mm/slub.c:1412
>  slab_free mm/slub.c:2988
>  kfree+0xf6/0x2f0 mm/slub.c:3919
>  free_rb_tree_fname+0x8a/0xe0 fs/ext4/dir.c:402
>  ext4_htree_free_dir_info fs/ext4/dir.c:424
>  ext4_release_dir+0x49/0x70 fs/ext4/dir.c:622
>  __fput+0x33e/0x800 fs/file_table.c:210
>  ____fput+0x1a/0x20 fs/file_table.c:244
>  task_work_run+0x1af/0x280 kernel/task_work.c:112
>  tracehook_notify_resume ./include/linux/tracehook.h:191
>  exit_to_usermode_loop+0x1e1/0x220 arch/x86/entry/common.c:162
>  prepare_exit_to_usermode arch/x86/entry/common.c:197
>  syscall_return_slowpath+0x414/0x480 arch/x86/entry/common.c:266
>  entry_SYSCALL_64_fastpath+0xc0/0xc2 arch/x86/entry/entry_64.S:238
>
> The buggy address belongs to the object at ffff88006c5f8ea0
>  which belongs to the cache kmalloc-64 of size 64
> The buggy address is located 63 bytes inside of
>  64-byte region [ffff88006c5f8ea0, ffff88006c5f8ee0)
> The buggy address belongs to the page:
> page:ffffea0001b17e00 count:1 mapcount:0 mapping:          (null) index:0x0
> flags: 0x100000000000100(slab)
> raw: 0100000000000100 0000000000000000 0000000000000000 00000001002a002a
> raw: ffffea0001a83000 0000001500000015 ffff88006c403800 0000000000000000
> page dumped because: kasan: bad access detected
>
> Memory state around the buggy address:
>  ffff88006c5f8d80: fb fb fb fb fb fb fb fb fc fc fc fc 00 00 00 00
>  ffff88006c5f8e00: 00 fc fc fc fc fc fc fc fb fb fb fb fb fb fb fb
>>ffff88006c5f8e80: fc fc fc fc 00 00 00 00 00 00 00 07 fc fc fc fc
>                                                     ^
>  ffff88006c5f8f00: fb fb fb fb fb fb fb fb fc fc fc fc fb fb fb fb
>  ffff88006c5f8f80: fb fb fb fb fc fc fc fc fc fc fc fc fc fc fc fc
> ==================================================================
> --
> 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

[-- Attachment #2: 0001-HID-usbhid-out-of-bound-in-hdesc-desc.patch --]
[-- Type: text/x-patch, Size: 1310 bytes --]

From 0699858f389e0ccbaba3eedfc9a6db7d570fc720 Mon Sep 17 00:00:00 2001
From: "jaejoong.kim" <jaejoong.kim@lge.com>
Date: Tue, 19 Sep 2017 20:39:21 +0900
Subject: [PATCH] HID: usbhid: out of bound in hdesc->desc

---
 drivers/hid/usbhid/hid-core.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 089bad8..7b6a0b6 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -974,7 +974,7 @@ static int usbhid_parse(struct hid_device *hid)
 	u32 quirks = 0;
 	unsigned int rsize = 0;
 	char *rdesc;
-	int ret, n;
+	int ret;
 
 	quirks = usbhid_lookup_quirk(le16_to_cpu(dev->descriptor.idVendor),
 			le16_to_cpu(dev->descriptor.idProduct));
@@ -1000,9 +1000,8 @@ static int usbhid_parse(struct hid_device *hid)
 	hid->version = le16_to_cpu(hdesc->bcdHID);
 	hid->country = hdesc->bCountryCode;
 
-	for (n = 0; n < hdesc->bNumDescriptors; n++)
-		if (hdesc->desc[n].bDescriptorType == HID_DT_REPORT)
-			rsize = le16_to_cpu(hdesc->desc[n].wDescriptorLength);
+	if (hdesc->desc[0].bDescriptorType == HID_DT_REPORT)
+		rsize = le16_to_cpu(hdesc->desc[0].wDescriptorLength);
 
 	if (!rsize || rsize > HID_MAX_DESCRIPTOR_SIZE) {
 		dbg_hid("weird size of report descriptor (%u)\n", rsize);
-- 
2.7.4

Re: usb/hid: slab-out-of-bounds read in usbhid_parse

[Andrey Konovalov]

On Tue, Sep 19, 2017 at 1:47 PM, Kim Jaejoong <climbbb.kim@gmail.com> wrote:
> Hi, Andrey Konovalov
>
> Thanks for the report.
>
> 2017-09-19 2:33 GMT+09:00 Andrey Konovalov <andreyknvl@google.com>:
>> Hi!
>>
>> I've got the following crash while fuzzing the kernel with syzkaller.
>>
>> On commit ebb2c2437d8008d46796902ff390653822af6cc4 (Sep 18).
>>
>> It seems that there's no proper check on the hdesc->bNumDescriptors
>> value in usbhid_parse(). it iterates over hdesc->desc and accesses
>> hdesc->desc[n] fields, which might be out-of-bounds.
>
> The bNumDescriptors in hid descriptor means 'numeric expression
> specifying the number of class descriptors'.
> The value bNumberDescriptors identifies the number of additional class
> specific descriptors present.
> (refer to the 6.1.2 HID Descriptor in hid documents :
> http://www.usb.org/developers/hidpage/HID1_11.pdf)
>
> So, it can be out-of-bounds in hdesc->desc[n] if there is an
> additional class descriptor.
>
> Does the patch below fix this?

Hi Kim,

I'm not sure. Is there a check on the bLength field of a
hid_descriptor struct? Can it be less than sizeof(struct
hid_descriptor)? If so, we still do an out-of-bounds access to
hdesc->desc[0] (or some other fields).

Thanks!

>
> Thanks, jaejoong
> ------
>
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index 089bad8..7b6a0b6 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -974,7 +974,7 @@ static int usbhid_parse(struct hid_device *hid)
>         u32 quirks = 0;
>         unsigned int rsize = 0;
>         char *rdesc;
> -       int ret, n;
> +       int ret;
>
>         quirks = usbhid_lookup_quirk(le16_to_cpu(dev->descriptor.idVendor),
>                         le16_to_cpu(dev->descriptor.idProduct));
> @@ -1000,9 +1000,8 @@ static int usbhid_parse(struct hid_device *hid)
>         hid->version = le16_to_cpu(hdesc->bcdHID);
>         hid->country = hdesc->bCountryCode;
>
> -       for (n = 0; n < hdesc->bNumDescriptors; n++)
> -               if (hdesc->desc[n].bDescriptorType == HID_DT_REPORT)
> -                       rsize = le16_to_cpu(hdesc->desc[n].wDescriptorLength);
> +       if (hdesc->desc[0].bDescriptorType == HID_DT_REPORT)
> +               rsize = le16_to_cpu(hdesc->desc[0].wDescriptorLength);
>
>         if (!rsize || rsize > HID_MAX_DESCRIPTOR_SIZE) {
>                 dbg_hid("weird size of report descriptor (%u)\n", rsize);
>
>
>>
>> ==================================================================
>> 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
>>
>> Allocated by task 1261:
>>  save_stack_trace+0x1b/0x20 arch/x86/kernel/stacktrace.c:59
>>  save_stack+0x43/0xd0 mm/kasan/kasan.c:447
>>  set_track mm/kasan/kasan.c:459
>>  kasan_kmalloc+0xad/0xe0 mm/kasan/kasan.c:551
>>  __kmalloc+0x14e/0x310 mm/slub.c:3782
>>  kmalloc ./include/linux/slab.h:498
>>  usb_get_configuration+0x372/0x2a60 drivers/usb/core/config.c:848
>>  usb_enumerate_device drivers/usb/core/hub.c:2290
>>  usb_new_device+0xaae/0x1020 drivers/usb/core/hub.c:2426
>>  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
>>
>> Freed by task 2927:
>>  save_stack_trace+0x1b/0x20 arch/x86/kernel/stacktrace.c:59
>>  save_stack+0x43/0xd0 mm/kasan/kasan.c:447
>>  set_track mm/kasan/kasan.c:459
>>  kasan_slab_free+0x72/0xc0 mm/kasan/kasan.c:524
>>  slab_free_hook mm/slub.c:1390
>>  slab_free_freelist_hook mm/slub.c:1412
>>  slab_free mm/slub.c:2988
>>  kfree+0xf6/0x2f0 mm/slub.c:3919
>>  free_rb_tree_fname+0x8a/0xe0 fs/ext4/dir.c:402
>>  ext4_htree_free_dir_info fs/ext4/dir.c:424
>>  ext4_release_dir+0x49/0x70 fs/ext4/dir.c:622
>>  __fput+0x33e/0x800 fs/file_table.c:210
>>  ____fput+0x1a/0x20 fs/file_table.c:244
>>  task_work_run+0x1af/0x280 kernel/task_work.c:112
>>  tracehook_notify_resume ./include/linux/tracehook.h:191
>>  exit_to_usermode_loop+0x1e1/0x220 arch/x86/entry/common.c:162
>>  prepare_exit_to_usermode arch/x86/entry/common.c:197
>>  syscall_return_slowpath+0x414/0x480 arch/x86/entry/common.c:266
>>  entry_SYSCALL_64_fastpath+0xc0/0xc2 arch/x86/entry/entry_64.S:238
>>
>> The buggy address belongs to the object at ffff88006c5f8ea0
>>  which belongs to the cache kmalloc-64 of size 64
>> The buggy address is located 63 bytes inside of
>>  64-byte region [ffff88006c5f8ea0, ffff88006c5f8ee0)
>> The buggy address belongs to the page:
>> page:ffffea0001b17e00 count:1 mapcount:0 mapping:          (null) index:0x0
>> flags: 0x100000000000100(slab)
>> raw: 0100000000000100 0000000000000000 0000000000000000 00000001002a002a
>> raw: ffffea0001a83000 0000001500000015 ffff88006c403800 0000000000000000
>> page dumped because: kasan: bad access detected
>>
>> Memory state around the buggy address:
>>  ffff88006c5f8d80: fb fb fb fb fb fb fb fb fc fc fc fc 00 00 00 00
>>  ffff88006c5f8e00: 00 fc fc fc fc fc fc fc fb fb fb fb fb fb fb fb
>>>ffff88006c5f8e80: fc fc fc fc 00 00 00 00 00 00 00 07 fc fc fc fc
>>                                                     ^
>>  ffff88006c5f8f00: fb fb fb fb fb fb fb fb fc fc fc fc fb fb fb fb
>>  ffff88006c5f8f80: fb fb fb fb fc fc fc fc fc fc fc fc fc fc fc fc
>> ==================================================================
>> --
>> 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
>
> --
> 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.

Re: usb/hid: slab-out-of-bounds read in usbhid_parse

[Kim Jaejoong]

Hi Andrey

2017-09-19 21:38 GMT+09:00 Andrey Konovalov <andreyknvl@google.com>:
> On Tue, Sep 19, 2017 at 1:47 PM, Kim Jaejoong <climbbb.kim@gmail.com> wrote:
>> Hi, Andrey Konovalov
>>
>> Thanks for the report.
>>
>> 2017-09-19 2:33 GMT+09:00 Andrey Konovalov <andreyknvl@google.com>:
>>> Hi!
>>>
>>> I've got the following crash while fuzzing the kernel with syzkaller.
>>>
>>> On commit ebb2c2437d8008d46796902ff390653822af6cc4 (Sep 18).
>>>
>>> It seems that there's no proper check on the hdesc->bNumDescriptors
>>> value in usbhid_parse(). it iterates over hdesc->desc and accesses
>>> hdesc->desc[n] fields, which might be out-of-bounds.
>>
>> The bNumDescriptors in hid descriptor means 'numeric expression
>> specifying the number of class descriptors'.
>> The value bNumberDescriptors identifies the number of additional class
>> specific descriptors present.
>> (refer to the 6.1.2 HID Descriptor in hid documents :
>> http://www.usb.org/developers/hidpage/HID1_11.pdf)
>>
>> So, it can be out-of-bounds in hdesc->desc[n] if there is an
>> additional class descriptor.
>>
>> Does the patch below fix this?
>
> Hi Kim,
>
> I'm not sure. Is there a check on the bLength field of a
> hid_descriptor struct? Can it be less than sizeof(struct
> hid_descriptor)? If so, we still do an out-of-bounds access to
> hdesc->desc[0] (or some other fields).

You are right. I add hid descriptr size from HID device with bLength field.

Could you test and review below patch?

To. usb & input guys.

While dig this report, i was wondering about bNumDescriptors in HID descriptor.
HID document from usb.org said, 'this number must be at least one (1)
as a Report descriptor will always be present.'

There is no mention of the order of class descriptors. Suppose you
have a HID device with a report descriptor and a physical descriptor.

If you have the following hid descriptor in this case,
HID descriptor
   bLength: 12
   bDescriptor Type: HID
   .. skip
   bNumDescriptors: 2
   bDescriptorType: physical
   bDescriptorLength: any
   bDescriptorType: Report
   bDescriptorLength: any

If the order of the report descriptor is the second as above,
usbhid_parse () will fail because my patch is only check the first
bDescriptor Type.
But If the order of the report descriptor is always first, there is no
problem. How do you think this?

Thanks, jaejoong

-----------------

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 089bad8..94c3805 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -974,7 +974,7 @@ static int usbhid_parse(struct hid_device *hid)
        u32 quirks = 0;
        unsigned int rsize = 0;
        char *rdesc;
-       int ret, n;
+       int ret;

        quirks = usbhid_lookup_quirk(le16_to_cpu(dev->descriptor.idVendor),
                        le16_to_cpu(dev->descriptor.idProduct));
@@ -997,12 +997,16 @@ static int usbhid_parse(struct hid_device *hid)
                return -ENODEV;
        }

+       if (hdesc->bLength < sizeof(*hdesc)) {
+               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++)
-               if (hdesc->desc[n].bDescriptorType == HID_DT_REPORT)
-                       rsize = le16_to_cpu(hdesc->desc[n].wDescriptorLength);
+       if (hdesc->desc[0].bDescriptorType == HID_DT_REPORT)
+               rsize = le16_to_cpu(hdesc->desc[0].wDescriptorLength);

        if (!rsize || rsize > HID_MAX_DESCRIPTOR_SIZE) {
                dbg_hid("weird size of report descriptor (%u)\n", rsize);
-----------

>
> Thanks!
>
>>
>> Thanks, jaejoong
>> ------
>>
>> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
>> index 089bad8..7b6a0b6 100644
>> --- a/drivers/hid/usbhid/hid-core.c
>> +++ b/drivers/hid/usbhid/hid-core.c
>> @@ -974,7 +974,7 @@ static int usbhid_parse(struct hid_device *hid)
>>         u32 quirks = 0;
>>         unsigned int rsize = 0;
>>         char *rdesc;
>> -       int ret, n;
>> +       int ret;
>>
>>         quirks = usbhid_lookup_quirk(le16_to_cpu(dev->descriptor.idVendor),
>>                         le16_to_cpu(dev->descriptor.idProduct));
>> @@ -1000,9 +1000,8 @@ static int usbhid_parse(struct hid_device *hid)
>>         hid->version = le16_to_cpu(hdesc->bcdHID);
>>         hid->country = hdesc->bCountryCode;
>>
>> -       for (n = 0; n < hdesc->bNumDescriptors; n++)
>> -               if (hdesc->desc[n].bDescriptorType == HID_DT_REPORT)
>> -                       rsize = le16_to_cpu(hdesc->desc[n].wDescriptorLength);
>> +       if (hdesc->desc[0].bDescriptorType == HID_DT_REPORT)
>> +               rsize = le16_to_cpu(hdesc->desc[0].wDescriptorLength);
>>
>>         if (!rsize || rsize > HID_MAX_DESCRIPTOR_SIZE) {
>>                 dbg_hid("weird size of report descriptor (%u)\n", rsize);
>>
>>
>>>
>>> ==================================================================
>>> 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
>>>
>>> Allocated by task 1261:
>>>  save_stack_trace+0x1b/0x20 arch/x86/kernel/stacktrace.c:59
>>>  save_stack+0x43/0xd0 mm/kasan/kasan.c:447
>>>  set_track mm/kasan/kasan.c:459
>>>  kasan_kmalloc+0xad/0xe0 mm/kasan/kasan.c:551
>>>  __kmalloc+0x14e/0x310 mm/slub.c:3782
>>>  kmalloc ./include/linux/slab.h:498
>>>  usb_get_configuration+0x372/0x2a60 drivers/usb/core/config.c:848
>>>  usb_enumerate_device drivers/usb/core/hub.c:2290
>>>  usb_new_device+0xaae/0x1020 drivers/usb/core/hub.c:2426
>>>  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
>>>
>>> Freed by task 2927:
>>>  save_stack_trace+0x1b/0x20 arch/x86/kernel/stacktrace.c:59
>>>  save_stack+0x43/0xd0 mm/kasan/kasan.c:447
>>>  set_track mm/kasan/kasan.c:459
>>>  kasan_slab_free+0x72/0xc0 mm/kasan/kasan.c:524
>>>  slab_free_hook mm/slub.c:1390
>>>  slab_free_freelist_hook mm/slub.c:1412
>>>  slab_free mm/slub.c:2988
>>>  kfree+0xf6/0x2f0 mm/slub.c:3919
>>>  free_rb_tree_fname+0x8a/0xe0 fs/ext4/dir.c:402
>>>  ext4_htree_free_dir_info fs/ext4/dir.c:424
>>>  ext4_release_dir+0x49/0x70 fs/ext4/dir.c:622
>>>  __fput+0x33e/0x800 fs/file_table.c:210
>>>  ____fput+0x1a/0x20 fs/file_table.c:244
>>>  task_work_run+0x1af/0x280 kernel/task_work.c:112
>>>  tracehook_notify_resume ./include/linux/tracehook.h:191
>>>  exit_to_usermode_loop+0x1e1/0x220 arch/x86/entry/common.c:162
>>>  prepare_exit_to_usermode arch/x86/entry/common.c:197
>>>  syscall_return_slowpath+0x414/0x480 arch/x86/entry/common.c:266
>>>  entry_SYSCALL_64_fastpath+0xc0/0xc2 arch/x86/entry/entry_64.S:238
>>>
>>> The buggy address belongs to the object at ffff88006c5f8ea0
>>>  which belongs to the cache kmalloc-64 of size 64
>>> The buggy address is located 63 bytes inside of
>>>  64-byte region [ffff88006c5f8ea0, ffff88006c5f8ee0)
>>> The buggy address belongs to the page:
>>> page:ffffea0001b17e00 count:1 mapcount:0 mapping:          (null) index:0x0
>>> flags: 0x100000000000100(slab)
>>> raw: 0100000000000100 0000000000000000 0000000000000000 00000001002a002a
>>> raw: ffffea0001a83000 0000001500000015 ffff88006c403800 0000000000000000
>>> page dumped because: kasan: bad access detected
>>>
>>> Memory state around the buggy address:
>>>  ffff88006c5f8d80: fb fb fb fb fb fb fb fb fc fc fc fc 00 00 00 00
>>>  ffff88006c5f8e00: 00 fc fc fc fc fc fc fc fb fb fb fb fb fb fb fb
>>>>ffff88006c5f8e80: fc fc fc fc 00 00 00 00 00 00 00 07 fc fc fc fc
>>>                                                     ^
>>>  ffff88006c5f8f00: fb fb fb fb fb fb fb fb fc fc fc fc fb fb fb fb
>>>  ffff88006c5f8f80: fb fb fb fb fc fc fc fc fc fc fc fc fc fc fc fc
>>> ==================================================================
>>> --
>>> 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
>>
>> --
>> 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.

Re: usb/hid: slab-out-of-bounds read in usbhid_parse

[Andrey Konovalov]

On Wed, Sep 20, 2017 at 6:57 AM, Kim Jaejoong <climbbb.kim@gmail.com> wrote:
> Hi Andrey
>
> 2017-09-19 21:38 GMT+09:00 Andrey Konovalov <andreyknvl@google.com>:
>> Hi Kim,
>>
>> I'm not sure. Is there a check on the bLength field of a
>> hid_descriptor struct? Can it be less than sizeof(struct
>> hid_descriptor)? If so, we still do an out-of-bounds access to
>> hdesc->desc[0] (or some other fields).
>
> You are right. I add hid descriptr size from HID device with bLength field.
>
> Could you test and review below patch?

It seems that this patch fixes the issue. I'll keep fuzzing to make sure.

Tested-by: Andrey Konovalov <andreyknvl@google.com>

Thanks!

>
> To. usb & input guys.
>
> While dig this report, i was wondering about bNumDescriptors in HID descriptor.
> HID document from usb.org said, 'this number must be at least one (1)
> as a Report descriptor will always be present.'
>
> There is no mention of the order of class descriptors. Suppose you
> have a HID device with a report descriptor and a physical descriptor.
>
> If you have the following hid descriptor in this case,
> HID descriptor
>    bLength: 12
>    bDescriptor Type: HID
>    .. skip
>    bNumDescriptors: 2
>    bDescriptorType: physical
>    bDescriptorLength: any
>    bDescriptorType: Report
>    bDescriptorLength: any
>
> If the order of the report descriptor is the second as above,
> usbhid_parse () will fail because my patch is only check the first
> bDescriptor Type.
> But If the order of the report descriptor is always first, there is no
> problem. How do you think this?
>
> Thanks, jaejoong
>
> -----------------
>
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index 089bad8..94c3805 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -974,7 +974,7 @@ static int usbhid_parse(struct hid_device *hid)
>         u32 quirks = 0;
>         unsigned int rsize = 0;
>         char *rdesc;
> -       int ret, n;
> +       int ret;
>
>         quirks = usbhid_lookup_quirk(le16_to_cpu(dev->descriptor.idVendor),
>                         le16_to_cpu(dev->descriptor.idProduct));
> @@ -997,12 +997,16 @@ static int usbhid_parse(struct hid_device *hid)
>                 return -ENODEV;
>         }
>
> +       if (hdesc->bLength < sizeof(*hdesc)) {
> +               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++)
> -               if (hdesc->desc[n].bDescriptorType == HID_DT_REPORT)
> -                       rsize = le16_to_cpu(hdesc->desc[n].wDescriptorLength);
> +       if (hdesc->desc[0].bDescriptorType == HID_DT_REPORT)
> +               rsize = le16_to_cpu(hdesc->desc[0].wDescriptorLength);
>
>         if (!rsize || rsize > HID_MAX_DESCRIPTOR_SIZE) {
>                 dbg_hid("weird size of report descriptor (%u)\n", rsize);
> -----------
>

Re: usb/hid: slab-out-of-bounds read in usbhid_parse

[Alan Stern]

On Wed, 20 Sep 2017, Kim Jaejoong wrote:

> To. usb & input guys.
> 
> While dig this report, i was wondering about bNumDescriptors in HID descriptor.
> HID document from usb.org said, 'this number must be at least one (1)
> as a Report descriptor will always be present.'
> 
> There is no mention of the order of class descriptors. Suppose you
> have a HID device with a report descriptor and a physical descriptor.
> 
> If you have the following hid descriptor in this case,
> HID descriptor
>    bLength: 12
>    bDescriptor Type: HID
>    .. skip
>    bNumDescriptors: 2
>    bDescriptorType: physical
>    bDescriptorLength: any
>    bDescriptorType: Report
>    bDescriptorLength: any
> 
> If the order of the report descriptor is the second as above,
> usbhid_parse () will fail because my patch is only check the first
> bDescriptor Type.
> But If the order of the report descriptor is always first, there is no
> problem. How do you think this?

The descriptors can appear in any order.  You should not assume that 
the report descriptor will always come first.

Alan Stern

Re: usb/hid: slab-out-of-bounds read in usbhid_parse

[Alan Stern]

On Wed, 20 Sep 2017, Kim Jaejoong wrote:

> To. usb & input guys.
> 
> While dig this report, i was wondering about bNumDescriptors in HID descriptor.
> HID document from usb.org said, 'this number must be at least one (1)
> as a Report descriptor will always be present.'
> 
> There is no mention of the order of class descriptors. Suppose you
> have a HID device with a report descriptor and a physical descriptor.
> 
> If you have the following hid descriptor in this case,
> HID descriptor
>    bLength: 12
>    bDescriptor Type: HID
>    .. skip
>    bNumDescriptors: 2
>    bDescriptorType: physical
>    bDescriptorLength: any
>    bDescriptorType: Report
>    bDescriptorLength: any
> 
> If the order of the report descriptor is the second as above,
> usbhid_parse () will fail because my patch is only check the first
> bDescriptor Type.
> But If the order of the report descriptor is always first, there is no
> problem. How do you think this?

The descriptors can appear in any order.  You should not assume that 
the report descriptor will always come first.

Alan Stern

Re: usb/hid: slab-out-of-bounds read in usbhid_parse

[Jaejoong Kim]

Hi Alan

2017-09-21 0:50 GMT+09:00 Alan Stern <stern@rowland.harvard.edu>:
> On Wed, 20 Sep 2017, Kim Jaejoong wrote:
>
>> To. usb & input guys.
>>
>> While dig this report, i was wondering about bNumDescriptors in HID descriptor.
>> HID document from usb.org said, 'this number must be at least one (1)
>> as a Report descriptor will always be present.'
>>
>> There is no mention of the order of class descriptors. Suppose you
>> have a HID device with a report descriptor and a physical descriptor.
>>
>> If you have the following hid descriptor in this case,
>> HID descriptor
>>    bLength: 12
>>    bDescriptor Type: HID
>>    .. skip
>>    bNumDescriptors: 2
>>    bDescriptorType: physical
>>    bDescriptorLength: any
>>    bDescriptorType: Report
>>    bDescriptorLength: any
>>
>> If the order of the report descriptor is the second as above,
>> usbhid_parse () will fail because my patch is only check the first
>> bDescriptor Type.
>> But If the order of the report descriptor is always first, there is no
>> problem. How do you think this?
>
> The descriptors can appear in any order.  You should not assume that
> the report descriptor will always come first.

Thanks for clarifying. I will resend patch with modification.

Jaejoong

>
> Alan Stern
>

[PATCH] HID: usbhid: fix out-of-bounds bug

[Jaejoong Kim]

The starting address of the hid descriptor is obtained via
usb_get_extra_descriptor(). If the hid descriptor has the wrong size, it
is possible to access the wrong address. So, before accessing the hid
descriptor, we need to check the entire size through the bLength field.

It also shows how many class descriptors it has through the bNumDescriptors
of the hid descriptor. Assuming that the connected hid descriptor has two
class descriptors(report and physical descriptors), the code below can
cause OOB because hdesc->desc is an array of size 1.

for (n = 0; n < hdesc->bNumDescriptors; n++)
	if (hdesc->desc[n].bDescriptorType == HID_DT_REPORT)
		rsize = le16_to_cpu(hdesc->desc[n].wDescriptorLength);

Since we know the starting address of the hid descriptor and the value of
the bNumDescriptors is variable, we directly access the buffer containing
the hid descriptor without usbing hdesc->desc to obtain the size of the
report descriptor.

	==================================================================
	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

	Allocated by task 1261:
	save_stack_trace+0x1b/0x20 arch/x86/kernel/stacktrace.c:59
	save_stack+0x43/0xd0 mm/kasan/kasan.c:447
	set_track mm/kasan/kasan.c:459
	kasan_kmalloc+0xad/0xe0 mm/kasan/kasan.c:551
	__kmalloc+0x14e/0x310 mm/slub.c:3782
	kmalloc ./include/linux/slab.h:498
	usb_get_configuration+0x372/0x2a60 drivers/usb/core/config.c:848
	usb_enumerate_device drivers/usb/core/hub.c:2290
	usb_new_device+0xaae/0x1020 drivers/usb/core/hub.c:2426
	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

	Freed by task 2927:
	save_stack_trace+0x1b/0x20 arch/x86/kernel/stacktrace.c:59
	save_stack+0x43/0xd0 mm/kasan/kasan.c:447
	set_track mm/kasan/kasan.c:459
	kasan_slab_free+0x72/0xc0 mm/kasan/kasan.c:524
	slab_free_hook mm/slub.c:1390
	slab_free_freelist_hook mm/slub.c:1412
	slab_free mm/slub.c:2988
	kfree+0xf6/0x2f0 mm/slub.c:3919
	free_rb_tree_fname+0x8a/0xe0 fs/ext4/dir.c:402
	ext4_htree_free_dir_info fs/ext4/dir.c:424
	ext4_release_dir+0x49/0x70 fs/ext4/dir.c:622
	__fput+0x33e/0x800 fs/file_table.c:210
	____fput+0x1a/0x20 fs/file_table.c:244
	task_work_run+0x1af/0x280 kernel/task_work.c:112
	tracehook_notify_resume ./include/linux/tracehook.h:191
	exit_to_usermode_loop+0x1e1/0x220 arch/x86/entry/common.c:162
	prepare_exit_to_usermode arch/x86/entry/common.c:197
	syscall_return_slowpath+0x414/0x480 arch/x86/entry/common.c:266
	entry_SYSCALL_64_fastpath+0xc0/0xc2 arch/x86/entry/entry_64.S:238

	The buggy address belongs to the object at ffff88006c5f8ea0
	which belongs to the cache kmalloc-64 of size 64
	The buggy address is located 63 bytes inside of
	64-byte region [ffff88006c5f8ea0, ffff88006c5f8ee0)
	The buggy address belongs to the page:
	page:ffffea0001b17e00 count:1 mapcount:0 mapping:          (null) index:0x0
	flags: 0x100000000000100(slab)
	raw: 0100000000000100 0000000000000000 0000000000000000 00000001002a002a
	raw: ffffea0001a83000 0000001500000015 ffff88006c403800 0000000000000000
	page dumped because: kasan: bad access detected

	Memory state around the buggy address:
	ffff88006c5f8d80: fb fb fb fb fb fb fb fb fc fc fc fc 00 00 00 00
	ffff88006c5f8e00: 00 fc fc fc fc fc fc fc fb fb fb fb fb fb fb fb
	>ffff88006c5f8e80: fc fc fc fc 00 00 00 00 00 00 00 07 fc fc fc fc
	^
	ffff88006c5f8f00: fb fb fb fb fb fb fb fb fc fc fc fc fb fb fb fb
	ffff88006c5f8f80: fb fb fb fb fc fc fc fc fc fc fc fc fc fc fc fc
	==================================================================

Reported-by: Alexander Potapenko <glider@google.com>
Signed-off-by: Jaejoong Kim <climbbb.kim@gmail.com>
---
 drivers/hid/usbhid/hid-core.c | 39 +++++++++++++++++++++++++++------------
 include/linux/hid.h           |  2 ++
 2 files changed, 29 insertions(+), 12 deletions(-)

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 089bad8..7bad173 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -970,12 +970,19 @@ static int usbhid_parse(struct hid_device *hid)
 	struct usb_interface *intf = to_usb_interface(hid->dev.parent);
 	struct usb_host_interface *interface = intf->cur_altsetting;
 	struct usb_device *dev = interface_to_usbdev (intf);
-	struct hid_descriptor *hdesc;
+	unsigned char *buffer = intf->altsetting->extra;
+	int buflen = intf->altsetting->extralen;
+	int length;
 	u32 quirks = 0;
 	unsigned int rsize = 0;
 	char *rdesc;
 	int ret, n;
 
+	if (!buffer) {
+		dbg_hid("class descriptor not present\n");
+		return -ENODEV;
+	}
+
 	quirks = usbhid_lookup_quirk(le16_to_cpu(dev->descriptor.idVendor),
 			le16_to_cpu(dev->descriptor.idProduct));
 
@@ -990,19 +997,27 @@ static int usbhid_parse(struct hid_device *hid)
 				quirks |= HID_QUIRK_NOGET;
 	}
 
-	if (usb_get_extra_descriptor(interface, HID_DT_HID, &hdesc) &&
-	    (!interface->desc.bNumEndpoints ||
-	     usb_get_extra_descriptor(&interface->endpoint[0], HID_DT_HID, &hdesc))) {
-		dbg_hid("class descriptor not present\n");
-		return -ENODEV;
-	}
+	while (buflen > 2) {
+		length = buffer[0];
+		if (!length || length < HID_DESCRIPTOR_MIN_SIZE)
+			goto next_desc;
 
-	hid->version = le16_to_cpu(hdesc->bcdHID);
-	hid->country = hdesc->bCountryCode;
+		if (buffer[1] == HID_DT_HID) {
+			hid->version = get_unaligned_le16(&buffer[2]);
+			hid->country = buffer[4];
 
-	for (n = 0; n < hdesc->bNumDescriptors; n++)
-		if (hdesc->desc[n].bDescriptorType == HID_DT_REPORT)
-			rsize = le16_to_cpu(hdesc->desc[n].wDescriptorLength);
+			for (n = 0; n < buffer[5]; n++) {
+				/* we are just interested in report descriptor */
+				if (buffer[6+3*n] != HID_DT_REPORT)
+					continue;
+				rsize = get_unaligned_le16(&buffer[7+3*n]);
+			}
+		}
+
+next_desc:
+		buflen -= length;
+		buffer += length;
+	}
 
 	if (!rsize || rsize > HID_MAX_DESCRIPTOR_SIZE) {
 		dbg_hid("weird size of report descriptor (%u)\n", rsize);
diff --git a/include/linux/hid.h b/include/linux/hid.h
index ab05a86..2d53c0f 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -638,6 +638,8 @@ struct hid_descriptor {
 	struct hid_class_descriptor desc[1];
 } __attribute__ ((packed));
 
+#define HID_DESCRIPTOR_MIN_SIZE	9
+
 #define HID_DEVICE(b, g, ven, prod)					\
 	.bus = (b), .group = (g), .vendor = (ven), .product = (prod)
 #define HID_USB_DEVICE(ven, prod)				\
-- 
2.7.4

Re: [PATCH] HID: usbhid: fix out-of-bounds bug

[Alan Stern]

On Tue, 26 Sep 2017, Jaejoong Kim wrote:

> The starting address of the hid descriptor is obtained via
> usb_get_extra_descriptor(). If the hid descriptor has the wrong size, it
> is possible to access the wrong address. So, before accessing the hid
> descriptor, we need to check the entire size through the bLength field.
> 
> It also shows how many class descriptors it has through the bNumDescriptors
> of the hid descriptor. Assuming that the connected hid descriptor has two
> class descriptors(report and physical descriptors), the code below can
> cause OOB because hdesc->desc is an array of size 1.
> 
> for (n = 0; n < hdesc->bNumDescriptors; n++)
> 	if (hdesc->desc[n].bDescriptorType == HID_DT_REPORT)
> 		rsize = le16_to_cpu(hdesc->desc[n].wDescriptorLength);
> 
> Since we know the starting address of the hid descriptor and the value of
> the bNumDescriptors is variable, we directly access the buffer containing
> the hid descriptor without usbing hdesc->desc to obtain the size of the
> report descriptor.

I do not like this patch at all.

> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index 089bad8..7bad173 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -970,12 +970,19 @@ static int usbhid_parse(struct hid_device *hid)
>  	struct usb_interface *intf = to_usb_interface(hid->dev.parent);
>  	struct usb_host_interface *interface = intf->cur_altsetting;
>  	struct usb_device *dev = interface_to_usbdev (intf);
> -	struct hid_descriptor *hdesc;
> +	unsigned char *buffer = intf->altsetting->extra;
> +	int buflen = intf->altsetting->extralen;
> +	int length;
>  	u32 quirks = 0;
>  	unsigned int rsize = 0;
>  	char *rdesc;
>  	int ret, n;
>  
> +	if (!buffer) {
> +		dbg_hid("class descriptor not present\n");
> +		return -ENODEV;
> +	}
> +
>  	quirks = usbhid_lookup_quirk(le16_to_cpu(dev->descriptor.idVendor),
>  			le16_to_cpu(dev->descriptor.idProduct));
>  
> @@ -990,19 +997,27 @@ static int usbhid_parse(struct hid_device *hid)
>  				quirks |= HID_QUIRK_NOGET;
>  	}
>  
> -	if (usb_get_extra_descriptor(interface, HID_DT_HID, &hdesc) &&
> -	    (!interface->desc.bNumEndpoints ||
> -	     usb_get_extra_descriptor(&interface->endpoint[0], HID_DT_HID, &hdesc))) {
> -		dbg_hid("class descriptor not present\n");
> -		return -ENODEV;
> -	}
> +	while (buflen > 2) {
> +		length = buffer[0];
> +		if (!length || length < HID_DESCRIPTOR_MIN_SIZE)
> +			goto next_desc;

It's silly to duplicate the code that is already present in
usb_get_extra_descriptor().  There's no reason to avoid using that
function here.

> -	hid->version = le16_to_cpu(hdesc->bcdHID);
> -	hid->country = hdesc->bCountryCode;
> +		if (buffer[1] == HID_DT_HID) {
> +			hid->version = get_unaligned_le16(&buffer[2]);
> +			hid->country = buffer[4];

It's also silly not to use the preformatted hid_descriptor structure and
instead put error-prone byte offsets directly into the code.

> -	for (n = 0; n < hdesc->bNumDescriptors; n++)
> -		if (hdesc->desc[n].bDescriptorType == HID_DT_REPORT)
> -			rsize = le16_to_cpu(hdesc->desc[n].wDescriptorLength);
> +			for (n = 0; n < buffer[5]; n++) {
> +				/* we are just interested in report descriptor */
> +				if (buffer[6+3*n] != HID_DT_REPORT)
> +					continue;
> +				rsize = get_unaligned_le16(&buffer[7+3*n]);

Finally, this patch doesn't fix the actual problem!  You don't check
here whether 8+3*n >= length.

This whole thing could be fixed with a much smaller change to the
original code.  Just do something like this:

	num_descriptors = min_t(int, hdesc->bNumDescriptors,
			(hdesc->bLength - 6) / 3);
	for (n = 0; n < num_descriptors; n++)

Alan Stern

> +			}
> +		}
> +
> +next_desc:
> +		buflen -= length;
> +		buffer += length;
> +	}
>  
>  	if (!rsize || rsize > HID_MAX_DESCRIPTOR_SIZE) {
>  		dbg_hid("weird size of report descriptor (%u)\n", rsize);
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index ab05a86..2d53c0f 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -638,6 +638,8 @@ struct hid_descriptor {
>  	struct hid_class_descriptor desc[1];
>  } __attribute__ ((packed));
>  
> +#define HID_DESCRIPTOR_MIN_SIZE	9
> +
>  #define HID_DEVICE(b, g, ven, prod)					\
>  	.bus = (b), .group = (g), .vendor = (ven), .product = (prod)
>  #define HID_USB_DEVICE(ven, prod)				\
> 

Re: [PATCH] HID: usbhid: fix out-of-bounds bug

[Alan Stern]

On Tue, 26 Sep 2017, Jaejoong Kim wrote:

> The starting address of the hid descriptor is obtained via
> usb_get_extra_descriptor(). If the hid descriptor has the wrong size, it
> is possible to access the wrong address. So, before accessing the hid
> descriptor, we need to check the entire size through the bLength field.
> 
> It also shows how many class descriptors it has through the bNumDescriptors
> of the hid descriptor. Assuming that the connected hid descriptor has two
> class descriptors(report and physical descriptors), the code below can
> cause OOB because hdesc->desc is an array of size 1.
> 
> for (n = 0; n < hdesc->bNumDescriptors; n++)
> 	if (hdesc->desc[n].bDescriptorType == HID_DT_REPORT)
> 		rsize = le16_to_cpu(hdesc->desc[n].wDescriptorLength);
> 
> Since we know the starting address of the hid descriptor and the value of
> the bNumDescriptors is variable, we directly access the buffer containing
> the hid descriptor without usbing hdesc->desc to obtain the size of the
> report descriptor.

I do not like this patch at all.

> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index 089bad8..7bad173 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -970,12 +970,19 @@ static int usbhid_parse(struct hid_device *hid)
>  	struct usb_interface *intf = to_usb_interface(hid->dev.parent);
>  	struct usb_host_interface *interface = intf->cur_altsetting;
>  	struct usb_device *dev = interface_to_usbdev (intf);
> -	struct hid_descriptor *hdesc;
> +	unsigned char *buffer = intf->altsetting->extra;
> +	int buflen = intf->altsetting->extralen;
> +	int length;
>  	u32 quirks = 0;
>  	unsigned int rsize = 0;
>  	char *rdesc;
>  	int ret, n;
>  
> +	if (!buffer) {
> +		dbg_hid("class descriptor not present\n");
> +		return -ENODEV;
> +	}
> +
>  	quirks = usbhid_lookup_quirk(le16_to_cpu(dev->descriptor.idVendor),
>  			le16_to_cpu(dev->descriptor.idProduct));
>  
> @@ -990,19 +997,27 @@ static int usbhid_parse(struct hid_device *hid)
>  				quirks |= HID_QUIRK_NOGET;
>  	}
>  
> -	if (usb_get_extra_descriptor(interface, HID_DT_HID, &hdesc) &&
> -	    (!interface->desc.bNumEndpoints ||
> -	     usb_get_extra_descriptor(&interface->endpoint[0], HID_DT_HID, &hdesc))) {
> -		dbg_hid("class descriptor not present\n");
> -		return -ENODEV;
> -	}
> +	while (buflen > 2) {
> +		length = buffer[0];
> +		if (!length || length < HID_DESCRIPTOR_MIN_SIZE)
> +			goto next_desc;

It's silly to duplicate the code that is already present in
usb_get_extra_descriptor().  There's no reason to avoid using that
function here.

> -	hid->version = le16_to_cpu(hdesc->bcdHID);
> -	hid->country = hdesc->bCountryCode;
> +		if (buffer[1] == HID_DT_HID) {
> +			hid->version = get_unaligned_le16(&buffer[2]);
> +			hid->country = buffer[4];

It's also silly not to use the preformatted hid_descriptor structure and
instead put error-prone byte offsets directly into the code.

> -	for (n = 0; n < hdesc->bNumDescriptors; n++)
> -		if (hdesc->desc[n].bDescriptorType == HID_DT_REPORT)
> -			rsize = le16_to_cpu(hdesc->desc[n].wDescriptorLength);
> +			for (n = 0; n < buffer[5]; n++) {
> +				/* we are just interested in report descriptor */
> +				if (buffer[6+3*n] != HID_DT_REPORT)
> +					continue;
> +				rsize = get_unaligned_le16(&buffer[7+3*n]);

Finally, this patch doesn't fix the actual problem!  You don't check
here whether 8+3*n >= length.

This whole thing could be fixed with a much smaller change to the
original code.  Just do something like this:

	num_descriptors = min_t(int, hdesc->bNumDescriptors,
			(hdesc->bLength - 6) / 3);
	for (n = 0; n < num_descriptors; n++)

Alan Stern

> +			}
> +		}
> +
> +next_desc:
> +		buflen -= length;
> +		buffer += length;
> +	}
>  
>  	if (!rsize || rsize > HID_MAX_DESCRIPTOR_SIZE) {
>  		dbg_hid("weird size of report descriptor (%u)\n", rsize);
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index ab05a86..2d53c0f 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -638,6 +638,8 @@ struct hid_descriptor {
>  	struct hid_class_descriptor desc[1];
>  } __attribute__ ((packed));
>  
> +#define HID_DESCRIPTOR_MIN_SIZE	9
> +
>  #define HID_DEVICE(b, g, ven, prod)					\
>  	.bus = (b), .group = (g), .vendor = (ven), .product = (prod)
>  #define HID_USB_DEVICE(ven, prod)				\
> 

Re: [PATCH] HID: usbhid: fix out-of-bounds bug

[Jaejoong Kim]

Hi, Alan,

Thanks for the review.

2017-09-26 23:18 GMT+09:00 Alan Stern <stern@rowland.harvard.edu>:
> On Tue, 26 Sep 2017, Jaejoong Kim wrote:
>
>> The starting address of the hid descriptor is obtained via
>> usb_get_extra_descriptor(). If the hid descriptor has the wrong size, it
>> is possible to access the wrong address. So, before accessing the hid
>> descriptor, we need to check the entire size through the bLength field.
>>
>> It also shows how many class descriptors it has through the bNumDescriptors
>> of the hid descriptor. Assuming that the connected hid descriptor has two
>> class descriptors(report and physical descriptors), the code below can
>> cause OOB because hdesc->desc is an array of size 1.
>>
>> for (n = 0; n < hdesc->bNumDescriptors; n++)
>>       if (hdesc->desc[n].bDescriptorType == HID_DT_REPORT)
>>               rsize = le16_to_cpu(hdesc->desc[n].wDescriptorLength);
>>
>> Since we know the starting address of the hid descriptor and the value of
>> the bNumDescriptors is variable, we directly access the buffer containing
>> the hid descriptor without usbing hdesc->desc to obtain the size of the
>> report descriptor.
>
> I do not like this patch at all.
>
>> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
>> index 089bad8..7bad173 100644
>> --- a/drivers/hid/usbhid/hid-core.c
>> +++ b/drivers/hid/usbhid/hid-core.c
>> @@ -970,12 +970,19 @@ static int usbhid_parse(struct hid_device *hid)
>>       struct usb_interface *intf = to_usb_interface(hid->dev.parent);
>>       struct usb_host_interface *interface = intf->cur_altsetting;
>>       struct usb_device *dev = interface_to_usbdev (intf);
>> -     struct hid_descriptor *hdesc;
>> +     unsigned char *buffer = intf->altsetting->extra;
>> +     int buflen = intf->altsetting->extralen;
>> +     int length;
>>       u32 quirks = 0;
>>       unsigned int rsize = 0;
>>       char *rdesc;
>>       int ret, n;
>>
>> +     if (!buffer) {
>> +             dbg_hid("class descriptor not present\n");
>> +             return -ENODEV;
>> +     }
>> +
>>       quirks = usbhid_lookup_quirk(le16_to_cpu(dev->descriptor.idVendor),
>>                       le16_to_cpu(dev->descriptor.idProduct));
>>
>> @@ -990,19 +997,27 @@ static int usbhid_parse(struct hid_device *hid)
>>                               quirks |= HID_QUIRK_NOGET;
>>       }
>>
>> -     if (usb_get_extra_descriptor(interface, HID_DT_HID, &hdesc) &&
>> -         (!interface->desc.bNumEndpoints ||
>> -          usb_get_extra_descriptor(&interface->endpoint[0], HID_DT_HID, &hdesc))) {
>> -             dbg_hid("class descriptor not present\n");
>> -             return -ENODEV;
>> -     }
>> +     while (buflen > 2) {
>> +             length = buffer[0];
>> +             if (!length || length < HID_DESCRIPTOR_MIN_SIZE)
>> +                     goto next_desc;
>
> It's silly to duplicate the code that is already present in
> usb_get_extra_descriptor().  There's no reason to avoid using that
> function here.

To be honest, I was fooled. You are right. That is a duplicate code
with usb_get_extra_descriptor().

>
>> -     hid->version = le16_to_cpu(hdesc->bcdHID);
>> -     hid->country = hdesc->bCountryCode;
>> +             if (buffer[1] == HID_DT_HID) {
>> +                     hid->version = get_unaligned_le16(&buffer[2]);
>> +                     hid->country = buffer[4];
>
> It's also silly not to use the preformatted hid_descriptor structure and
> instead put error-prone byte offsets directly into the code.

Right.

>
>> -     for (n = 0; n < hdesc->bNumDescriptors; n++)
>> -             if (hdesc->desc[n].bDescriptorType == HID_DT_REPORT)
>> -                     rsize = le16_to_cpu(hdesc->desc[n].wDescriptorLength);
>> +                     for (n = 0; n < buffer[5]; n++) {
>> +                             /* we are just interested in report descriptor */
>> +                             if (buffer[6+3*n] != HID_DT_REPORT)
>> +                                     continue;
>> +                             rsize = get_unaligned_le16(&buffer[7+3*n]);
>
> Finally, this patch doesn't fix the actual problem!  You don't check
> here whether 8+3*n >= length.
>
> This whole thing could be fixed with a much smaller change to the
> original code.  Just do something like this:
>
>         num_descriptors = min_t(int, hdesc->bNumDescriptors,
>                         (hdesc->bLength - 6) / 3);
>         for (n = 0; n < num_descriptors; n++)

Right. Your change is better and clear.
With your change, I think we need check the size of hdesc before
accessing hdesc->desc[n] as Andrey Konovalov has already pointed out,

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 089bad8..0b41d44 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -975,6 +975,7 @@ static int usbhid_parse(struct hid_device *hid)
        unsigned int rsize = 0;
        char *rdesc;
        int ret, n;
+       int num_descriptors;

        quirks = usbhid_lookup_quirk(le16_to_cpu(dev->descriptor.idVendor),
                        le16_to_cpu(dev->descriptor.idProduct));
@@ -997,10 +998,17 @@ 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 - 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);

Jaejoong

>
> Alan Stern
>
>> +                     }
>> +             }
>> +
>> +next_desc:
>> +             buflen -= length;
>> +             buffer += length;
>> +     }
>>
>>       if (!rsize || rsize > HID_MAX_DESCRIPTOR_SIZE) {
>>               dbg_hid("weird size of report descriptor (%u)\n", rsize);
>> diff --git a/include/linux/hid.h b/include/linux/hid.h
>> index ab05a86..2d53c0f 100644
>> --- a/include/linux/hid.h
>> +++ b/include/linux/hid.h
>> @@ -638,6 +638,8 @@ struct hid_descriptor {
>>       struct hid_class_descriptor desc[1];
>>  } __attribute__ ((packed));
>>
>> +#define HID_DESCRIPTOR_MIN_SIZE      9
>> +
>>  #define HID_DEVICE(b, g, ven, prod)                                  \
>>       .bus = (b), .group = (g), .vendor = (ven), .product = (prod)
>>  #define HID_USB_DEVICE(ven, prod)                            \
>>
>

Re: [PATCH] HID: usbhid: fix out-of-bounds bug

[Alan Stern]

On Wed, 27 Sep 2017, Jaejoong Kim wrote:

> Hi, Alan,
> 
> Thanks for the review.
> 
> 2017-09-26 23:18 GMT+09:00 Alan Stern <stern@rowland.harvard.edu>:
> > On Tue, 26 Sep 2017, Jaejoong Kim wrote:
> >
> >> The starting address of the hid descriptor is obtained via
> >> usb_get_extra_descriptor(). If the hid descriptor has the wrong size, it
> >> is possible to access the wrong address. So, before accessing the hid
> >> descriptor, we need to check the entire size through the bLength field.
> >>
> >> It also shows how many class descriptors it has through the bNumDescriptors
> >> of the hid descriptor. Assuming that the connected hid descriptor has two
> >> class descriptors(report and physical descriptors), the code below can
> >> cause OOB because hdesc->desc is an array of size 1.
> >>
> >> for (n = 0; n < hdesc->bNumDescriptors; n++)
> >>       if (hdesc->desc[n].bDescriptorType == HID_DT_REPORT)
> >>               rsize = le16_to_cpu(hdesc->desc[n].wDescriptorLength);
> >>
> >> Since we know the starting address of the hid descriptor and the value of
> >> the bNumDescriptors is variable, we directly access the buffer containing
> >> the hid descriptor without usbing hdesc->desc to obtain the size of the
> >> report descriptor.
> >
> > I do not like this patch at all.
> >
> >> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> >> index 089bad8..7bad173 100644
> >> --- a/drivers/hid/usbhid/hid-core.c
> >> +++ b/drivers/hid/usbhid/hid-core.c
> >> @@ -970,12 +970,19 @@ static int usbhid_parse(struct hid_device *hid)
> >>       struct usb_interface *intf = to_usb_interface(hid->dev.parent);
> >>       struct usb_host_interface *interface = intf->cur_altsetting;
> >>       struct usb_device *dev = interface_to_usbdev (intf);
> >> -     struct hid_descriptor *hdesc;
> >> +     unsigned char *buffer = intf->altsetting->extra;
> >> +     int buflen = intf->altsetting->extralen;
> >> +     int length;
> >>       u32 quirks = 0;
> >>       unsigned int rsize = 0;
> >>       char *rdesc;
> >>       int ret, n;
> >>
> >> +     if (!buffer) {
> >> +             dbg_hid("class descriptor not present\n");
> >> +             return -ENODEV;
> >> +     }
> >> +
> >>       quirks = usbhid_lookup_quirk(le16_to_cpu(dev->descriptor.idVendor),
> >>                       le16_to_cpu(dev->descriptor.idProduct));
> >>
> >> @@ -990,19 +997,27 @@ static int usbhid_parse(struct hid_device *hid)
> >>                               quirks |= HID_QUIRK_NOGET;
> >>       }
> >>
> >> -     if (usb_get_extra_descriptor(interface, HID_DT_HID, &hdesc) &&
> >> -         (!interface->desc.bNumEndpoints ||
> >> -          usb_get_extra_descriptor(&interface->endpoint[0], HID_DT_HID, &hdesc))) {
> >> -             dbg_hid("class descriptor not present\n");
> >> -             return -ENODEV;
> >> -     }
> >> +     while (buflen > 2) {
> >> +             length = buffer[0];
> >> +             if (!length || length < HID_DESCRIPTOR_MIN_SIZE)
> >> +                     goto next_desc;
> >
> > It's silly to duplicate the code that is already present in
> > usb_get_extra_descriptor().  There's no reason to avoid using that
> > function here.
> 
> To be honest, I was fooled. You are right. That is a duplicate code
> with usb_get_extra_descriptor().
> 
> >
> >> -     hid->version = le16_to_cpu(hdesc->bcdHID);
> >> -     hid->country = hdesc->bCountryCode;
> >> +             if (buffer[1] == HID_DT_HID) {
> >> +                     hid->version = get_unaligned_le16(&buffer[2]);
> >> +                     hid->country = buffer[4];
> >
> > It's also silly not to use the preformatted hid_descriptor structure and
> > instead put error-prone byte offsets directly into the code.
> 
> Right.
> 
> >
> >> -     for (n = 0; n < hdesc->bNumDescriptors; n++)
> >> -             if (hdesc->desc[n].bDescriptorType == HID_DT_REPORT)
> >> -                     rsize = le16_to_cpu(hdesc->desc[n].wDescriptorLength);
> >> +                     for (n = 0; n < buffer[5]; n++) {
> >> +                             /* we are just interested in report descriptor */
> >> +                             if (buffer[6+3*n] != HID_DT_REPORT)
> >> +                                     continue;
> >> +                             rsize = get_unaligned_le16(&buffer[7+3*n]);
> >
> > Finally, this patch doesn't fix the actual problem!  You don't check
> > here whether 8+3*n >= length.
> >
> > This whole thing could be fixed with a much smaller change to the
> > original code.  Just do something like this:
> >
> >         num_descriptors = min_t(int, hdesc->bNumDescriptors,
> >                         (hdesc->bLength - 6) / 3);
> >         for (n = 0; n < num_descriptors; n++)
> 
> Right. Your change is better and clear.
> With your change, I think we need check the size of hdesc before
> accessing hdesc->desc[n] as Andrey Konovalov has already pointed out,
> 
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index 089bad8..0b41d44 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -975,6 +975,7 @@ static int usbhid_parse(struct hid_device *hid)
>         unsigned int rsize = 0;
>         char *rdesc;
>         int ret, n;
> +       int num_descriptors;
> 
>         quirks = usbhid_lookup_quirk(le16_to_cpu(dev->descriptor.idVendor),
>                         le16_to_cpu(dev->descriptor.idProduct));
> @@ -997,10 +998,17 @@ 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 - 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.

Alan Stern

Re: [PATCH] HID: usbhid: fix out-of-bounds bug

[Alan Stern]

On Wed, 27 Sep 2017, Jaejoong Kim wrote:

> Hi, Alan,
> 
> Thanks for the review.
> 
> 2017-09-26 23:18 GMT+09:00 Alan Stern <stern@rowland.harvard.edu>:
> > On Tue, 26 Sep 2017, Jaejoong Kim wrote:
> >
> >> The starting address of the hid descriptor is obtained via
> >> usb_get_extra_descriptor(). If the hid descriptor has the wrong size, it
> >> is possible to access the wrong address. So, before accessing the hid
> >> descriptor, we need to check the entire size through the bLength field.
> >>
> >> It also shows how many class descriptors it has through the bNumDescriptors
> >> of the hid descriptor. Assuming that the connected hid descriptor has two
> >> class descriptors(report and physical descriptors), the code below can
> >> cause OOB because hdesc->desc is an array of size 1.
> >>
> >> for (n = 0; n < hdesc->bNumDescriptors; n++)
> >>       if (hdesc->desc[n].bDescriptorType == HID_DT_REPORT)
> >>               rsize = le16_to_cpu(hdesc->desc[n].wDescriptorLength);
> >>
> >> Since we know the starting address of the hid descriptor and the value of
> >> the bNumDescriptors is variable, we directly access the buffer containing
> >> the hid descriptor without usbing hdesc->desc to obtain the size of the
> >> report descriptor.
> >
> > I do not like this patch at all.
> >
> >> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> >> index 089bad8..7bad173 100644
> >> --- a/drivers/hid/usbhid/hid-core.c
> >> +++ b/drivers/hid/usbhid/hid-core.c
> >> @@ -970,12 +970,19 @@ static int usbhid_parse(struct hid_device *hid)
> >>       struct usb_interface *intf = to_usb_interface(hid->dev.parent);
> >>       struct usb_host_interface *interface = intf->cur_altsetting;
> >>       struct usb_device *dev = interface_to_usbdev (intf);
> >> -     struct hid_descriptor *hdesc;
> >> +     unsigned char *buffer = intf->altsetting->extra;
> >> +     int buflen = intf->altsetting->extralen;
> >> +     int length;
> >>       u32 quirks = 0;
> >>       unsigned int rsize = 0;
> >>       char *rdesc;
> >>       int ret, n;
> >>
> >> +     if (!buffer) {
> >> +             dbg_hid("class descriptor not present\n");
> >> +             return -ENODEV;
> >> +     }
> >> +
> >>       quirks = usbhid_lookup_quirk(le16_to_cpu(dev->descriptor.idVendor),
> >>                       le16_to_cpu(dev->descriptor.idProduct));
> >>
> >> @@ -990,19 +997,27 @@ static int usbhid_parse(struct hid_device *hid)
> >>                               quirks |= HID_QUIRK_NOGET;
> >>       }
> >>
> >> -     if (usb_get_extra_descriptor(interface, HID_DT_HID, &hdesc) &&
> >> -         (!interface->desc.bNumEndpoints ||
> >> -          usb_get_extra_descriptor(&interface->endpoint[0], HID_DT_HID, &hdesc))) {
> >> -             dbg_hid("class descriptor not present\n");
> >> -             return -ENODEV;
> >> -     }
> >> +     while (buflen > 2) {
> >> +             length = buffer[0];
> >> +             if (!length || length < HID_DESCRIPTOR_MIN_SIZE)
> >> +                     goto next_desc;
> >
> > It's silly to duplicate the code that is already present in
> > usb_get_extra_descriptor().  There's no reason to avoid using that
> > function here.
> 
> To be honest, I was fooled. You are right. That is a duplicate code
> with usb_get_extra_descriptor().
> 
> >
> >> -     hid->version = le16_to_cpu(hdesc->bcdHID);
> >> -     hid->country = hdesc->bCountryCode;
> >> +             if (buffer[1] == HID_DT_HID) {
> >> +                     hid->version = get_unaligned_le16(&buffer[2]);
> >> +                     hid->country = buffer[4];
> >
> > It's also silly not to use the preformatted hid_descriptor structure and
> > instead put error-prone byte offsets directly into the code.
> 
> Right.
> 
> >
> >> -     for (n = 0; n < hdesc->bNumDescriptors; n++)
> >> -             if (hdesc->desc[n].bDescriptorType == HID_DT_REPORT)
> >> -                     rsize = le16_to_cpu(hdesc->desc[n].wDescriptorLength);
> >> +                     for (n = 0; n < buffer[5]; n++) {
> >> +                             /* we are just interested in report descriptor */
> >> +                             if (buffer[6+3*n] != HID_DT_REPORT)
> >> +                                     continue;
> >> +                             rsize = get_unaligned_le16(&buffer[7+3*n]);
> >
> > Finally, this patch doesn't fix the actual problem!  You don't check
> > here whether 8+3*n >= length.
> >
> > This whole thing could be fixed with a much smaller change to the
> > original code.  Just do something like this:
> >
> >         num_descriptors = min_t(int, hdesc->bNumDescriptors,
> >                         (hdesc->bLength - 6) / 3);
> >         for (n = 0; n < num_descriptors; n++)
> 
> Right. Your change is better and clear.
> With your change, I think we need check the size of hdesc before
> accessing hdesc->desc[n] as Andrey Konovalov has already pointed out,
> 
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index 089bad8..0b41d44 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -975,6 +975,7 @@ static int usbhid_parse(struct hid_device *hid)
>         unsigned int rsize = 0;
>         char *rdesc;
>         int ret, n;
> +       int num_descriptors;
> 
>         quirks = usbhid_lookup_quirk(le16_to_cpu(dev->descriptor.idVendor),
>                         le16_to_cpu(dev->descriptor.idProduct));
> @@ -997,10 +998,17 @@ 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 - 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.

Alan Stern

Re: [PATCH] HID: usbhid: fix out-of-bounds bug

[Jaejoong Kim]

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

Re: [PATCH] HID: usbhid: fix out-of-bounds bug

[Alan Stern]

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

Re: [PATCH] HID: usbhid: fix out-of-bounds bug

[Alan Stern]

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