Crash on evdev disconnect.
PATCH 1/1 Crash on evdev disconnect.
While trying to figure out the best way to handle an odd mouse, I found that we oopsed when doing a rmmod on usbhid while someone had a USB device open through evdev.
We try to loop through evdev->list, calling kill_fasync on each member, however by the time we try to get the next pointer, we have already freed the member and poisoned the next/last.
The fix is fairly simple, and if nobody objects I think we should try and get this into -stable too.
drivers/input/evdev.c +2 -2
@@ -659,7 +659,7 @@static void evdev_disconnect (struct input_handle *handle) {struct evdev *evdev = handle->private; -struct evdev_list *list; +struct evdev_list *list, *next;sysfs_remove_link (&input_class.subsys.kset.kobj, evdev->name); class_device_destroy(&input_class,
@@ -669,7 +669,7 @@if (evdev->open) {input_close_device (handle);wake_up_interruptible (&evdev->wait); -list_for_each_entry (list, &evdev->list, node) +list_for_each_entry_safe (list, next, &evdev->list, node)kill_fasync (&list->fasync,SIGIO ,POLL_HUP ); }else evdev_free (evdev);
Signature · 8 lines
1024D/E65A7801 Zephaniah E. Hull <[email protected]> 92ED 94E4 B1E6 3624 226D 5727 4453 008B E65A 7801 CCs of replies from mailing lists are requested. "Sir," barked one of those useless aristocratic generals to William Howard Russell, the great Times war correspondent, "I do not like what you write." "Then, sir," retorted Russell, "I suggest you do not do what I write about."
Dmitry Torokhov
maintainerHi,
drivers/input/evdev.c line 673
Quoted 6 lines · Zephaniah E. Hull
if (evdev->open) { input_close_device(handle); wake_up_interruptible(&evdev->wait);drivers/input/evdev.c +1
-1-list_for_each_entry (list, &evdev->list, node) +list_for_each_entry_safe (list, next, &evdev->list, node)kill_fasync (&list->fasync,SIGIO ,POLL_HUP );
NAK. kill_fasync does not affect the list state so using _safe does not buy us anything.
BTW, [email protected] address is dead, please use [email protected] or [email protected] or [email protected].
Signature · 1 line
Dmitry
Quoted 10 lines · Dmitry Torokhov
Hi,
drivers/input/evdev.c line 673
Quoted 6 lines · Zephaniah E. Hull
if (evdev->open) { input_close_device(handle); wake_up_interruptible(&evdev->wait);drivers/input/evdev.c +1
-1-list_for_each_entry (list, &evdev->list, node) +list_for_each_entry_safe (list, next, &evdev->list, node)kill_fasync (&list->fasync,SIGIO ,POLL_HUP );
Quoted 2 lines
NAK.
kill_fasyncdoes not affect the list state so using_safedoes not buy us anything.
Sorry, but you're wrong.
Immediately before the kill_fasync call list->node.next is a valid pointer, immediately afterwords it is 0x100100, which happens to be list_poison. kill_fasync is triggering a close somehow, evdev_close deletes that element of the list, which poisons the next value, which can make us crash and burn.
I have a 100% reproducible crash case, which is fixed by the change.
If kill_fasync shouldn't be making it close that's another issue, but at the moment it is and this is a fairly non-invasive change which fixes it.
Quoted 3 lines
BTW, [email protected] address is dead, please use [email protected] or [email protected] or [email protected].
Noted, recommend updating the entry in MAINTAINERS. :)
Zephaniah E. Hull.
Signature · 3 lines
1024D/E65A7801 Zephaniah E. Hull <[email protected]> 92ED 94E4 B1E6 3624 226D 5727 4453 008B E65A 7801 CCs of replies from mailing lists are requested.
Quoted 3 lines
Is there an API or other means to determine what video card, namely the chipset, that the user has installed on his machine?
On a modern X86 machine use the PCI/AGP bus data. On a PS/2 use the MCA bus data. On nubus use the nubus probe data. On old style ISA bus PCs done a large pointy hat and spend several years reading arcane and forbidden scrolls
-- Alan Cox
Dmitry Torokhov
maintainerQuoted 22 lines · Zephaniah E. Hull
Quoted 10 lines · Dmitry Torokhov
Hi,
drivers/input/evdev.c line 673
Quoted 6 lines · Zephaniah E. Hull
if (evdev->open) { input_close_device(handle); wake_up_interruptible(&evdev->wait);drivers/input/evdev.c +1
-1-list_for_each_entry (list, &evdev->list, node) +list_for_each_entry_safe (list, next, &evdev->list, node)kill_fasync (&list->fasync,SIGIO ,POLL_HUP );Quoted 2 lines
NAK.
kill_fasyncdoes not affect the list state so using_safedoes not buy us anything.Sorry, but you're wrong.
Immediately before the
kill_fasynccalllist->node.nextis a valid pointer, immediately afterwords it is0x100100, which happens to belist_poison.kill_fasyncis triggering a close somehow,evdev_closedeletes that element of the list, which poisons the next value, which can make us crash and burn.
Quoted 6 lines
I have a 100% reproducible crash case, which is fixed by the change.
If
kill_fasyncshouldn't be making it close that's another issue, but at the moment it is and this is a fairly non-invasive change which fixes it.
Unfortunately it does not really fix the problem, it just papers over the issue. The crash will still happen if for some reason evdev_release runs at a bad moment.
Quoted 5 lines
Quoted 2 lines
BTW, [email protected] address is dead, please use [email protected] or [email protected] or [email protected].
Noted, recommend updating the entry in
MAINTAINERS. :)
Already done ;)
Signature · 1 line
Dmitry
Quoted 25 lines · Dmitry Torokhov
Quoted 24 lines · Zephaniah E. Hull
Quoted 10 lines · Dmitry Torokhov
Hi,
drivers/input/evdev.c line 673
Quoted 6 lines · Zephaniah E. Hull
if (evdev->open) { input_close_device(handle); wake_up_interruptible(&evdev->wait);drivers/input/evdev.c +1
-1-list_for_each_entry (list, &evdev->list, node) +list_for_each_entry_safe (list, next, &evdev->list, node)kill_fasync (&list->fasync,SIGIO ,POLL_HUP );Quoted 2 lines
NAK.
kill_fasyncdoes not affect the list state so using_safedoes not buy us anything.Sorry, but you're wrong.
Immediately before the
kill_fasynccalllist->node.nextis a valid pointer, immediately afterwords it is0x100100, which happens to belist_poison.kill_fasyncis triggering a close somehow,evdev_closedeletes that element of the list, which poisons the next value, which can make us crash and burn.I have a 100% reproducible crash case, which is fixed by the change.
Quoted 8 lines
Quoted 4 lines
If
kill_fasyncshouldn't be making it close that's another issue, but at the moment it is and this is a fairly non-invasive change which fixes it.Unfortunately it does not really fix the problem, it just papers over the issue. The crash will still happen if for some reason
evdev_releaseruns at a bad moment.
Almost agreed, it papers over the lack of locking, however most forms of locking may need this to avoid a deadlock anyhow. (I don't know the semantics of kill_fasync, and thus I don't know if attempting to take a lock in the close that is held here would deadlock, or if execution would move back to here, but I have a nasty feeling that it's the former.)
That said, at the moment, we have a 100% guaranteed oops on rmmod usbhid if something has the device open, which is bad.
Zephaniah E. Hull.
Signature · 8 lines
1024D/E65A7801 Zephaniah E. Hull <[email protected]> 92ED 94E4 B1E6 3624 226D 5727 4453 008B E65A 7801 CCs of replies from mailing lists are requested. "I would rather spend 10 hours reading someone else's source code than 10 minutes listening to Musak waiting for technical support which isn't." (By Dr. Greg Wettstein, Roger Maris Cancer Center)