* general protection fault in smsusb_init_device @ 2019-04-18 12:06 syzbot 2019-04-19 20:29 ` Alan Stern 2019-05-06 20:41 ` Alan Stern 0 siblings, 2 replies; 11+ messages in thread From: syzbot @ 2019-04-18 12:06 UTC (permalink / raw) To: andreyknvl, linux-kernel, linux-media, linux-usb, mchehab, syzkaller-bugs, wen.yang99 Hello, syzbot found the following crash on: HEAD commit: d34f9519 usb-fuzzer: main usb gadget fuzzer driver git tree: https://github.com/google/kasan/tree/usb-fuzzer console output: https://syzkaller.appspot.com/x/log.txt?x=128ec3fd200000 kernel config: https://syzkaller.appspot.com/x/.config?x=c73d1bb5aeaeae20 dashboard link: https://syzkaller.appspot.com/bug?extid=53f029db71c19a47325a compiler: gcc (GCC) 9.0.0 20181231 (experimental) syz repro: https://syzkaller.appspot.com/x/repro.syz?x=16138e67200000 C reproducer: https://syzkaller.appspot.com/x/repro.c?x=128dddbf200000 IMPORTANT: if you fix the bug, please add the following tag to the commit: Reported-by: syzbot+53f029db71c19a47325a@syzkaller.appspotmail.com usb 1-1: config 0 descriptor?? usb 1-1: string descriptor 0 read error: -71 smsusb:smsusb_probe: board id=18, interface number 0 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 PTI CPU: 1 PID: 22 Comm: kworker/1:1 Not tainted 5.1.0-rc5-319617-gd34f951 #4 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 Workqueue: usb_hub_wq hub_event RIP: 0010:smsusb_init_device+0x366/0x937 drivers/media/usb/siano/smsusb.c:429 Code: 48 c1 ea 03 80 3c 02 00 74 05 e8 24 1e 66 f7 4d 8b b6 f0 04 00 00 b8 ff ff 37 00 48 c1 e0 2a 49 8d 7e 04 48 89 fa 48 c1 ea 03 <8a> 14 02 48 89 f8 83 e0 07 ff c0 38 d0 7c 09 84 d2 74 05 e8 b1 1d RSP: 0018:ffff8880a86570d0 EFLAGS: 00010247 RAX: dffffc0000000000 RBX: ffff88809a81b300 RCX: ffffffff8a42b5b3 RDX: 0000000000000000 RSI: ffffffff8a42b6a3 RDI: 0000000000000004 RBP: ffff88808ca70000 R08: ffff8880a8503100 R09: ffff8880a8657130 R10: ffffed10150cae34 R11: ffff8880a86571a7 R12: ffff88809a81be54 R13: ffff88809a81be5c R14: 0000000000000000 R15: ffff88808ca70000 FS: 0000000000000000(0000) GS:ffff8880ad100000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00007f1ad259d000 CR3: 000000009a3aa000 CR4: 00000000001406e0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Call Trace: smsusb_probe+0xd64/0xe08 drivers/media/usb/siano/smsusb.c:570 usb_probe_interface+0x31d/0x820 drivers/usb/core/driver.c:361 really_probe+0x2da/0xb10 drivers/base/dd.c:509 driver_probe_device+0x21d/0x350 drivers/base/dd.c:671 __device_attach_driver+0x1d8/0x290 drivers/base/dd.c:778 bus_for_each_drv+0x163/0x1e0 drivers/base/bus.c:454 __device_attach+0x223/0x3a0 drivers/base/dd.c:844 bus_probe_device+0x1f1/0x2a0 drivers/base/bus.c:514 device_add+0xad2/0x16e0 drivers/base/core.c:2106 usb_set_configuration+0xdf7/0x1740 drivers/usb/core/message.c:2021 generic_probe+0xa2/0xda drivers/usb/core/generic.c:210 usb_probe_device+0xc0/0x150 drivers/usb/core/driver.c:266 really_probe+0x2da/0xb10 drivers/base/dd.c:509 driver_probe_device+0x21d/0x350 drivers/base/dd.c:671 __device_attach_driver+0x1d8/0x290 drivers/base/dd.c:778 bus_for_each_drv+0x163/0x1e0 drivers/base/bus.c:454 __device_attach+0x223/0x3a0 drivers/base/dd.c:844 bus_probe_device+0x1f1/0x2a0 drivers/base/bus.c:514 device_add+0xad2/0x16e0 drivers/base/core.c:2106 usb_new_device.cold+0x537/0xccf drivers/usb/core/hub.c:2534 hub_port_connect drivers/usb/core/hub.c:5089 [inline] hub_port_connect_change drivers/usb/core/hub.c:5204 [inline] port_event drivers/usb/core/hub.c:5350 [inline] hub_event+0x1398/0x3b00 drivers/usb/core/hub.c:5432 process_one_work+0x90f/0x1580 kernel/workqueue.c:2269 worker_thread+0x9b/0xe20 kernel/workqueue.c:2415 kthread+0x313/0x420 kernel/kthread.c:253 ret_from_fork+0x3a/0x50 arch/x86/entry/entry_64.S:352 Modules linked in: ---[ end trace 6175778b99b10882 ]--- RIP: 0010:smsusb_init_device+0x366/0x937 drivers/media/usb/siano/smsusb.c:429 Code: 48 c1 ea 03 80 3c 02 00 74 05 e8 24 1e 66 f7 4d 8b b6 f0 04 00 00 b8 ff ff 37 00 48 c1 e0 2a 49 8d 7e 04 48 89 fa 48 c1 ea 03 <8a> 14 02 48 89 f8 83 e0 07 ff c0 38 d0 7c 09 84 d2 74 05 e8 b1 1d RSP: 0018:ffff8880a86570d0 EFLAGS: 00010247 RAX: dffffc0000000000 RBX: ffff88809a81b300 RCX: ffffffff8a42b5b3 RDX: 0000000000000000 RSI: ffffffff8a42b6a3 RDI: 0000000000000004 RBP: ffff88808ca70000 R08: ffff8880a8503100 R09: ffff8880a8657130 R10: ffffed10150cae34 R11: ffff8880a86571a7 R12: ffff88809a81be54 R13: ffff88809a81be5c R14: 0000000000000000 R15: ffff88808ca70000 FS: 0000000000000000(0000) GS:ffff8880ad100000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00007f1ad259d000 CR3: 000000009a3aa000 CR4: 00000000001406e0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 --- This bug is generated by a bot. It may contain errors. See https://goo.gl/tpsmEJ for more information about syzbot. syzbot engineers can be reached at syzkaller@googlegroups.com. syzbot will keep track of this bug report. See: https://goo.gl/tpsmEJ#status for how to communicate with syzbot. syzbot can test patches for this bug, for details see: https://goo.gl/tpsmEJ#testing-patches ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: general protection fault in smsusb_init_device 2019-04-18 12:06 general protection fault in smsusb_init_device syzbot @ 2019-04-19 20:29 ` Alan Stern 2019-05-06 20:41 ` Alan Stern 1 sibling, 0 replies; 11+ messages in thread From: Alan Stern @ 2019-04-19 20:29 UTC (permalink / raw) To: syzbot, Mauro Carvalho Chehab Cc: andreyknvl, Kernel development list, linux-media, USB list, syzkaller-bugs, wen.yang99 On Thu, 18 Apr 2019, syzbot wrote: > Hello, > > syzbot found the following crash on: > > HEAD commit: d34f9519 usb-fuzzer: main usb gadget fuzzer driver > git tree: https://github.com/google/kasan/tree/usb-fuzzer > console output: https://syzkaller.appspot.com/x/log.txt?x=128ec3fd200000 > kernel config: https://syzkaller.appspot.com/x/.config?x=c73d1bb5aeaeae20 > dashboard link: https://syzkaller.appspot.com/bug?extid=53f029db71c19a47325a > compiler: gcc (GCC) 9.0.0 20181231 (experimental) > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=16138e67200000 > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=128dddbf200000 > > IMPORTANT: if you fix the bug, please add the following tag to the commit: > Reported-by: syzbot+53f029db71c19a47325a@syzkaller.appspotmail.com > > usb 1-1: config 0 descriptor?? > usb 1-1: string descriptor 0 read error: -71 > smsusb:smsusb_probe: board id=18, interface number 0 > 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 PTI > CPU: 1 PID: 22 Comm: kworker/1:1 Not tainted 5.1.0-rc5-319617-gd34f951 #4 > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS > Google 01/01/2011 > Workqueue: usb_hub_wq hub_event > RIP: 0010:smsusb_init_device+0x366/0x937 > drivers/media/usb/siano/smsusb.c:429 > Code: 48 c1 ea 03 80 3c 02 00 74 05 e8 24 1e 66 f7 4d 8b b6 f0 04 00 00 b8 > ff ff 37 00 48 c1 e0 2a 49 8d 7e 04 48 89 fa 48 c1 ea 03 <8a> 14 02 48 89 > f8 83 e0 07 ff c0 38 d0 7c 09 84 d2 74 05 e8 b1 1d > RSP: 0018:ffff8880a86570d0 EFLAGS: 00010247 > RAX: dffffc0000000000 RBX: ffff88809a81b300 RCX: ffffffff8a42b5b3 > RDX: 0000000000000000 RSI: ffffffff8a42b6a3 RDI: 0000000000000004 > RBP: ffff88808ca70000 R08: ffff8880a8503100 R09: ffff8880a8657130 > R10: ffffed10150cae34 R11: ffff8880a86571a7 R12: ffff88809a81be54 > R13: ffff88809a81be5c R14: 0000000000000000 R15: ffff88808ca70000 > FS: 0000000000000000(0000) GS:ffff8880ad100000(0000) knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: 00007f1ad259d000 CR3: 000000009a3aa000 CR4: 00000000001406e0 > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 > Call Trace: > smsusb_probe+0xd64/0xe08 drivers/media/usb/siano/smsusb.c:570 The reason for this bug is clear. The code in smsusb_probe() at line 429 does this: dev->response_alignment = le16_to_cpu(dev->udev->ep_in[1]->desc.wMaxPacketSize) - sizeof(struct sms_msg_hdr); which assumes there really is an ep1-IN endpoint. If there isn't, the code crashes. Testing that the endpoint exists is easy enough, but I'm not sure how this test should be integrated with the rest of the function. Someone who knows the code better ought to be able to do it with no trouble. Alan Stern ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: general protection fault in smsusb_init_device 2019-04-18 12:06 general protection fault in smsusb_init_device syzbot 2019-04-19 20:29 ` Alan Stern @ 2019-05-06 20:41 ` Alan Stern 2019-05-06 21:21 ` syzbot 2019-05-07 8:34 ` general protection fault in smsusb_init_device Johan Hovold 1 sibling, 2 replies; 11+ messages in thread From: Alan Stern @ 2019-05-06 20:41 UTC (permalink / raw) To: syzbot Cc: andreyknvl, linux-kernel, linux-media, linux-usb, mchehab, syzkaller-bugs, wen.yang99 On Thu, 18 Apr 2019, syzbot wrote: > Hello, > > syzbot found the following crash on: > > HEAD commit: d34f9519 usb-fuzzer: main usb gadget fuzzer driver > git tree: https://github.com/google/kasan/tree/usb-fuzzer > console output: https://syzkaller.appspot.com/x/log.txt?x=128ec3fd200000 > kernel config: https://syzkaller.appspot.com/x/.config?x=c73d1bb5aeaeae20 > dashboard link: https://syzkaller.appspot.com/bug?extid=53f029db71c19a47325a > compiler: gcc (GCC) 9.0.0 20181231 (experimental) > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=16138e67200000 > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=128dddbf200000 > > IMPORTANT: if you fix the bug, please add the following tag to the commit: > Reported-by: syzbot+53f029db71c19a47325a@syzkaller.appspotmail.com > > usb 1-1: config 0 descriptor?? > usb 1-1: string descriptor 0 read error: -71 > smsusb:smsusb_probe: board id=18, interface number 0 > 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 PTI > CPU: 1 PID: 22 Comm: kworker/1:1 Not tainted 5.1.0-rc5-319617-gd34f951 #4 > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS > Google 01/01/2011 > Workqueue: usb_hub_wq hub_event > RIP: 0010:smsusb_init_device+0x366/0x937 > drivers/media/usb/siano/smsusb.c:429 The driver assumes endpoint 1in exists, and doesn't check the existence of the endpoints it uses. Alan Stern #syz test: https://github.com/google/kasan.git usb-fuzzer drivers/media/usb/siano/smsusb.c | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-) Index: usb-devel/drivers/media/usb/siano/smsusb.c =================================================================== --- usb-devel.orig/drivers/media/usb/siano/smsusb.c +++ usb-devel/drivers/media/usb/siano/smsusb.c @@ -400,6 +400,7 @@ static int smsusb_init_device(struct usb struct smsusb_device_t *dev; void *mdev; int i, rc; + int in_maxp; /* create device object */ dev = kzalloc(sizeof(struct smsusb_device_t), GFP_KERNEL); @@ -411,6 +412,23 @@ static int smsusb_init_device(struct usb dev->udev = interface_to_usbdev(intf); dev->state = SMSUSB_DISCONNECTED; + for (i = 0; i < intf->cur_altsetting->desc.bNumEndpoints; i++) { + struct usb_endpoint_descriptor *desc = + &intf->cur_altsetting->endpoint[i].desc; + + if (desc->bEndpointAddress & USB_DIR_IN) { + dev->in_ep = desc->bEndpointAddress; + in_maxp = usb_endpoint_maxp(desc); + } else { + dev->out_ep = desc->bEndpointAddress; + } + } + + pr_debug("in_ep = %02x, out_ep = %02x\n", + dev->in_ep, dev->out_ep); + if (!dev->in_ep || !dev->out_ep) /* Missing endpoints? */ + return -EINVAL; + params.device_type = sms_get_board(board_id)->type; switch (params.device_type) { @@ -425,24 +443,12 @@ static int smsusb_init_device(struct usb /* fall-thru */ default: dev->buffer_size = USB2_BUFFER_SIZE; - dev->response_alignment = - le16_to_cpu(dev->udev->ep_in[1]->desc.wMaxPacketSize) - - sizeof(struct sms_msg_hdr); + dev->response_alignment = in_maxp - sizeof(struct sms_msg_hdr); params.flags |= SMS_DEVICE_FAMILY2; break; } - for (i = 0; i < intf->cur_altsetting->desc.bNumEndpoints; i++) { - if (intf->cur_altsetting->endpoint[i].desc. bEndpointAddress & USB_DIR_IN) - dev->in_ep = intf->cur_altsetting->endpoint[i].desc.bEndpointAddress; - else - dev->out_ep = intf->cur_altsetting->endpoint[i].desc.bEndpointAddress; - } - - pr_debug("in_ep = %02x, out_ep = %02x\n", - dev->in_ep, dev->out_ep); - params.device = &dev->udev->dev; params.usb_device = dev->udev; params.buffer_size = dev->buffer_size; ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: general protection fault in smsusb_init_device 2019-05-06 20:41 ` Alan Stern @ 2019-05-06 21:21 ` syzbot 2019-05-07 16:39 ` [PATCH] media: usb: siano: Fix general protection fault in smsusb Alan Stern 2019-05-07 8:34 ` general protection fault in smsusb_init_device Johan Hovold 1 sibling, 1 reply; 11+ messages in thread From: syzbot @ 2019-05-06 21:21 UTC (permalink / raw) To: andreyknvl, linux-kernel, linux-media, linux-usb, mchehab, stern, syzkaller-bugs, wen.yang99 Hello, syzbot has tested the proposed patch and the reproducer did not trigger crash: Reported-and-tested-by: syzbot+53f029db71c19a47325a@syzkaller.appspotmail.com Tested on: commit: 43151d6c usb-fuzzer: main usb gadget fuzzer driver git tree: https://github.com/google/kasan.git usb-fuzzer kernel config: https://syzkaller.appspot.com/x/.config?x=274aad0cf966c3bc compiler: gcc (GCC) 9.0.0 20181231 (experimental) patch: https://syzkaller.appspot.com/x/patch.diff?x=12c3fda4a00000 Note: testing is done by a robot and is best-effort only. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] media: usb: siano: Fix general protection fault in smsusb 2019-05-06 21:21 ` syzbot @ 2019-05-07 16:39 ` Alan Stern 2019-05-08 6:01 ` Johan Hovold 2019-05-24 13:35 ` Mauro Carvalho Chehab 0 siblings, 2 replies; 11+ messages in thread From: Alan Stern @ 2019-05-07 16:39 UTC (permalink / raw) To: mchehab Cc: andreyknvl, Kernel development list, linux-media, USB list, syzkaller-bugs, wen.yang99 The syzkaller USB fuzzer found a general-protection-fault bug in the smsusb part of the Siano DVB driver. The fault occurs during probe because the driver assumes without checking that the device has both IN and OUT endpoints and the IN endpoint is ep1. By slightly rearranging the driver's initialization code, we can make the appropriate checks early on and thus avoid the problem. If the expected endpoints aren't present, the new code safely returns -ENODEV from the probe routine. Signed-off-by: Alan Stern <stern@rowland.harvard.edu> Reported-and-tested-by: syzbot+53f029db71c19a47325a@syzkaller.appspotmail.com CC: <stable@vger.kernel.org> --- [as1897] drivers/media/usb/siano/smsusb.c | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-) Index: usb-devel/drivers/media/usb/siano/smsusb.c =================================================================== --- usb-devel.orig/drivers/media/usb/siano/smsusb.c +++ usb-devel/drivers/media/usb/siano/smsusb.c @@ -400,6 +400,7 @@ static int smsusb_init_device(struct usb struct smsusb_device_t *dev; void *mdev; int i, rc; + int in_maxp; /* create device object */ dev = kzalloc(sizeof(struct smsusb_device_t), GFP_KERNEL); @@ -411,6 +412,24 @@ static int smsusb_init_device(struct usb dev->udev = interface_to_usbdev(intf); dev->state = SMSUSB_DISCONNECTED; + for (i = 0; i < intf->cur_altsetting->desc.bNumEndpoints; i++) { + struct usb_endpoint_descriptor *desc = + &intf->cur_altsetting->endpoint[i].desc; + + if (desc->bEndpointAddress & USB_DIR_IN) { + dev->in_ep = desc->bEndpointAddress; + in_maxp = usb_endpoint_maxp(desc); + } else { + dev->out_ep = desc->bEndpointAddress; + } + } + + pr_debug("in_ep = %02x, out_ep = %02x\n", dev->in_ep, dev->out_ep); + if (!dev->in_ep || !dev->out_ep) { /* Missing endpoints? */ + smsusb_term_device(intf); + return -ENODEV; + } + params.device_type = sms_get_board(board_id)->type; switch (params.device_type) { @@ -425,24 +444,12 @@ static int smsusb_init_device(struct usb /* fall-thru */ default: dev->buffer_size = USB2_BUFFER_SIZE; - dev->response_alignment = - le16_to_cpu(dev->udev->ep_in[1]->desc.wMaxPacketSize) - - sizeof(struct sms_msg_hdr); + dev->response_alignment = in_maxp - sizeof(struct sms_msg_hdr); params.flags |= SMS_DEVICE_FAMILY2; break; } - for (i = 0; i < intf->cur_altsetting->desc.bNumEndpoints; i++) { - if (intf->cur_altsetting->endpoint[i].desc. bEndpointAddress & USB_DIR_IN) - dev->in_ep = intf->cur_altsetting->endpoint[i].desc.bEndpointAddress; - else - dev->out_ep = intf->cur_altsetting->endpoint[i].desc.bEndpointAddress; - } - - pr_debug("in_ep = %02x, out_ep = %02x\n", - dev->in_ep, dev->out_ep); - params.device = &dev->udev->dev; params.usb_device = dev->udev; params.buffer_size = dev->buffer_size; ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] media: usb: siano: Fix general protection fault in smsusb 2019-05-07 16:39 ` [PATCH] media: usb: siano: Fix general protection fault in smsusb Alan Stern @ 2019-05-08 6:01 ` Johan Hovold 2019-05-24 13:35 ` Mauro Carvalho Chehab 1 sibling, 0 replies; 11+ messages in thread From: Johan Hovold @ 2019-05-08 6:01 UTC (permalink / raw) To: Alan Stern Cc: mchehab, andreyknvl, Kernel development list, linux-media, USB list, syzkaller-bugs, wen.yang99 On Tue, May 07, 2019 at 12:39:47PM -0400, Alan Stern wrote: > The syzkaller USB fuzzer found a general-protection-fault bug in the > smsusb part of the Siano DVB driver. The fault occurs during probe > because the driver assumes without checking that the device has both > IN and OUT endpoints and the IN endpoint is ep1. > > By slightly rearranging the driver's initialization code, we can make > the appropriate checks early on and thus avoid the problem. If the > expected endpoints aren't present, the new code safely returns -ENODEV > from the probe routine. > > Signed-off-by: Alan Stern <stern@rowland.harvard.edu> > Reported-and-tested-by: syzbot+53f029db71c19a47325a@syzkaller.appspotmail.com > CC: <stable@vger.kernel.org> Reviewed-by: Johan Hovold <johan@kernel.org> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] media: usb: siano: Fix general protection fault in smsusb 2019-05-07 16:39 ` [PATCH] media: usb: siano: Fix general protection fault in smsusb Alan Stern 2019-05-08 6:01 ` Johan Hovold @ 2019-05-24 13:35 ` Mauro Carvalho Chehab 2019-05-24 13:54 ` Alan Stern 1 sibling, 1 reply; 11+ messages in thread From: Mauro Carvalho Chehab @ 2019-05-24 13:35 UTC (permalink / raw) To: Alan Stern Cc: andreyknvl, Kernel development list, linux-media, USB list, syzkaller-bugs, wen.yang99 Em Tue, 7 May 2019 12:39:47 -0400 (EDT) Alan Stern <stern@rowland.harvard.edu> escreveu: > The syzkaller USB fuzzer found a general-protection-fault bug in the > smsusb part of the Siano DVB driver. The fault occurs during probe > because the driver assumes without checking that the device has both > IN and OUT endpoints and the IN endpoint is ep1. > > By slightly rearranging the driver's initialization code, we can make > the appropriate checks early on and thus avoid the problem. If the > expected endpoints aren't present, the new code safely returns -ENODEV > from the probe routine. > > Signed-off-by: Alan Stern <stern@rowland.harvard.edu> > Reported-and-tested-by: syzbot+53f029db71c19a47325a@syzkaller.appspotmail.com > CC: <stable@vger.kernel.org> > > --- > > > [as1897] > > > drivers/media/usb/siano/smsusb.c | 33 ++++++++++++++++++++------------- > 1 file changed, 20 insertions(+), 13 deletions(-) > > Index: usb-devel/drivers/media/usb/siano/smsusb.c > =================================================================== > --- usb-devel.orig/drivers/media/usb/siano/smsusb.c > +++ usb-devel/drivers/media/usb/siano/smsusb.c > @@ -400,6 +400,7 @@ static int smsusb_init_device(struct usb > struct smsusb_device_t *dev; > void *mdev; > int i, rc; > + int in_maxp; > > /* create device object */ > dev = kzalloc(sizeof(struct smsusb_device_t), GFP_KERNEL); > @@ -411,6 +412,24 @@ static int smsusb_init_device(struct usb > dev->udev = interface_to_usbdev(intf); > dev->state = SMSUSB_DISCONNECTED; > > + for (i = 0; i < intf->cur_altsetting->desc.bNumEndpoints; i++) { > + struct usb_endpoint_descriptor *desc = > + &intf->cur_altsetting->endpoint[i].desc; > + > + if (desc->bEndpointAddress & USB_DIR_IN) { > + dev->in_ep = desc->bEndpointAddress; > + in_maxp = usb_endpoint_maxp(desc); > + } else { > + dev->out_ep = desc->bEndpointAddress; > + } > + } > + > + pr_debug("in_ep = %02x, out_ep = %02x\n", dev->in_ep, dev->out_ep); > + if (!dev->in_ep || !dev->out_ep) { /* Missing endpoints? */ > + smsusb_term_device(intf); > + return -ENODEV; > + } > + > params.device_type = sms_get_board(board_id)->type; > > switch (params.device_type) { > @@ -425,24 +444,12 @@ static int smsusb_init_device(struct usb > /* fall-thru */ > default: > dev->buffer_size = USB2_BUFFER_SIZE; > - dev->response_alignment = > - le16_to_cpu(dev->udev->ep_in[1]->desc.wMaxPacketSize) - > - sizeof(struct sms_msg_hdr); > + dev->response_alignment = in_maxp - sizeof(struct sms_msg_hdr); > > params.flags |= SMS_DEVICE_FAMILY2; > break; > } > > - for (i = 0; i < intf->cur_altsetting->desc.bNumEndpoints; i++) { > - if (intf->cur_altsetting->endpoint[i].desc. bEndpointAddress & USB_DIR_IN) > - dev->in_ep = intf->cur_altsetting->endpoint[i].desc.bEndpointAddress; > - else > - dev->out_ep = intf->cur_altsetting->endpoint[i].desc.bEndpointAddress; > - } > - > - pr_debug("in_ep = %02x, out_ep = %02x\n", > - dev->in_ep, dev->out_ep); > - > params.device = &dev->udev->dev; > params.usb_device = dev->udev; > params.buffer_size = dev->buffer_size; > Patch looks correct, and I'm applying it. It exposes another potential problem though: what happens if sizeof(desc.wMaxPacketSize) < sizeof(struct sms_msg_hdr)? I'm enclosing a followup patch that should solve this situation (and clean up a sparse warning). Thanks, Mauro [PATCH] media: smsusb: better handle optional alignment Most Siano devices require an alignment for the response. Changeset f3be52b0056a ("media: usb: siano: Fix general protection fault in smsusb") changed the logic with gets such aligment, but it now produces a sparce warning: drivers/media/usb/siano/smsusb.c: In function 'smsusb_init_device': drivers/media/usb/siano/smsusb.c:447:37: warning: 'in_maxp' may be used uninitialized in this function [-Wmaybe-uninitialized] 447 | dev->response_alignment = in_maxp - sizeof(struct sms_msg_hdr); | ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~ The sparse message itself is bogus, but a broken (or fake) USB eeprom could produce a negative value for response_alignment. So, change the code in order to check if the result is not negative. Fixes: f3be52b0056a ("media: usb: siano: Fix general protection fault in smsusb") CC: <stable@vger.kernel.org> Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> diff --git a/drivers/media/usb/siano/smsusb.c b/drivers/media/usb/siano/smsusb.c index 27ad14a3f831..e39f3f40dfdd 100644 --- a/drivers/media/usb/siano/smsusb.c +++ b/drivers/media/usb/siano/smsusb.c @@ -400,7 +400,7 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id) struct smsusb_device_t *dev; void *mdev; int i, rc; - int in_maxp; + int align = 0; /* create device object */ dev = kzalloc(sizeof(struct smsusb_device_t), GFP_KERNEL); @@ -418,14 +418,14 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id) if (desc->bEndpointAddress & USB_DIR_IN) { dev->in_ep = desc->bEndpointAddress; - in_maxp = usb_endpoint_maxp(desc); + align = usb_endpoint_maxp(desc) - sizeof(struct sms_msg_hdr); } else { dev->out_ep = desc->bEndpointAddress; } } pr_debug("in_ep = %02x, out_ep = %02x\n", dev->in_ep, dev->out_ep); - if (!dev->in_ep || !dev->out_ep) { /* Missing endpoints? */ + if (!dev->in_ep || !dev->out_ep || align < 0) { /* Missing endpoints? */ smsusb_term_device(intf); return -ENODEV; } @@ -444,7 +444,7 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id) /* fall-thru */ default: dev->buffer_size = USB2_BUFFER_SIZE; - dev->response_alignment = in_maxp - sizeof(struct sms_msg_hdr); + dev->response_alignment = align; params.flags |= SMS_DEVICE_FAMILY2; break; ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] media: usb: siano: Fix general protection fault in smsusb 2019-05-24 13:35 ` Mauro Carvalho Chehab @ 2019-05-24 13:54 ` Alan Stern 0 siblings, 0 replies; 11+ messages in thread From: Alan Stern @ 2019-05-24 13:54 UTC (permalink / raw) To: Mauro Carvalho Chehab, Greg KH Cc: andreyknvl, Kernel development list, linux-media, USB list, syzkaller-bugs, wen.yang99 On Fri, 24 May 2019, Mauro Carvalho Chehab wrote: > Em Tue, 7 May 2019 12:39:47 -0400 (EDT) > Alan Stern <stern@rowland.harvard.edu> escreveu: > > > The syzkaller USB fuzzer found a general-protection-fault bug in the > > smsusb part of the Siano DVB driver. The fault occurs during probe > > because the driver assumes without checking that the device has both > > IN and OUT endpoints and the IN endpoint is ep1. > > > > By slightly rearranging the driver's initialization code, we can make > > the appropriate checks early on and thus avoid the problem. If the > > expected endpoints aren't present, the new code safely returns -ENODEV > > from the probe routine. > > > > Signed-off-by: Alan Stern <stern@rowland.harvard.edu> > > Reported-and-tested-by: syzbot+53f029db71c19a47325a@syzkaller.appspotmail.com > > CC: <stable@vger.kernel.org> > Patch looks correct, and I'm applying it. It exposes another potential > problem though: what happens if sizeof(desc.wMaxPacketSize) < sizeof(struct sms_msg_hdr)? > > I'm enclosing a followup patch that should solve this situation > (and clean up a sparse warning). > > Thanks, > Mauro Your points are well taken. However, Greg KH has already taken the original patch and a fix for the sparse warning into his tree. I guess the two of you should figure out how best to straighten this out. Alan Stern ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: general protection fault in smsusb_init_device 2019-05-06 20:41 ` Alan Stern 2019-05-06 21:21 ` syzbot @ 2019-05-07 8:34 ` Johan Hovold 2019-05-07 14:42 ` Alan Stern 1 sibling, 1 reply; 11+ messages in thread From: Johan Hovold @ 2019-05-07 8:34 UTC (permalink / raw) To: Alan Stern Cc: syzbot, andreyknvl, linux-kernel, linux-media, linux-usb, mchehab, syzkaller-bugs, wen.yang99 On Mon, May 06, 2019 at 04:41:41PM -0400, Alan Stern wrote: > On Thu, 18 Apr 2019, syzbot wrote: > > > Hello, > > > > syzbot found the following crash on: > > > > HEAD commit: d34f9519 usb-fuzzer: main usb gadget fuzzer driver > > git tree: https://github.com/google/kasan/tree/usb-fuzzer > > console output: https://syzkaller.appspot.com/x/log.txt?x=128ec3fd200000 > > kernel config: https://syzkaller.appspot.com/x/.config?x=c73d1bb5aeaeae20 > > dashboard link: https://syzkaller.appspot.com/bug?extid=53f029db71c19a47325a > > compiler: gcc (GCC) 9.0.0 20181231 (experimental) > > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=16138e67200000 > > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=128dddbf200000 > > > > IMPORTANT: if you fix the bug, please add the following tag to the commit: > > Reported-by: syzbot+53f029db71c19a47325a@syzkaller.appspotmail.com > > > > usb 1-1: config 0 descriptor?? > > usb 1-1: string descriptor 0 read error: -71 > > smsusb:smsusb_probe: board id=18, interface number 0 > > 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 PTI > > CPU: 1 PID: 22 Comm: kworker/1:1 Not tainted 5.1.0-rc5-319617-gd34f951 #4 > > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS > > Google 01/01/2011 > > Workqueue: usb_hub_wq hub_event > > RIP: 0010:smsusb_init_device+0x366/0x937 > > drivers/media/usb/siano/smsusb.c:429 > > The driver assumes endpoint 1in exists, and doesn't check the existence > of the endpoints it uses. > > Alan Stern > > > #syz test: https://github.com/google/kasan.git usb-fuzzer > > drivers/media/usb/siano/smsusb.c | 32 +++++++++++++++++++------------- > 1 file changed, 19 insertions(+), 13 deletions(-) > > Index: usb-devel/drivers/media/usb/siano/smsusb.c > =================================================================== > --- usb-devel.orig/drivers/media/usb/siano/smsusb.c > +++ usb-devel/drivers/media/usb/siano/smsusb.c > @@ -400,6 +400,7 @@ static int smsusb_init_device(struct usb > struct smsusb_device_t *dev; > void *mdev; > int i, rc; > + int in_maxp; > > /* create device object */ > dev = kzalloc(sizeof(struct smsusb_device_t), GFP_KERNEL); > @@ -411,6 +412,23 @@ static int smsusb_init_device(struct usb > dev->udev = interface_to_usbdev(intf); > dev->state = SMSUSB_DISCONNECTED; > > + for (i = 0; i < intf->cur_altsetting->desc.bNumEndpoints; i++) { > + struct usb_endpoint_descriptor *desc = > + &intf->cur_altsetting->endpoint[i].desc; > + > + if (desc->bEndpointAddress & USB_DIR_IN) { > + dev->in_ep = desc->bEndpointAddress; > + in_maxp = usb_endpoint_maxp(desc); > + } else { > + dev->out_ep = desc->bEndpointAddress; > + } > + } > + > + pr_debug("in_ep = %02x, out_ep = %02x\n", > + dev->in_ep, dev->out_ep); > + if (!dev->in_ep || !dev->out_ep) /* Missing endpoints? */ > + return -EINVAL; Looks like you're now leaking dev here, and so is the current code in the later error paths. Since this return value will be returned from probe, you may want to use -ENXIO or -ENODEV instead of -EINVAL. Looks good otherwise. Johan ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: general protection fault in smsusb_init_device 2019-05-07 8:34 ` general protection fault in smsusb_init_device Johan Hovold @ 2019-05-07 14:42 ` Alan Stern 2019-05-07 15:07 ` Johan Hovold 0 siblings, 1 reply; 11+ messages in thread From: Alan Stern @ 2019-05-07 14:42 UTC (permalink / raw) To: Johan Hovold Cc: syzbot, andreyknvl, linux-kernel, linux-media, linux-usb, mchehab, syzkaller-bugs, wen.yang99 On Tue, 7 May 2019, Johan Hovold wrote: > On Mon, May 06, 2019 at 04:41:41PM -0400, Alan Stern wrote: > > On Thu, 18 Apr 2019, syzbot wrote: > > > > > Hello, > > > > > > syzbot found the following crash on: > > > > > > HEAD commit: d34f9519 usb-fuzzer: main usb gadget fuzzer driver > > > git tree: https://github.com/google/kasan/tree/usb-fuzzer > > > console output: https://syzkaller.appspot.com/x/log.txt?x=128ec3fd200000 > > > kernel config: https://syzkaller.appspot.com/x/.config?x=c73d1bb5aeaeae20 > > > dashboard link: https://syzkaller.appspot.com/bug?extid=53f029db71c19a47325a > > > compiler: gcc (GCC) 9.0.0 20181231 (experimental) > > > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=16138e67200000 > > > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=128dddbf200000 > > > > > > IMPORTANT: if you fix the bug, please add the following tag to the commit: > > > Reported-by: syzbot+53f029db71c19a47325a@syzkaller.appspotmail.com > > > > > > usb 1-1: config 0 descriptor?? > > > usb 1-1: string descriptor 0 read error: -71 > > > smsusb:smsusb_probe: board id=18, interface number 0 > > > 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 PTI > > > CPU: 1 PID: 22 Comm: kworker/1:1 Not tainted 5.1.0-rc5-319617-gd34f951 #4 > > > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS > > > Google 01/01/2011 > > > Workqueue: usb_hub_wq hub_event > > > RIP: 0010:smsusb_init_device+0x366/0x937 > > > drivers/media/usb/siano/smsusb.c:429 > > > > The driver assumes endpoint 1in exists, and doesn't check the existence > > of the endpoints it uses. > > > > Alan Stern > > > > > > #syz test: https://github.com/google/kasan.git usb-fuzzer > > > > drivers/media/usb/siano/smsusb.c | 32 +++++++++++++++++++------------- > > 1 file changed, 19 insertions(+), 13 deletions(-) > > > > Index: usb-devel/drivers/media/usb/siano/smsusb.c > > =================================================================== > > --- usb-devel.orig/drivers/media/usb/siano/smsusb.c > > +++ usb-devel/drivers/media/usb/siano/smsusb.c > > @@ -400,6 +400,7 @@ static int smsusb_init_device(struct usb > > struct smsusb_device_t *dev; > > void *mdev; > > int i, rc; > > + int in_maxp; > > > > /* create device object */ > > dev = kzalloc(sizeof(struct smsusb_device_t), GFP_KERNEL); > > @@ -411,6 +412,23 @@ static int smsusb_init_device(struct usb > > dev->udev = interface_to_usbdev(intf); > > dev->state = SMSUSB_DISCONNECTED; > > > > + for (i = 0; i < intf->cur_altsetting->desc.bNumEndpoints; i++) { > > + struct usb_endpoint_descriptor *desc = > > + &intf->cur_altsetting->endpoint[i].desc; > > + > > + if (desc->bEndpointAddress & USB_DIR_IN) { > > + dev->in_ep = desc->bEndpointAddress; > > + in_maxp = usb_endpoint_maxp(desc); > > + } else { > > + dev->out_ep = desc->bEndpointAddress; > > + } > > + } > > + > > + pr_debug("in_ep = %02x, out_ep = %02x\n", > > + dev->in_ep, dev->out_ep); > > + if (!dev->in_ep || !dev->out_ep) /* Missing endpoints? */ > > + return -EINVAL; > > Looks like you're now leaking dev here, and so is the current code in > the later error paths. > > Since this return value will be returned from probe, you may want to use > -ENXIO or -ENODEV instead of -EINVAL. > > Looks good otherwise. Thanks for the review. You're right about the memory leak (although you're wrong about the later error paths: smsusb_term_device() deallocates dev). And -ENODEV does seem like a better return code. I'll update the patch as you suggest. Alan Stern ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: general protection fault in smsusb_init_device 2019-05-07 14:42 ` Alan Stern @ 2019-05-07 15:07 ` Johan Hovold 0 siblings, 0 replies; 11+ messages in thread From: Johan Hovold @ 2019-05-07 15:07 UTC (permalink / raw) To: Alan Stern Cc: Johan Hovold, syzbot, andreyknvl, linux-kernel, linux-media, linux-usb, mchehab, syzkaller-bugs, wen.yang99 On Tue, May 07, 2019 at 10:42:58AM -0400, Alan Stern wrote: > On Tue, 7 May 2019, Johan Hovold wrote: > > > @@ -411,6 +412,23 @@ static int smsusb_init_device(struct usb > > > dev->udev = interface_to_usbdev(intf); > > > dev->state = SMSUSB_DISCONNECTED; > > > > > > + for (i = 0; i < intf->cur_altsetting->desc.bNumEndpoints; i++) { > > > + struct usb_endpoint_descriptor *desc = > > > + &intf->cur_altsetting->endpoint[i].desc; > > > + > > > + if (desc->bEndpointAddress & USB_DIR_IN) { > > > + dev->in_ep = desc->bEndpointAddress; > > > + in_maxp = usb_endpoint_maxp(desc); > > > + } else { > > > + dev->out_ep = desc->bEndpointAddress; > > > + } > > > + } > > > + > > > + pr_debug("in_ep = %02x, out_ep = %02x\n", > > > + dev->in_ep, dev->out_ep); > > > + if (!dev->in_ep || !dev->out_ep) /* Missing endpoints? */ > > > + return -EINVAL; > > > > Looks like you're now leaking dev here, and so is the current code in > > the later error paths. > > > > Since this return value will be returned from probe, you may want to use > > -ENXIO or -ENODEV instead of -EINVAL. > > > > Looks good otherwise. > > Thanks for the review. You're right about the memory leak (although > you're wrong about the later error paths: smsusb_term_device() > deallocates dev). Indeed, I missed the free in smsusb_term_device(). Sorry about that. Johan ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-05-24 13:54 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-04-18 12:06 general protection fault in smsusb_init_device syzbot 2019-04-19 20:29 ` Alan Stern 2019-05-06 20:41 ` Alan Stern 2019-05-06 21:21 ` syzbot 2019-05-07 16:39 ` [PATCH] media: usb: siano: Fix general protection fault in smsusb Alan Stern 2019-05-08 6:01 ` Johan Hovold 2019-05-24 13:35 ` Mauro Carvalho Chehab 2019-05-24 13:54 ` Alan Stern 2019-05-07 8:34 ` general protection fault in smsusb_init_device Johan Hovold 2019-05-07 14:42 ` Alan Stern 2019-05-07 15:07 ` Johan Hovold
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).