| Message ID | Pine.LNX.4.44L0.1908201557220.1573-100000@iolanthe.rowland.org (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 5f9242775bb61f390f0885f23fc16397262c7538 |
| Headers | show |
| Series | HID: USB: Fix general protection fault caused by Logitech driver | expand |
On Tue, Aug 20, 2019 at 10:00 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > The syzbot fuzzer found a general protection fault in the HID subsystem: > > kasan: CONFIG_KASAN_INLINE enabled > kasan: GPF could be caused by NULL-ptr deref or user memory access > general protection fault: 0000 [#1] SMP KASAN > CPU: 0 PID: 3715 Comm: syz-executor.3 Not tainted 5.2.0-rc6+ #15 > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS > Google 01/01/2011 > RIP: 0010:__pm_runtime_resume+0x49/0x180 drivers/base/power/runtime.c:1069 > Code: ed 74 d5 fe 45 85 ed 0f 85 9a 00 00 00 e8 6f 73 d5 fe 48 8d bd c1 02 > 00 00 48 b8 00 00 00 00 00 fc ff df 48 89 fa 48 c1 ea 03 <0f> b6 04 02 48 > 89 fa 83 e2 07 38 d0 7f 08 84 c0 0f 85 fe 00 00 00 > RSP: 0018:ffff8881d99d78e0 EFLAGS: 00010202 > RAX: dffffc0000000000 RBX: 0000000000000020 RCX: ffffc90003f3f000 > RDX: 0000000416d8686d RSI: ffffffff82676841 RDI: 00000020b6c3436a > RBP: 00000020b6c340a9 R08: ffff8881c6d64800 R09: fffffbfff0e84c25 > R10: ffff8881d99d7940 R11: ffffffff87426127 R12: 0000000000000004 > R13: 0000000000000000 R14: ffff8881d9b94000 R15: ffffffff897f9048 > FS: 00007f047f542700(0000) GS:ffff8881db200000(0000) knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: 0000001b30f21000 CR3: 00000001ca032000 CR4: 00000000001406f0 > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 > Call Trace: > pm_runtime_get_sync include/linux/pm_runtime.h:226 [inline] > usb_autopm_get_interface+0x1b/0x50 drivers/usb/core/driver.c:1707 > usbhid_power+0x7c/0xe0 drivers/hid/usbhid/hid-core.c:1234 > hid_hw_power include/linux/hid.h:1038 [inline] > hidraw_open+0x20d/0x740 drivers/hid/hidraw.c:282 > chrdev_open+0x219/0x5c0 fs/char_dev.c:413 > do_dentry_open+0x497/0x1040 fs/open.c:778 > do_last fs/namei.c:3416 [inline] > path_openat+0x1430/0x3ff0 fs/namei.c:3533 > do_filp_open+0x1a1/0x280 fs/namei.c:3563 > do_sys_open+0x3c0/0x580 fs/open.c:1070 > do_syscall_64+0xb7/0x560 arch/x86/entry/common.c:301 > entry_SYSCALL_64_after_hwframe+0x49/0xbe > > It turns out the fault was caused by a bug in the HID Logitech driver, > which violates the requirement that every pathway calling > hid_hw_start() must also call hid_hw_stop(). This patch fixes the bug > by making sure the requirement is met. > > Reported-and-tested-by: syzbot+3cbe5cd105d2ad56a1df@syzkaller.appspotmail.com > Signed-off-by: Alan Stern <stern@rowland.harvard.edu> > CC: <stable@vger.kernel.org> This bug has manifested in a bunch of different ways and produced multiple confusing syzbot reports. Thank you for tracking this down and fixing it, Alan! > > --- > > [as1909] > > > drivers/hid/hid-lg.c | 10 ++++++---- > drivers/hid/hid-lg4ff.c | 1 - > 2 files changed, 6 insertions(+), 5 deletions(-) > > Index: usb-devel/drivers/hid/hid-lg.c > =================================================================== > --- usb-devel.orig/drivers/hid/hid-lg.c > +++ usb-devel/drivers/hid/hid-lg.c > @@ -818,7 +818,7 @@ static int lg_probe(struct hid_device *h > > if (!buf) { > ret = -ENOMEM; > - goto err_free; > + goto err_stop; > } > > ret = hid_hw_raw_request(hdev, buf[0], buf, sizeof(cbuf), > @@ -850,9 +850,12 @@ static int lg_probe(struct hid_device *h > ret = lg4ff_init(hdev); > > if (ret) > - goto err_free; > + goto err_stop; > > return 0; > + > +err_stop: > + hid_hw_stop(hdev); > err_free: > kfree(drv_data); > return ret; > @@ -863,8 +866,7 @@ static void lg_remove(struct hid_device > struct lg_drv_data *drv_data = hid_get_drvdata(hdev); > if (drv_data->quirks & LG_FF4) > lg4ff_deinit(hdev); > - else > - hid_hw_stop(hdev); > + hid_hw_stop(hdev); > kfree(drv_data); > } > > Index: usb-devel/drivers/hid/hid-lg4ff.c > =================================================================== > --- usb-devel.orig/drivers/hid/hid-lg4ff.c > +++ usb-devel/drivers/hid/hid-lg4ff.c > @@ -1477,7 +1477,6 @@ int lg4ff_deinit(struct hid_device *hid) > } > } > #endif > - hid_hw_stop(hid); > drv_data->device_props = NULL; > > kfree(entry); >
On Tue, 20 Aug 2019, Alan Stern wrote:
> The syzbot fuzzer found a general protection fault in the HID subsystem:
Applied, thanks a lot Alan.
On Tue, Aug 20, 2019 at 10:00 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > The syzbot fuzzer found a general protection fault in the HID subsystem: > > kasan: CONFIG_KASAN_INLINE enabled > kasan: GPF could be caused by NULL-ptr deref or user memory access > general protection fault: 0000 [#1] SMP KASAN > CPU: 0 PID: 3715 Comm: syz-executor.3 Not tainted 5.2.0-rc6+ #15 > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS > Google 01/01/2011 > RIP: 0010:__pm_runtime_resume+0x49/0x180 drivers/base/power/runtime.c:1069 > Code: ed 74 d5 fe 45 85 ed 0f 85 9a 00 00 00 e8 6f 73 d5 fe 48 8d bd c1 02 > 00 00 48 b8 00 00 00 00 00 fc ff df 48 89 fa 48 c1 ea 03 <0f> b6 04 02 48 > 89 fa 83 e2 07 38 d0 7f 08 84 c0 0f 85 fe 00 00 00 > RSP: 0018:ffff8881d99d78e0 EFLAGS: 00010202 > RAX: dffffc0000000000 RBX: 0000000000000020 RCX: ffffc90003f3f000 > RDX: 0000000416d8686d RSI: ffffffff82676841 RDI: 00000020b6c3436a > RBP: 00000020b6c340a9 R08: ffff8881c6d64800 R09: fffffbfff0e84c25 > R10: ffff8881d99d7940 R11: ffffffff87426127 R12: 0000000000000004 > R13: 0000000000000000 R14: ffff8881d9b94000 R15: ffffffff897f9048 > FS: 00007f047f542700(0000) GS:ffff8881db200000(0000) knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: 0000001b30f21000 CR3: 00000001ca032000 CR4: 00000000001406f0 > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 > Call Trace: > pm_runtime_get_sync include/linux/pm_runtime.h:226 [inline] > usb_autopm_get_interface+0x1b/0x50 drivers/usb/core/driver.c:1707 > usbhid_power+0x7c/0xe0 drivers/hid/usbhid/hid-core.c:1234 > hid_hw_power include/linux/hid.h:1038 [inline] > hidraw_open+0x20d/0x740 drivers/hid/hidraw.c:282 > chrdev_open+0x219/0x5c0 fs/char_dev.c:413 > do_dentry_open+0x497/0x1040 fs/open.c:778 > do_last fs/namei.c:3416 [inline] > path_openat+0x1430/0x3ff0 fs/namei.c:3533 > do_filp_open+0x1a1/0x280 fs/namei.c:3563 > do_sys_open+0x3c0/0x580 fs/open.c:1070 > do_syscall_64+0xb7/0x560 arch/x86/entry/common.c:301 > entry_SYSCALL_64_after_hwframe+0x49/0xbe > > It turns out the fault was caused by a bug in the HID Logitech driver, > which violates the requirement that every pathway calling > hid_hw_start() must also call hid_hw_stop(). This patch fixes the bug > by making sure the requirement is met. > > Reported-and-tested-by: syzbot+3cbe5cd105d2ad56a1df@syzkaller.appspotmail.com > Signed-off-by: Alan Stern <stern@rowland.harvard.edu> > CC: <stable@vger.kernel.org> > > --- > > [as1909] > > > drivers/hid/hid-lg.c | 10 ++++++---- > drivers/hid/hid-lg4ff.c | 1 - > 2 files changed, 6 insertions(+), 5 deletions(-) > > Index: usb-devel/drivers/hid/hid-lg.c > =================================================================== > --- usb-devel.orig/drivers/hid/hid-lg.c > +++ usb-devel/drivers/hid/hid-lg.c > @@ -818,7 +818,7 @@ static int lg_probe(struct hid_device *h > > if (!buf) { > ret = -ENOMEM; > - goto err_free; > + goto err_stop; > } > > ret = hid_hw_raw_request(hdev, buf[0], buf, sizeof(cbuf), > @@ -850,9 +850,12 @@ static int lg_probe(struct hid_device *h > ret = lg4ff_init(hdev); > > if (ret) > - goto err_free; > + goto err_stop; > > return 0; > + > +err_stop: > + hid_hw_stop(hdev); > err_free: > kfree(drv_data); > return ret; > @@ -863,8 +866,7 @@ static void lg_remove(struct hid_device > struct lg_drv_data *drv_data = hid_get_drvdata(hdev); > if (drv_data->quirks & LG_FF4) > lg4ff_deinit(hdev); > - else > - hid_hw_stop(hdev); > + hid_hw_stop(hdev); > kfree(drv_data); > } > > Index: usb-devel/drivers/hid/hid-lg4ff.c > =================================================================== > --- usb-devel.orig/drivers/hid/hid-lg4ff.c > +++ usb-devel/drivers/hid/hid-lg4ff.c > @@ -1477,7 +1477,6 @@ int lg4ff_deinit(struct hid_device *hid) > } > } > #endif > - hid_hw_stop(hid); > drv_data->device_props = NULL; > > kfree(entry); > Hi Alan, I've ran the fuzzer with your patches applied overnight and noticed another fallout of similar bugs. I think they are caused by a similar issue in the sony HID driver. There's no hid_hw_stop() call in the "if (!(hdev->claimed & HID_CLAIMED_INPUT))" case in sony_probe(). Does it look like a bug to you? Thanks!
On Thu, 22 Aug 2019, Andrey Konovalov wrote: > Hi Alan, > > I've ran the fuzzer with your patches applied overnight and noticed > another fallout of similar bugs. I think they are caused by a similar > issue in the sony HID driver. There's no hid_hw_stop() call in the "if > (!(hdev->claimed & HID_CLAIMED_INPUT))" case in sony_probe(). Does it > look like a bug to you? It looks like the relevant hid_hw_stop() call is the one at the end of sony_configure_input(). But I can't tell if doing that way is valid or not -- in practice the code would end up calling hid_disconnect() while hid_connect() was still running, which doesn't seem like a good idea. There's a comment about this near the end of sony_probe(). I suspect it would be better to call hid_hw_stop() in the conditional code following that comment rather than in sony_configure_input(). Either way, these are all things Jiri should know about or check up on. Have you gotten any test results from syzbot exercising these pathways? You ought to be able to tell which HID driver is involved by looking through the console output. Alan Stern
On Thu, Aug 22, 2019 at 7:11 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > On Thu, 22 Aug 2019, Andrey Konovalov wrote: > > > Hi Alan, > > > > I've ran the fuzzer with your patches applied overnight and noticed > > another fallout of similar bugs. I think they are caused by a similar > > issue in the sony HID driver. There's no hid_hw_stop() call in the "if > > (!(hdev->claimed & HID_CLAIMED_INPUT))" case in sony_probe(). Does it > > look like a bug to you? > > It looks like the relevant hid_hw_stop() call is the one at the end of > sony_configure_input(). But I can't tell if doing that way is valid or > not -- in practice the code would end up calling hid_disconnect() while > hid_connect() was still running, which doesn't seem like a good idea. > > There's a comment about this near the end of sony_probe(). I suspect > it would be better to call hid_hw_stop() in the conditional code > following that comment rather than in sony_configure_input(). > > Either way, these are all things Jiri should know about or check up on. > > Have you gotten any test results from syzbot exercising these pathways? > You ought to be able to tell which HID driver is involved by looking > through the console output. Yes, a typical crash is below, that's why I thought it's the sony driver. Adding hid_hw_stop() in sony_probe() stops the issue from happening, but I don't know whether it's the right fix. usb 1-1: new high-speed USB device number 3 using dummy_hcd usb 1-1: Using ep0 maxpacket: 8 usb 1-1: config 0 interface 0 altsetting 0 endpoint 0x81 has an invalid bInterval 0, changing7 usb 1-1: config 0 interface 0 altsetting 0 has 1 endpoint descriptor, different from the inte9 usb 1-1: New USB device found, idVendor=054c, idProduct=024b, bcdDevice= 0.00 usb 1-1: New USB device strings: Mfr=0, Product=0, SerialNumber=0 usb 1-1: config 0 descriptor?? sony 0003:054C:024B.0002: unknown main item tag 0x0 sony 0003:054C:024B.0002: unknown main item tag 0x2 sony 0003:054C:024B.0002: unknown main item tag 0x0 sony 0003:054C:024B.0002: unknown main item tag 0x0 ... sony 0003:054C:024B.0002: unknown main item tag 0x0 ================================================================== BUG: KASAN: use-after-free in usbhid_power+0xca/0xe0 Read of size 8 at addr ffff88805d590008 by task syz-executor/1808 CPU: 1 PID: 1808 Comm: syz-executor Not tainted 5.3.0-rc5+ #203 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014 Call Trace: __dump_stack lib/dump_stack.c:77 dump_stack+0xca/0x13e lib/dump_stack.c:113 print_address_description+0x6a/0x32c mm/kasan/report.c:351 __kasan_report.cold+0x1a/0x33 mm/kasan/report.c:482 kasan_report+0xe/0x12 mm/kasan/common.c:612 usbhid_power+0xca/0xe0 drivers/hid/usbhid/hid-core.c:1234 hid_hw_power ./include/linux/hid.h:1038 hidraw_open+0x20d/0x740 drivers/hid/hidraw.c:282 chrdev_open+0x219/0x5c0 fs/char_dev.c:414 do_dentry_open+0x494/0x1120 fs/open.c:797 do_last fs/namei.c:3416 path_openat+0x1430/0x3f50 fs/namei.c:3533 do_filp_open+0x1a1/0x280 fs/namei.c:3563 do_sys_open+0x3c0/0x580 fs/open.c:1089 do_syscall_64+0xb7/0x580 arch/x86/entry/common.c:296 entry_SYSCALL_64_after_hwframe+0x49/0xbe arch/x86/entry/entry_64.S:175 RIP: 0033:0x413a0e Code: 89 54 24 08 e8 a3 f9 ff ff 8b 74 24 0c 48 8b 3c 24 41 89 c0 44 8b 54 24 08 b8 01 01 00 4 RSP: 002b:00007f7bf0a66730 EFLAGS: 00000293 ORIG_RAX: 0000000000000101 RAX: ffffffffffffffda RBX: 6666666666666667 RCX: 0000000000413a0e RDX: 0000000000000200 RSI: 00007f7bf0a66840 RDI: 00000000ffffff9c RBP: 00007f7bf0a66840 R08: 0000000000000000 R09: 0000000000000000 R10: 0000000000000000 R11: 0000000000000293 R12: 00000000004a521c R13: 00000000004ef7d0 R14: 00000000004ae881 R15: 00007f7bf0a676bc Allocated by task 78: save_stack+0x1b/0x80 mm/kasan/common.c:69 set_track mm/kasan/common.c:77 __kasan_kmalloc mm/kasan/common.c:487 __kasan_kmalloc.constprop.0+0xbf/0xd0 mm/kasan/common.c:460 slab_post_alloc_hook mm/slab.h:520 slab_alloc_node mm/slub.c:2770 __kmalloc_node_track_caller+0xfc/0x380 mm/slub.c:4365 __kmalloc_reserve.isra.0+0x39/0xe0 net/core/skbuff.c:141 __alloc_skb+0xef/0x5a0 net/core/skbuff.c:209 alloc_skb ./include/linux/skbuff.h:1055 alloc_uevent_skb+0x7b/0x210 lib/kobject_uevent.c:289 uevent_net_broadcast_untagged lib/kobject_uevent.c:325 kobject_uevent_net_broadcast lib/kobject_uevent.c:408 kobject_uevent_env+0x8ee/0x1160 lib/kobject_uevent.c:592 device_del+0x6b2/0xb10 drivers/base/core.c:2349 usb_disable_device+0x211/0x690 drivers/usb/core/message.c:1237 usb_disconnect+0x284/0x8d0 drivers/usb/core/hub.c:2199 hub_port_connect drivers/usb/core/hub.c:4949 hub_port_connect_change drivers/usb/core/hub.c:5213 port_event drivers/usb/core/hub.c:5359 hub_event+0x1454/0x3640 drivers/usb/core/hub.c:5441 process_one_work+0x92b/0x1530 kernel/workqueue.c:2269 worker_thread+0x96/0xe20 kernel/workqueue.c:2415 kthread+0x318/0x420 kernel/kthread.c:255 ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352 Freed by task 242: save_stack+0x1b/0x80 mm/kasan/common.c:69 set_track mm/kasan/common.c:77 __kasan_slab_free+0x130/0x180 mm/kasan/common.c:449 slab_free_hook mm/slub.c:1423 slab_free_freelist_hook mm/slub.c:1474 slab_free mm/slub.c:3016 kfree+0xe4/0x2f0 mm/slub.c:3957 skb_free_head+0x8b/0xa0 net/core/skbuff.c:591 skb_release_data+0x41f/0x7c0 net/core/skbuff.c:611 skb_release_all+0x46/0x60 net/core/skbuff.c:665 __kfree_skb net/core/skbuff.c:679 consume_skb net/core/skbuff.c:838 consume_skb+0xd9/0x320 net/core/skbuff.c:832 skb_free_datagram+0x16/0xf0 net/core/datagram.c:328 netlink_recvmsg+0x65e/0xee0 net/netlink/af_netlink.c:1996 sock_recvmsg_nosec net/socket.c:871 sock_recvmsg net/socket.c:889 sock_recvmsg+0xca/0x110 net/socket.c:885 ___sys_recvmsg+0x271/0x5a0 net/socket.c:2480 __sys_recvmsg+0xe9/0x1b0 net/socket.c:2537 do_syscall_64+0xb7/0x580 arch/x86/entry/common.c:296 entry_SYSCALL_64_after_hwframe+0x49/0xbe arch/x86/entry/entry_64.S:175 The buggy address belongs to the object at ffff88805d590000 which belongs to the cache kmalloc-1k of size 1024 The buggy address is located 8 bytes inside of 1024-byte region [ffff88805d590000, ffff88805d590400) The buggy address belongs to the page: page:ffffea0001756400 refcount:1 mapcount:0 mapping:ffff88806c402280 index:0x0 compound_mapco0 flags: 0x100000000010200(slab|head) raw: 0100000000010200 dead000000000100 dead000000000122 ffff88806c402280 raw: 0000000000000000 00000000000e000e 00000001ffffffff 0000000000000000 page dumped because: kasan: bad access detected Memory state around the buggy address: ffff88805d58ff00: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe ffff88805d58ff80: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe >ffff88805d590000: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb ^ ffff88805d590080: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb ffff88805d590100: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb ==================================================================
On Thu, 22 Aug 2019, Andrey Konovalov wrote: > On Thu, Aug 22, 2019 at 7:11 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > On Thu, 22 Aug 2019, Andrey Konovalov wrote: > > > > > Hi Alan, > > > > > > I've ran the fuzzer with your patches applied overnight and noticed > > > another fallout of similar bugs. I think they are caused by a similar > > > issue in the sony HID driver. There's no hid_hw_stop() call in the "if > > > (!(hdev->claimed & HID_CLAIMED_INPUT))" case in sony_probe(). Does it > > > look like a bug to you? > > > > It looks like the relevant hid_hw_stop() call is the one at the end of > > sony_configure_input(). But I can't tell if doing that way is valid or > > not -- in practice the code would end up calling hid_disconnect() while > > hid_connect() was still running, which doesn't seem like a good idea. > > > > There's a comment about this near the end of sony_probe(). I suspect > > it would be better to call hid_hw_stop() in the conditional code > > following that comment rather than in sony_configure_input(). > > > > Either way, these are all things Jiri should know about or check up on. > > > > Have you gotten any test results from syzbot exercising these pathways? > > You ought to be able to tell which HID driver is involved by looking > > through the console output. > > Yes, a typical crash is below, that's why I thought it's the sony > driver. Adding hid_hw_stop() in sony_probe() stops the issue from > happening, but I don't know whether it's the right fix. Probably you have to add hid_hw_stop() in sony_probe() and remove it from sony_configure_input(). But like I said above, Jiri should look into this. Alan Stern
On Thu, 22 Aug 2019, Alan Stern wrote: > > > > I've ran the fuzzer with your patches applied overnight and noticed > > > > another fallout of similar bugs. I think they are caused by a similar > > > > issue in the sony HID driver. There's no hid_hw_stop() call in the "if > > > > (!(hdev->claimed & HID_CLAIMED_INPUT))" case in sony_probe(). Does it > > > > look like a bug to you? > > > > > > It looks like the relevant hid_hw_stop() call is the one at the end of > > > sony_configure_input(). But I can't tell if doing that way is valid or > > > not -- in practice the code would end up calling hid_disconnect() while > > > hid_connect() was still running, which doesn't seem like a good idea. > > > > > > There's a comment about this near the end of sony_probe(). I suspect > > > it would be better to call hid_hw_stop() in the conditional code > > > following that comment rather than in sony_configure_input(). > > > > > > Either way, these are all things Jiri should know about or check up on. > > > > > > Have you gotten any test results from syzbot exercising these pathways? > > > You ought to be able to tell which HID driver is involved by looking > > > through the console output. > > > > Yes, a typical crash is below, that's why I thought it's the sony > > driver. Adding hid_hw_stop() in sony_probe() stops the issue from > > happening, but I don't know whether it's the right fix. > > Probably you have to add hid_hw_stop() in sony_probe() and remove it > from sony_configure_input(). But like I said above, Jiri should look > into this. It almost certainly is, thanks. Adding Roderick to CC ... Roderick, will you be able to test and submit a patch fixing that?
On Thu, 22 Aug 2019, Alan Stern wrote: > > > > > I've ran the fuzzer with your patches applied overnight and noticed > > > > > another fallout of similar bugs. I think they are caused by a similar > > > > > issue in the sony HID driver. There's no hid_hw_stop() call in the "if > > > > > (!(hdev->claimed & HID_CLAIMED_INPUT))" case in sony_probe(). Does it > > > > > look like a bug to you? > > > > > > > > It looks like the relevant hid_hw_stop() call is the one at the end of > > > > sony_configure_input(). But I can't tell if doing that way is valid or > > > > not -- in practice the code would end up calling hid_disconnect() while > > > > hid_connect() was still running, which doesn't seem like a good idea. > > > > > > > > There's a comment about this near the end of sony_probe(). I suspect > > > > it would be better to call hid_hw_stop() in the conditional code > > > > following that comment rather than in sony_configure_input(). > > > > > > > > Either way, these are all things Jiri should know about or check up on. > > > > > > > > Have you gotten any test results from syzbot exercising these pathways? > > > > You ought to be able to tell which HID driver is involved by looking > > > > through the console output. > > > > > > Yes, a typical crash is below, that's why I thought it's the sony > > > driver. Adding hid_hw_stop() in sony_probe() stops the issue from > > > happening, but I don't know whether it's the right fix. > > > > Probably you have to add hid_hw_stop() in sony_probe() and remove it > > from sony_configure_input(). But like I said above, Jiri should look > > into this. > It almost certainly is, thanks. > Adding Roderick to CC ... Roderick, will you be able to test and submit > a patch fixing that? > > -- > Jiri Kosina > SUSE Labs Sure we will have a look and do some testing. Hopefully we can share a patch some time next week. Thanks, Roderick
On Sat, Aug 24, 2019 at 2:41 AM <Roderick.Colenbrander@sony.com> wrote: > > On Thu, 22 Aug 2019, Alan Stern wrote: > > > > > > > I've ran the fuzzer with your patches applied overnight and noticed > > > > > > another fallout of similar bugs. I think they are caused by a similar > > > > > > issue in the sony HID driver. There's no hid_hw_stop() call in the "if > > > > > > (!(hdev->claimed & HID_CLAIMED_INPUT))" case in sony_probe(). Does it > > > > > > look like a bug to you? > > > > > > > > > > It looks like the relevant hid_hw_stop() call is the one at the end of > > > > > sony_configure_input(). But I can't tell if doing that way is valid or > > > > > not -- in practice the code would end up calling hid_disconnect() while > > > > > hid_connect() was still running, which doesn't seem like a good idea. > > > > > > > > > > There's a comment about this near the end of sony_probe(). I suspect > > > > > it would be better to call hid_hw_stop() in the conditional code > > > > > following that comment rather than in sony_configure_input(). > > > > > > > > > > Either way, these are all things Jiri should know about or check up on. > > > > > > > > > > Have you gotten any test results from syzbot exercising these pathways? > > > > > You ought to be able to tell which HID driver is involved by looking > > > > > through the console output. > > > > > > > > Yes, a typical crash is below, that's why I thought it's the sony > > > > driver. Adding hid_hw_stop() in sony_probe() stops the issue from > > > > happening, but I don't know whether it's the right fix. > > > > > > Probably you have to add hid_hw_stop() in sony_probe() and remove it > > > from sony_configure_input(). But like I said above, Jiri should look > > > into this. > > > It almost certainly is, thanks. > > > Adding Roderick to CC ... Roderick, will you be able to test and submit > > a patch fixing that? > > > > -- > > Jiri Kosina > > SUSE Labs > > Sure we will have a look and do some testing. Hopefully we can share a patch some time next week. Hi Roderick, I was wondering if you had a chance to look into this? Once the Logitech fix is upstream, this similar Sony bug will start producing a large number of similar syzbot reports since it causes a major memory corruption and we'll need to triage them all again. It would be nice to get the Sony fix merged together with the Logitech one. Or at least to have it available so I can apply it manually until it is merged. Thanks!
> From: Andrey Konovalov [andreyknvl@google.com] > Sent: Tuesday, September 03, 2019 3:46 AM > To: Colenbrander, Roderick > Cc: Jiri Kosina; Alan Stern; Gustavo A. R. Silva; Hillf Danton; syzkaller-bugs; linux-input@vger.kernel.org; USB list > Subject: Re: [PATCH] HID: USB: Fix general protection fault caused by Logitech driver > On Sat, Aug 24, 2019 at 2:41 AM <Roderick.Colenbrander@sony.com> wrote: > > On Thu, 22 Aug 2019, Alan Stern wrote: > > > > > > > > > I've ran the fuzzer with your patches applied overnight and noticed > > > > > > > another fallout of similar bugs. I think they are caused by a similar > > > > > > > issue in the sony HID driver. There's no hid_hw_stop() call in the "if > > > > > > > (!(hdev->claimed & HID_CLAIMED_INPUT))" case in sony_probe(). Does it > > > > > > > look like a bug to you? > > > > > > > > > > > > It looks like the relevant hid_hw_stop() call is the one at the end of > > > > > > sony_configure_input(). But I can't tell if doing that way is valid or > > > > > > not -- in practice the code would end up calling hid_disconnect() while > > > > > > hid_connect() was still running, which doesn't seem like a good idea. > > > > > > > > > > > > There's a comment about this near the end of sony_probe(). I suspect > > > > > > it would be better to call hid_hw_stop() in the conditional code > > > > > > following that comment rather than in sony_configure_input(). > > > > > > > > > > > > Either way, these are all things Jiri should know about or check up on. > > > > > > > > > > > > Have you gotten any test results from syzbot exercising these pathways? > > > > > > You ought to be able to tell which HID driver is involved by looking > > > > > > through the console output. > > > > > > > > > > Yes, a typical crash is below, that's why I thought it's the sony > > > > > driver. Adding hid_hw_stop() in sony_probe() stops the issue from > > > > > happening, but I don't know whether it's the right fix. > > > > > > > > Probably you have to add hid_hw_stop() in sony_probe() and remove it > > > > from sony_configure_input(). But like I said above, Jiri should look > > > > into this. > > > > > It almost certainly is, thanks. > > > > > Adding Roderick to CC ... Roderick, will you be able to test and submit > > > a patch fixing that? > > > > > > -- > > > Jiri Kosina > > > SUSE Labs > > > > Sure we will have a look and do some testing. Hopefully we can share a patch some time next week. > Hi Roderick, > > I was wondering if you had a chance to look into this? > > Once the Logitech fix is upstream, this similar Sony bug will start > producing a large number of similar syzbot reports since it causes a > major memory corruption and we'll need to triage them all again. It > would be nice to get the Sony fix merged together with the Logitech > one. Or at least to have it available so I can apply it manually until > it is merged. > > Thanks! Hi Andrey, We are still looking at the issue. I had one of my guys try the fix (and trigger an error in sony_input_configured), but he claimed he crashed his kernel. Due to the long weekend in the US, we didn't get back to looking at it yet. We are looking at it some more today or tomorrow. Thanks, Roderick
Index: usb-devel/drivers/hid/hid-lg.c =================================================================== --- usb-devel.orig/drivers/hid/hid-lg.c +++ usb-devel/drivers/hid/hid-lg.c @@ -818,7 +818,7 @@ static int lg_probe(struct hid_device *h if (!buf) { ret = -ENOMEM; - goto err_free; + goto err_stop; } ret = hid_hw_raw_request(hdev, buf[0], buf, sizeof(cbuf), @@ -850,9 +850,12 @@ static int lg_probe(struct hid_device *h ret = lg4ff_init(hdev); if (ret) - goto err_free; + goto err_stop; return 0; + +err_stop: + hid_hw_stop(hdev); err_free: kfree(drv_data); return ret; @@ -863,8 +866,7 @@ static void lg_remove(struct hid_device struct lg_drv_data *drv_data = hid_get_drvdata(hdev); if (drv_data->quirks & LG_FF4) lg4ff_deinit(hdev); - else - hid_hw_stop(hdev); + hid_hw_stop(hdev); kfree(drv_data); } Index: usb-devel/drivers/hid/hid-lg4ff.c =================================================================== --- usb-devel.orig/drivers/hid/hid-lg4ff.c +++ usb-devel/drivers/hid/hid-lg4ff.c @@ -1477,7 +1477,6 @@ int lg4ff_deinit(struct hid_device *hid) } } #endif - hid_hw_stop(hid); drv_data->device_props = NULL; kfree(entry);
The syzbot fuzzer found a general protection fault in the HID subsystem: kasan: CONFIG_KASAN_INLINE enabled kasan: GPF could be caused by NULL-ptr deref or user memory access general protection fault: 0000 [#1] SMP KASAN CPU: 0 PID: 3715 Comm: syz-executor.3 Not tainted 5.2.0-rc6+ #15 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 RIP: 0010:__pm_runtime_resume+0x49/0x180 drivers/base/power/runtime.c:1069 Code: ed 74 d5 fe 45 85 ed 0f 85 9a 00 00 00 e8 6f 73 d5 fe 48 8d bd c1 02 00 00 48 b8 00 00 00 00 00 fc ff df 48 89 fa 48 c1 ea 03 <0f> b6 04 02 48 89 fa 83 e2 07 38 d0 7f 08 84 c0 0f 85 fe 00 00 00 RSP: 0018:ffff8881d99d78e0 EFLAGS: 00010202 RAX: dffffc0000000000 RBX: 0000000000000020 RCX: ffffc90003f3f000 RDX: 0000000416d8686d RSI: ffffffff82676841 RDI: 00000020b6c3436a RBP: 00000020b6c340a9 R08: ffff8881c6d64800 R09: fffffbfff0e84c25 R10: ffff8881d99d7940 R11: ffffffff87426127 R12: 0000000000000004 R13: 0000000000000000 R14: ffff8881d9b94000 R15: ffffffff897f9048 FS: 00007f047f542700(0000) GS:ffff8881db200000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 0000001b30f21000 CR3: 00000001ca032000 CR4: 00000000001406f0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Call Trace: pm_runtime_get_sync include/linux/pm_runtime.h:226 [inline] usb_autopm_get_interface+0x1b/0x50 drivers/usb/core/driver.c:1707 usbhid_power+0x7c/0xe0 drivers/hid/usbhid/hid-core.c:1234 hid_hw_power include/linux/hid.h:1038 [inline] hidraw_open+0x20d/0x740 drivers/hid/hidraw.c:282 chrdev_open+0x219/0x5c0 fs/char_dev.c:413 do_dentry_open+0x497/0x1040 fs/open.c:778 do_last fs/namei.c:3416 [inline] path_openat+0x1430/0x3ff0 fs/namei.c:3533 do_filp_open+0x1a1/0x280 fs/namei.c:3563 do_sys_open+0x3c0/0x580 fs/open.c:1070 do_syscall_64+0xb7/0x560 arch/x86/entry/common.c:301 entry_SYSCALL_64_after_hwframe+0x49/0xbe It turns out the fault was caused by a bug in the HID Logitech driver, which violates the requirement that every pathway calling hid_hw_start() must also call hid_hw_stop(). This patch fixes the bug by making sure the requirement is met. Reported-and-tested-by: syzbot+3cbe5cd105d2ad56a1df@syzkaller.appspotmail.com Signed-off-by: Alan Stern <stern@rowland.harvard.edu> CC: <stable@vger.kernel.org> --- [as1909] drivers/hid/hid-lg.c | 10 ++++++---- drivers/hid/hid-lg4ff.c | 1 - 2 files changed, 6 insertions(+), 5 deletions(-)