Re: KASAN: use-after-free Read in usbhid_close (3)

Re: KASAN: use-after-free Read in usbhid_close (3)

[Alan Stern]

linux-input people:

syzbot has found a bug related to USB/HID/input, and I have narrowed it
down to the wacom driver.  As far as I can tell, the problem is caused
the fact that drivers/hid/wacom_sys.c calls input_register_device()
in several places, but it never calls input_unregister_device().

I know very little about the input subsystem, but this certainly seems 
like a bug.

When the device is unplugged, the disconnect pathway doesn't call
hid_hw_close().  That routine doesn't get called until the user closes
the device file (which can be long after the device is gone and
hid_hw_stop() has run).  Then usbhid_close() gets a use-after-free
error when it tries to access data structures that were deallocated by
usbhid_stop().  No doubt there are other problems too, but this is
the one that syzbot found.

Can any of you help fix this?  Thanks.

Alan Stern

Re: KASAN: use-after-free Read in usbhid_close (3)

[Dmitry Torokhov]

Hi Alan,

On Sat, Apr 18, 2020 at 10:16:32PM -0400, Alan Stern wrote:
> linux-input people:
> 
> syzbot has found a bug related to USB/HID/input, and I have narrowed it
> down to the wacom driver.  As far as I can tell, the problem is caused
> the fact that drivers/hid/wacom_sys.c calls input_register_device()
> in several places, but it never calls input_unregister_device().
> 
> I know very little about the input subsystem, but this certainly seems 
> like a bug.

Wacom driver uses devm_input_allocate_device(), so unregister should
happen automatically on device removal once we exit wacom_probe().

> 
> When the device is unplugged, the disconnect pathway doesn't call
> hid_hw_close().  That routine doesn't get called until the user closes
> the device file (which can be long after the device is gone and
> hid_hw_stop() has run).  Then usbhid_close() gets a use-after-free
> error when it tries to access data structures that were deallocated by
> usbhid_stop().  No doubt there are other problems too, but this is
> the one that syzbot found.

Unregistering the input device should result in calling wacom_close()
(if device was previously opened), which, as far as I can tell, calls
hid_hw_close().

I wonder if it is valid to call hid_hw_stop() before hid_hw_close()?

It could be that we again get confused by the "easiness" of devm APIs
and completely screwing up unwind order.

Thanks.

-- 
Dmitry

Re: KASAN: use-after-free Read in usbhid_close (3)

[Dmitry Torokhov]

On Sat, Apr 18, 2020 at 09:09:44PM -0700, Dmitry Torokhov wrote:
> Hi Alan,
> 
> On Sat, Apr 18, 2020 at 10:16:32PM -0400, Alan Stern wrote:
> > linux-input people:
> > 
> > syzbot has found a bug related to USB/HID/input, and I have narrowed it
> > down to the wacom driver.  As far as I can tell, the problem is caused
> > the fact that drivers/hid/wacom_sys.c calls input_register_device()
> > in several places, but it never calls input_unregister_device().
> > 
> > I know very little about the input subsystem, but this certainly seems 
> > like a bug.
> 
> Wacom driver uses devm_input_allocate_device(), so unregister should
> happen automatically on device removal once we exit wacom_probe().
> 
> > 
> > When the device is unplugged, the disconnect pathway doesn't call
> > hid_hw_close().  That routine doesn't get called until the user closes
> > the device file (which can be long after the device is gone and
> > hid_hw_stop() has run).  Then usbhid_close() gets a use-after-free
> > error when it tries to access data structures that were deallocated by
> > usbhid_stop().  No doubt there are other problems too, but this is
> > the one that syzbot found.
> 
> Unregistering the input device should result in calling wacom_close()
> (if device was previously opened), which, as far as I can tell, calls
> hid_hw_close().
> 
> I wonder if it is valid to call hid_hw_stop() before hid_hw_close()?
> 
> It could be that we again get confused by the "easiness" of devm APIs
> and completely screwing up unwind order.

Let's also add Ping and Jason to the conversation...

-- 
Dmitry

Re: KASAN: use-after-free Read in usbhid_close (3)

[Alan Stern]

On Sat, 18 Apr 2020, Dmitry Torokhov wrote:

> On Sat, Apr 18, 2020 at 09:09:44PM -0700, Dmitry Torokhov wrote:
> > Hi Alan,
> > 
> > On Sat, Apr 18, 2020 at 10:16:32PM -0400, Alan Stern wrote:
> > > linux-input people:
> > > 
> > > syzbot has found a bug related to USB/HID/input, and I have narrowed it
> > > down to the wacom driver.  As far as I can tell, the problem is caused
> > > the fact that drivers/hid/wacom_sys.c calls input_register_device()
> > > in several places, but it never calls input_unregister_device().
> > > 
> > > I know very little about the input subsystem, but this certainly seems 
> > > like a bug.
> > 
> > Wacom driver uses devm_input_allocate_device(), so unregister should
> > happen automatically on device removal once we exit wacom_probe().
> > 
> > > 
> > > When the device is unplugged, the disconnect pathway doesn't call
> > > hid_hw_close().  That routine doesn't get called until the user closes
> > > the device file (which can be long after the device is gone and
> > > hid_hw_stop() has run).  Then usbhid_close() gets a use-after-free
> > > error when it tries to access data structures that were deallocated by
> > > usbhid_stop().  No doubt there are other problems too, but this is
> > > the one that syzbot found.
> > 
> > Unregistering the input device should result in calling wacom_close()
> > (if device was previously opened), which, as far as I can tell, calls
> > hid_hw_close().
> > 
> > I wonder if it is valid to call hid_hw_stop() before hid_hw_close()?

No, it isn't.  If it were, for example, why would evdev_disconnect() -> 
evdev_cleanup() need to call input_close_device()?  And why would 
usbhid_disconnect() deallocate the usbhid structure which usbhid_stop()
accesses?

> > It could be that we again get confused by the "easiness" of devm APIs
> > and completely screwing up unwind order.

That's probably what happened.

Alan Stern

> Let's also add Ping and Jason to the conversation...

Re: KASAN: use-after-free Read in usbhid_close (3)

[Dmitry Torokhov]

On Sun, Apr 19, 2020 at 10:07:34AM -0400, Alan Stern wrote:
> On Sat, 18 Apr 2020, Dmitry Torokhov wrote:
> 
> > On Sat, Apr 18, 2020 at 09:09:44PM -0700, Dmitry Torokhov wrote:
> > > Hi Alan,
> > > 
> > > On Sat, Apr 18, 2020 at 10:16:32PM -0400, Alan Stern wrote:
> > > > linux-input people:
> > > > 
> > > > syzbot has found a bug related to USB/HID/input, and I have narrowed it
> > > > down to the wacom driver.  As far as I can tell, the problem is caused
> > > > the fact that drivers/hid/wacom_sys.c calls input_register_device()
> > > > in several places, but it never calls input_unregister_device().
> > > > 
> > > > I know very little about the input subsystem, but this certainly seems 
> > > > like a bug.
> > > 
> > > Wacom driver uses devm_input_allocate_device(), so unregister should
> > > happen automatically on device removal once we exit wacom_probe().
> > > 
> > > > 
> > > > When the device is unplugged, the disconnect pathway doesn't call
> > > > hid_hw_close().  That routine doesn't get called until the user closes
> > > > the device file (which can be long after the device is gone and
> > > > hid_hw_stop() has run).  Then usbhid_close() gets a use-after-free
> > > > error when it tries to access data structures that were deallocated by
> > > > usbhid_stop().  No doubt there are other problems too, but this is
> > > > the one that syzbot found.
> > > 
> > > Unregistering the input device should result in calling wacom_close()
> > > (if device was previously opened), which, as far as I can tell, calls
> > > hid_hw_close().
> > > 
> > > I wonder if it is valid to call hid_hw_stop() before hid_hw_close()?
> 
> No, it isn't.  If it were, for example, why would evdev_disconnect() -> 
> evdev_cleanup() need to call input_close_device()?

Because input and HID are not the same. For input, when we attempt to
unregister an input device we will go through all attached input
handlers (like evdev) and if they believe they have the device open they
will attempt to close it. How close is implemented is up to particular
driver.

I am not sure about HID implementation details, but I could envision
transports where you can tell the transport that you no longer want
events to be delivered to you ("close") vs you want to disable hardware
("stop") and support any order of them.

> And why would 
> usbhid_disconnect() deallocate the usbhid structure which usbhid_stop()
> accesses?

This happens only after we return from hid_destroy_device(), so
even in the presence of devm I'd expect that all devm-related stuff
instantiated by hid-wacom would have been completed before we get back
to usbhid_disconnect().

Can we validate that calls to wacom_close() happen?

> 
> > > It could be that we again get confused by the "easiness" of devm APIs
> > > and completely screwing up unwind order.
> 
> That's probably what happened.

Thanks.

-- 
Dmitry

Re: KASAN: use-after-free Read in usbhid_close (3)

[Alan Stern]

On Sun, 19 Apr 2020, Dmitry Torokhov wrote:

> On Sun, Apr 19, 2020 at 10:07:34AM -0400, Alan Stern wrote:
> > On Sat, 18 Apr 2020, Dmitry Torokhov wrote:
> > 
> > > On Sat, Apr 18, 2020 at 09:09:44PM -0700, Dmitry Torokhov wrote:
> > > > Hi Alan,
> > > > 
> > > > On Sat, Apr 18, 2020 at 10:16:32PM -0400, Alan Stern wrote:
> > > > > linux-input people:
> > > > > 
> > > > > syzbot has found a bug related to USB/HID/input, and I have narrowed it
> > > > > down to the wacom driver.  As far as I can tell, the problem is caused
> > > > > the fact that drivers/hid/wacom_sys.c calls input_register_device()
> > > > > in several places, but it never calls input_unregister_device().
> > > > > 
> > > > > I know very little about the input subsystem, but this certainly seems 
> > > > > like a bug.
> > > > 
> > > > Wacom driver uses devm_input_allocate_device(), so unregister should
> > > > happen automatically on device removal once we exit wacom_probe().
> > > > 
> > > > > 
> > > > > When the device is unplugged, the disconnect pathway doesn't call
> > > > > hid_hw_close().  That routine doesn't get called until the user closes
> > > > > the device file (which can be long after the device is gone and
> > > > > hid_hw_stop() has run).  Then usbhid_close() gets a use-after-free
> > > > > error when it tries to access data structures that were deallocated by
> > > > > usbhid_stop().  No doubt there are other problems too, but this is
> > > > > the one that syzbot found.
> > > > 
> > > > Unregistering the input device should result in calling wacom_close()
> > > > (if device was previously opened), which, as far as I can tell, calls
> > > > hid_hw_close().
> > > > 
> > > > I wonder if it is valid to call hid_hw_stop() before hid_hw_close()?
> > 
> > No, it isn't.  If it were, for example, why would evdev_disconnect() -> 
> > evdev_cleanup() need to call input_close_device()?
> 
> Because input and HID are not the same. For input, when we attempt to
> unregister an input device we will go through all attached input
> handlers (like evdev) and if they believe they have the device open they
> will attempt to close it. How close is implemented is up to particular
> driver.
> 
> I am not sure about HID implementation details, but I could envision
> transports where you can tell the transport that you no longer want
> events to be delivered to you ("close") vs you want to disable hardware
> ("stop") and support any order of them.

Jiri, you should know: Are HID drivers supposed to work okay when the
->close callback is issued after (or concurrently with) the ->stop
callback?

The actual bug found by syzbot was a race between those two routines in 
usbhid.

> > And why would 
> > usbhid_disconnect() deallocate the usbhid structure which usbhid_stop()
> > accesses?
> 
> This happens only after we return from hid_destroy_device(), so
> even in the presence of devm I'd expect that all devm-related stuff
> instantiated by hid-wacom would have been completed before we get back
> to usbhid_disconnect().
> 
> Can we validate that calls to wacom_close() happen?

I could find out if you think it's important.  In the syzbot tests, the 
crash occurs before wacom_close() is called.

Alan Stern

Re: KASAN: use-after-free Read in usbhid_close (3)

[Alan Stern]

On Sun, 19 Apr 2020, Alan Stern wrote:

> Jiri, you should know: Are HID drivers supposed to work okay when the
> ->close callback is issued after (or concurrently with) the ->stop
> callback?

No response.  I'll assume that strange callback orderings should be 
supported.  Let's see if the patch below fixes the race in usbhid.

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
@@ -682,16 +682,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 +730,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 +740,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 +753,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);
 }
 
 /*
@@ -1057,6 +1068,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 +1190,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 +1202,7 @@ fail:
 	usbhid->urbout = NULL;
 	usbhid->urbctrl = NULL;
 	hid_free_buffers(dev, hid);
+	mutex_unlock(&usbhid->mutex);
 	return ret;
 }
 
@@ -1202,6 +1218,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 +1240,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)
@@ -1382,6 +1402,7 @@ 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);
 
 	ret = hid_add_device(hid);
 	if (ret) {
Index: usb-devel/drivers/hid/usbhid/usbhid.h
===================================================================
--- usb-devel.orig/drivers/hid/usbhid/usbhid.h
+++ usb-devel/drivers/hid/usbhid/usbhid.h
@@ -80,6 +80,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 */

Re: KASAN: use-after-free Read in usbhid_close (3)

[syzbot]

Hello,

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

Reported-and-tested-by: syzbot+7bf5a7b0f0a1f9446f4c@syzkaller.appspotmail.com

Tested on:

commit:         0fa84af8 Merge tag 'usb-serial-5.7-rc1' of https://git.ker..
git tree:       https://github.com/google/kasan.git
kernel config:  https://syzkaller.appspot.com/x/.config?x=6b9c154b0c23aecf
dashboard link: https://syzkaller.appspot.com/bug?extid=7bf5a7b0f0a1f9446f4c
compiler:       gcc (GCC) 9.0.0 20181231 (experimental)
patch:          https://syzkaller.appspot.com/x/patch.diff?x=14f40acfe00000

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

Re: KASAN: use-after-free Read in usbhid_close (3)

[Jiri Kosina]

On Wed, 22 Apr 2020, Alan Stern wrote:

> > Jiri, you should know: Are HID drivers supposed to work okay when the
> > ->close callback is issued after (or concurrently with) the ->stop
> > callback?
> 
> No response.  

Sorry, I've been a bit swamped recently. Thanks a lot for taking care of 
this.

> I'll assume that strange callback orderings should be supported.  Let's 
> see if the patch below fixes the race in usbhid.

Unfortunately I don't believe the supportability of this is fully defined. 
I have tried to quickly go over the few major drivers and didn't find 
anything relying various orderings, but I might have easily missed some 
case.

So unless we have a programatic way to check it, the patch you created for 
mutual exclusion is a good bandaid I believe.

Thanks again Alan, I'll push it to Linus for 5.7.

-- 
Jiri Kosina
SUSE Labs