From: Alan Stern <stern@rowland.harvard.edu> Date: 2021-08-18 18:49:35
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
From: Alan Stern <stern@rowland.harvard.edu> Date: 2021-08-19 15:26:29
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
From: Alan Stern <stern@rowland.harvard.edu> Date: 2021-08-19 19:53:11
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
===================================================================
From: Alan Stern <stern@rowland.harvard.edu> Date: 2021-08-20 14:06:23
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
From: Michal Kubecek <hidden> Date: 2021-08-24 11:50:14
On Thu, Aug 19, 2021 at 03:53:00PM -0400, Alan Stern wrote:
quoted hunk
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
===================================================================
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 <redacted>
Michal
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
From: Benjamin Tissoires <hidden> Date: 2021-08-24 12:34:47
On Tue, Aug 24, 2021 at 1:54 PM Jiri Kosina [off-list ref] wrote:
On Fri, 20 Aug 2021, Alan Stern wrote:
quoted
quoted
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.
quoted
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
Hello.
On čtvrtek 19. srpna 2021 21:53:00 CEST Alan Stern wrote:
quoted hunk
On Thu, Aug 19, 2021 at 10:35:11AM -0700, syzbot wrote:
quoted
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
===================================================================
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 <redacted>
once the fix is submitted.
Thanks!
--
Oleksandr Natalenko (post-factum)
From: Benjamin Tissoires <hidden> Date: 2021-08-31 09:51:54
On Tue, Aug 24, 2021 at 1:54 PM Jiri Kosina [off-list ref] wrote:
On Fri, 20 Aug 2021, Alan Stern wrote:
quoted
quoted
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.
quoted
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 <redacted>
Acked-by: Benjamin Tissoires <redacted>
Alan, would you mind resending the patch with the various tags with a
commit description? (unless I missed it...)
Cheers,
Benjamin
From: Alan Stern <stern@rowland.harvard.edu> Date: 2021-08-31 13:34:43
On Tue, Aug 31, 2021 at 11:51:31AM +0200, Benjamin Tissoires wrote:
On Tue, Aug 24, 2021 at 1:54 PM Jiri Kosina [off-list ref] wrote:
quoted
On Fri, 20 Aug 2021, Alan Stern wrote:
quoted
quoted
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.
quoted
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 <redacted>
Acked-by: Benjamin Tissoires <redacted>
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
From: Alan Stern <stern@rowland.harvard.edu> Date: 2021-09-01 15:38:16
On Tue, Aug 31, 2021 at 11:51:31AM +0200, Benjamin Tissoires wrote:
Tested-by: Benjamin Tissoires <redacted>
Acked-by: Benjamin Tissoires <redacted>
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
From: Michal Kubecek <hidden> Date: 2021-09-01 15:51:53
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:
quoted
Tested-by: Benjamin Tissoires <redacted>
Acked-by: Benjamin Tissoires <redacted>
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?
From: Alan Stern <stern@rowland.harvard.edu> Date: 2021-09-01 16:35:59
From: Michal Kubecek <redacted>
[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 <redacted>
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 <redacted>
Tested-by: Benjamin Tissoires <redacted>
Acked-by: Benjamin Tissoires <redacted>
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
===================================================================
From: Michal Kubecek <redacted>
[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 <redacted>
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 <redacted>
Tested-by: Benjamin Tissoires <redacted>
Acked-by: Benjamin Tissoires <redacted>
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