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> (cherry picked from commit 59584701f1e2ce8ce024570576b206bea6ac69cf)
This commit is contained in:
parent
dd5826152c
commit
462acca281
1 changed files with 60 additions and 1 deletions
|
@ -60,6 +60,10 @@ struct keychord_device {
|
||||||
unsigned char head;
|
unsigned char head;
|
||||||
unsigned char tail;
|
unsigned char tail;
|
||||||
__u16 buff[BUFFER_SIZE];
|
__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,
|
static int check_keychord(struct keychord_device *kdev,
|
||||||
|
@ -172,7 +176,6 @@ static int keychord_connect(struct input_handler *handler,
|
||||||
goto err_input_open_device;
|
goto err_input_open_device;
|
||||||
|
|
||||||
pr_info("keychord: using input dev %s for fevent\n", dev->name);
|
pr_info("keychord: using input dev %s for fevent\n", dev->name);
|
||||||
|
|
||||||
return 0;
|
return 0;
|
||||||
|
|
||||||
err_input_open_device:
|
err_input_open_device:
|
||||||
|
@ -224,6 +227,41 @@ static ssize_t keychord_read(struct file *file, char __user *buffer,
|
||||||
return count;
|
return count;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/*
|
||||||
|
* 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
|
* keychord_write is used to configure the driver
|
||||||
*/
|
*/
|
||||||
|
@ -250,6 +288,22 @@ static ssize_t keychord_write(struct file *file, const char __user *buffer,
|
||||||
return -EFAULT;
|
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 */
|
/* unregister handler before changing configuration */
|
||||||
if (kdev->registered) {
|
if (kdev->registered) {
|
||||||
input_unregister_handler(&kdev->input_handler);
|
input_unregister_handler(&kdev->input_handler);
|
||||||
|
@ -318,15 +372,19 @@ static ssize_t keychord_write(struct file *file, const char __user *buffer,
|
||||||
if (ret) {
|
if (ret) {
|
||||||
kfree(keychords);
|
kfree(keychords);
|
||||||
kdev->keychords = 0;
|
kdev->keychords = 0;
|
||||||
|
keychord_write_unlock(kdev);
|
||||||
return ret;
|
return ret;
|
||||||
}
|
}
|
||||||
kdev->registered = 1;
|
kdev->registered = 1;
|
||||||
|
|
||||||
|
keychord_write_unlock(kdev);
|
||||||
|
|
||||||
return count;
|
return count;
|
||||||
|
|
||||||
err_unlock_return:
|
err_unlock_return:
|
||||||
spin_unlock_irqrestore(&kdev->lock, flags);
|
spin_unlock_irqrestore(&kdev->lock, flags);
|
||||||
kfree(keychords);
|
kfree(keychords);
|
||||||
|
keychord_write_unlock(kdev);
|
||||||
return -EINVAL;
|
return -EINVAL;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@ -352,6 +410,7 @@ static int keychord_open(struct inode *inode, struct file *file)
|
||||||
|
|
||||||
spin_lock_init(&kdev->lock);
|
spin_lock_init(&kdev->lock);
|
||||||
init_waitqueue_head(&kdev->waitq);
|
init_waitqueue_head(&kdev->waitq);
|
||||||
|
init_waitqueue_head(&kdev->write_waitq);
|
||||||
|
|
||||||
kdev->input_handler.event = keychord_event;
|
kdev->input_handler.event = keychord_event;
|
||||||
kdev->input_handler.connect = keychord_connect;
|
kdev->input_handler.connect = keychord_connect;
|
||||||
|
|
Loading…
Add table
Reference in a new issue