[syzbot] [fbdev?] [usb?] WARNING in dlfb_submit_urb/usb_submit_urb (2)

[syzbot] [fbdev?] [usb?] WARNING in dlfb_submit_urb/usb_submit_urb (2)

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit:    a4422ff22142 usb: typec: qcom: Add Qualcomm PMIC Type-C dr..
git tree:       https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
console output: https://syzkaller.appspot.com/x/log.txt?x=15245566280000
kernel config:  https://syzkaller.appspot.com/x/.config?x=2414a945e4542ec1
dashboard link: https://syzkaller.appspot.com/bug?extid=0e22d63dcebb802b9bc8
compiler:       gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=1720fd3a280000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=171a73ea280000

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/414817142fb7/disk-a4422ff2.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/448dba0d344e/vmlinux-a4422ff2.xz
kernel image: https://storage.googleapis.com/syzbot-assets/d0ad9fe848e2/bzImage-a4422ff2.xz

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+0e22d63dcebb802b9bc8@syzkaller.appspotmail.com

usb 1-1: Read EDID byte 0 failed: -71
usb 1-1: Unable to get valid EDID from device/display
------------[ cut here ]------------
usb 1-1: BOGUS urb xfer, pipe 3 != type 1
WARNING: CPU: 0 PID: 9 at drivers/usb/core/urb.c:504 usb_submit_urb+0xed6/0x1880 drivers/usb/core/urb.c:504
Modules linked in:
CPU: 0 PID: 9 Comm: kworker/0:1 Not tainted 6.4.0-rc1-syzkaller-00016-ga4422ff22142 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 04/28/2023
Workqueue: usb_hub_wq hub_event
RIP: 0010:usb_submit_urb+0xed6/0x1880 drivers/usb/core/urb.c:504
Code: 7c 24 18 e8 7c dc 5a fd 48 8b 7c 24 18 e8 42 ca 0b ff 41 89 d8 44 89 e1 4c 89 ea 48 89 c6 48 c7 c7 60 34 cc 86 e8 0a fa 25 fd <0f> 0b e9 58 f8 ff ff e8 4e dc 5a fd 48 81 c5 b8 05 00 00 e9 84 f7
RSP: 0018:ffffc9000009ed48 EFLAGS: 00010282
RAX: 0000000000000000 RBX: 0000000000000001 RCX: 0000000000000000
RDX: ffff888103650000 RSI: ffffffff81163677 RDI: 0000000000000001
RBP: ffff88810cb32940 R08: 0000000000000001 R09: 0000000000000000
R10: 0000000000000001 R11: 0000000000000001 R12: 0000000000000003
R13: ffff88810cf426b8 R14: 0000000000000003 R15: ffff888104272100
FS:  0000000000000000(0000) GS:ffff8881f6600000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000562147be3b70 CR3: 0000000110380000 CR4: 00000000003506f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
 <TASK>
 dlfb_submit_urb+0x92/0x180 drivers/video/fbdev/udlfb.c:1980
 dlfb_set_video_mode+0x21f0/0x2950 drivers/video/fbdev/udlfb.c:315
 dlfb_ops_set_par+0x2a7/0x8d0 drivers/video/fbdev/udlfb.c:1111
 dlfb_usb_probe+0x149a/0x2710 drivers/video/fbdev/udlfb.c:1743
 usb_probe_interface+0x30f/0x960 drivers/usb/core/driver.c:396
 call_driver_probe drivers/base/dd.c:579 [inline]
 really_probe+0x240/0xca0 drivers/base/dd.c:658
 __driver_probe_device+0x1df/0x4b0 drivers/base/dd.c:800
 driver_probe_device+0x4c/0x1a0 drivers/base/dd.c:830
 __device_attach_driver+0x1d4/0x2e0 drivers/base/dd.c:958
 bus_for_each_drv+0x149/0x1d0 drivers/base/bus.c:457
 __device_attach+0x1e4/0x4b0 drivers/base/dd.c:1030
 bus_probe_device+0x17c/0x1c0 drivers/base/bus.c:532
 device_add+0x112d/0x1a40 drivers/base/core.c:3625
 usb_set_configuration+0x1196/0x1bc0 drivers/usb/core/message.c:2211
 usb_generic_driver_probe+0xcf/0x130 drivers/usb/core/generic.c:238
 usb_probe_device+0xd8/0x2c0 drivers/usb/core/driver.c:293
 call_driver_probe drivers/base/dd.c:579 [inline]
 really_probe+0x240/0xca0 drivers/base/dd.c:658
 __driver_probe_device+0x1df/0x4b0 drivers/base/dd.c:800
 driver_probe_device+0x4c/0x1a0 drivers/base/dd.c:830
 __device_attach_driver+0x1d4/0x2e0 drivers/base/dd.c:958
 bus_for_each_drv+0x149/0x1d0 drivers/base/bus.c:457
 __device_attach+0x1e4/0x4b0 drivers/base/dd.c:1030
 bus_probe_device+0x17c/0x1c0 drivers/base/bus.c:532
 device_add+0x112d/0x1a40 drivers/base/core.c:3625
 usb_new_device+0xcb2/0x19d0 drivers/usb/core/hub.c:2575
 hub_port_connect drivers/usb/core/hub.c:5407 [inline]
 hub_port_connect_change drivers/usb/core/hub.c:5551 [inline]
 port_event drivers/usb/core/hub.c:5711 [inline]
 hub_event+0x2e3d/0x4ed0 drivers/usb/core/hub.c:5793
 process_one_work+0x99a/0x15e0 kernel/workqueue.c:2405
 worker_thread+0x67d/0x10c0 kernel/workqueue.c:2552
 kthread+0x344/0x440 kernel/kthread.c:379
 ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:308
 </TASK>


---
This report 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 issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.

If the bug is already fixed, let syzbot know by replying with:
#syz fix: exact-commit-title

If you want syzbot to run the reproducer, reply with:
#syz test: git://repo/address.git branch-or-commit-hash
If you attach or paste a git patch, syzbot will apply it before testing.

If you want to change bug's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)

If the bug is a duplicate of another bug, reply with:
#syz dup: exact-subject-of-another-report

If you want to undo deduplication, reply with:
#syz undup

Re: [syzbot] [fbdev?] [usb?] WARNING in dlfb_submit_urb/usb_submit_urb (2)

[Helge Deller]

* syzbot <syzbot+0e22d63dcebb802b9bc8@syzkaller.appspotmail.com>:
> syzbot found the following issue on:
>
> HEAD commit:    a4422ff22142 usb: typec: qcom: Add Qualcomm PMIC Type-C dr..
> git tree:       https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
> console output: https://syzkaller.appspot.com/x/log.txt?x=15245566280000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=2414a945e4542ec1
> dashboard link: https://syzkaller.appspot.com/bug?extid=0e22d63dcebb802b9bc8
> compiler:       gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=1720fd3a280000
> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=171a73ea280000
>
> Downloadable assets:
> disk image: https://storage.googleapis.com/syzbot-assets/414817142fb7/disk-a4422ff2.raw.xz
> vmlinux: https://storage.googleapis.com/syzbot-assets/448dba0d344e/vmlinux-a4422ff2.xz
> kernel image: https://storage.googleapis.com/syzbot-assets/d0ad9fe848e2/bzImage-a4422ff2.xz
>
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: syzbot+0e22d63dcebb802b9bc8@syzkaller.appspotmail.com
>
> usb 1-1: Read EDID byte 0 failed: -71
> usb 1-1: Unable to get valid EDID from device/display
> ------------[ cut here ]------------
> usb 1-1: BOGUS urb xfer, pipe 3 != type 1
> WARNING: CPU: 0 PID: 9 at drivers/usb/core/urb.c:504 usb_submit_urb+0xed6/0x1880 drivers/usb/core/urb.c:504
> Modules linked in:
> CPU: 0 PID: 9 Comm: kworker/0:1 Not tainted 6.4.0-rc1-syzkaller-00016-ga4422ff22142 #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 04/28/2023
> Workqueue: usb_hub_wq hub_event
> RIP: 0010:usb_submit_urb+0xed6/0x1880 drivers/usb/core/urb.c:504
> Code: 7c 24 18 e8 7c dc 5a fd 48 8b 7c 24 18 e8 42 ca 0b ff 41 89 d8 44 89 e1 4c 89 ea 48 89 c6 48 c7 c7 60 34 cc 86 e8 0a fa 25 fd <0f> 0b e9 58 f8 ff ff e8 4e dc 5a fd 48 81 c5 b8 05 00 00 e9 84 f7
> RSP: 0018:ffffc9000009ed48 EFLAGS: 00010282
> RAX: 0000000000000000 RBX: 0000000000000001 RCX: 0000000000000000
> RDX: ffff888103650000 RSI: ffffffff81163677 RDI: 0000000000000001
> RBP: ffff88810cb32940 R08: 0000000000000001 R09: 0000000000000000
> R10: 0000000000000001 R11: 0000000000000001 R12: 0000000000000003
> R13: ffff88810cf426b8 R14: 0000000000000003 R15: ffff888104272100
> FS:  0000000000000000(0000) GS:ffff8881f6600000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000562147be3b70 CR3: 0000000110380000 CR4: 00000000003506f0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
>  <TASK>
>  dlfb_submit_urb+0x92/0x180 drivers/video/fbdev/udlfb.c:1980
>  dlfb_set_video_mode+0x21f0/0x2950 drivers/video/fbdev/udlfb.c:315
>  dlfb_ops_set_par+0x2a7/0x8d0 drivers/video/fbdev/udlfb.c:1111
>  dlfb_usb_probe+0x149a/0x2710 drivers/video/fbdev/udlfb.c:1743
>  usb_probe_interface+0x30f/0x960 drivers/usb/core/driver.c:396
>  call_driver_probe drivers/base/dd.c:579 [inline]
>  really_probe+0x240/0xca0 drivers/base/dd.c:658
>  __driver_probe_device+0x1df/0x4b0 drivers/base/dd.c:800
>  driver_probe_device+0x4c/0x1a0 drivers/base/dd.c:830
>  __device_attach_driver+0x1d4/0x2e0 drivers/base/dd.c:958
>  bus_for_each_drv+0x149/0x1d0 drivers/base/bus.c:457
>  __device_attach+0x1e4/0x4b0 drivers/base/dd.c:1030
>  bus_probe_device+0x17c/0x1c0 drivers/base/bus.c:532
>  device_add+0x112d/0x1a40 drivers/base/core.c:3625
>  usb_set_configuration+0x1196/0x1bc0 drivers/usb/core/message.c:2211
>  usb_generic_driver_probe+0xcf/0x130 drivers/usb/core/generic.c:238
>  usb_probe_device+0xd8/0x2c0 drivers/usb/core/driver.c:293
>  call_driver_probe drivers/base/dd.c:579 [inline]
>  really_probe+0x240/0xca0 drivers/base/dd.c:658
>  __driver_probe_device+0x1df/0x4b0 drivers/base/dd.c:800
>  driver_probe_device+0x4c/0x1a0 drivers/base/dd.c:830
>  __device_attach_driver+0x1d4/0x2e0 drivers/base/dd.c:958
>  bus_for_each_drv+0x149/0x1d0 drivers/base/bus.c:457
>  __device_attach+0x1e4/0x4b0 drivers/base/dd.c:1030
>  bus_probe_device+0x17c/0x1c0 drivers/base/bus.c:532
>  device_add+0x112d/0x1a40 drivers/base/core.c:3625
>  usb_new_device+0xcb2/0x19d0 drivers/usb/core/hub.c:2575
>  hub_port_connect drivers/usb/core/hub.c:5407 [inline]
>  hub_port_connect_change drivers/usb/core/hub.c:5551 [inline]
>  port_event drivers/usb/core/hub.c:5711 [inline]
>  hub_event+0x2e3d/0x4ed0 drivers/usb/core/hub.c:5793
>  process_one_work+0x99a/0x15e0 kernel/workqueue.c:2405
>  worker_thread+0x67d/0x10c0 kernel/workqueue.c:2552
>  kthread+0x344/0x440 kernel/kthread.c:379
>  ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:308

I think this is an informational warning from the USB stack,
since the syzbot usb device doesn't behave as expected.

What happens with this patch applied?

#syz test: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing

diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
index 9f3c54032556..dd77b9e757da 100644
--- a/drivers/usb/core/urb.c
+++ b/drivers/usb/core/urb.c
@@ -501,7 +501,7 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags)

 	/* Check that the pipe's type matches the endpoint's type */
 	if (usb_pipe_type_check(urb->dev, urb->pipe))
-		dev_WARN(&dev->dev, "BOGUS urb xfer, pipe %x != type %x\n",
+		printk("BOGUS urb xfer, pipe %x != type %x (hardware misbehaviour?)\n",
 			usb_pipetype(urb->pipe), pipetypes[xfertype]);

 	/* Check against a simple/standard policy */

Helge

Re: [syzbot] [fbdev?] [usb?] WARNING in dlfb_submit_urb/usb_submit_urb (2)

[syzbot]

Hello,

syzbot tried to test the proposed patch but the build/boot failed:

failed to apply patch:
checking file drivers/usb/core/urb.c
patch: **** unexpected end of file in patch



Tested on:

commit:         a4422ff2 usb: typec: qcom: Add Qualcomm PMIC Type-C dr..
git tree:       https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
dashboard link: https://syzkaller.appspot.com/bug?extid=0e22d63dcebb802b9bc8
compiler:       
patch:          https://syzkaller.appspot.com/x/patch.diff?x=1524090e280000

Re: [syzbot] [fbdev?] [usb?] WARNING in dlfb_submit_urb/usb_submit_urb (2)

[Alan Stern]

On Thu, May 18, 2023 at 09:34:24AM +0200, Helge Deller wrote:
> * syzbot <syzbot+0e22d63dcebb802b9bc8@syzkaller.appspotmail.com>:
> > syzbot found the following issue on:
> >
> > HEAD commit:    a4422ff22142 usb: typec: qcom: Add Qualcomm PMIC Type-C dr..
> > git tree:       https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
> > console output: https://syzkaller.appspot.com/x/log.txt?x=15245566280000
> > kernel config:  https://syzkaller.appspot.com/x/.config?x=2414a945e4542ec1
> > dashboard link: https://syzkaller.appspot.com/bug?extid=0e22d63dcebb802b9bc8
> > compiler:       gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
> > syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=1720fd3a280000
> > C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=171a73ea280000
> >
> > Downloadable assets:
> > disk image: https://storage.googleapis.com/syzbot-assets/414817142fb7/disk-a4422ff2.raw.xz
> > vmlinux: https://storage.googleapis.com/syzbot-assets/448dba0d344e/vmlinux-a4422ff2.xz
> > kernel image: https://storage.googleapis.com/syzbot-assets/d0ad9fe848e2/bzImage-a4422ff2.xz
> >
> > IMPORTANT: if you fix the issue, please add the following tag to the commit:
> > Reported-by: syzbot+0e22d63dcebb802b9bc8@syzkaller.appspotmail.com
> >
> > usb 1-1: Read EDID byte 0 failed: -71
> > usb 1-1: Unable to get valid EDID from device/display
> > ------------[ cut here ]------------
> > usb 1-1: BOGUS urb xfer, pipe 3 != type 1
> > WARNING: CPU: 0 PID: 9 at drivers/usb/core/urb.c:504 usb_submit_urb+0xed6/0x1880 drivers/usb/core/urb.c:504
> > Modules linked in:
> > CPU: 0 PID: 9 Comm: kworker/0:1 Not tainted 6.4.0-rc1-syzkaller-00016-ga4422ff22142 #0
> > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 04/28/2023
> > Workqueue: usb_hub_wq hub_event
> > RIP: 0010:usb_submit_urb+0xed6/0x1880 drivers/usb/core/urb.c:504
> > Code: 7c 24 18 e8 7c dc 5a fd 48 8b 7c 24 18 e8 42 ca 0b ff 41 89 d8 44 89 e1 4c 89 ea 48 89 c6 48 c7 c7 60 34 cc 86 e8 0a fa 25 fd <0f> 0b e9 58 f8 ff ff e8 4e dc 5a fd 48 81 c5 b8 05 00 00 e9 84 f7
> > RSP: 0018:ffffc9000009ed48 EFLAGS: 00010282
> > RAX: 0000000000000000 RBX: 0000000000000001 RCX: 0000000000000000
> > RDX: ffff888103650000 RSI: ffffffff81163677 RDI: 0000000000000001
> > RBP: ffff88810cb32940 R08: 0000000000000001 R09: 0000000000000000
> > R10: 0000000000000001 R11: 0000000000000001 R12: 0000000000000003
> > R13: ffff88810cf426b8 R14: 0000000000000003 R15: ffff888104272100
> > FS:  0000000000000000(0000) GS:ffff8881f6600000(0000) knlGS:0000000000000000
> > CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > CR2: 0000562147be3b70 CR3: 0000000110380000 CR4: 00000000003506f0
> > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > Call Trace:
> >  <TASK>
> >  dlfb_submit_urb+0x92/0x180 drivers/video/fbdev/udlfb.c:1980
> >  dlfb_set_video_mode+0x21f0/0x2950 drivers/video/fbdev/udlfb.c:315
> >  dlfb_ops_set_par+0x2a7/0x8d0 drivers/video/fbdev/udlfb.c:1111
> >  dlfb_usb_probe+0x149a/0x2710 drivers/video/fbdev/udlfb.c:1743
> >  usb_probe_interface+0x30f/0x960 drivers/usb/core/driver.c:396
> >  call_driver_probe drivers/base/dd.c:579 [inline]
> >  really_probe+0x240/0xca0 drivers/base/dd.c:658
> >  __driver_probe_device+0x1df/0x4b0 drivers/base/dd.c:800
> >  driver_probe_device+0x4c/0x1a0 drivers/base/dd.c:830
> >  __device_attach_driver+0x1d4/0x2e0 drivers/base/dd.c:958
> >  bus_for_each_drv+0x149/0x1d0 drivers/base/bus.c:457
> >  __device_attach+0x1e4/0x4b0 drivers/base/dd.c:1030
> >  bus_probe_device+0x17c/0x1c0 drivers/base/bus.c:532
> >  device_add+0x112d/0x1a40 drivers/base/core.c:3625
> >  usb_set_configuration+0x1196/0x1bc0 drivers/usb/core/message.c:2211
> >  usb_generic_driver_probe+0xcf/0x130 drivers/usb/core/generic.c:238
> >  usb_probe_device+0xd8/0x2c0 drivers/usb/core/driver.c:293
> >  call_driver_probe drivers/base/dd.c:579 [inline]
> >  really_probe+0x240/0xca0 drivers/base/dd.c:658
> >  __driver_probe_device+0x1df/0x4b0 drivers/base/dd.c:800
> >  driver_probe_device+0x4c/0x1a0 drivers/base/dd.c:830
> >  __device_attach_driver+0x1d4/0x2e0 drivers/base/dd.c:958
> >  bus_for_each_drv+0x149/0x1d0 drivers/base/bus.c:457
> >  __device_attach+0x1e4/0x4b0 drivers/base/dd.c:1030
> >  bus_probe_device+0x17c/0x1c0 drivers/base/bus.c:532
> >  device_add+0x112d/0x1a40 drivers/base/core.c:3625
> >  usb_new_device+0xcb2/0x19d0 drivers/usb/core/hub.c:2575
> >  hub_port_connect drivers/usb/core/hub.c:5407 [inline]
> >  hub_port_connect_change drivers/usb/core/hub.c:5551 [inline]
> >  port_event drivers/usb/core/hub.c:5711 [inline]
> >  hub_event+0x2e3d/0x4ed0 drivers/usb/core/hub.c:5793
> >  process_one_work+0x99a/0x15e0 kernel/workqueue.c:2405
> >  worker_thread+0x67d/0x10c0 kernel/workqueue.c:2552
> >  kthread+0x344/0x440 kernel/kthread.c:379
> >  ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:308
> 
> I think this is an informational warning from the USB stack,

It is not informational.  It is a warning that the caller has a bug.

> since the syzbot usb device doesn't behave as expected.
> 
> What happens with this patch applied?
> 
> #syz test: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
> 
> diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
> index 9f3c54032556..dd77b9e757da 100644
> --- a/drivers/usb/core/urb.c
> +++ b/drivers/usb/core/urb.c
> @@ -501,7 +501,7 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
> 
>  	/* Check that the pipe's type matches the endpoint's type */
>  	if (usb_pipe_type_check(urb->dev, urb->pipe))
> -		dev_WARN(&dev->dev, "BOGUS urb xfer, pipe %x != type %x\n",
> +		printk("BOGUS urb xfer, pipe %x != type %x (hardware misbehaviour?)\n",
>  			usb_pipetype(urb->pipe), pipetypes[xfertype]);
> 
>  	/* Check against a simple/standard policy */

You can't fix a bug by changing the line that reports it from dev_WARN 
to printk!

In this case it looks like dlfb_usb_probe() or one of the routines it 
calls is wrong; it assumes that an endpoint has the expected type 
without checking.  More precisely, it thinks an endpoint is BULK when 
actually it is INTERRUPT.  That's what needs to be fixed.

Alan Stern

Re: [syzbot] [fbdev?] [usb?] WARNING in dlfb_submit_urb/usb_submit_urb (2)

[Helge Deller]

On 5/18/23 15:54, Alan Stern wrote:
> On Thu, May 18, 2023 at 09:34:24AM +0200, Helge Deller wrote:
>> * syzbot <syzbot+0e22d63dcebb802b9bc8@syzkaller.appspotmail.com>:
>>> syzbot found the following issue on:
>>>
>>> HEAD commit:    a4422ff22142 usb: typec: qcom: Add Qualcomm PMIC Type-C dr..
>>> git tree:       https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
>>> console output: https://syzkaller.appspot.com/x/log.txt?x=15245566280000
>>> kernel config:  https://syzkaller.appspot.com/x/.config?x=2414a945e4542ec1
>>> dashboard link: https://syzkaller.appspot.com/bug?extid=0e22d63dcebb802b9bc8
>>> compiler:       gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
>>> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=1720fd3a280000
>>> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=171a73ea280000
>>>
>>> Downloadable assets:
>>> disk image: https://storage.googleapis.com/syzbot-assets/414817142fb7/disk-a4422ff2.raw.xz
>>> vmlinux: https://storage.googleapis.com/syzbot-assets/448dba0d344e/vmlinux-a4422ff2.xz
>>> kernel image: https://storage.googleapis.com/syzbot-assets/d0ad9fe848e2/bzImage-a4422ff2.xz
>>>
>>> IMPORTANT: if you fix the issue, please add the following tag to the commit:
>>> Reported-by: syzbot+0e22d63dcebb802b9bc8@syzkaller.appspotmail.com
>>>
>>> usb 1-1: Read EDID byte 0 failed: -71
>>> usb 1-1: Unable to get valid EDID from device/display
>>> ------------[ cut here ]------------
>>> usb 1-1: BOGUS urb xfer, pipe 3 != type 1
>>> WARNING: CPU: 0 PID: 9 at drivers/usb/core/urb.c:504 usb_submit_urb+0xed6/0x1880 drivers/usb/core/urb.c:504
>>> Modules linked in:
>>> CPU: 0 PID: 9 Comm: kworker/0:1 Not tainted 6.4.0-rc1-syzkaller-00016-ga4422ff22142 #0
>>> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 04/28/2023
>>> Workqueue: usb_hub_wq hub_event
>>> RIP: 0010:usb_submit_urb+0xed6/0x1880 drivers/usb/core/urb.c:504
>>> Code: 7c 24 18 e8 7c dc 5a fd 48 8b 7c 24 18 e8 42 ca 0b ff 41 89 d8 44 89 e1 4c 89 ea 48 89 c6 48 c7 c7 60 34 cc 86 e8 0a fa 25 fd <0f> 0b e9 58 f8 ff ff e8 4e dc 5a fd 48 81 c5 b8 05 00 00 e9 84 f7
>>> RSP: 0018:ffffc9000009ed48 EFLAGS: 00010282
>>> RAX: 0000000000000000 RBX: 0000000000000001 RCX: 0000000000000000
>>> RDX: ffff888103650000 RSI: ffffffff81163677 RDI: 0000000000000001
>>> RBP: ffff88810cb32940 R08: 0000000000000001 R09: 0000000000000000
>>> R10: 0000000000000001 R11: 0000000000000001 R12: 0000000000000003
>>> R13: ffff88810cf426b8 R14: 0000000000000003 R15: ffff888104272100
>>> FS:  0000000000000000(0000) GS:ffff8881f6600000(0000) knlGS:0000000000000000
>>> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>> CR2: 0000562147be3b70 CR3: 0000000110380000 CR4: 00000000003506f0
>>> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>>> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>>> Call Trace:
>>>   <TASK>
>>>   dlfb_submit_urb+0x92/0x180 drivers/video/fbdev/udlfb.c:1980
>>>   dlfb_set_video_mode+0x21f0/0x2950 drivers/video/fbdev/udlfb.c:315
>>>   dlfb_ops_set_par+0x2a7/0x8d0 drivers/video/fbdev/udlfb.c:1111
>>>   dlfb_usb_probe+0x149a/0x2710 drivers/video/fbdev/udlfb.c:1743
>>>   usb_probe_interface+0x30f/0x960 drivers/usb/core/driver.c:396
>>>   call_driver_probe drivers/base/dd.c:579 [inline]
>>>   really_probe+0x240/0xca0 drivers/base/dd.c:658
>>>   __driver_probe_device+0x1df/0x4b0 drivers/base/dd.c:800
>>>   driver_probe_device+0x4c/0x1a0 drivers/base/dd.c:830
>>>   __device_attach_driver+0x1d4/0x2e0 drivers/base/dd.c:958
>>>   bus_for_each_drv+0x149/0x1d0 drivers/base/bus.c:457
>>>   __device_attach+0x1e4/0x4b0 drivers/base/dd.c:1030
>>>   bus_probe_device+0x17c/0x1c0 drivers/base/bus.c:532
>>>   device_add+0x112d/0x1a40 drivers/base/core.c:3625
>>>   usb_set_configuration+0x1196/0x1bc0 drivers/usb/core/message.c:2211
>>>   usb_generic_driver_probe+0xcf/0x130 drivers/usb/core/generic.c:238
>>>   usb_probe_device+0xd8/0x2c0 drivers/usb/core/driver.c:293
>>>   call_driver_probe drivers/base/dd.c:579 [inline]
>>>   really_probe+0x240/0xca0 drivers/base/dd.c:658
>>>   __driver_probe_device+0x1df/0x4b0 drivers/base/dd.c:800
>>>   driver_probe_device+0x4c/0x1a0 drivers/base/dd.c:830
>>>   __device_attach_driver+0x1d4/0x2e0 drivers/base/dd.c:958
>>>   bus_for_each_drv+0x149/0x1d0 drivers/base/bus.c:457
>>>   __device_attach+0x1e4/0x4b0 drivers/base/dd.c:1030
>>>   bus_probe_device+0x17c/0x1c0 drivers/base/bus.c:532
>>>   device_add+0x112d/0x1a40 drivers/base/core.c:3625
>>>   usb_new_device+0xcb2/0x19d0 drivers/usb/core/hub.c:2575
>>>   hub_port_connect drivers/usb/core/hub.c:5407 [inline]
>>>   hub_port_connect_change drivers/usb/core/hub.c:5551 [inline]
>>>   port_event drivers/usb/core/hub.c:5711 [inline]
>>>   hub_event+0x2e3d/0x4ed0 drivers/usb/core/hub.c:5793
>>>   process_one_work+0x99a/0x15e0 kernel/workqueue.c:2405
>>>   worker_thread+0x67d/0x10c0 kernel/workqueue.c:2552
>>>   kthread+0x344/0x440 kernel/kthread.c:379
>>>   ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:308
>>
>> I think this is an informational warning from the USB stack,
>
> It is not informational.  It is a warning that the caller has a bug.

I'm not a USB expert, so I searched for such bug reports, and it seems
people sometimes faced this warning with different USB devices.

>> since the syzbot usb device doesn't behave as expected.
>>
>> What happens with this patch applied?
>>
>> #syz test: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
>>
>> diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
>> index 9f3c54032556..dd77b9e757da 100644
>> --- a/drivers/usb/core/urb.c
>> +++ b/drivers/usb/core/urb.c
>> @@ -501,7 +501,7 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
>>
>>   	/* Check that the pipe's type matches the endpoint's type */
>>   	if (usb_pipe_type_check(urb->dev, urb->pipe))
>> -		dev_WARN(&dev->dev, "BOGUS urb xfer, pipe %x != type %x\n",
>> +		printk("BOGUS urb xfer, pipe %x != type %x (hardware misbehaviour?)\n",
>>   			usb_pipetype(urb->pipe), pipetypes[xfertype]);
>>
>>   	/* Check against a simple/standard policy */
>
> You can't fix a bug by changing the line that reports it from dev_WARN
> to printk!

Of course this patch wasn't intended as "fix".
It was intended to see how the udlfb driver behaves in this situation, e.g.
if the driver then crashes afterwards.

Furthermore, why does usb_submit_urb() prints this WARNING and then continues?
If it's a real bug, why doesn't it returns an error instead?
So, in principle I still think this warning is kind of informational,
which of course points to some kind of problem which should be fixed.

> In this case it looks like dlfb_usb_probe() or one of the routines it
> calls is wrong; it assumes that an endpoint has the expected type
> without checking.  More precisely, it thinks an endpoint is BULK when
> actually it is INTERRUPT.  That's what needs to be fixed.

Maybe usb_submit_urb() should return an error so that drivers can
react on it, instead of adding the same kind of checks to all drivers?

Helge

Re: [syzbot] [fbdev?] [usb?] WARNING in dlfb_submit_urb/usb_submit_urb (2)

[Alan Stern]

On Thu, May 18, 2023 at 04:16:33PM +0200, Helge Deller wrote:
> On 5/18/23 15:54, Alan Stern wrote:
> > On Thu, May 18, 2023 at 09:34:24AM +0200, Helge Deller wrote:
> > > I think this is an informational warning from the USB stack,
> > 
> > It is not informational.  It is a warning that the caller has a bug.
> 
> I'm not a USB expert, so I searched for such bug reports, and it seems
> people sometimes faced this warning with different USB devices.

Yes.

> > You can't fix a bug by changing the line that reports it from dev_WARN
> > to printk!
> 
> Of course this patch wasn't intended as "fix".
> It was intended to see how the udlfb driver behaves in this situation, e.g.
> if the driver then crashes afterwards.
> 
> Furthermore, why does usb_submit_urb() prints this WARNING and then continues?
> If it's a real bug, why doesn't it returns an error instead?
> So, in principle I still think this warning is kind of informational,
> which of course points to some kind of problem which should be fixed.

Depending on the situation, the bug may or may not lead to an error.  At 
the time the dev_WARN was added, we were less careful about these sorts 
of checks; I did not want to cause previously working devices to stop 
working by failing the URB submission.

> > In this case it looks like dlfb_usb_probe() or one of the routines it
> > calls is wrong; it assumes that an endpoint has the expected type
> > without checking.  More precisely, it thinks an endpoint is BULK when
> > actually it is INTERRUPT.  That's what needs to be fixed.
> 
> Maybe usb_submit_urb() should return an error so that drivers can
> react on it, instead of adding the same kind of checks to all drivers?

Feel free to submit a patch doing this.  But the checks should be added 
in any case; without them the drivers are simply wrong.

Alan Stern

Re: [syzbot] [fbdev?] [usb?] WARNING in dlfb_submit_urb/usb_submit_urb (2)

[Helge Deller]

* Alan Stern <stern@rowland.harvard.edu>:
> On Thu, May 18, 2023 at 04:16:33PM +0200, Helge Deller wrote:
> > On 5/18/23 15:54, Alan Stern wrote:
> > > On Thu, May 18, 2023 at 09:34:24AM +0200, Helge Deller wrote:
> > > > I think this is an informational warning from the USB stack,
> > >
> > > It is not informational.  It is a warning that the caller has a bug.
> >
> > I'm not a USB expert, so I searched for such bug reports, and it seems
> > people sometimes faced this warning with different USB devices.
>
> Yes.
>
> > > You can't fix a bug by changing the line that reports it from dev_WARN
> > > to printk!
> >
> > Of course this patch wasn't intended as "fix".
> > It was intended to see how the udlfb driver behaves in this situation, e.g.
> > if the driver then crashes afterwards.
> >
> > Furthermore, why does usb_submit_urb() prints this WARNING and then continues?
> > If it's a real bug, why doesn't it returns an error instead?
> > So, in principle I still think this warning is kind of informational,
> > which of course points to some kind of problem which should be fixed.
>
> Depending on the situation, the bug may or may not lead to an error.  At
> the time the dev_WARN was added, we were less careful about these sorts
> of checks; I did not want to cause previously working devices to stop
> working by failing the URB submission.

Fair enough.

> > > In this case it looks like dlfb_usb_probe() or one of the routines it
> > > calls is wrong; it assumes that an endpoint has the expected type
> > > without checking.  More precisely, it thinks an endpoint is BULK when
> > > actually it is INTERRUPT.  That's what needs to be fixed.
> >
> > Maybe usb_submit_urb() should return an error so that drivers can
> > react on it, instead of adding the same kind of checks to all drivers?
>
> Feel free to submit a patch doing this.

As you wrote above, this may break other drivers too, so I'd leave that
discussion & decision to the USB maintainers (like you).

> But the checks should be added
> in any case; without them the drivers are simply wrong.

I pushed the hackish patch below through the syz tests which gives this log:
(see https://syzkaller.appspot.com/text?tag=CrashLog&x=160b7509280000)
[   77.559566][    T9] usb 1-1: Unable to get valid EDID from device/display
[   77.587021][    T9] WARNING: BOGUS urb xfer, pipe 3 != type 1 (fix driver to choose correct endpoint)
[   77.596448][    T9] usb 1-1: dlfb_urb_completion - nonzero write bulk status received: -115
[   77.605308][    T9] usb 1-1: submit urb error: -22
[   77.613225][    T9] udlfb: probe of 1-1:0.52 failed with error -22

So, basically there is no urgent fix needed for the dlfb fbdev driver,
as it will gracefully fail as is (which is correct).

What do you suggest we should do with this syzkaller-bug ?
I'd rate it as false-alarm, but it will continue to complain because of
the dev_WARN() in urb.c

Helge
---

From: Helge Deller <deller@gmx.de>
Date: Thu, 18 May 2023 19:03:56 +0200
Subject: [PATCH] fbdev: udlfb: check endpoint type, again

Temporary patch to anaylze syzbot regression:
https://syzkaller.appspot.com/bug?extid=0e22d63dcebb802b9bc8
It's not planned to apply as-is!

Fixes: aaf7dbe07385 ("video: fbdev: udlfb: properly check endpoint type")
Signed-off-by: Helge Deller <deller@gmx.de>

diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
index 9f3c54032556..bb889a1da3ef 100644
--- a/drivers/usb/core/urb.c
+++ b/drivers/usb/core/urb.c
@@ -500,9 +500,12 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
 	 */

 	/* Check that the pipe's type matches the endpoint's type */
-	if (usb_pipe_type_check(urb->dev, urb->pipe))
-		dev_WARN(&dev->dev, "BOGUS urb xfer, pipe %x != type %x\n",
+	if (usb_pipe_type_check(urb->dev, urb->pipe)) {
+		/* temporarily use printk() instead of WARN() to fix bug in udlfb driver */
+		printk("WARNING: BOGUS urb xfer, pipe %x != type %x (fix driver to choose correct endpoint)\n",
 			usb_pipetype(urb->pipe), pipetypes[xfertype]);
+		return -EINVAL;
+	}

 	/* Check against a simple/standard policy */
 	allowed = (URB_NO_TRANSFER_DMA_MAP | URB_NO_INTERRUPT | URB_DIR_MASK |
diff --git a/drivers/video/fbdev/udlfb.c b/drivers/video/fbdev/udlfb.c
index 216d49c9d47e..5e56b2889c8c 100644
--- a/drivers/video/fbdev/udlfb.c
+++ b/drivers/video/fbdev/udlfb.c
@@ -1667,8 +1667,9 @@ static int dlfb_usb_probe(struct usb_interface *intf,
 	usb_set_intfdata(intf, dlfb);

 	retval = usb_find_common_endpoints(intf->cur_altsetting, NULL, &out, NULL, NULL);
-	if (retval) {
-		dev_err(&intf->dev, "Device should have at lease 1 bulk endpoint!\n");
+	if (retval || out == NULL) {
+		retval = -ENODEV;
+		dev_err(&intf->dev, "Device should have at least one bulk endpoint!\n");
 		goto error;
 	}

Re: [syzbot] [fbdev?] [usb?] WARNING in dlfb_submit_urb/usb_submit_urb (2)

[Alan Stern]

On Thu, May 18, 2023 at 09:06:12PM +0200, Helge Deller wrote:
> * Alan Stern <stern@rowland.harvard.edu>:
> > On Thu, May 18, 2023 at 04:16:33PM +0200, Helge Deller wrote:
> > > On 5/18/23 15:54, Alan Stern wrote:
> > > > In this case it looks like dlfb_usb_probe() or one of the routines it
> > > > calls is wrong; it assumes that an endpoint has the expected type
> > > > without checking.  More precisely, it thinks an endpoint is BULK when
> > > > actually it is INTERRUPT.  That's what needs to be fixed.
> > >
> > > Maybe usb_submit_urb() should return an error so that drivers can
> > > react on it, instead of adding the same kind of checks to all drivers?
> >
> > Feel free to submit a patch doing this.
> 
> As you wrote above, this may break other drivers too, so I'd leave that
> discussion & decision to the USB maintainers (like you).
> 
> > But the checks should be added
> > in any case; without them the drivers are simply wrong.
> 
> I pushed the hackish patch below through the syz tests which gives this log:
> (see https://syzkaller.appspot.com/text?tag=CrashLog&x=160b7509280000)
> [   77.559566][    T9] usb 1-1: Unable to get valid EDID from device/display
> [   77.587021][    T9] WARNING: BOGUS urb xfer, pipe 3 != type 1 (fix driver to choose correct endpoint)
> [   77.596448][    T9] usb 1-1: dlfb_urb_completion - nonzero write bulk status received: -115
> [   77.605308][    T9] usb 1-1: submit urb error: -22
> [   77.613225][    T9] udlfb: probe of 1-1:0.52 failed with error -22
> 
> So, basically there is no urgent fix needed for the dlfb fbdev driver,
> as it will gracefully fail as is (which is correct).
> 
> What do you suggest we should do with this syzkaller-bug ?
> I'd rate it as false-alarm, but it will continue to complain because of
> the dev_WARN() in urb.c

Let's try this patch instead.  It might contain a stupid error because I 
haven't even tried to compile it, but it ought to fix the real problem.

Alan Stern

#syz test: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git a4422ff22142

Index: usb-devel/drivers/video/fbdev/udlfb.c
===================================================================
--- usb-devel.orig/drivers/video/fbdev/udlfb.c
+++ usb-devel/drivers/video/fbdev/udlfb.c
@@ -1652,7 +1652,7 @@ static int dlfb_usb_probe(struct usb_int
 	struct fb_info *info;
 	int retval;
 	struct usb_device *usbdev = interface_to_usbdev(intf);
-	struct usb_endpoint_descriptor *out;
+	static u8 out_ep[] = {1 + USB_DIR_OUT, 0};
 
 	/* usb initialization */
 	dlfb = kzalloc(sizeof(*dlfb), GFP_KERNEL);
@@ -1666,9 +1666,9 @@ static int dlfb_usb_probe(struct usb_int
 	dlfb->udev = usb_get_dev(usbdev);
 	usb_set_intfdata(intf, dlfb);
 
-	retval = usb_find_common_endpoints(intf->cur_altsetting, NULL, &out, NULL, NULL);
-	if (retval) {
-		dev_err(&intf->dev, "Device should have at lease 1 bulk endpoint!\n");
+	if (!usb_check_bulk_endpoints(intf, out_ep)) {
+		dev_err(&intf->dev, "Invalid DisplayLink device!\n");
+		retval = -EINVAL;
 		goto error;
 	}
 

Re: [syzbot] [fbdev?] [usb?] WARNING in dlfb_submit_urb/usb_submit_urb (2)

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-and-tested-by: syzbot+0e22d63dcebb802b9bc8@syzkaller.appspotmail.com

Tested on:

commit:         a4422ff2 usb: typec: qcom: Add Qualcomm PMIC Type-C dr..
git tree:       https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
console output: https://syzkaller.appspot.com/x/log.txt?x=10b6b9a6280000
kernel config:  https://syzkaller.appspot.com/x/.config?x=2414a945e4542ec1
dashboard link: https://syzkaller.appspot.com/bug?extid=0e22d63dcebb802b9bc8
compiler:       gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
patch:          https://syzkaller.appspot.com/x/patch.diff?x=1374e5a6280000

Note: testing is done by a robot and is best-effort only.

Re: [syzbot] [fbdev?] [usb?] WARNING in dlfb_submit_urb/usb_submit_urb (2)

[Helge Deller]

On 5/18/23 22:35, Alan Stern wrote:
> On Thu, May 18, 2023 at 09:06:12PM +0200, Helge Deller wrote:
>> * Alan Stern <stern@rowland.harvard.edu>:
>>> On Thu, May 18, 2023 at 04:16:33PM +0200, Helge Deller wrote:
>>>> On 5/18/23 15:54, Alan Stern wrote:
>>>>> In this case it looks like dlfb_usb_probe() or one of the routines it
>>>>> calls is wrong; it assumes that an endpoint has the expected type
>>>>> without checking.  More precisely, it thinks an endpoint is BULK when
>>>>> actually it is INTERRUPT.  That's what needs to be fixed.
>>>>
>>>> Maybe usb_submit_urb() should return an error so that drivers can
>>>> react on it, instead of adding the same kind of checks to all drivers?
>>>
>>> Feel free to submit a patch doing this.
>>
>> As you wrote above, this may break other drivers too, so I'd leave that
>> discussion & decision to the USB maintainers (like you).
>>
>>> But the checks should be added
>>> in any case; without them the drivers are simply wrong.
>>
>> I pushed the hackish patch below through the syz tests which gives this log:
>> (see https://syzkaller.appspot.com/text?tag=CrashLog&x=160b7509280000)
>> [   77.559566][    T9] usb 1-1: Unable to get valid EDID from device/display
>> [   77.587021][    T9] WARNING: BOGUS urb xfer, pipe 3 != type 1 (fix driver to choose correct endpoint)
>> [   77.596448][    T9] usb 1-1: dlfb_urb_completion - nonzero write bulk status received: -115
>> [   77.605308][    T9] usb 1-1: submit urb error: -22
>> [   77.613225][    T9] udlfb: probe of 1-1:0.52 failed with error -22
>>
>> So, basically there is no urgent fix needed for the dlfb fbdev driver,
>> as it will gracefully fail as is (which is correct).
>>
>> What do you suggest we should do with this syzkaller-bug ?
>> I'd rate it as false-alarm, but it will continue to complain because of
>> the dev_WARN() in urb.c
>
> Let's try this patch instead.  It might contain a stupid error because I
> haven't even tried to compile it, but it ought to fix the real problem.

Patch looks good and survived the test.

Will you send a proper patch to the fbdev mailing list, so that I can
include it?

Helge

>
> #syz test: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git a4422ff22142
>
> Index: usb-devel/drivers/video/fbdev/udlfb.c
> ===================================================================
> --- usb-devel.orig/drivers/video/fbdev/udlfb.c
> +++ usb-devel/drivers/video/fbdev/udlfb.c
> @@ -1652,7 +1652,7 @@ static int dlfb_usb_probe(struct usb_int
>   	struct fb_info *info;
>   	int retval;
>   	struct usb_device *usbdev = interface_to_usbdev(intf);
> -	struct usb_endpoint_descriptor *out;
> +	static u8 out_ep[] = {1 + USB_DIR_OUT, 0};
>
>   	/* usb initialization */
>   	dlfb = kzalloc(sizeof(*dlfb), GFP_KERNEL);
> @@ -1666,9 +1666,9 @@ static int dlfb_usb_probe(struct usb_int
>   	dlfb->udev = usb_get_dev(usbdev);
>   	usb_set_intfdata(intf, dlfb);
>
> -	retval = usb_find_common_endpoints(intf->cur_altsetting, NULL, &out, NULL, NULL);
> -	if (retval) {
> -		dev_err(&intf->dev, "Device should have at lease 1 bulk endpoint!\n");
> +	if (!usb_check_bulk_endpoints(intf, out_ep)) {
> +		dev_err(&intf->dev, "Invalid DisplayLink device!\n");
> +		retval = -EINVAL;
>   		goto error;
>   	}
>

Re: [syzbot] [fbdev?] [usb?] WARNING in dlfb_submit_urb/usb_submit_urb (2)

[Alan Stern]

On Fri, May 19, 2023 at 12:38:15PM +0200, Helge Deller wrote:
> Patch looks good and survived the test.
> 
> Will you send a proper patch to the fbdev mailing list, so that I can
> include it?

Will do.

While you're working on this driver, here's a suggestion for another 
improvement you can make.  The temporary buffer allocations and calls to 
usb_control_msg() in dlfb_get_edid() and dlfb_select_std_channel() can 
be replaced with calls to usb_control_msg_recv() and 
usb_control_msg_send() respectively.

Alan Stern

Re: [syzbot] [fbdev?] [usb?] WARNING in dlfb_submit_urb/usb_submit_urb (2)

[Helge Deller]

On 5/19/23 17:42, Alan Stern wrote:
> On Fri, May 19, 2023 at 12:38:15PM +0200, Helge Deller wrote:
>> Patch looks good and survived the test.
>>
>> Will you send a proper patch to the fbdev mailing list, so that I can
>> include it?
>
> Will do.

Great! Thanks!

> While you're working on this driver,

I'm not working on that driver. Just looked into it because of this
sysbot issue. I even don't have that hardware to test.

> here's a suggestion for another
> improvement you can make.  The temporary buffer allocations and calls to
> usb_control_msg() in dlfb_get_edid() and dlfb_select_std_channel() can
> be replaced with calls to usb_control_msg_recv() and
> usb_control_msg_send() respectively.

Ok, I'll look into it.

Helge

[PATCH] video: udlfb: Fix endpoint check

[Alan Stern]

The syzbot fuzzer detected a problem in the udlfb driver, caused by an
endpoint not having the expected type:


usb 1-1: Read EDID byte 0 failed: -71
usb 1-1: Unable to get valid EDID from device/display
------------[ cut here ]------------
usb 1-1: BOGUS urb xfer, pipe 3 != type 1
WARNING: CPU: 0 PID: 9 at drivers/usb/core/urb.c:504 usb_submit_urb+0xed6/0x1880
drivers/usb/core/urb.c:504
Modules linked in:
CPU: 0 PID: 9 Comm: kworker/0:1 Not tainted
6.4.0-rc1-syzkaller-00016-ga4422ff22142 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google
04/28/2023
Workqueue: usb_hub_wq hub_event
RIP: 0010:usb_submit_urb+0xed6/0x1880 drivers/usb/core/urb.c:504
...
Call Trace:
 <TASK>
 dlfb_submit_urb+0x92/0x180 drivers/video/fbdev/udlfb.c:1980
 dlfb_set_video_mode+0x21f0/0x2950 drivers/video/fbdev/udlfb.c:315
 dlfb_ops_set_par+0x2a7/0x8d0 drivers/video/fbdev/udlfb.c:1111
 dlfb_usb_probe+0x149a/0x2710 drivers/video/fbdev/udlfb.c:1743


The current approach for this issue failed to catch the problem
because it only checks for the existence of a bulk-OUT endpoint; it
doesn't check whether this endpoint is the one that the driver will
actually use.

We can fix the problem by instead checking that the endpoint used by
the driver does exist and is bulk-OUT.

Reported-and-tested-by: syzbot+0e22d63dcebb802b9bc8@syzkaller.appspotmail.com
Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
CC: Pavel Skripkin <paskripkin@gmail.com>
Fixes: aaf7dbe07385 ("video: fbdev: udlfb: properly check endpoint type")
CC: <stable@vger.kernel.org>

---

 drivers/video/fbdev/udlfb.c |   13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

Index: usb-devel/drivers/video/fbdev/udlfb.c
===================================================================
--- usb-devel.orig/drivers/video/fbdev/udlfb.c
+++ usb-devel/drivers/video/fbdev/udlfb.c
@@ -27,6 +27,8 @@
 #include <video/udlfb.h>
 #include "edid.h"
 
+#define OUT_EP_NUM	1	/* The endpoint number we will use */
+
 static const struct fb_fix_screeninfo dlfb_fix = {
 	.id =           "udlfb",
 	.type =         FB_TYPE_PACKED_PIXELS,
@@ -1652,7 +1654,7 @@ static int dlfb_usb_probe(struct usb_int
 	struct fb_info *info;
 	int retval;
 	struct usb_device *usbdev = interface_to_usbdev(intf);
-	struct usb_endpoint_descriptor *out;
+	static u8 out_ep[] = {OUT_EP_NUM + USB_DIR_OUT, 0};
 
 	/* usb initialization */
 	dlfb = kzalloc(sizeof(*dlfb), GFP_KERNEL);
@@ -1666,9 +1668,9 @@ static int dlfb_usb_probe(struct usb_int
 	dlfb->udev = usb_get_dev(usbdev);
 	usb_set_intfdata(intf, dlfb);
 
-	retval = usb_find_common_endpoints(intf->cur_altsetting, NULL, &out, NULL, NULL);
-	if (retval) {
-		dev_err(&intf->dev, "Device should have at lease 1 bulk endpoint!\n");
+	if (!usb_check_bulk_endpoints(intf, out_ep)) {
+		dev_err(&intf->dev, "Invalid DisplayLink device!\n");
+		retval = -EINVAL;
 		goto error;
 	}
 
@@ -1927,7 +1929,8 @@ retry:
 		}
 
 		/* urb->transfer_buffer_length set to actual before submit */
-		usb_fill_bulk_urb(urb, dlfb->udev, usb_sndbulkpipe(dlfb->udev, 1),
+		usb_fill_bulk_urb(urb, dlfb->udev,
+			usb_sndbulkpipe(dlfb->udev, OUT_EP_NUM),
 			buf, size, dlfb_urb_completion, unode);
 		urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
 

Re: [PATCH] video: udlfb: Fix endpoint check

[Helge Deller]

On 5/19/23 21:32, Alan Stern wrote:
> The syzbot fuzzer detected a problem in the udlfb driver, caused by an
> endpoint not having the expected type:
>
>
> usb 1-1: Read EDID byte 0 failed: -71
> usb 1-1: Unable to get valid EDID from device/display
> ------------[ cut here ]------------
> usb 1-1: BOGUS urb xfer, pipe 3 != type 1
> WARNING: CPU: 0 PID: 9 at drivers/usb/core/urb.c:504 usb_submit_urb+0xed6/0x1880
> drivers/usb/core/urb.c:504
> Modules linked in:
> CPU: 0 PID: 9 Comm: kworker/0:1 Not tainted
> 6.4.0-rc1-syzkaller-00016-ga4422ff22142 #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google
> 04/28/2023
> Workqueue: usb_hub_wq hub_event
> RIP: 0010:usb_submit_urb+0xed6/0x1880 drivers/usb/core/urb.c:504
> ...
> Call Trace:
>   <TASK>
>   dlfb_submit_urb+0x92/0x180 drivers/video/fbdev/udlfb.c:1980
>   dlfb_set_video_mode+0x21f0/0x2950 drivers/video/fbdev/udlfb.c:315
>   dlfb_ops_set_par+0x2a7/0x8d0 drivers/video/fbdev/udlfb.c:1111
>   dlfb_usb_probe+0x149a/0x2710 drivers/video/fbdev/udlfb.c:1743
>
>
> The current approach for this issue failed to catch the problem
> because it only checks for the existence of a bulk-OUT endpoint; it
> doesn't check whether this endpoint is the one that the driver will
> actually use.
>
> We can fix the problem by instead checking that the endpoint used by
> the driver does exist and is bulk-OUT.
>
> Reported-and-tested-by: syzbot+0e22d63dcebb802b9bc8@syzkaller.appspotmail.com
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> CC: Pavel Skripkin <paskripkin@gmail.com>
> Fixes: aaf7dbe07385 ("video: fbdev: udlfb: properly check endpoint type")
> CC: <stable@vger.kernel.org>

applied to fbdev git tree.

Thanks!
Helge

>
> ---
>
>   drivers/video/fbdev/udlfb.c |   13 ++++++++-----
>   1 file changed, 8 insertions(+), 5 deletions(-)
>
> Index: usb-devel/drivers/video/fbdev/udlfb.c
> ===================================================================
> --- usb-devel.orig/drivers/video/fbdev/udlfb.c
> +++ usb-devel/drivers/video/fbdev/udlfb.c
> @@ -27,6 +27,8 @@
>   #include <video/udlfb.h>
>   #include "edid.h"
>
> +#define OUT_EP_NUM	1	/* The endpoint number we will use */
> +
>   static const struct fb_fix_screeninfo dlfb_fix = {
>   	.id =           "udlfb",
>   	.type =         FB_TYPE_PACKED_PIXELS,
> @@ -1652,7 +1654,7 @@ static int dlfb_usb_probe(struct usb_int
>   	struct fb_info *info;
>   	int retval;
>   	struct usb_device *usbdev = interface_to_usbdev(intf);
> -	struct usb_endpoint_descriptor *out;
> +	static u8 out_ep[] = {OUT_EP_NUM + USB_DIR_OUT, 0};
>
>   	/* usb initialization */
>   	dlfb = kzalloc(sizeof(*dlfb), GFP_KERNEL);
> @@ -1666,9 +1668,9 @@ static int dlfb_usb_probe(struct usb_int
>   	dlfb->udev = usb_get_dev(usbdev);
>   	usb_set_intfdata(intf, dlfb);
>
> -	retval = usb_find_common_endpoints(intf->cur_altsetting, NULL, &out, NULL, NULL);
> -	if (retval) {
> -		dev_err(&intf->dev, "Device should have at lease 1 bulk endpoint!\n");
> +	if (!usb_check_bulk_endpoints(intf, out_ep)) {
> +		dev_err(&intf->dev, "Invalid DisplayLink device!\n");
> +		retval = -EINVAL;
>   		goto error;
>   	}
>
> @@ -1927,7 +1929,8 @@ retry:
>   		}
>
>   		/* urb->transfer_buffer_length set to actual before submit */
> -		usb_fill_bulk_urb(urb, dlfb->udev, usb_sndbulkpipe(dlfb->udev, 1),
> +		usb_fill_bulk_urb(urb, dlfb->udev,
> +			usb_sndbulkpipe(dlfb->udev, OUT_EP_NUM),
>   			buf, size, dlfb_urb_completion, unode);
>   		urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
>