[syzbot] WARNING in hid_submit_ctrl/usb_submit_urb

[syzbot] WARNING in hid_submit_ctrl/usb_submit_urb

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit:    dd9c7df94c1b Merge branch 'akpm' (patches from Andrew)
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=14a9f66a300000
kernel config:  https://syzkaller.appspot.com/x/.config?x=f1b998c1afc13578
dashboard link: https://syzkaller.appspot.com/bug?extid=9b57a46bf1801ce2a2ca
userspace arch: i386

Unfortunately, I don't have any reproducer for this issue yet.

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

------------[ cut here ]------------
usb 7-1: BOGUS control dir, pipe 80000280 doesn't match bRequestType a1
WARNING: CPU: 0 PID: 15508 at drivers/usb/core/urb.c:410 usb_submit_urb+0x149d/0x18a0 drivers/usb/core/urb.c:410
Modules linked in:
CPU: 0 PID: 15508 Comm: syz-executor.2 Not tainted 5.14.0-rc1-syzkaller #0
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.14.0-2 04/01/2014
RIP: 0010:usb_submit_urb+0x149d/0x18a0 drivers/usb/core/urb.c:410
Code: 7c 24 40 e8 a5 e9 1f fc 48 8b 7c 24 40 e8 db 25 0c ff 45 89 e8 44 89 f1 4c 89 e2 48 89 c6 48 c7 c7 60 96 27 8a e8 e4 b2 91 03 <0f> 0b e9 a5 ee ff ff e8 77 e9 1f fc 0f b6 1d 37 2e 02 08 31 ff 41
RSP: 0018:ffffc900021cfb88 EFLAGS: 00010082
RAX: 0000000000000000 RBX: ffff8880786df058 RCX: 0000000000000000
RDX: 0000000000040000 RSI: ffffffff815d6855 RDI: fffff52000439f63
RBP: ffff88804a27bbe0 R08: 0000000000000000 R09: 0000000000000000
R10: ffffffff815d068e R11: 0000000000000000 R12: ffff8880168a42a8
R13: 00000000000000a1 R14: 0000000080000280 R15: ffff888029c86f00
FS:  0000000000000000(0000) GS:ffff88802ca00000(0063) knlGS:00000000f558cb40
CS:  0010 DS: 002b ES: 002b CR0: 0000000080050033
CR2: 00000000f5580db0 CR3: 0000000077c36000 CR4: 0000000000150ef0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
 hid_submit_ctrl+0x6ec/0xd80 drivers/hid/usbhid/hid-core.c:416
 usbhid_restart_ctrl_queue.isra.0+0x244/0x3a0 drivers/hid/usbhid/hid-core.c:258
 __usbhid_submit_report+0x6f0/0xd50 drivers/hid/usbhid/hid-core.c:603
 usbhid_submit_report drivers/hid/usbhid/hid-core.c:640 [inline]
 usbhid_init_reports+0x16e/0x3b0 drivers/hid/usbhid/hid-core.c:784
 hiddev_ioctl+0x10d4/0x1630 drivers/hid/usbhid/hiddev.c:794
 compat_ptr_ioctl+0x67/0x90 fs/ioctl.c:1105
 __do_compat_sys_ioctl+0x1c7/0x290 fs/ioctl.c:1167
 do_syscall_32_irqs_on arch/x86/entry/common.c:112 [inline]
 __do_fast_syscall_32+0x65/0xf0 arch/x86/entry/common.c:178
 do_fast_syscall_32+0x2f/0x70 arch/x86/entry/common.c:203
 entry_SYSENTER_compat_after_hwframe+0x4d/0x5c
RIP: 0023:0xf7fb3549
Code: 03 74 c0 01 10 05 03 74 b8 01 10 06 03 74 b4 01 10 07 03 74 b0 01 10 08 03 74 d8 01 00 00 00 00 00 51 52 55 89 e5 0f 34 cd 80 <5d> 5a 59 c3 90 90 90 90 8d b4 26 00 00 00 00 8d b4 26 00 00 00 00
RSP: 002b:00000000f558c5fc EFLAGS: 00000296 ORIG_RAX: 0000000000000036
RAX: ffffffffffffffda RBX: 0000000000000003 RCX: 00000000c018480d
RDX: 0000000020000080 RSI: 0000000000000000 RDI: 0000000000000000
RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000


---
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.

Re: [syzbot] WARNING in hid_submit_ctrl/usb_submit_urb

[syzbot]

syzbot has found a reproducer for the following issue on:

HEAD commit:    794c7931a242 Merge branch 'linus' of git://git.kernel.org/..
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=13af2205300000
kernel config:  https://syzkaller.appspot.com/x/.config?x=96f0602203250753
dashboard link: https://syzkaller.appspot.com/bug?extid=9b57a46bf1801ce2a2ca
compiler:       gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.1
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=11ae58ce300000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=11d71731300000

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

------------[ cut here ]------------
usb 1-1: BOGUS control dir, pipe 80000280 doesn't match bRequestType a1
WARNING: CPU: 0 PID: 8434 at drivers/usb/core/urb.c:410 usb_submit_urb+0x149d/0x18a0 drivers/usb/core/urb.c:410
Modules linked in:
CPU: 0 PID: 8434 Comm: syz-executor752 Not tainted 5.14.0-rc6-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
RIP: 0010:usb_submit_urb+0x149d/0x18a0 drivers/usb/core/urb.c:410
Code: 7c 24 40 e8 45 64 1f fc 48 8b 7c 24 40 e8 4b fc 0b ff 45 89 e8 44 89 f1 4c 89 e2 48 89 c6 48 c7 c7 e0 b2 27 8a e8 01 fc 91 03 <0f> 0b e9 a5 ee ff ff e8 17 64 1f fc 0f b6 1d 19 ca 01 08 31 ff 41
RSP: 0018:ffffc90000effbd0 EFLAGS: 00010082
RAX: 0000000000000000 RBX: ffff888027944058 RCX: 0000000000000000
RDX: ffff8880235db880 RSI: ffffffff815d85c5 RDI: fffff520001dff6c
RBP: ffff888021618140 R08: 0000000000000000 R09: 0000000000000000
R10: ffffffff815d23fe R11: 0000000000000000 R12: ffff888018aff118
R13: 00000000000000a1 R14: 0000000080000280 R15: ffff888021900400
FS:  000000000223d300(0000) GS:ffff8880b9c00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00005614a6c2a160 CR3: 00000000222ca000 CR4: 0000000000350ef0
Call Trace:
 hid_submit_ctrl+0x6ec/0xd80 drivers/hid/usbhid/hid-core.c:416
 usbhid_restart_ctrl_queue.isra.0+0x244/0x3a0 drivers/hid/usbhid/hid-core.c:258
 __usbhid_submit_report+0x6f0/0xd50 drivers/hid/usbhid/hid-core.c:603
 usbhid_submit_report drivers/hid/usbhid/hid-core.c:640 [inline]
 usbhid_init_reports+0xd7/0x3b0 drivers/hid/usbhid/hid-core.c:780
 hiddev_ioctl+0xb27/0x1630 drivers/hid/usbhid/hiddev.c:689
 vfs_ioctl fs/ioctl.c:51 [inline]
 __do_sys_ioctl fs/ioctl.c:1069 [inline]
 __se_sys_ioctl fs/ioctl.c:1055 [inline]
 __x64_sys_ioctl+0x193/0x200 fs/ioctl.c:1055
 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
 do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
 entry_SYSCALL_64_after_hwframe+0x44/0xae
RIP: 0033:0x444619
Code: 28 00 00 00 75 05 48 83 c4 28 c3 e8 21 14 00 00 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 c0 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007ffe70eb96d8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
RAX: ffffffffffffffda RBX: 00000000004004a0 RCX: 0000000000444619
RDX: 0000000000000000 RSI: 0000000000004805 RDI: 0000000000000004
RBP: 0000000000403ea0 R08: 0000000000000001 R09: 00000000004004a0
R10: 000000000000001f R11: 0000000000000246 R12: 0000000000403f30
R13: 0000000000000000 R14: 00000000004b2018 R15: 00000000004004a0
----------------
Code disassembly (best guess):
   0:	7c 24                	jl     0x26
   2:	40 e8 45 64 1f fc    	rex callq 0xfc1f644d
   8:	48 8b 7c 24 40       	mov    0x40(%rsp),%rdi
   d:	e8 4b fc 0b ff       	callq  0xff0bfc5d
  12:	45 89 e8             	mov    %r13d,%r8d
  15:	44 89 f1             	mov    %r14d,%ecx
  18:	4c 89 e2             	mov    %r12,%rdx
  1b:	48 89 c6             	mov    %rax,%rsi
  1e:	48 c7 c7 e0 b2 27 8a 	mov    $0xffffffff8a27b2e0,%rdi
  25:	e8 01 fc 91 03       	callq  0x391fc2b
  2a:	0f 0b                	ud2     <-- trapping instruction
  2c:	e9 a5 ee ff ff       	jmpq   0xffffeed6
  31:	e8 17 64 1f fc       	callq  0xfc1f644d
  36:	0f b6 1d 19 ca 01 08 	movzbl 0x801ca19(%rip),%ebx        # 0x801ca56
  3d:	31 ff                	xor    %edi,%edi
  3f:	41                   	rex.B

Re: [syzbot] WARNING in hid_submit_ctrl/usb_submit_urb

[Alan Stern]

On Wed, Aug 18, 2021 at 02:14:23AM -0700, syzbot wrote:
> syzbot has found a reproducer for the following issue on:
> 
> HEAD commit:    794c7931a242 Merge branch 'linus' of git://git.kernel.org/..
> git tree:       upstream
> console output: https://syzkaller.appspot.com/x/log.txt?x=13af2205300000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=96f0602203250753
> dashboard link: https://syzkaller.appspot.com/bug?extid=9b57a46bf1801ce2a2ca
> compiler:       gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.1
> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=11ae58ce300000
> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=11d71731300000
> 
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: syzbot+9b57a46bf1801ce2a2ca@syzkaller.appspotmail.com
> 
> ------------[ cut here ]------------
> usb 1-1: BOGUS control dir, pipe 80000280 doesn't match bRequestType a1
> WARNING: CPU: 0 PID: 8434 at drivers/usb/core/urb.c:410 usb_submit_urb+0x149d/0x18a0 drivers/usb/core/urb.c:410
> Modules linked in:
> CPU: 0 PID: 8434 Comm: syz-executor752 Not tainted 5.14.0-rc6-syzkaller #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> RIP: 0010:usb_submit_urb+0x149d/0x18a0 drivers/usb/core/urb.c:410
> Code: 7c 24 40 e8 45 64 1f fc 48 8b 7c 24 40 e8 4b fc 0b ff 45 89 e8 44 89 f1 4c 89 e2 48 89 c6 48 c7 c7 e0 b2 27 8a e8 01 fc 91 03 <0f> 0b e9 a5 ee ff ff e8 17 64 1f fc 0f b6 1d 19 ca 01 08 31 ff 41
> RSP: 0018:ffffc90000effbd0 EFLAGS: 00010082
> RAX: 0000000000000000 RBX: ffff888027944058 RCX: 0000000000000000
> RDX: ffff8880235db880 RSI: ffffffff815d85c5 RDI: fffff520001dff6c
> RBP: ffff888021618140 R08: 0000000000000000 R09: 0000000000000000
> R10: ffffffff815d23fe R11: 0000000000000000 R12: ffff888018aff118
> R13: 00000000000000a1 R14: 0000000080000280 R15: ffff888021900400
> FS:  000000000223d300(0000) GS:ffff8880b9c00000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00005614a6c2a160 CR3: 00000000222ca000 CR4: 0000000000350ef0
> Call Trace:
>  hid_submit_ctrl+0x6ec/0xd80 drivers/hid/usbhid/hid-core.c:416
>  usbhid_restart_ctrl_queue.isra.0+0x244/0x3a0 drivers/hid/usbhid/hid-core.c:258

The problem is that syzbot has created a device with a report length of 
zero.  If we use the padded length instead of the actual length, the 
error should vanish.

I believe this is fixed by Michal's patch, below.

Alan Stern

#syz test: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 794c7931a242

--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -377,27 +377,26 @@ static int hid_submit_ctrl(struct hid_device *hid)
 	len = hid_report_len(report);
 	if (dir == USB_DIR_OUT) {
 		usbhid->urbctrl->pipe = usb_sndctrlpipe(hid_to_usb_dev(hid), 0);
-		usbhid->urbctrl->transfer_buffer_length = len;
 		if (raw_report) {
 			memcpy(usbhid->ctrlbuf, raw_report, len);
 			kfree(raw_report);
 			usbhid->ctrl[usbhid->ctrltail].raw_report = NULL;
 		}
 	} else {
-		int maxpacket, padlen;
+		int maxpacket;
 
 		usbhid->urbctrl->pipe = usb_rcvctrlpipe(hid_to_usb_dev(hid), 0);
 		maxpacket = usb_maxpacket(hid_to_usb_dev(hid),
 					  usbhid->urbctrl->pipe, 0);
 		if (maxpacket > 0) {
-			padlen = DIV_ROUND_UP(len, maxpacket);
-			padlen *= maxpacket;
-			if (padlen > usbhid->bufsize)
-				padlen = usbhid->bufsize;
+			len = DIV_ROUND_UP(len, maxpacket);
+			len *= maxpacket;
+			if (len > usbhid->bufsize)
+				len = usbhid->bufsize;
 		} else
-			padlen = 0;
-		usbhid->urbctrl->transfer_buffer_length = padlen;
+			len = 0;
 	}
+	usbhid->urbctrl->transfer_buffer_length = len;
 	usbhid->urbctrl->dev = hid_to_usb_dev(hid);
 
 	usbhid->cr->bRequestType = USB_TYPE_CLASS | USB_RECIP_INTERFACE | dir;

Re: [syzbot] WARNING in hid_submit_ctrl/usb_submit_urb

[Alan Stern]

On Wed, Aug 18, 2021 at 02:14:23AM -0700, syzbot wrote:
> syzbot has found a reproducer for the following issue on:
> 
> HEAD commit:    794c7931a242 Merge branch 'linus' of git://git.kernel.org/..
> git tree:       upstream

Dmitry:

Why does syzbot persist in reporting useless names like "upstream" for 
the git tree being tested?  How is anyone supposed to know what that 
actually refers to?  Why doesn't it put the real name (maybe with 
"upstream" in parentheses as an additional comment)?

Furthermore, the last time I tried to submit a test patch with something 
like "#syz test: upstream 794c7931a242", it didn't work because syzbot 
itself didn't recognize the repository name!

This should already be fixed -- I can't remember when I first reported 
the problem but it must have been at least two years ago.  

If there isn't already a change request pending for this issue, can you 
add one?

Thanks,

Alan Stern

Re: [syzbot] WARNING in hid_submit_ctrl/usb_submit_urb

[syzbot]

syzbot has bisected this issue to:

commit 5cc59c418fde9d02859996707b9d5dfd2941c50b
Author: Alan Stern <stern@rowland.harvard.edu>
Date:   Sat May 22 02:16:23 2021 +0000

    USB: core: WARN if pipe direction != setup packet direction

bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=126f1731300000
start commit:   794c7931a242 Merge branch 'linus' of git://git.kernel.org/..
git tree:       upstream
final oops:     https://syzkaller.appspot.com/x/report.txt?x=116f1731300000
console output: https://syzkaller.appspot.com/x/log.txt?x=166f1731300000
kernel config:  https://syzkaller.appspot.com/x/.config?x=96f0602203250753
dashboard link: https://syzkaller.appspot.com/bug?extid=9b57a46bf1801ce2a2ca
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=11ae58ce300000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=11d71731300000

Reported-by: syzbot+9b57a46bf1801ce2a2ca@syzkaller.appspotmail.com
Fixes: 5cc59c418fde ("USB: core: WARN if pipe direction != setup packet direction")

For information about bisection process see: https://goo.gl/tpsmEJ#bisection

Re: [syzbot] WARNING in hid_submit_ctrl/usb_submit_urb

[syzbot]

Hello,

syzbot has tested the proposed patch but the reproducer is still triggering an issue:
WARNING in hid_submit_ctrl/usb_submit_urb

------------[ cut here ]------------
usb 1-1: BOGUS control dir, pipe 80000280 doesn't match bRequestType a1
WARNING: CPU: 1 PID: 10180 at drivers/usb/core/urb.c:410 usb_submit_urb+0x149d/0x18a0 drivers/usb/core/urb.c:410
Modules linked in:
CPU: 1 PID: 10180 Comm: syz-executor.0 Not tainted 5.14.0-rc6-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
RIP: 0010:usb_submit_urb+0x149d/0x18a0 drivers/usb/core/urb.c:410
Code: 7c 24 40 e8 45 64 1f fc 48 8b 7c 24 40 e8 4b fc 0b ff 45 89 e8 44 89 f1 4c 89 e2 48 89 c6 48 c7 c7 e0 b2 27 8a e8 01 fc 91 03 <0f> 0b e9 a5 ee ff ff e8 17 64 1f fc 0f b6 1d 19 ca 01 08 31 ff 41
RSP: 0018:ffffc9000a68fbd0 EFLAGS: 00010082
RAX: 0000000000000000 RBX: ffff88802e22d058 RCX: 0000000000000000
RDX: ffff88801b2a1c40 RSI: ffffffff815d85c5 RDI: fffff520014d1f6c
RBP: ffff888018fcd910 R08: 0000000000000000 R09: 0000000000000000
R10: ffffffff815d23fe R11: 0000000000000000 R12: ffff8880155fb9d8
R13: 00000000000000a1 R14: 0000000080000280 R15: ffff88801c247600
FS:  00007fdbff87b700(0000) GS:ffff8880b9d00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 000000000050eab0 CR3: 000000003d108000 CR4: 0000000000350ee0
Call Trace:
 hid_submit_ctrl+0x6ff/0xde0 drivers/hid/usbhid/hid-core.c:415
 usbhid_restart_ctrl_queue.isra.0+0x244/0x3a0 drivers/hid/usbhid/hid-core.c:258
 __usbhid_submit_report+0x6f0/0xd50 drivers/hid/usbhid/hid-core.c:602
 usbhid_submit_report drivers/hid/usbhid/hid-core.c:639 [inline]
 usbhid_init_reports+0xd7/0x3b0 drivers/hid/usbhid/hid-core.c:779
 hiddev_ioctl+0xb27/0x1630 drivers/hid/usbhid/hiddev.c:689
 vfs_ioctl fs/ioctl.c:51 [inline]
 __do_sys_ioctl fs/ioctl.c:1069 [inline]
 __se_sys_ioctl fs/ioctl.c:1055 [inline]
 __x64_sys_ioctl+0x193/0x200 fs/ioctl.c:1055
 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
 do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
 entry_SYSCALL_64_after_hwframe+0x44/0xae
RIP: 0033:0x4665e9
Code: ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 bc ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007fdbff87b188 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
RAX: ffffffffffffffda RBX: 000000000056bf80 RCX: 00000000004665e9
RDX: 0000000000000000 RSI: 0000000000004805 RDI: 0000000000000004
RBP: 00000000004bfcc4 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 000000000056bf80
R13: 00007ffddbcdc2ff R14: 00007fdbff87b300 R15: 0000000000022000
----------------
Code disassembly (best guess):
   0:	7c 24                	jl     0x26
   2:	40 e8 45 64 1f fc    	rex callq 0xfc1f644d
   8:	48 8b 7c 24 40       	mov    0x40(%rsp),%rdi
   d:	e8 4b fc 0b ff       	callq  0xff0bfc5d
  12:	45 89 e8             	mov    %r13d,%r8d
  15:	44 89 f1             	mov    %r14d,%ecx
  18:	4c 89 e2             	mov    %r12,%rdx
  1b:	48 89 c6             	mov    %rax,%rsi
  1e:	48 c7 c7 e0 b2 27 8a 	mov    $0xffffffff8a27b2e0,%rdi
  25:	e8 01 fc 91 03       	callq  0x391fc2b
  2a:	0f 0b                	ud2     <-- trapping instruction
  2c:	e9 a5 ee ff ff       	jmpq   0xffffeed6
  31:	e8 17 64 1f fc       	callq  0xfc1f644d
  36:	0f b6 1d 19 ca 01 08 	movzbl 0x801ca19(%rip),%ebx        # 0x801ca56
  3d:	31 ff                	xor    %edi,%edi
  3f:	41                   	rex.B


Tested on:

commit:         794c7931 Merge branch 'linus' of git://git.kernel.org/..
git tree:       https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
console output: https://syzkaller.appspot.com/x/log.txt?x=126c1765300000
kernel config:  https://syzkaller.appspot.com/x/.config?x=96f0602203250753
dashboard link: https://syzkaller.appspot.com/bug?extid=9b57a46bf1801ce2a2ca
compiler:       gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.1
patch:          https://syzkaller.appspot.com/x/patch.diff?x=152c3561300000

Re: [syzbot] WARNING in hid_submit_ctrl/usb_submit_urb

[Alan Stern]

On Wed, Aug 18, 2021 at 01:13:06PM -0700, syzbot wrote:
> Hello,
> 
> syzbot has tested the proposed patch but the reproducer is still triggering an issue:
> WARNING in hid_submit_ctrl/usb_submit_urb
> 
> ------------[ cut here ]------------
> usb 1-1: BOGUS control dir, pipe 80000280 doesn't match bRequestType a1
> WARNING: CPU: 1 PID: 10180 at drivers/usb/core/urb.c:410 usb_submit_urb+0x149d/0x18a0 drivers/usb/core/urb.c:410

Looks like I was wrong.  Let's see what's really happening.

Alan Stern

#syz test: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 794c7931a242

--- usb-devel.orig/drivers/hid/usbhid/hid-core.c
+++ usb-devel/drivers/hid/usbhid/hid-core.c
@@ -397,6 +397,8 @@ static int hid_submit_ctrl(struct hid_de
 		} else
 			padlen = 0;
 		usbhid->urbctrl->transfer_buffer_length = padlen;
+		hid_err(hid, "submit_ctrl: maxpacket %d len %d padlen %d\n",
+				maxpacket, len, padlen);
 	}
 	usbhid->urbctrl->dev = hid_to_usb_dev(hid);
 

Re: [syzbot] WARNING in hid_submit_ctrl/usb_submit_urb

[syzbot]

Hello,

syzbot has tested the proposed patch but the reproducer is still triggering an issue:
WARNING in hid_submit_ctrl/usb_submit_urb

cm6533_jd 0003:0D8C:0022.0001: submit_ctrl: maxpacket 64 len 0 padlen 0
------------[ cut here ]------------
usb 1-1: BOGUS control dir, pipe 80000280 doesn't match bRequestType a1
WARNING: CPU: 0 PID: 10203 at drivers/usb/core/urb.c:410 usb_submit_urb+0x149d/0x18a0 drivers/usb/core/urb.c:410
Modules linked in:
CPU: 0 PID: 10203 Comm: syz-executor.0 Not tainted 5.14.0-rc6-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
RIP: 0010:usb_submit_urb+0x149d/0x18a0 drivers/usb/core/urb.c:410
Code: 7c 24 40 e8 45 64 1f fc 48 8b 7c 24 40 e8 4b fc 0b ff 45 89 e8 44 89 f1 4c 89 e2 48 89 c6 48 c7 c7 e0 b2 27 8a e8 01 ec 91 03 <0f> 0b e9 a5 ee ff ff e8 17 64 1f fc 0f b6 1d 59 ca 01 08 31 ff 41
RSP: 0018:ffffc9000a7f7bd0 EFLAGS: 00010082
RAX: 0000000000000000 RBX: ffff888033e83058 RCX: 0000000000000000
RDX: ffff88802bfd8000 RSI: ffffffff815d85c5 RDI: fffff520014fef6c
RBP: ffff8880168b2410 R08: 0000000000000000 R09: 0000000000000000
R10: ffffffff815d23fe R11: 0000000000000000 R12: ffff8880217a0230
R13: 00000000000000a1 R14: 0000000080000280 R15: ffff88801803aa00
FS:  00007f92577f8700(0000) GS:ffff8880b9c00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 000000000050eab0 CR3: 000000003dd81000 CR4: 0000000000350ef0
Call Trace:
 hid_submit_ctrl+0x6db/0xa70 drivers/hid/usbhid/hid-core.c:418
 usbhid_restart_ctrl_queue.isra.0+0x244/0x3a0 drivers/hid/usbhid/hid-core.c:258
 __usbhid_submit_report+0x6f0/0xd50 drivers/hid/usbhid/hid-core.c:605
 usbhid_submit_report drivers/hid/usbhid/hid-core.c:642 [inline]
 usbhid_init_reports+0xd7/0x3b0 drivers/hid/usbhid/hid-core.c:782
 hiddev_ioctl+0xb27/0x1630 drivers/hid/usbhid/hiddev.c:689
 vfs_ioctl fs/ioctl.c:51 [inline]
 __do_sys_ioctl fs/ioctl.c:1069 [inline]
 __se_sys_ioctl fs/ioctl.c:1055 [inline]
 __x64_sys_ioctl+0x193/0x200 fs/ioctl.c:1055
 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
 do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
 entry_SYSCALL_64_after_hwframe+0x44/0xae
RIP: 0033:0x4665e9
Code: ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 bc ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007f92577f8188 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
RAX: ffffffffffffffda RBX: 000000000056bf80 RCX: 00000000004665e9
RDX: 0000000000000000 RSI: 0000000000004805 RDI: 0000000000000004
RBP: 00000000004bfcc4 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 000000000056bf80
R13: 00007ffcad46f49f R14: 00007f92577f8300 R15: 0000000000022000


Tested on:

commit:         794c7931 Merge branch 'linus' of git://git.kernel.org/..
git tree:       https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
console output: https://syzkaller.appspot.com/x/log.txt?x=107e8c99300000
kernel config:  https://syzkaller.appspot.com/x/.config?x=96f0602203250753
dashboard link: https://syzkaller.appspot.com/bug?extid=9b57a46bf1801ce2a2ca
compiler:       gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.1
patch:          https://syzkaller.appspot.com/x/patch.diff?x=14dde499300000

Re: [syzbot] WARNING in hid_submit_ctrl/usb_submit_urb

[Alan Stern]

On Thu, Aug 19, 2021 at 10:35:11AM -0700, syzbot wrote:
> Hello,
> 
> syzbot has tested the proposed patch but the reproducer is still triggering an issue:
> WARNING in hid_submit_ctrl/usb_submit_urb
> 
> cm6533_jd 0003:0D8C:0022.0001: submit_ctrl: maxpacket 64 len 0 padlen 0
> ------------[ cut here ]------------
> usb 1-1: BOGUS control dir, pipe 80000280 doesn't match bRequestType a1

Ah.   The padding code doesn't add anything if the length is 
already a multiple of the maxpacket value, and of course 0 is such 
a multiple.

The following simplified variant of Michal's patch should fix the 
problem.

Alan Stern

#syz test: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 794c7931a242

Index: usb-devel/drivers/hid/usbhid/hid-core.c
===================================================================
--- usb-devel.orig/drivers/hid/usbhid/hid-core.c
+++ usb-devel/drivers/hid/usbhid/hid-core.c
@@ -377,27 +377,23 @@ static int hid_submit_ctrl(struct hid_de
 	len = hid_report_len(report);
 	if (dir == USB_DIR_OUT) {
 		usbhid->urbctrl->pipe = usb_sndctrlpipe(hid_to_usb_dev(hid), 0);
-		usbhid->urbctrl->transfer_buffer_length = len;
 		if (raw_report) {
 			memcpy(usbhid->ctrlbuf, raw_report, len);
 			kfree(raw_report);
 			usbhid->ctrl[usbhid->ctrltail].raw_report = NULL;
 		}
 	} else {
-		int maxpacket, padlen;
+		int maxpacket;
 
 		usbhid->urbctrl->pipe = usb_rcvctrlpipe(hid_to_usb_dev(hid), 0);
 		maxpacket = usb_maxpacket(hid_to_usb_dev(hid),
 					  usbhid->urbctrl->pipe, 0);
-		if (maxpacket > 0) {
-			padlen = DIV_ROUND_UP(len, maxpacket);
-			padlen *= maxpacket;
-			if (padlen > usbhid->bufsize)
-				padlen = usbhid->bufsize;
-		} else
-			padlen = 0;
-		usbhid->urbctrl->transfer_buffer_length = padlen;
+		len += (len == 0);	/* Don't allow 0-length reports */
+		len = round_up(len, maxpacket);
+		if (len > usbhid->bufsize)
+			len = usbhid->bufsize;
 	}
+	usbhid->urbctrl->transfer_buffer_length = len;
 	usbhid->urbctrl->dev = hid_to_usb_dev(hid);
 
 	usbhid->cr->bRequestType = USB_TYPE_CLASS | USB_RECIP_INTERFACE | dir;

Re: [syzbot] WARNING in hid_submit_ctrl/usb_submit_urb

[syzbot]

Hello,

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

Reported-and-tested-by: syzbot+9b57a46bf1801ce2a2ca@syzkaller.appspotmail.com

Tested on:

commit:         794c7931 Merge branch 'linus' of git://git.kernel.org/..
git tree:       https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel config:  https://syzkaller.appspot.com/x/.config?x=96f0602203250753
dashboard link: https://syzkaller.appspot.com/bug?extid=9b57a46bf1801ce2a2ca
compiler:       gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.1
patch:          https://syzkaller.appspot.com/x/patch.diff?x=119cfb31300000

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

Re: [syzbot] WARNING in hid_submit_ctrl/usb_submit_urb

[Alan Stern]

On Thu, Aug 19, 2021 at 05:40:07PM -0700, syzbot wrote:
> Hello,
> 
> syzbot has tested the proposed patch and the reproducer did not trigger any issue:

That's good to know.  Still, I suspect there's a better way of handling 
this condition.

In particular, does it make sense to accept descriptors for input or 
feature reports with length zero?  I can't imagine what good such 
reports would do.

On the other hand, I'm not familiar enough with the code to know the 
right way to reject these descriptors and reports.  It looks like the 
HID subsystem was not designed with this sort of check in mind.

Benjamin and Jiri, what do you think?  Is it okay to allow descriptors 
for zero-length reports and just pretend they have length 1 (as the 
patch tested by syzbot did), or should we instead reject them during 
probing?

Alan Stern

Re: [syzbot] WARNING in hid_submit_ctrl/usb_submit_urb

[Michal Kubecek]

[-- Attachment #1: Type: text/plain, Size: 2066 bytes --]

On Thu, Aug 19, 2021 at 03:53:00PM -0400, Alan Stern wrote:
> The following simplified variant of Michal's patch should fix the 
> problem.
> 
> Alan Stern
> 
> #syz test: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 794c7931a242
> 
> Index: usb-devel/drivers/hid/usbhid/hid-core.c
> ===================================================================
> --- usb-devel.orig/drivers/hid/usbhid/hid-core.c
> +++ usb-devel/drivers/hid/usbhid/hid-core.c
> @@ -377,27 +377,23 @@ static int hid_submit_ctrl(struct hid_de
>  	len = hid_report_len(report);
>  	if (dir == USB_DIR_OUT) {
>  		usbhid->urbctrl->pipe = usb_sndctrlpipe(hid_to_usb_dev(hid), 0);
> -		usbhid->urbctrl->transfer_buffer_length = len;
>  		if (raw_report) {
>  			memcpy(usbhid->ctrlbuf, raw_report, len);
>  			kfree(raw_report);
>  			usbhid->ctrl[usbhid->ctrltail].raw_report = NULL;
>  		}
>  	} else {
> -		int maxpacket, padlen;
> +		int maxpacket;
>  
>  		usbhid->urbctrl->pipe = usb_rcvctrlpipe(hid_to_usb_dev(hid), 0);
>  		maxpacket = usb_maxpacket(hid_to_usb_dev(hid),
>  					  usbhid->urbctrl->pipe, 0);
> -		if (maxpacket > 0) {
> -			padlen = DIV_ROUND_UP(len, maxpacket);
> -			padlen *= maxpacket;
> -			if (padlen > usbhid->bufsize)
> -				padlen = usbhid->bufsize;
> -		} else
> -			padlen = 0;
> -		usbhid->urbctrl->transfer_buffer_length = padlen;
> +		len += (len == 0);	/* Don't allow 0-length reports */
> +		len = round_up(len, maxpacket);
> +		if (len > usbhid->bufsize)
> +			len = usbhid->bufsize;
>  	}
> +	usbhid->urbctrl->transfer_buffer_length = len;
>  	usbhid->urbctrl->dev = hid_to_usb_dev(hid);
>  
>  	usbhid->cr->bRequestType = USB_TYPE_CLASS | USB_RECIP_INTERFACE | dir;

I have also tested this patch on top of 5.14-rc7 on my system and it
does address the original issue (no error/warning messages in kernel log
and communication with the UPS works correctly). So if you are going to
submit the patch formally, feel free to use

Tested-by: Michal Kubecek <mkubecek@suse.cz>

Michal

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [syzbot] WARNING in hid_submit_ctrl/usb_submit_urb

[Jiri Kosina]

On Fri, 20 Aug 2021, Alan Stern wrote:

> > syzbot has tested the proposed patch and the reproducer did not trigger any issue:
> 
> That's good to know.  Still, I suspect there's a better way of handling 
> this condition.
> 
> In particular, does it make sense to accept descriptors for input or 
> feature reports with length zero?  I can't imagine what good such 
> reports would do.

I quickly went through drivers + some hidraw users, and can't spot any use 
case for it.

> On the other hand, I'm not familiar enough with the code to know the 
> right way to reject these descriptors and reports.  It looks like the 
> HID subsystem was not designed with this sort of check in mind.
> 
> Benjamin and Jiri, what do you think?  Is it okay to allow descriptors 
> for zero-length reports and just pretend they have length 1 (as the 
> patch tested by syzbot did), or should we instead reject them during 
> probing?

I think it's a good band-aid for 5.14 (or 5.14-stable if we don't make 
it), and if it turns out to break something (which I don't expect), than 
we can look into rejecting already during probe.

Benjamin, is there a way to run this quickly through your HID regression 
testing machinery?

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [syzbot] WARNING in hid_submit_ctrl/usb_submit_urb

[Benjamin Tissoires]

On Tue, Aug 24, 2021 at 1:54 PM Jiri Kosina <jikos@kernel.org> wrote:
>
> On Fri, 20 Aug 2021, Alan Stern wrote:
>
> > > syzbot has tested the proposed patch and the reproducer did not trigger any issue:
> >
> > That's good to know.  Still, I suspect there's a better way of handling
> > this condition.
> >
> > In particular, does it make sense to accept descriptors for input or
> > feature reports with length zero?  I can't imagine what good such
> > reports would do.
>
> I quickly went through drivers + some hidraw users, and can't spot any use
> case for it.
>
> > On the other hand, I'm not familiar enough with the code to know the
> > right way to reject these descriptors and reports.  It looks like the
> > HID subsystem was not designed with this sort of check in mind.
> >
> > Benjamin and Jiri, what do you think?  Is it okay to allow descriptors
> > for zero-length reports and just pretend they have length 1 (as the
> > patch tested by syzbot did), or should we instead reject them during
> > probing?
>
> I think it's a good band-aid for 5.14 (or 5.14-stable if we don't make
> it), and if it turns out to break something (which I don't expect), than
> we can look into rejecting already during probe.
>
> Benjamin, is there a way to run this quickly through your HID regression
> testing machinery?

[Sorry, on holidays since last week until the end of this one]

This patch addresses usbhid, so I don't have tests on this unless I
manually plug mice, keyboards or random input hardware :(

I can try to quickly get a logitech dj receiver test that should use
heavily control endpoints, and probably get a Wacom test. No guarantee
I can get it today though.

Cheers,
Benjamin

>
> Thanks,
>
> --
> Jiri Kosina
> SUSE Labs
>

Re: [syzbot] WARNING in hid_submit_ctrl/usb_submit_urb

[Oleksandr Natalenko]

Hello.

On čtvrtek 19. srpna 2021 21:53:00 CEST Alan Stern wrote:
> On Thu, Aug 19, 2021 at 10:35:11AM -0700, syzbot wrote:
> > Hello,
> > 
> > syzbot has tested the proposed patch but the reproducer is still
> > triggering an issue: WARNING in hid_submit_ctrl/usb_submit_urb
> > 
> > cm6533_jd 0003:0D8C:0022.0001: submit_ctrl: maxpacket 64 len 0 padlen 0
> > ------------[ cut here ]------------
> > usb 1-1: BOGUS control dir, pipe 80000280 doesn't match bRequestType a1
> 
> Ah.   The padding code doesn't add anything if the length is
> already a multiple of the maxpacket value, and of course 0 is such
> a multiple.
> 
> The following simplified variant of Michal's patch should fix the
> problem.
> 
> Alan Stern
> 
> #syz test:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> 794c7931a242
> 
> Index: usb-devel/drivers/hid/usbhid/hid-core.c
> ===================================================================
> --- usb-devel.orig/drivers/hid/usbhid/hid-core.c
> +++ usb-devel/drivers/hid/usbhid/hid-core.c
> @@ -377,27 +377,23 @@ static int hid_submit_ctrl(struct hid_de
>  	len = hid_report_len(report);
>  	if (dir == USB_DIR_OUT) {
>  		usbhid->urbctrl->pipe = usb_sndctrlpipe(hid_to_usb_dev(hid), 
0);
> -		usbhid->urbctrl->transfer_buffer_length = len;
>  		if (raw_report) {
>  			memcpy(usbhid->ctrlbuf, raw_report, len);
>  			kfree(raw_report);
>  			usbhid->ctrl[usbhid->ctrltail].raw_report = NULL;
>  		}
>  	} else {
> -		int maxpacket, padlen;
> +		int maxpacket;
> 
>  		usbhid->urbctrl->pipe = usb_rcvctrlpipe(hid_to_usb_dev(hid), 
0);
>  		maxpacket = usb_maxpacket(hid_to_usb_dev(hid),
>  					  usbhid->urbctrl->pipe, 0);
> -		if (maxpacket > 0) {
> -			padlen = DIV_ROUND_UP(len, maxpacket);
> -			padlen *= maxpacket;
> -			if (padlen > usbhid->bufsize)
> -				padlen = usbhid->bufsize;
> -		} else
> -			padlen = 0;
> -		usbhid->urbctrl->transfer_buffer_length = padlen;
> +		len += (len == 0);	/* Don't allow 0-length reports */
> +		len = round_up(len, maxpacket);
> +		if (len > usbhid->bufsize)
> +			len = usbhid->bufsize;
>  	}
> +	usbhid->urbctrl->transfer_buffer_length = len;
>  	usbhid->urbctrl->dev = hid_to_usb_dev(hid);
> 
>  	usbhid->cr->bRequestType = USB_TYPE_CLASS | USB_RECIP_INTERFACE | dir;

I've tried both Michal's patch as well as this one, and both work for me, 
hence feel free to add this:

Tested-by: Oleksandr Natalenko <oleksandr@natalenko.name>

once the fix is submitted.

Thanks!

-- 
Oleksandr Natalenko (post-factum)

Re: [syzbot] WARNING in hid_submit_ctrl/usb_submit_urb

[Benjamin Tissoires]

On Tue, Aug 24, 2021 at 1:54 PM Jiri Kosina <jikos@kernel.org> wrote:
>
> On Fri, 20 Aug 2021, Alan Stern wrote:
>
> > > syzbot has tested the proposed patch and the reproducer did not trigger any issue:
> >
> > That's good to know.  Still, I suspect there's a better way of handling
> > this condition.
> >
> > In particular, does it make sense to accept descriptors for input or
> > feature reports with length zero?  I can't imagine what good such
> > reports would do.
>
> I quickly went through drivers + some hidraw users, and can't spot any use
> case for it.
>
> > On the other hand, I'm not familiar enough with the code to know the
> > right way to reject these descriptors and reports.  It looks like the
> > HID subsystem was not designed with this sort of check in mind.
> >
> > Benjamin and Jiri, what do you think?  Is it okay to allow descriptors
> > for zero-length reports and just pretend they have length 1 (as the
> > patch tested by syzbot did), or should we instead reject them during
> > probing?
>
> I think it's a good band-aid for 5.14 (or 5.14-stable if we don't make
> it), and if it turns out to break something (which I don't expect), than
> we can look into rejecting already during probe.
>
> Benjamin, is there a way to run this quickly through your HID regression
> testing machinery?
>

I have finally been able to test this patch:
- the testsuite is still passing (of course, this is not hid-core related)
- Logitech unify receivers are fine (according to the automated tests)
- Gaming mice with hidraw calls works (with libratbag in userspace)
- Wacom Intuos Pro still works (so the usbhid calls to enable the
tablet mode are still OK)

->
Tested-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Acked-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Alan, would you mind resending the patch with the various tags with a
commit description? (unless I missed it...)

Cheers,
Benjamin

Re: [syzbot] WARNING in hid_submit_ctrl/usb_submit_urb

[Alan Stern]

On Tue, Aug 31, 2021 at 11:51:31AM +0200, Benjamin Tissoires wrote:
> On Tue, Aug 24, 2021 at 1:54 PM Jiri Kosina <jikos@kernel.org> wrote:
> >
> > On Fri, 20 Aug 2021, Alan Stern wrote:
> >
> > > > syzbot has tested the proposed patch and the reproducer did not trigger any issue:
> > >
> > > That's good to know.  Still, I suspect there's a better way of handling
> > > this condition.
> > >
> > > In particular, does it make sense to accept descriptors for input or
> > > feature reports with length zero?  I can't imagine what good such
> > > reports would do.
> >
> > I quickly went through drivers + some hidraw users, and can't spot any use
> > case for it.
> >
> > > On the other hand, I'm not familiar enough with the code to know the
> > > right way to reject these descriptors and reports.  It looks like the
> > > HID subsystem was not designed with this sort of check in mind.
> > >
> > > Benjamin and Jiri, what do you think?  Is it okay to allow descriptors
> > > for zero-length reports and just pretend they have length 1 (as the
> > > patch tested by syzbot did), or should we instead reject them during
> > > probing?
> >
> > I think it's a good band-aid for 5.14 (or 5.14-stable if we don't make
> > it), and if it turns out to break something (which I don't expect), than
> > we can look into rejecting already during probe.
> >
> > Benjamin, is there a way to run this quickly through your HID regression
> > testing machinery?
> >
> 
> I have finally been able to test this patch:
> - the testsuite is still passing (of course, this is not hid-core related)
> - Logitech unify receivers are fine (according to the automated tests)
> - Gaming mice with hidraw calls works (with libratbag in userspace)
> - Wacom Intuos Pro still works (so the usbhid calls to enable the
> tablet mode are still OK)
> 
> ->
> Tested-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> Acked-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> 
> Alan, would you mind resending the patch with the various tags with a
> commit description? (unless I missed it...)

Will do.  I'm rather busy today, so it may have to wait until tomorrow.

Alan Stern

Re: [syzbot] WARNING in hid_submit_ctrl/usb_submit_urb

[Jiri Kosina]

On Tue, 31 Aug 2021, Alan Stern wrote:

> > Alan, would you mind resending the patch with the various tags with a
> > commit description? (unless I missed it...)
> 
> Will do.  I'm rather busy today, so it may have to wait until tomorrow.

Thanks. I'll wait with my pull request to Linus for tomorrow then, to make 
sure we get the fix into -rc1 already.

-- 
Jiri Kosina
SUSE Labs

Re: [syzbot] WARNING in hid_submit_ctrl/usb_submit_urb

[Alan Stern]

On Tue, Aug 31, 2021 at 11:51:31AM +0200, Benjamin Tissoires wrote:
> Tested-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> Acked-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> 
> Alan, would you mind resending the patch with the various tags with a
> commit description? (unless I missed it...)

I plan to break this up into three patches, each doing a single thing.  The 
first patch in the series will be the one written by Michal.  The second 
will fix the problem found by syzbot, and the third will be a general 
cleanup.

Michal, is it okay to add your Signed-off-by: tag to the first patch?

Alan Stern

Re: [syzbot] WARNING in hid_submit_ctrl/usb_submit_urb

[Michal Kubecek]

[-- Attachment #1: Type: text/plain, Size: 743 bytes --]

On Wed, Sep 01, 2021 at 11:38:11AM -0400, Alan Stern wrote:
> On Tue, Aug 31, 2021 at 11:51:31AM +0200, Benjamin Tissoires wrote:
> > Tested-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > Acked-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > 
> > Alan, would you mind resending the patch with the various tags with a
> > commit description? (unless I missed it...)
> 
> I plan to break this up into three patches, each doing a single thing.  The 
> first patch in the series will be the one written by Michal.  The second 
> will fix the problem found by syzbot, and the third will be a general 
> cleanup.
> 
> Michal, is it okay to add your Signed-off-by: tag to the first patch?

Yes, sure.

Michal

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 1/3] HID: usbhid: Fix flood of "control queue full" messages

[Alan Stern]

From: Michal Kubecek <mkubecek@suse.cz>

[patch description by Alan Stern]

Commit 7652dd2c5cb7 ("USB: core: Check buffer length matches wLength
for control transfers") causes control URB submissions to fail if the
transfer_buffer_length value disagrees with the setup packet's wLength
valuel.  Unfortunately, it turns out that the usbhid can trigger this
failure mode when it submits a control request for an input report: It
pads the transfer buffer size to a multiple of the maxpacket value but
does not increase wLength correspondingly.

These failures have caused problems for people using an APS UPC, in
the form of a flood of log messages resembling:

	hid-generic 0003:051D:0002.0002: control queue full

This patch fixes the problem by setting the wLength value equal to the
padded transfer_buffer_length value in hid_submit_ctrl().  As a nice
bonus, the code which stores the transfer_buffer_length value is now
shared between the two branches of an "if" statement, so it can be
de-duplicated.

Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Fixes: 7652dd2c5cb7 ("USB: core: Check buffer length matches wLength for control transfers")
Tested-by: Oleksandr Natalenko <oleksandr@natalenko.name>
Tested-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Acked-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cc: stable@vger.kernel.org

---

 drivers/hid/usbhid/hid-core.c |   15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

Index: usb-devel/drivers/hid/usbhid/hid-core.c
===================================================================
--- usb-devel.orig/drivers/hid/usbhid/hid-core.c
+++ usb-devel/drivers/hid/usbhid/hid-core.c
@@ -377,27 +377,26 @@ static int hid_submit_ctrl(struct hid_de
 	len = hid_report_len(report);
 	if (dir == USB_DIR_OUT) {
 		usbhid->urbctrl->pipe = usb_sndctrlpipe(hid_to_usb_dev(hid), 0);
-		usbhid->urbctrl->transfer_buffer_length = len;
 		if (raw_report) {
 			memcpy(usbhid->ctrlbuf, raw_report, len);
 			kfree(raw_report);
 			usbhid->ctrl[usbhid->ctrltail].raw_report = NULL;
 		}
 	} else {
-		int maxpacket, padlen;
+		int maxpacket;
 
 		usbhid->urbctrl->pipe = usb_rcvctrlpipe(hid_to_usb_dev(hid), 0);
 		maxpacket = usb_maxpacket(hid_to_usb_dev(hid),
 					  usbhid->urbctrl->pipe, 0);
 		if (maxpacket > 0) {
-			padlen = DIV_ROUND_UP(len, maxpacket);
-			padlen *= maxpacket;
-			if (padlen > usbhid->bufsize)
-				padlen = usbhid->bufsize;
+			len = DIV_ROUND_UP(len, maxpacket);
+			len *= maxpacket;
+			if (len > usbhid->bufsize)
+				len = usbhid->bufsize;
 		} else
-			padlen = 0;
-		usbhid->urbctrl->transfer_buffer_length = padlen;
+			len = 0;
 	}
+	usbhid->urbctrl->transfer_buffer_length = len;
 	usbhid->urbctrl->dev = hid_to_usb_dev(hid);
 
 	usbhid->cr->bRequestType = USB_TYPE_CLASS | USB_RECIP_INTERFACE | dir;

Re: [PATCH 1/3] HID: usbhid: Fix flood of "control queue full" messages

[Jiri Kosina]

On Wed, 1 Sep 2021, Alan Stern wrote:

> From: Michal Kubecek <mkubecek@suse.cz>
> 
> [patch description by Alan Stern]
> 
> Commit 7652dd2c5cb7 ("USB: core: Check buffer length matches wLength
> for control transfers") causes control URB submissions to fail if the
> transfer_buffer_length value disagrees with the setup packet's wLength
> valuel.  Unfortunately, it turns out that the usbhid can trigger this
> failure mode when it submits a control request for an input report: It
> pads the transfer buffer size to a multiple of the maxpacket value but
> does not increase wLength correspondingly.
> 
> These failures have caused problems for people using an APS UPC, in
> the form of a flood of log messages resembling:
> 
> 	hid-generic 0003:051D:0002.0002: control queue full
> 
> This patch fixes the problem by setting the wLength value equal to the
> padded transfer_buffer_length value in hid_submit_ctrl().  As a nice
> bonus, the code which stores the transfer_buffer_length value is now
> shared between the two branches of an "if" statement, so it can be
> de-duplicated.
> 
> Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> Fixes: 7652dd2c5cb7 ("USB: core: Check buffer length matches wLength for control transfers")
> Tested-by: Oleksandr Natalenko <oleksandr@natalenko.name>
> Tested-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> Acked-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> Cc: stable@vger.kernel.org
> 

Thanks Alan, applied and I will be sending whole HID tree to Linus soon.

(BTW, something broke your threading, so 2/3 and 3/3 were not threaded 
together with 1/3).

-- 
Jiri Kosina
SUSE Labs