| Message ID | 20191108154838.21487-1-will@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RESEND,RESEND] media: uvc: Avoid cyclic entity chains due to malformed USB descriptors | expand |
Hi Will, Thank you for the patch. I'm sorry for the delay, and will have to ask you to be a bit more patient I'm afraid. I will leave tomorrow for a week without computer access and will only be able to go through my backlog when I will be back on the 17th. On Fri, Nov 08, 2019 at 03:48:38PM +0000, Will Deacon wrote: > Way back in 2017, fuzzing the 4.14-rc2 USB stack with syzkaller kicked > up the following WARNING from the UVC chain scanning code: > > | list_add double add: new=ffff880069084010, prev=ffff880069084010, > | next=ffff880067d22298. > | ------------[ cut here ]------------ > | WARNING: CPU: 1 PID: 1846 at lib/list_debug.c:31 __list_add_valid+0xbd/0xf0 > | Modules linked in: > | CPU: 1 PID: 1846 Comm: kworker/1:2 Not tainted > | 4.14.0-rc2-42613-g1488251d1a98 #238 > | Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 > | Workqueue: usb_hub_wq hub_event > | task: ffff88006b01ca40 task.stack: ffff880064358000 > | RIP: 0010:__list_add_valid+0xbd/0xf0 lib/list_debug.c:29 > | RSP: 0018:ffff88006435ddd0 EFLAGS: 00010286 > | RAX: 0000000000000058 RBX: ffff880067d22298 RCX: 0000000000000000 > | RDX: 0000000000000058 RSI: ffffffff85a58800 RDI: ffffed000c86bbac > | RBP: ffff88006435dde8 R08: 1ffff1000c86ba52 R09: 0000000000000000 > | R10: 0000000000000002 R11: 0000000000000000 R12: ffff880069084010 > | R13: ffff880067d22298 R14: ffff880069084010 R15: ffff880067d222a0 > | FS: 0000000000000000(0000) GS:ffff88006c900000(0000) knlGS:0000000000000000 > | CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > | CR2: 0000000020004ff2 CR3: 000000006b447000 CR4: 00000000000006e0 > | Call Trace: > | __list_add ./include/linux/list.h:59 > | list_add_tail+0x8c/0x1b0 ./include/linux/list.h:92 > | uvc_scan_chain_forward.isra.8+0x373/0x416 > | drivers/media/usb/uvc/uvc_driver.c:1471 > | uvc_scan_chain drivers/media/usb/uvc/uvc_driver.c:1585 > | uvc_scan_device drivers/media/usb/uvc/uvc_driver.c:1769 > | uvc_probe+0x77f2/0x8f00 drivers/media/usb/uvc/uvc_driver.c:2104 > > Looking into the output from usbmon, the interesting part is the > following data packet: > > ffff880069c63e00 30710169 C Ci:1:002:0 0 143 = 09028f00 01030080 > 00090403 00000e01 00000924 03000103 7c003328 010204db > > If we drop the lead configuration and interface descriptors, we're left > with an output terminal descriptor describing a generic display: > > /* Output terminal descriptor */ > buf[0] 09 > buf[1] 24 > buf[2] 03 /* UVC_VC_OUTPUT_TERMINAL */ > buf[3] 00 /* ID */ > buf[4] 01 /* type == 0x0301 (UVC_OTT_DISPLAY) */ > buf[5] 03 > buf[6] 7c > buf[7] 00 /* source ID refers to self! */ > buf[8] 33 > > The problem with this descriptor is that it is self-referential: the > source ID of 0 matches itself! This causes the 'struct uvc_entity' > representing the display to be added to its chain list twice during > 'uvc_scan_chain()': once via 'uvc_scan_chain_entity()' when it is > processed directly from the 'dev->entities' list and then again > immediately afterwards when trying to follow the source ID in > 'uvc_scan_chain_forward()' > > Add a check before adding an entity to a chain list to ensure that the > entity is not already part of a chain. > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: Mauro Carvalho Chehab <mchehab@kernel.org> > Cc: Dmitry Vyukov <dvyukov@google.com> > Cc: Kostya Serebryany <kcc@google.com> > Cc: <stable@vger.kernel.org> > Fixes: c0efd232929c ("V4L/DVB (8145a): USB Video Class driver") > Reported-by: Andrey Konovalov <andreyknvl@google.com> > Link: https://lore.kernel.org/linux-media/CAAeHK+z+Si69jUR+N-SjN9q4O+o5KFiNManqEa-PjUta7EOb7A@mail.gmail.com/ > Signed-off-by: Will Deacon <will@kernel.org> > --- > > That's right, it's the same patch again! No changes since either of: > > http://lkml.kernel.org/r/20191002112753.21630-1-will@kernel.org > https://lore.kernel.org/lkml/20191016195800.22099-1-will@kernel.org > > Please consider merging. > > drivers/media/usb/uvc/uvc_driver.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c > index 66ee168ddc7e..e24420b1750a 100644 > --- a/drivers/media/usb/uvc/uvc_driver.c > +++ b/drivers/media/usb/uvc/uvc_driver.c > @@ -1493,6 +1493,11 @@ static int uvc_scan_chain_forward(struct uvc_video_chain *chain, > break; > if (forward == prev) > continue; > + if (forward->chain.next || forward->chain.prev) { > + uvc_trace(UVC_TRACE_DESCR, "Found reference to " > + "entity %d already in chain.\n", forward->id); > + return -EINVAL; > + } > > switch (UVC_ENTITY_TYPE(forward)) { > case UVC_VC_EXTENSION_UNIT: > @@ -1574,6 +1579,13 @@ static int uvc_scan_chain_backward(struct uvc_video_chain *chain, > return -1; > } > > + if (term->chain.next || term->chain.prev) { > + uvc_trace(UVC_TRACE_DESCR, "Found reference to " > + "entity %d already in chain.\n", > + term->id); > + return -EINVAL; > + } > + > if (uvc_trace_param & UVC_TRACE_PROBE) > printk(KERN_CONT " %d", term->id); > > -- > 2.24.0.rc1.363.gb1bccd3e3d-goog >
Hi Laurent, On Fri, Nov 08, 2019 at 05:55:03PM +0200, Laurent Pinchart wrote: > I'm sorry for the delay, and will have to ask you to be a bit more > patient I'm afraid. I will leave tomorrow for a week without computer > access and will only be able to go through my backlog when I will be > back on the 17th. Ok, thanks for letting me know. I'll poke you again when you're back if I don't hear anything -- I haven't actually changed the patch for ages, since I don't think it needs further work [1]. Will [1] https://lore.kernel.org/linux-media/20191007162709.3vrtbcpoymmnqikl@willie-the-truck/
Hi Laurent, On Fri, Nov 08, 2019 at 05:55:03PM +0200, Laurent Pinchart wrote: > Thank you for the patch. > > I'm sorry for the delay, and will have to ask you to be a bit more > patient I'm afraid. I will leave tomorrow for a week without computer > access and will only be able to go through my backlog when I will be > back on the 17th. Gentle reminder on this, now you've been back a month ;) Will
On Mon, Dec 16, 2019 at 1:16 PM Will Deacon <will@kernel.org> wrote: > > Hi Laurent, > > On Fri, Nov 08, 2019 at 05:55:03PM +0200, Laurent Pinchart wrote: > > Thank you for the patch. > > > > I'm sorry for the delay, and will have to ask you to be a bit more > > patient I'm afraid. I will leave tomorrow for a week without computer > > access and will only be able to go through my backlog when I will be > > back on the 17th. > > Gentle reminder on this, now you've been back a month ;) Hi Will, I think we now have a reproducer for this issue that syzbot just reported: https://syzkaller.appspot.com/bug?extid=0a5c96772a9b26f2a876 You can try you patch on it :) There's also another one, which looks related: https://syzkaller.appspot.com/bug?extid=0b0095300dfeb8a83dc8 Thanks!
On Mon, Dec 16, 2019 at 02:17:52PM +0100, Andrey Konovalov wrote: > On Mon, Dec 16, 2019 at 1:16 PM Will Deacon <will@kernel.org> wrote: > > On Fri, Nov 08, 2019 at 05:55:03PM +0200, Laurent Pinchart wrote: > > > Thank you for the patch. > > > > > > I'm sorry for the delay, and will have to ask you to be a bit more > > > patient I'm afraid. I will leave tomorrow for a week without computer > > > access and will only be able to go through my backlog when I will be > > > back on the 17th. > > > > Gentle reminder on this, now you've been back a month ;) > > I think we now have a reproducer for this issue that syzbot just reported: > > https://syzkaller.appspot.com/bug?extid=0a5c96772a9b26f2a876 > > You can try you patch on it :) Oh wow, I *really* like the raw USB gadget thingy you have to reproduce these! I also really like that this patch fixes the issue. Logs below. Laurent -- can we please merge this now? Will --->8 Before: bash-5.0# ./repro [ 31.749418][ T92] usb 1-1: new high-speed USB device number 2 using dummy_hcd [ 31.989356][ T92] usb 1-1: Using ep0 maxpacket: 8 [ 32.109448][ T92] usb 1-1: config index 0 descriptor too short (expected 51150, got 70) [ 32.111898][ T92] usb 1-1: config 0 contains an unexpected descriptor of type 0x2, skipping [ 32.114317][ T92] usb 1-1: config 0 has an invalid descriptor of length 0, skipping remainder of the config [ 32.117145][ T92] usb 1-1: config 0 interface 0 altsetting 0 has 0 endpoint descriptors, different from the interface descriptor's value: 16 [ 32.120554][ T92] usb 1-1: New USB device found, idVendor=0bd3, idProduct=0755, bcdDevice=69.f1 [ 32.122875][ T92] usb 1-1: New USB device strings: Mfr=0, Product=0, SerialNumber=0 [ 32.126602][ T92] usb 1-1: config 0 descriptor?? [ 32.399436][ T92] usb 1-1: string descriptor 0 read error: -71 [ 32.401266][ T92] uvcvideo: Found UVC 0.00 device <unnamed> (0bd3:0755) [ 32.403266][ T92] ------------[ cut here ]------------ [ 32.404790][ T92] list_add double add: new=ffff888015992010, prev=ffff888015992010, next=ffff8880146c6a18. [ 32.407819][ T92] WARNING: CPU: 2 PID: 92 at lib/list_debug.c:31 __list_add_valid+0xab/0xe0 [ 32.410214][ T92] Modules linked in: [ 32.411071][ T92] CPU: 2 PID: 92 Comm: kworker/2:1 Not tainted 5.5.0-rc2+ #1 [ 32.412432][ T92] Workqueue: usb_hub_wq hub_event [ 32.413364][ T92] RIP: 0010:__list_add_valid+0xab/0xe0 [ 32.414382][ T92] Code: 48 c7 c7 a0 ae fa 85 48 89 de e8 19 eb 2a ff 0f 0b 31 c0 eb cc 48 89 f2 48 89 d9 48 89 ee 48 c7 c7 20 af fa 85 e8 fe ea 2a ff <0f> 0b 31 c0 eb b1 48 89 34 24 e8 36 e8 7e ff 48 8b 34 24 e9 68 ff [ 32.418007][ T92] RSP: 0018:ffff8880158d7008 EFLAGS: 00010286 [ 32.419127][ T92] RAX: 0000000000000000 RBX: ffff8880146c6a18 RCX: ffffffff81293978 [ 32.420589][ T92] RDX: 0000000000000000 RSI: ffffffff812990fc RDI: 0000000000000006 [ 32.421692][ T92] RBP: ffff888015992010 R08: ffff88801551de80 R09: fffffbfff11ea4b5 [ 32.422744][ T92] R10: fffffbfff11ea4b4 R11: ffffffff88f525a7 R12: dffffc0000000000 [ 32.423784][ T92] R13: ffff888015992000 R14: ffff8880146c6a20 R15: ffff8880146c6a18 [ 32.424838][ T92] FS: 0000000000000000(0000) GS:ffff888016800000(0000) knlGS:0000000000000000 [ 32.425996][ T92] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 32.426867][ T92] CR2: 0000000000478f10 CR3: 000000001327e005 CR4: 0000000000760ea0 [ 32.427935][ T92] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 [ 32.428972][ T92] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 [ 32.430012][ T92] PKRU: 55555554 [ 32.430473][ T92] Call Trace: [ 32.430944][ T92] uvc_scan_chain_forward.isra.9+0x4df/0x635 [ 32.431600][ T92] uvc_probe.cold.19+0x1ef2/0x29bc [ 32.432175][ T92] ? __lock_acquire+0xeda/0x41a0 [ 32.432712][ T92] ? mark_lock+0xbe/0x10f0 [ 32.433209][ T92] ? pm_runtime_enable+0x2a/0x310 [ 32.433773][ T92] ? find_held_lock+0x33/0x1c0 [ 32.434307][ T92] ? usb_probe_interface+0x307/0x7b0 [ 32.434869][ T92] usb_probe_interface+0x307/0x7b0 [ 32.435414][ T92] ? usb_probe_device+0xf0/0xf0 [ 32.435938][ T92] really_probe+0x281/0x700 [ 32.436424][ T92] ? driver_allows_async_probing+0x150/0x150 [ 32.437065][ T92] driver_probe_device+0x105/0x200 [ 32.437611][ T92] __device_attach_driver+0x1b9/0x230 [ 32.438190][ T92] bus_for_each_drv+0x156/0x1d0 [ 32.438708][ T92] ? bus_rescan_devices+0x20/0x20 [ 32.439248][ T92] ? lockdep_hardirqs_on+0x388/0x570 [ 32.439812][ T92] __device_attach+0x20b/0x350 [ 32.440323][ T92] ? device_bind_driver+0xc0/0xc0 [ 32.440870][ T92] bus_probe_device+0x1e5/0x290 [ 32.441386][ T92] device_add+0x1420/0x1b90 [ 32.441887][ T92] ? wait_for_completion+0x3c0/0x3c0 [ 32.442466][ T92] ? device_link_remove+0x150/0x150 [ 32.443037][ T92] usb_set_configuration+0xd6f/0x1750 [ 32.443633][ T92] generic_probe+0x95/0xcd [ 32.444146][ T92] usb_probe_device+0x97/0xf0 [ 32.444650][ T92] ? usb_suspend+0x630/0x630 [ 32.445151][ T92] really_probe+0x281/0x700 [ 32.445642][ T92] ? driver_allows_async_probing+0x150/0x150 [ 32.446299][ T92] driver_probe_device+0x105/0x200 [ 32.446857][ T92] __device_attach_driver+0x1b9/0x230 [ 32.447448][ T92] bus_for_each_drv+0x156/0x1d0 [ 32.447981][ T92] ? bus_rescan_devices+0x20/0x20 [ 32.448523][ T92] ? lockdep_hardirqs_on+0x388/0x570 [ 32.449095][ T92] __device_attach+0x20b/0x350 [ 32.449612][ T92] ? device_bind_driver+0xc0/0xc0 [ 32.450167][ T92] bus_probe_device+0x1e5/0x290 [ 32.450686][ T92] device_add+0x1420/0x1b90 [ 32.451164][ T92] ? device_link_remove+0x150/0x150 [ 32.451715][ T92] ? _raw_spin_unlock_irq+0x1f/0x30 [ 32.452267][ T92] usb_new_device.cold.65+0x66e/0xe63 [ 32.452835][ T92] hub_event+0x1ebd/0x3810 [ 32.453300][ T92] ? hub_port_debounce+0x270/0x270 [ 32.453837][ T92] ? __lock_acquire+0xeda/0x41a0 [ 32.454389][ T92] ? find_held_lock+0x33/0x1c0 [ 32.454904][ T92] ? process_one_work+0x8fc/0x1720 [ 32.455445][ T92] ? mark_held_locks+0x110/0x110 [ 32.455954][ T92] ? rcu_read_lock_sched_held+0x9c/0xd0 [ 32.456536][ T92] ? rcu_read_lock_bh_held+0xb0/0xb0 [ 32.457093][ T92] process_one_work+0x9f2/0x1720 [ 32.457616][ T92] ? mark_held_locks+0x110/0x110 [ 32.458138][ T92] ? pwq_dec_nr_in_flight+0x310/0x310 [ 32.458701][ T92] ? do_raw_spin_lock+0x11b/0x280 [ 32.459237][ T92] worker_thread+0x8c/0xd10 [ 32.459715][ T92] ? process_one_work+0x1720/0x1720 [ 32.460266][ T92] kthread+0x352/0x420 [ 32.460702][ T92] ? kthread_create_on_node+0xe0/0xe0 [ 32.461275][ T92] ret_from_fork+0x24/0x30 [ 32.461738][ T92] irq event stamp: 2238 [ 32.462183][ T92] hardirqs last enabled at (2237): [<ffffffff81293b92>] console_unlock+0x8f2/0xc40 [ 32.463174][ T92] hardirqs last disabled at (2238): [<ffffffff8100468d>] trace_hardirqs_off_thunk+0x1a/0x1c [ 32.464244][ T92] softirqs last enabled at (1196): [<ffffffff85c00643>] __do_softirq+0x643/0x8fc [ 32.465225][ T92] softirqs last disabled at (1187): [<ffffffff8115a035>] irq_exit+0x175/0x1a0 [ 32.466155][ T92] ---[ end trace ef28d8c60b68a46d ]--- [ 32.466781][ T92] uvcvideo: No valid video chain found. [ 32.468076][ T92] usb 1-1: USB disconnect, device number 2 After: bash-5.0# ./repro [ 19.067221][ T92] usb 1-1: new high-speed USB device number 2 using dummy_hcd [ 19.307154][ T92] usb 1-1: Using ep0 maxpacket: 8 [ 19.427261][ T92] usb 1-1: config index 0 descriptor too short (expected 51150, got 70) [ 19.429709][ T92] usb 1-1: config 0 contains an unexpected descriptor of type 0x2, skipping [ 19.432150][ T92] usb 1-1: config 0 has an invalid descriptor of length 0, skipping remainder of the config [ 19.435003][ T92] usb 1-1: config 0 interface 0 altsetting 0 has 0 endpoint descriptors, different from the interface descriptor's value: 16 [ 19.438655][ T92] usb 1-1: New USB device found, idVendor=0bd3, idProduct=0755, bcdDevice=69.f1 [ 19.441166][ T92] usb 1-1: New USB device strings: Mfr=0, Product=0, SerialNumber=0 [ 19.445163][ T92] usb 1-1: config 0 descriptor?? [ 19.717195][ T92] usb 1-1: string descriptor 0 read error: -71 [ 19.719038][ T92] uvcvideo: Found UVC 0.00 device <unnamed> (0bd3:0755) [ 19.721087][ T92] uvcvideo: No valid video chain found. [ 19.725262][ T92] usb 1-1: USB disconnect, device number 2
On Wed, Dec 18, 2019 at 11:41:38AM +0000, Will Deacon wrote: > On Mon, Dec 16, 2019 at 02:17:52PM +0100, Andrey Konovalov wrote: > > On Mon, Dec 16, 2019 at 1:16 PM Will Deacon <will@kernel.org> wrote: > > > On Fri, Nov 08, 2019 at 05:55:03PM +0200, Laurent Pinchart wrote: > > > > Thank you for the patch. > > > > > > > > I'm sorry for the delay, and will have to ask you to be a bit more > > > > patient I'm afraid. I will leave tomorrow for a week without computer > > > > access and will only be able to go through my backlog when I will be > > > > back on the 17th. > > > > > > Gentle reminder on this, now you've been back a month ;) > > > > I think we now have a reproducer for this issue that syzbot just reported: > > > > https://syzkaller.appspot.com/bug?extid=0a5c96772a9b26f2a876 > > > > You can try you patch on it :) > > Oh wow, I *really* like the raw USB gadget thingy you have to reproduce > these! I also really like that this patch fixes the issue. Logs below. Ok, that's a good poke for me to go review that raw gadget code to see if it can be merged upstream :) > Laurent -- can we please merge this now? Yes, that would be good to have, as this obviously fixes a problem, and I can take it off of my "patches to track" list.... thanks, greg k-h
On Wed, Dec 18, 2019 at 1:23 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Wed, Dec 18, 2019 at 11:41:38AM +0000, Will Deacon wrote: > > On Mon, Dec 16, 2019 at 02:17:52PM +0100, Andrey Konovalov wrote: > > > On Mon, Dec 16, 2019 at 1:16 PM Will Deacon <will@kernel.org> wrote: > > > > On Fri, Nov 08, 2019 at 05:55:03PM +0200, Laurent Pinchart wrote: > > > > > Thank you for the patch. > > > > > > > > > > I'm sorry for the delay, and will have to ask you to be a bit more > > > > > patient I'm afraid. I will leave tomorrow for a week without computer > > > > > access and will only be able to go through my backlog when I will be > > > > > back on the 17th. > > > > > > > > Gentle reminder on this, now you've been back a month ;) > > > > > > I think we now have a reproducer for this issue that syzbot just reported: > > > > > > https://syzkaller.appspot.com/bug?extid=0a5c96772a9b26f2a876 > > > > > > You can try you patch on it :) > > > > Oh wow, I *really* like the raw USB gadget thingy you have to reproduce > > these! I also really like that this patch fixes the issue. Logs below. Thanks! An easier way to test the patch would be to issue a syz test command, but I'm glad you managed to set up raw gadget manually and it worked for you. > > Ok, that's a good poke for me to go review that raw gadget code to see > if it can be merged upstream :) Looking forward to it! =) > > > Laurent -- can we please merge this now? > > Yes, that would be good to have, as this obviously fixes a problem, and > I can take it off of my "patches to track" list.... > > thanks, > > greg k-h
On Wed, Dec 18, 2019 at 01:46:00PM +0100, Andrey Konovalov wrote: > On Wed, Dec 18, 2019 at 1:23 PM Greg Kroah-Hartman wrote: > > On Wed, Dec 18, 2019 at 11:41:38AM +0000, Will Deacon wrote: > >> On Mon, Dec 16, 2019 at 02:17:52PM +0100, Andrey Konovalov wrote: > >>> On Mon, Dec 16, 2019 at 1:16 PM Will Deacon <will@kernel.org> wrote: > >>>> On Fri, Nov 08, 2019 at 05:55:03PM +0200, Laurent Pinchart wrote: > >>>>> Thank you for the patch. > >>>>> > >>>>> I'm sorry for the delay, and will have to ask you to be a bit more > >>>>> patient I'm afraid. I will leave tomorrow for a week without computer > >>>>> access and will only be able to go through my backlog when I will be > >>>>> back on the 17th. > >>>> > >>>> Gentle reminder on this, now you've been back a month ;) > >>> > >>> I think we now have a reproducer for this issue that syzbot just reported: > >>> > >>> https://syzkaller.appspot.com/bug?extid=0a5c96772a9b26f2a876 > >>> > >>> You can try you patch on it :) > >> > >> Oh wow, I *really* like the raw USB gadget thingy you have to reproduce > >> these! I also really like that this patch fixes the issue. Logs below. > > Thanks! An easier way to test the patch would be to issue a syz test > command, but I'm glad you managed to set up raw gadget manually and it > worked for you. > > > > > Ok, that's a good poke for me to go review that raw gadget code to see > > if it can be merged upstream :) > > Looking forward to it! =) Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> and merged in my tree. I'm so sorry for the way too long delay. > >> Laurent -- can we please merge this now? > > > > Yes, that would be good to have, as this obviously fixes a problem, and > > I can take it off of my "patches to track" list....
On Wed, Dec 18, 2019 at 06:51:53PM +0200, Laurent Pinchart wrote: > On Wed, Dec 18, 2019 at 01:46:00PM +0100, Andrey Konovalov wrote: > > On Wed, Dec 18, 2019 at 1:23 PM Greg Kroah-Hartman wrote: > > > On Wed, Dec 18, 2019 at 11:41:38AM +0000, Will Deacon wrote: > > >> On Mon, Dec 16, 2019 at 02:17:52PM +0100, Andrey Konovalov wrote: > > >>> On Mon, Dec 16, 2019 at 1:16 PM Will Deacon <will@kernel.org> wrote: > > >>>> On Fri, Nov 08, 2019 at 05:55:03PM +0200, Laurent Pinchart wrote: > > >>>>> Thank you for the patch. > > >>>>> > > >>>>> I'm sorry for the delay, and will have to ask you to be a bit more > > >>>>> patient I'm afraid. I will leave tomorrow for a week without computer > > >>>>> access and will only be able to go through my backlog when I will be > > >>>>> back on the 17th. > > >>>> > > >>>> Gentle reminder on this, now you've been back a month ;) > > >>> > > >>> I think we now have a reproducer for this issue that syzbot just reported: > > >>> > > >>> https://syzkaller.appspot.com/bug?extid=0a5c96772a9b26f2a876 > > >>> > > >>> You can try you patch on it :) > > >> > > >> Oh wow, I *really* like the raw USB gadget thingy you have to reproduce > > >> these! I also really like that this patch fixes the issue. Logs below. > > > > Thanks! An easier way to test the patch would be to issue a syz test > > command, but I'm glad you managed to set up raw gadget manually and it > > worked for you. > > > > > > > > Ok, that's a good poke for me to go review that raw gadget code to see > > > if it can be merged upstream :) > > > > Looking forward to it! =) > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > and merged in my tree. I'm so sorry for the way too long delay. Thanks, Laurent. Will
Hi Laurent, On Wed, Dec 18, 2019 at 06:51:53PM +0200, Laurent Pinchart wrote: > On Wed, Dec 18, 2019 at 01:46:00PM +0100, Andrey Konovalov wrote: > > On Wed, Dec 18, 2019 at 1:23 PM Greg Kroah-Hartman wrote: > > > On Wed, Dec 18, 2019 at 11:41:38AM +0000, Will Deacon wrote: > > >> On Mon, Dec 16, 2019 at 02:17:52PM +0100, Andrey Konovalov wrote: > > >>> On Mon, Dec 16, 2019 at 1:16 PM Will Deacon <will@kernel.org> wrote: > > >>>> On Fri, Nov 08, 2019 at 05:55:03PM +0200, Laurent Pinchart wrote: > > >>>>> Thank you for the patch. > > >>>>> > > >>>>> I'm sorry for the delay, and will have to ask you to be a bit more > > >>>>> patient I'm afraid. I will leave tomorrow for a week without computer > > >>>>> access and will only be able to go through my backlog when I will be > > >>>>> back on the 17th. > > >>>> > > >>>> Gentle reminder on this, now you've been back a month ;) > > >>> > > >>> I think we now have a reproducer for this issue that syzbot just reported: > > >>> > > >>> https://syzkaller.appspot.com/bug?extid=0a5c96772a9b26f2a876 > > >>> > > >>> You can try you patch on it :) > > >> > > >> Oh wow, I *really* like the raw USB gadget thingy you have to reproduce > > >> these! I also really like that this patch fixes the issue. Logs below. > > > > Thanks! An easier way to test the patch would be to issue a syz test > > command, but I'm glad you managed to set up raw gadget manually and it > > worked for you. > > > > > > > > Ok, that's a good poke for me to go review that raw gadget code to see > > > if it can be merged upstream :) > > > > Looking forward to it! =) > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > and merged in my tree. I'm so sorry for the way too long delay. Please can you send this upstream and/or put it in linux-next? I can't see it anywhere at the moment :( Thanks, Will
Hi Will, On Tue, Jan 21, 2020 at 07:01:42PM +0000, Will Deacon wrote: > On Wed, Dec 18, 2019 at 06:51:53PM +0200, Laurent Pinchart wrote: > > On Wed, Dec 18, 2019 at 01:46:00PM +0100, Andrey Konovalov wrote: > > > On Wed, Dec 18, 2019 at 1:23 PM Greg Kroah-Hartman wrote: > > > > On Wed, Dec 18, 2019 at 11:41:38AM +0000, Will Deacon wrote: > > > >> On Mon, Dec 16, 2019 at 02:17:52PM +0100, Andrey Konovalov wrote: > > > >>> On Mon, Dec 16, 2019 at 1:16 PM Will Deacon <will@kernel.org> wrote: > > > >>>> On Fri, Nov 08, 2019 at 05:55:03PM +0200, Laurent Pinchart wrote: > > > >>>>> Thank you for the patch. > > > >>>>> > > > >>>>> I'm sorry for the delay, and will have to ask you to be a bit more > > > >>>>> patient I'm afraid. I will leave tomorrow for a week without computer > > > >>>>> access and will only be able to go through my backlog when I will be > > > >>>>> back on the 17th. > > > >>>> > > > >>>> Gentle reminder on this, now you've been back a month ;) > > > >>> > > > >>> I think we now have a reproducer for this issue that syzbot just reported: > > > >>> > > > >>> https://syzkaller.appspot.com/bug?extid=0a5c96772a9b26f2a876 > > > >>> > > > >>> You can try you patch on it :) > > > >> > > > >> Oh wow, I *really* like the raw USB gadget thingy you have to reproduce > > > >> these! I also really like that this patch fixes the issue. Logs below. > > > > > > Thanks! An easier way to test the patch would be to issue a syz test > > > command, but I'm glad you managed to set up raw gadget manually and it > > > worked for you. > > > > > > > > > > > Ok, that's a good poke for me to go review that raw gadget code to see > > > > if it can be merged upstream :) > > > > > > Looking forward to it! =) > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > and merged in my tree. I'm so sorry for the way too long delay. > > Please can you send this upstream and/or put it in linux-next? I can't see > it anywhere at the moment :( I've now sent the pull request. Seems I failed the schedule from A to Z with this patch. I'm extremely sorry :-(
On Wed, Jan 22, 2020 at 12:53:05AM +0200, Laurent Pinchart wrote: > On Tue, Jan 21, 2020 at 07:01:42PM +0000, Will Deacon wrote: > > On Wed, Dec 18, 2019 at 06:51:53PM +0200, Laurent Pinchart wrote: > > > On Wed, Dec 18, 2019 at 01:46:00PM +0100, Andrey Konovalov wrote: > > > > On Wed, Dec 18, 2019 at 1:23 PM Greg Kroah-Hartman wrote: > > > > > On Wed, Dec 18, 2019 at 11:41:38AM +0000, Will Deacon wrote: > > > > >> On Mon, Dec 16, 2019 at 02:17:52PM +0100, Andrey Konovalov wrote: > > > > >>> On Mon, Dec 16, 2019 at 1:16 PM Will Deacon <will@kernel.org> wrote: > > > > >>>> On Fri, Nov 08, 2019 at 05:55:03PM +0200, Laurent Pinchart wrote: > > > > >>>>> Thank you for the patch. > > > > >>>>> > > > > >>>>> I'm sorry for the delay, and will have to ask you to be a bit more > > > > >>>>> patient I'm afraid. I will leave tomorrow for a week without computer > > > > >>>>> access and will only be able to go through my backlog when I will be > > > > >>>>> back on the 17th. > > > > >>>> > > > > >>>> Gentle reminder on this, now you've been back a month ;) > > > > >>> > > > > >>> I think we now have a reproducer for this issue that syzbot just reported: > > > > >>> > > > > >>> https://syzkaller.appspot.com/bug?extid=0a5c96772a9b26f2a876 > > > > >>> > > > > >>> You can try you patch on it :) > > > > >> > > > > >> Oh wow, I *really* like the raw USB gadget thingy you have to reproduce > > > > >> these! I also really like that this patch fixes the issue. Logs below. > > > > > > > > Thanks! An easier way to test the patch would be to issue a syz test > > > > command, but I'm glad you managed to set up raw gadget manually and it > > > > worked for you. > > > > > > > > > > > > > > Ok, that's a good poke for me to go review that raw gadget code to see > > > > > if it can be merged upstream :) > > > > > > > > Looking forward to it! =) > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > and merged in my tree. I'm so sorry for the way too long delay. > > > > Please can you send this upstream and/or put it in linux-next? I can't see > > it anywhere at the moment :( > > I've now sent the pull request. Thanks, Laurent. > Seems I failed the schedule from A to Z with this patch. I'm extremely > sorry :-( Well, at least you were consistent ;) Will
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c index 66ee168ddc7e..e24420b1750a 100644 --- a/drivers/media/usb/uvc/uvc_driver.c +++ b/drivers/media/usb/uvc/uvc_driver.c @@ -1493,6 +1493,11 @@ static int uvc_scan_chain_forward(struct uvc_video_chain *chain, break; if (forward == prev) continue; + if (forward->chain.next || forward->chain.prev) { + uvc_trace(UVC_TRACE_DESCR, "Found reference to " + "entity %d already in chain.\n", forward->id); + return -EINVAL; + } switch (UVC_ENTITY_TYPE(forward)) { case UVC_VC_EXTENSION_UNIT: @@ -1574,6 +1579,13 @@ static int uvc_scan_chain_backward(struct uvc_video_chain *chain, return -1; } + if (term->chain.next || term->chain.prev) { + uvc_trace(UVC_TRACE_DESCR, "Found reference to " + "entity %d already in chain.\n", + term->id); + return -EINVAL; + } + if (uvc_trace_param & UVC_TRACE_PROBE) printk(KERN_CONT " %d", term->id);
Way back in 2017, fuzzing the 4.14-rc2 USB stack with syzkaller kicked up the following WARNING from the UVC chain scanning code: | list_add double add: new=ffff880069084010, prev=ffff880069084010, | next=ffff880067d22298. | ------------[ cut here ]------------ | WARNING: CPU: 1 PID: 1846 at lib/list_debug.c:31 __list_add_valid+0xbd/0xf0 | Modules linked in: | CPU: 1 PID: 1846 Comm: kworker/1:2 Not tainted | 4.14.0-rc2-42613-g1488251d1a98 #238 | Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 | Workqueue: usb_hub_wq hub_event | task: ffff88006b01ca40 task.stack: ffff880064358000 | RIP: 0010:__list_add_valid+0xbd/0xf0 lib/list_debug.c:29 | RSP: 0018:ffff88006435ddd0 EFLAGS: 00010286 | RAX: 0000000000000058 RBX: ffff880067d22298 RCX: 0000000000000000 | RDX: 0000000000000058 RSI: ffffffff85a58800 RDI: ffffed000c86bbac | RBP: ffff88006435dde8 R08: 1ffff1000c86ba52 R09: 0000000000000000 | R10: 0000000000000002 R11: 0000000000000000 R12: ffff880069084010 | R13: ffff880067d22298 R14: ffff880069084010 R15: ffff880067d222a0 | FS: 0000000000000000(0000) GS:ffff88006c900000(0000) knlGS:0000000000000000 | CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 | CR2: 0000000020004ff2 CR3: 000000006b447000 CR4: 00000000000006e0 | Call Trace: | __list_add ./include/linux/list.h:59 | list_add_tail+0x8c/0x1b0 ./include/linux/list.h:92 | uvc_scan_chain_forward.isra.8+0x373/0x416 | drivers/media/usb/uvc/uvc_driver.c:1471 | uvc_scan_chain drivers/media/usb/uvc/uvc_driver.c:1585 | uvc_scan_device drivers/media/usb/uvc/uvc_driver.c:1769 | uvc_probe+0x77f2/0x8f00 drivers/media/usb/uvc/uvc_driver.c:2104 Looking into the output from usbmon, the interesting part is the following data packet: ffff880069c63e00 30710169 C Ci:1:002:0 0 143 = 09028f00 01030080 00090403 00000e01 00000924 03000103 7c003328 010204db If we drop the lead configuration and interface descriptors, we're left with an output terminal descriptor describing a generic display: /* Output terminal descriptor */ buf[0] 09 buf[1] 24 buf[2] 03 /* UVC_VC_OUTPUT_TERMINAL */ buf[3] 00 /* ID */ buf[4] 01 /* type == 0x0301 (UVC_OTT_DISPLAY) */ buf[5] 03 buf[6] 7c buf[7] 00 /* source ID refers to self! */ buf[8] 33 The problem with this descriptor is that it is self-referential: the source ID of 0 matches itself! This causes the 'struct uvc_entity' representing the display to be added to its chain list twice during 'uvc_scan_chain()': once via 'uvc_scan_chain_entity()' when it is processed directly from the 'dev->entities' list and then again immediately afterwards when trying to follow the source ID in 'uvc_scan_chain_forward()' Add a check before adding an entity to a chain list to ensure that the entity is not already part of a chain. Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Cc: Mauro Carvalho Chehab <mchehab@kernel.org> Cc: Dmitry Vyukov <dvyukov@google.com> Cc: Kostya Serebryany <kcc@google.com> Cc: <stable@vger.kernel.org> Fixes: c0efd232929c ("V4L/DVB (8145a): USB Video Class driver") Reported-by: Andrey Konovalov <andreyknvl@google.com> Link: https://lore.kernel.org/linux-media/CAAeHK+z+Si69jUR+N-SjN9q4O+o5KFiNManqEa-PjUta7EOb7A@mail.gmail.com/ Signed-off-by: Will Deacon <will@kernel.org> --- That's right, it's the same patch again! No changes since either of: http://lkml.kernel.org/r/20191002112753.21630-1-will@kernel.org https://lore.kernel.org/lkml/20191016195800.22099-1-will@kernel.org Please consider merging. drivers/media/usb/uvc/uvc_driver.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)