Re: WARNING in usbhid_raw_request/usb_submit_urb (3)

From: Alan Stern
Date: Wed Apr 29 2020 - 16:11:42 EST


On Sat, 25 Apr 2020, syzbot wrote:

> Hello,
>
> syzbot has tested the proposed patch and the reproducer did not trigger crash:
>
> Reported-and-tested-by: syzbot+db339689b2101f6f6071@xxxxxxxxxxxxxxxxxxxxxxxxx

This is a two-pronged issue. The first problem is that usbhid does
something it shouldn't (it submits an URB while a device reset is in
progress). The second problem is that the USB core reports a driver
bug if a an URB is submitted for ep0 at the wrong time during a reset.

The patch tested above fixes the second issue but not the first. This
patch attempts to fix the first issue; it makes usbhid check for a
pending reset before submitting its URBs.

Alan Stern

#syz test: https://github.com/google/kasan.git 0fa84af8

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
@@ -87,6 +87,7 @@ static int hid_start_in(struct hid_devic
if (test_bit(HID_IN_POLLING, &usbhid->iofl) &&
!test_bit(HID_DISCONNECTED, &usbhid->iofl) &&
!test_bit(HID_SUSPENDED, &usbhid->iofl) &&
+ !test_bit(HID_RESET_PENDING, &usbhid->iofl) &&
!test_and_set_bit(HID_IN_RUNNING, &usbhid->iofl)) {
rc = usb_submit_urb(usbhid->urbin, GFP_ATOMIC);
if (rc != 0) {
@@ -273,6 +274,7 @@ static void hid_irq_in(struct urb *urb)
struct hid_device *hid = urb->context;
struct usbhid_device *usbhid = hid->driver_data;
int status;
+ unsigned long flags;

switch (urb->status) {
case 0: /* success */
@@ -319,7 +321,12 @@ static void hid_irq_in(struct urb *urb)
urb->status);
}

- status = usb_submit_urb(urb, GFP_ATOMIC);
+ spin_lock_irqsave(&usbhid->lock, flags);
+ if (test_bit(HID_RESET_PENDING, &usbhid->iofl))
+ status = -EPERM;
+ else
+ status = usb_submit_urb(urb, GFP_ATOMIC);
+ spin_unlock_irqrestore(&usbhid->lock, flags);
if (status) {
clear_bit(HID_IN_RUNNING, &usbhid->iofl);
if (status != -EPERM) {
@@ -339,6 +346,9 @@ static int hid_submit_out(struct hid_dev
struct usbhid_device *usbhid = hid->driver_data;
int r;

+ if (test_bit(HID_RESET_PENDING, &usbhid->iofl))
+ return -EAGAIN;
+
report = usbhid->out[usbhid->outtail].report;
raw_report = usbhid->out[usbhid->outtail].raw_report;

@@ -370,6 +380,9 @@ static int hid_submit_ctrl(struct hid_de
int len, r;
struct usbhid_device *usbhid = hid->driver_data;

+ if (test_bit(HID_RESET_PENDING, &usbhid->iofl))
+ return -EAGAIN;
+
report = usbhid->ctrl[usbhid->ctrltail].report;
raw_report = usbhid->ctrl[usbhid->ctrltail].raw_report;
dir = usbhid->ctrl[usbhid->ctrltail].dir;