ANDROID: keychord: Fix races in keychord_write.
There are multiple bugs caused by threads racing in keychord_write.
1) Threads racing through this function can cause the same element to
be added to a linked list twice (multiple calls to
input_register_handler() for the same input_handler struct). And the
races can also cause an element in a linked list that doesn't exist
attempted to be removed (multiple calls to input_unregister_handler()
with the same input_handler struct).
2) The races can also cause duplicate kfree's of the keychords
struct.
Bug: 64133562
Bug: 63974334
Change-Id: I6329a4d58c665fab5d3e96ef96391e07b4941e80
Signed-off-by: Mohan Srinivasan <srmohan@google.com>
diff --git a/drivers/input/misc/keychord.c b/drivers/input/misc/keychord.c
index c5ab3dd..1673e42 100644
--- a/drivers/input/misc/keychord.c
+++ b/drivers/input/misc/keychord.c
@@ -60,6 +60,10 @@
unsigned char head;
unsigned char tail;
__u16 buff[BUFFER_SIZE];
+ /* Bit to serialize writes to this device */
+#define KEYCHORD_BUSY 0x01
+ unsigned long flags;
+ wait_queue_head_t write_waitq;
};
static int check_keychord(struct keychord_device *kdev,
@@ -172,7 +176,6 @@
goto err_input_open_device;
pr_info("keychord: using input dev %s for fevent\n", dev->name);
-
return 0;
err_input_open_device:
@@ -225,6 +228,41 @@
}
/*
+ * serializes writes on a device. can use mutex_lock_interruptible()
+ * for this particular use case as well - a matter of preference.
+ */
+static int
+keychord_write_lock(struct keychord_device *kdev)
+{
+ int ret;
+ unsigned long flags;
+
+ spin_lock_irqsave(&kdev->lock, flags);
+ while (kdev->flags & KEYCHORD_BUSY) {
+ spin_unlock_irqrestore(&kdev->lock, flags);
+ ret = wait_event_interruptible(kdev->write_waitq,
+ ((kdev->flags & KEYCHORD_BUSY) == 0));
+ if (ret)
+ return ret;
+ spin_lock_irqsave(&kdev->lock, flags);
+ }
+ kdev->flags |= KEYCHORD_BUSY;
+ spin_unlock_irqrestore(&kdev->lock, flags);
+ return 0;
+}
+
+static void
+keychord_write_unlock(struct keychord_device *kdev)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&kdev->lock, flags);
+ kdev->flags &= ~KEYCHORD_BUSY;
+ spin_unlock_irqrestore(&kdev->lock, flags);
+ wake_up_interruptible(&kdev->write_waitq);
+}
+
+/*
* keychord_write is used to configure the driver
*/
static ssize_t keychord_write(struct file *file, const char __user *buffer,
@@ -250,6 +288,22 @@
return -EFAULT;
}
+ /*
+ * Serialize writes to this device to prevent various races.
+ * 1) writers racing here could do duplicate input_unregister_handler()
+ * calls, resulting in attempting to unlink a node from a list that
+ * does not exist.
+ * 2) writers racing here could do duplicate input_register_handler() calls
+ * below, resulting in a duplicate insertion of a node into the list.
+ * 3) a double kfree of keychords can occur (in the event that
+ * input_register_handler() fails below.
+ */
+ ret = keychord_write_lock(kdev);
+ if (ret) {
+ kfree(keychords);
+ return ret;
+ }
+
/* unregister handler before changing configuration */
if (kdev->registered) {
input_unregister_handler(&kdev->input_handler);
@@ -318,15 +372,19 @@
if (ret) {
kfree(keychords);
kdev->keychords = 0;
+ keychord_write_unlock(kdev);
return ret;
}
kdev->registered = 1;
+ keychord_write_unlock(kdev);
+
return count;
err_unlock_return:
spin_unlock_irqrestore(&kdev->lock, flags);
kfree(keychords);
+ keychord_write_unlock(kdev);
return -EINVAL;
}
@@ -352,6 +410,7 @@
spin_lock_init(&kdev->lock);
init_waitqueue_head(&kdev->waitq);
+ init_waitqueue_head(&kdev->write_waitq);
kdev->input_handler.event = keychord_event;
kdev->input_handler.connect = keychord_connect;