| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Pine.LNX.4.44L0.2004301050190.27217-100000@netrider.rowland.org>
Date: Thu, 30 Apr 2020 10:58:33 -0400 (EDT)
From: Alan Stern <stern@...land.harvard.edu>
To: syzbot <syzbot+db339689b2101f6f6071@...kaller.appspotmail.com>
cc: andreyknvl@...gle.com, <gregkh@...uxfoundation.org>,
<ingrassia@...genesys.com>, <linux-kernel@...r.kernel.org>,
<linux-usb@...r.kernel.org>, <syzkaller-bugs@...glegroups.com>
Subject: Re: WARNING in usbhid_raw_request/usb_submit_urb (3)
On Wed, 29 Apr 2020, syzbot wrote:
> Hello,
>
> syzbot has tested the proposed patch but the reproducer still triggered crash:
> WARNING in usbhid_raw_request/usb_submit_urb
Ugh. I forgot that usbhid also uses usb_control_msg() and
usb_interrupt_msg(). Those things can't be protected by a spinlock;
I'll have to add a semaphore. (A mutex won't work either, because it
has to be locked in hid_pre_reset() and unlocked in hid_post_reset().)
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) {
@@ -339,6 +340,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 +374,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;
@@ -654,26 +661,37 @@ static int usbhid_wait_io(struct hid_dev
return 0;
}
-static int hid_set_idle(struct usb_device *dev, int ifnum, int report, int idle)
+static int hid_set_idle(struct hid_device *hid,
+ struct usb_device *dev, int ifnum, int report, int idle)
{
- return usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
+ struct usbhid_device *usbhid = hid->driver_data;
+ int rc;
+
+ down(&usbhid->sem);
+ rc = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
HID_REQ_SET_IDLE, USB_TYPE_CLASS | USB_RECIP_INTERFACE, (idle << 8) | report,
ifnum, NULL, 0, USB_CTRL_SET_TIMEOUT);
+ up(&usbhid->sem);
+ return rc;
}
-static int hid_get_class_descriptor(struct usb_device *dev, int ifnum,
+static int hid_get_class_descriptor(struct hid_device *hid,
+ struct usb_device *dev, int ifnum,
unsigned char type, void *buf, int size)
{
+ struct usbhid_device *usbhid = hid->driver_data;
int result, retries = 4;
memset(buf, 0, size);
+ down(&usbhid->sem);
do {
result = usb_control_msg(dev, usb_rcvctrlpipe(dev, 0),
USB_REQ_GET_DESCRIPTOR, USB_RECIP_INTERFACE | USB_DIR_IN,
(type << 8), ifnum, buf, size, USB_CTRL_GET_TIMEOUT);
retries--;
} while (result < size && retries);
+ up(&usbhid->sem);
return result;
}
@@ -682,16 +700,21 @@ static int usbhid_open(struct hid_device
struct usbhid_device *usbhid = hid->driver_data;
int res;
+ mutex_lock(&usbhid->mutex);
+
set_bit(HID_OPENED, &usbhid->iofl);
- if (hid->quirks & HID_QUIRK_ALWAYS_POLL)
- return 0;
+ if (hid->quirks & HID_QUIRK_ALWAYS_POLL) {
+ res = 0;
+ goto Done;
+ }
res = usb_autopm_get_interface(usbhid->intf);
/* the device must be awake to reliably request remote wakeup */
if (res < 0) {
clear_bit(HID_OPENED, &usbhid->iofl);
- return -EIO;
+ res = -EIO;
+ goto Done;
}
usbhid->intf->needs_remote_wakeup = 1;
@@ -725,6 +748,9 @@ static int usbhid_open(struct hid_device
msleep(50);
clear_bit(HID_RESUME_RUNNING, &usbhid->iofl);
+
+ Done:
+ mutex_unlock(&usbhid->mutex);
return res;
}
@@ -732,6 +758,8 @@ static void usbhid_close(struct hid_devi
{
struct usbhid_device *usbhid = hid->driver_data;
+ mutex_lock(&usbhid->mutex);
+
/*
* Make sure we don't restart data acquisition due to
* a resumption we no longer care about by avoiding racing
@@ -743,12 +771,13 @@ static void usbhid_close(struct hid_devi
clear_bit(HID_IN_POLLING, &usbhid->iofl);
spin_unlock_irq(&usbhid->lock);
- if (hid->quirks & HID_QUIRK_ALWAYS_POLL)
- return;
+ if (!(hid->quirks & HID_QUIRK_ALWAYS_POLL)) {
+ hid_cancel_delayed_stuff(usbhid);
+ usb_kill_urb(usbhid->urbin);
+ usbhid->intf->needs_remote_wakeup = 0;
+ }
- hid_cancel_delayed_stuff(usbhid);
- usb_kill_urb(usbhid->urbin);
- usbhid->intf->needs_remote_wakeup = 0;
+ mutex_unlock(&usbhid->mutex);
}
/*
@@ -877,12 +906,14 @@ static int usbhid_get_raw_report(struct
count--;
skipped_report_id = 1;
}
+ down(&usbhid->sem);
ret = usb_control_msg(dev, usb_rcvctrlpipe(dev, 0),
HID_REQ_GET_REPORT,
USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
((report_type + 1) << 8) | report_number,
interface->desc.bInterfaceNumber, buf, count,
USB_CTRL_SET_TIMEOUT);
+ up(&usbhid->sem);
/* count also the report id */
if (ret > 0 && skipped_report_id)
@@ -914,12 +945,14 @@ static int usbhid_set_raw_report(struct
skipped_report_id = 1;
}
+ down(&usbhid->sem);
ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
HID_REQ_SET_REPORT,
USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
((rtype + 1) << 8) | reportnum,
interface->desc.bInterfaceNumber, buf, count,
USB_CTRL_SET_TIMEOUT);
+ up(&usbhid->sem);
/* count also the report id, if this was a numbered report. */
if (ret > 0 && skipped_report_id)
ret++;
@@ -943,9 +976,11 @@ static int usbhid_output_report(struct h
skipped_report_id = 1;
}
+ down(&usbhid->sem);
ret = usb_interrupt_msg(dev, usbhid->urbout->pipe,
buf, count, &actual_length,
USB_CTRL_SET_TIMEOUT);
+ up(&usbhid->sem);
/* return the number of bytes transferred */
if (ret == 0) {
ret = actual_length;
@@ -1024,9 +1059,10 @@ static int usbhid_parse(struct hid_devic
if (!rdesc)
return -ENOMEM;
- hid_set_idle(dev, interface->desc.bInterfaceNumber, 0, 0);
+ hid_set_idle(hid, dev, interface->desc.bInterfaceNumber, 0, 0);
- ret = hid_get_class_descriptor(dev, interface->desc.bInterfaceNumber,
+ ret = hid_get_class_descriptor(hid, dev,
+ interface->desc.bInterfaceNumber,
HID_DT_REPORT, rdesc, rsize);
if (ret < 0) {
dbg_hid("reading report descriptor failed\n");
@@ -1057,6 +1093,8 @@ static int usbhid_start(struct hid_devic
unsigned int n, insize = 0;
int ret;
+ mutex_lock(&usbhid->mutex);
+
clear_bit(HID_DISCONNECTED, &usbhid->iofl);
usbhid->bufsize = HID_MIN_BUFFER_SIZE;
@@ -1177,6 +1215,8 @@ static int usbhid_start(struct hid_devic
usbhid_set_leds(hid);
device_set_wakeup_enable(&dev->dev, 1);
}
+
+ mutex_unlock(&usbhid->mutex);
return 0;
fail:
@@ -1187,6 +1227,7 @@ fail:
usbhid->urbout = NULL;
usbhid->urbctrl = NULL;
hid_free_buffers(dev, hid);
+ mutex_unlock(&usbhid->mutex);
return ret;
}
@@ -1202,6 +1243,8 @@ static void usbhid_stop(struct hid_devic
usbhid->intf->needs_remote_wakeup = 0;
}
+ mutex_lock(&usbhid->mutex);
+
clear_bit(HID_STARTED, &usbhid->iofl);
spin_lock_irq(&usbhid->lock); /* Sync with error and led handlers */
set_bit(HID_DISCONNECTED, &usbhid->iofl);
@@ -1222,6 +1265,8 @@ static void usbhid_stop(struct hid_devic
usbhid->urbout = NULL;
hid_free_buffers(hid_to_usb_dev(hid), hid);
+
+ mutex_unlock(&usbhid->mutex);
}
static int usbhid_power(struct hid_device *hid, int lvl)
@@ -1279,7 +1324,7 @@ static int usbhid_idle(struct hid_device
if (reqtype != HID_REQ_SET_IDLE)
return -EINVAL;
- return hid_set_idle(dev, ifnum, report, idle);
+ return hid_set_idle(hid, dev, ifnum, report, idle);
}
struct hid_ll_driver usb_hid_driver = {
@@ -1382,6 +1427,8 @@ static int usbhid_probe(struct usb_inter
INIT_WORK(&usbhid->reset_work, hid_reset);
timer_setup(&usbhid->io_retry, hid_retry_timeout, 0);
spin_lock_init(&usbhid->lock);
+ mutex_init(&usbhid->mutex);
+ sema_init(&usbhid->sem, 1);
ret = hid_add_device(hid);
if (ret) {
@@ -1470,6 +1517,8 @@ static int hid_pre_reset(struct usb_inte
spin_unlock_irq(&usbhid->lock);
hid_cease_io(usbhid);
+ down(&usbhid->sem); /* released in hid_post_reset */
+
return 0;
}
@@ -1483,6 +1532,8 @@ static int hid_post_reset(struct usb_int
int status;
char *rdesc;
+ up(&usbhid->sem); /* acquired in hid_pre_reset */
+
/* Fetch and examine the HID report descriptor. If this
* has changed, then rebind. Since usbcore's check of the
* configuration descriptors passed, we already know that
@@ -1492,7 +1543,7 @@ static int hid_post_reset(struct usb_int
if (!rdesc)
return -ENOMEM;
- status = hid_get_class_descriptor(dev,
+ status = hid_get_class_descriptor(hid, dev,
interface->desc.bInterfaceNumber,
HID_DT_REPORT, rdesc, hid->dev_rsize);
if (status < 0) {
@@ -1512,7 +1563,7 @@ static int hid_post_reset(struct usb_int
clear_bit(HID_RESET_PENDING, &usbhid->iofl);
clear_bit(HID_CLEAR_HALT, &usbhid->iofl);
spin_unlock_irq(&usbhid->lock);
- hid_set_idle(dev, intf->cur_altsetting->desc.bInterfaceNumber, 0, 0);
+ hid_set_idle(hid, dev, intf->cur_altsetting->desc.bInterfaceNumber, 0, 0);
hid_restart_io(hid);
Index: usb-devel/drivers/hid/usbhid/usbhid.h
===================================================================
--- usb-devel.orig/drivers/hid/usbhid/usbhid.h
+++ usb-devel/drivers/hid/usbhid/usbhid.h
@@ -19,6 +19,7 @@
#include <linux/wait.h>
#include <linux/workqueue.h>
#include <linux/input.h>
+#include <linux/semaphore.h>
/* API provided by hid-core.c for USB HID drivers */
void usbhid_init_reports(struct hid_device *hid);
@@ -80,6 +81,9 @@ struct usbhid_device {
dma_addr_t outbuf_dma; /* Output buffer dma */
unsigned long last_out; /* record of last output for timeouts */
+ struct mutex mutex; /* start/stop/open/close */
+ struct semaphore sem; /* submit/pre_reset/post_reset */
+
spinlock_t lock; /* fifo spinlock */
unsigned long iofl; /* I/O flags (CTRL_RUNNING, OUT_RUNNING) */
struct timer_list io_retry; /* Retry timer */
Powered by blists - more mailing lists