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
