From: James Paradis <jparadis@redhat.com>
Date: Mon, 10 Nov 2008 13:02:20 -0500
Subject: [usb] fix locking for input devices
Message-id: 1742532028.1737271226340140388.JavaMail.root@zmail01.collab.prod.int.phx2.redhat.com
O-Subject: [RHEL5.3 Patch] Fix locking in input.c [repost**2]
Bugzilla: 468915
RH-Acked-by: Pete Zaitcev <zaitcev@redhat.com>

This patch fixes Bug 468915:

Description of problem:

Under certain circumstances, unplugging an input device can cause the system
to crash.  In particular, reading /proc/bus/input/devices while a
device removal is in progress can cause a NULL pointer dereference.

This happens because there is no locking around the list of input devices
(input_dev_list) that is traversed while reading
/proc/bus/input/devices.  The code even admits as much:

static void *input_devices_seq_start(struct seq_file *seq, loff_t *pos)
{
        /* acquire lock here ... Yes, we do need locking, I knowi, I know...
*/

        return list_get_nth_element(&input_dev_list, pos);
}

If a device is removed while such a traversal is in progress, one could hit
a dangling pointer.

Although this is a very small window, the Stratus system hits it pretty
reliably when switching the Active Console from one CRU to the other.
I have a reproducer that reliably tickles this bug within five minutes.
After applying this patch, I can run the reproducer for hours with
nary a hiccup.

Patch attached.

diff --git a/drivers/input/input.c b/drivers/input/input.c
index 9cb4b9a..655ac01 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -193,6 +193,14 @@ void input_event(struct input_dev *dev, unsigned int type, unsigned int code, in
 }
 EXPORT_SYMBOL(input_event);
 
+/*
+ * Input_mutex protects access to both input_dev_list and input_handler_list.
+ * This also causes input_[un]register_device and input_[un]register_handler
+ * be mutually exclusive which simplifies locking in drivers implementing
+ * input handlers.
+ */
+static DEFINE_MUTEX(input_mutex);
+
 /**
  * input_inject_event() - send input event from input handler
  * @handle: input handle to send event through
@@ -393,7 +401,8 @@ static struct list_head *list_get_next_element(struct list_head *list, struct li
 
 static void *input_devices_seq_start(struct seq_file *seq, loff_t *pos)
 {
-	/* acquire lock here ... Yes, we do need locking, I knowi, I know... */
+	if (mutex_lock_interruptible(&input_mutex))
+		return NULL;
 
 	return list_get_nth_element(&input_dev_list, pos);
 }
@@ -405,7 +414,7 @@ static void *input_devices_seq_next(struct seq_file *seq, void *v, loff_t *pos)
 
 static void input_devices_seq_stop(struct seq_file *seq, void *v)
 {
-	/* release lock here */
+	mutex_unlock(&input_mutex);
 }
 
 static void input_seq_print_bitmap(struct seq_file *seq, const char *name,
@@ -488,7 +497,9 @@ static struct file_operations input_devices_fileops = {
 
 static void *input_handlers_seq_start(struct seq_file *seq, loff_t *pos)
 {
-	/* acquire lock here ... Yes, we do need locking, I knowi, I know... */
+	if (mutex_lock_interruptible(&input_mutex))
+		return NULL;
+
 	seq->private = (void *)(unsigned long)*pos;
 	return list_get_nth_element(&input_handler_list, pos);
 }
@@ -501,7 +512,7 @@ static void *input_handlers_seq_next(struct seq_file *seq, void *v, loff_t *pos)
 
 static void input_handlers_seq_stop(struct seq_file *seq, void *v)
 {
-	/* release lock here */
+	mutex_unlock(&input_mutex);
 }
 
 static int input_handlers_seq_show(struct seq_file *seq, void *v)
@@ -956,7 +967,6 @@ int input_register_device(struct input_dev *dev)
 	}
 
 	INIT_LIST_HEAD(&dev->h_list);
-	list_add_tail(&dev->node, &input_dev_list);
 
 	dev->cdev.class = &input_class;
 	snprintf(dev->cdev.class_id, sizeof(dev->cdev.class_id),
@@ -985,6 +995,13 @@ int input_register_device(struct input_dev *dev)
 		dev->name ? dev->name : "Unspecified device", path ? path : "N/A");
 	kfree(path);
 
+	error = mutex_lock_interruptible(&input_mutex);
+	if (error) {
+		goto fail4;
+	}
+
+	list_add_tail(&dev->node, &input_dev_list);
+
 	list_for_each_entry(handler, &input_handler_list, node)
 		if (!handler->blacklist || !input_match_device(handler->blacklist, dev))
 			if ((id = input_match_device(handler->id_table, dev)))
@@ -996,8 +1013,12 @@ int input_register_device(struct input_dev *dev)
 
 	input_wakeup_procfs_readers();
 
+	mutex_unlock(&input_mutex);
+
 	return 0;
 
+ fail4: module_put(THIS_MODULE);
+	sysfs_remove_group(&dev->cdev.kobj, &input_dev_caps_attr_group);
  fail3:	sysfs_remove_group(&dev->cdev.kobj, &input_dev_id_attr_group);
  fail2:	sysfs_remove_group(&dev->cdev.kobj, &input_dev_attr_group);
  fail1:	class_device_del(&dev->cdev);
@@ -1014,6 +1035,8 @@ void input_unregister_device(struct input_dev *dev)
 
 	del_timer_sync(&dev->timer);
 
+	mutex_lock(&input_mutex);
+
 	list_for_each_safe(node, next, &dev->h_list) {
 		struct input_handle * handle = to_handle(node);
 		list_del_init(&handle->d_node);
@@ -1034,6 +1057,8 @@ void input_unregister_device(struct input_dev *dev)
 	class_device_unregister(&dev->cdev);
 
 	input_wakeup_procfs_readers();
+
+	mutex_unlock(&input_mutex);
 }
 EXPORT_SYMBOL(input_unregister_device);
 
@@ -1046,6 +1071,8 @@ void input_register_handler(struct input_handler *handler)
 	if (!handler)
 		return;
 
+	mutex_lock(&input_mutex);
+
 	INIT_LIST_HEAD(&handler->h_list);
 
 	if (handler->fops != NULL)
@@ -1063,6 +1090,8 @@ void input_register_handler(struct input_handler *handler)
 				}
 
 	input_wakeup_procfs_readers();
+
+	mutex_unlock(&input_mutex);
 }
 EXPORT_SYMBOL(input_register_handler);
 
@@ -1070,6 +1099,8 @@ void input_unregister_handler(struct input_handler *handler)
 {
 	struct list_head *node, *next;
 
+	mutex_lock(&input_mutex);
+
 	list_for_each_safe(node, next, &handler->h_list) {
 		struct input_handle * handle = to_handle_h(node);
 		list_del_init(&handle->h_node);
@@ -1083,6 +1114,8 @@ void input_unregister_handler(struct input_handler *handler)
 		input_table[handler->minor >> 5] = NULL;
 
 	input_wakeup_procfs_readers();
+
+	mutex_unlock(&input_mutex);
 }
 EXPORT_SYMBOL(input_unregister_handler);