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 <[email protected]>
Cc: Mauro Carvalho Chehab <[email protected]>
Cc: Dmitry Vyukov <[email protected]>
Cc: Kostya Serebryany <[email protected]>
Cc: <[email protected]>
Fixes: c0efd232929c ("V4L/DVB (8145a): USB Video Class driver")
Reported-by: Andrey Konovalov <[email protected]>
Link: https://lore.kernel.org/linux-media/CAAeHK+z+Si69jUR+N-SjN9q4O+o5KFiNManqEa-PjUta7EOb7A@mail.gmail.com/
Signed-off-by: Will Deacon <[email protected]>
---
That's right, it's the same patch again! No changes since either of:
http://lkml.kernel.org/r/[email protected]
https://lore.kernel.org/lkml/[email protected]
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 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 <[email protected]>
> Cc: Mauro Carvalho Chehab <[email protected]>
> Cc: Dmitry Vyukov <[email protected]>
> Cc: Kostya Serebryany <[email protected]>
> Cc: <[email protected]>
> Fixes: c0efd232929c ("V4L/DVB (8145a): USB Video Class driver")
> Reported-by: Andrey Konovalov <[email protected]>
> Link: https://lore.kernel.org/linux-media/CAAeHK+z+Si69jUR+N-SjN9q4O+o5KFiNManqEa-PjUta7EOb7A@mail.gmail.com/
> Signed-off-by: Will Deacon <[email protected]>
> ---
>
> That's right, it's the same patch again! No changes since either of:
>
> http://lkml.kernel.org/r/[email protected]
> https://lore.kernel.org/lkml/[email protected]
>
> 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
>
--
Regards,
Laurent Pinchart
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 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 <[email protected]> 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 <[email protected]>
>
> 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 <[email protected]> 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 <[email protected]>
> >
> > 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 :-(
--
Regards,
Laurent Pinchart
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 <[email protected]> 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 <[email protected]>
> > >
> > > 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