Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 86762f69 authored by Alan Stern's avatar Alan Stern Committed by Greg Kroah-Hartman
Browse files

HID: usbhid: Fix race between usbhid_close() and usbhid_stop() 



commit 0ed08faded1da03eb3def61502b27f81aef2e615 upstream.

The syzbot fuzzer discovered a bad race between in the usbhid driver
between usbhid_stop() and usbhid_close().  In particular,
usbhid_stop() does:

	usb_free_urb(usbhid->urbin);
	...
	usbhid->urbin = NULL; /* don't mess up next start */

and usbhid_close() does:

	usb_kill_urb(usbhid->urbin);

with no mutual exclusion.  If the two routines happen to run
concurrently so that usb_kill_urb() is called in between the
usb_free_urb() and the NULL assignment, it will access the
deallocated urb structure -- a use-after-free bug.

This patch adds a mutex to the usbhid private structure and uses it to
enforce mutual exclusion of the usbhid_start(), usbhid_stop(),
usbhid_open() and usbhid_close() callbacks.

Reported-and-tested-by: default avatar <syzbot+7bf5a7b0f0a1f9446f4c@syzkaller.appspotmail.com>
Signed-off-by: default avatarAlan Stern <stern@rowland.harvard.edu>
CC: <stable@vger.kernel.org>
Signed-off-by: default avatarJiri Kosina <jkosina@suse.cz>
Signed-off-by: default avatarGreg Kroah-Hartman <gregkh@linuxfoundation.org>
parent db8bf823

drivers/hid/usbhid/hid-core.c

+29 −8
Original line number Diff line number Diff line
@@ -685,16 +685,21 @@ static int usbhid_open(struct hid_device *hid) 
	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;
@@ -728,6 +733,9 @@ static int usbhid_open(struct hid_device *hid) 
		msleep(50);



	clear_bit(HID_RESUME_RUNNING, &usbhid->iofl);



 Done:

	mutex_unlock(&usbhid->mutex);

	return res;

}
@@ -735,6 +743,8 @@ static void usbhid_close(struct hid_device *hid) 
{

	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
@@ -746,14 +756,15 @@ static void usbhid_close(struct hid_device *hid) 
		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;

	}



	mutex_unlock(&usbhid->mutex);

}



/*

 * Initialize all reports

 */
@@ -1060,6 +1071,8 @@ static int usbhid_start(struct hid_device *hid) 
	unsigned int n, insize = 0;

	int ret;



	mutex_lock(&usbhid->mutex);



	clear_bit(HID_DISCONNECTED, &usbhid->iofl);



	usbhid->bufsize = HID_MIN_BUFFER_SIZE;
@@ -1180,6 +1193,8 @@ static int usbhid_start(struct hid_device *hid) 
		usbhid_set_leds(hid);

		device_set_wakeup_enable(&dev->dev, 1);

	}



	mutex_unlock(&usbhid->mutex);

	return 0;



fail:
@@ -1190,6 +1205,7 @@ static int usbhid_start(struct hid_device *hid) 
	usbhid->urbout = NULL;

	usbhid->urbctrl = NULL;

	hid_free_buffers(dev, hid);

	mutex_unlock(&usbhid->mutex);

	return ret;

}
@@ -1205,6 +1221,8 @@ static void usbhid_stop(struct hid_device *hid) 
		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);
@@ -1225,6 +1243,8 @@ static void usbhid_stop(struct hid_device *hid) 
	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)
@@ -1385,6 +1405,7 @@ static int usbhid_probe(struct usb_interface *intf, const struct usb_device_id * 
	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);



	ret = hid_add_device(hid);

	if (ret) {

drivers/hid/usbhid/usbhid.h

+1 −0
Original line number Diff line number Diff line
@@ -93,6 +93,7 @@ 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 */

	spinlock_t lock;						/* fifo spinlock */

	unsigned long iofl;                                             /* I/O flags (CTRL_RUNNING, OUT_RUNNING) */

	struct timer_list io_retry;                                     /* Retry timer */