Skip to content

Commit

Permalink
HID: debug: fix the ring buffer implementation
Browse files Browse the repository at this point in the history 
commit 13054ab upstream.

Ring buffer implementation in hid_debug_event() and hid_debug_events_read()
is strange allowing lost or corrupted data. After commit 717adfd
("HID: debug: check length before copy_to_user()") it is possible to enter
an infinite loop in hid_debug_events_read() by providing 0 as count, this
locks up a system. Fix this by rewriting the ring buffer implementation
with kfifo and simplify the code.

This fixes CVE-2019-3819.

v2: fix an execution logic and add a comment
v3: use __set_current_state() instead of set_current_state()

Backport to v4.9: some tree-wide patches are missing in v4.9 so
cherry-pick relevant pieces from:
 * 6396bb2 ("treewide: kzalloc() -> kcalloc()")
 * a9a0884 ("vfs: do bulk POLL* -> EPOLL* replacement")
 * 174cd4b ("sched/headers: Prepare to move signal wakeup & sigpending
   methods from <linux/sched.h> into <linux/sched/signal.h>")

Link: https://bugzilla.redhat.com/show_bug.cgi?id=1669187
Cc: stable@vger.kernel.org # v4.18+
Fixes: cd667ce ("HID: use debugfs for events/reports dumping")
Fixes: 717adfd ("HID: debug: check length before copy_to_user()")
Signed-off-by: Vladis Dronov <vdronov@redhat.com>
Reviewed-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
  • Loading branch information
Vladis Dronov authored and Greg Kroah-Hartman committed Feb 15, 2019
1 parent 877362f commit 64a9f5f
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 79 deletions.
122 changes: 48 additions & 74 deletions drivers/hid/hid-debug.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@

#include <linux/debugfs.h>
#include <linux/seq_file.h>
#include <linux/kfifo.h>
#include <linux/sched.h>
#include <linux/export.h>
#include <linux/slab.h>
Expand Down Expand Up @@ -455,7 +456,7 @@ static char *resolv_usage_page(unsigned page, struct seq_file *f) {
char *buf = NULL;

if (!f) {
buf = kzalloc(sizeof(char) * HID_DEBUG_BUFSIZE, GFP_ATOMIC);
buf = kzalloc(HID_DEBUG_BUFSIZE, GFP_ATOMIC);
if (!buf)
return ERR_PTR(-ENOMEM);
}
Expand Down Expand Up @@ -659,17 +660,12 @@ EXPORT_SYMBOL_GPL(hid_dump_device);
/* enqueue string to 'events' ring buffer */
void hid_debug_event(struct hid_device *hdev, char *buf)
{
unsigned i;
struct hid_debug_list *list;
unsigned long flags;

spin_lock_irqsave(&hdev->debug_list_lock, flags);
list_for_each_entry(list, &hdev->debug_list, node) {
for (i = 0; buf[i]; i++)
list->hid_debug_buf[(list->tail + i) % HID_DEBUG_BUFSIZE] =
buf[i];
list->tail = (list->tail + i) % HID_DEBUG_BUFSIZE;
}
list_for_each_entry(list, &hdev->debug_list, node)
kfifo_in(&list->hid_debug_fifo, buf, strlen(buf));
spin_unlock_irqrestore(&hdev->debug_list_lock, flags);

wake_up_interruptible(&hdev->debug_wait);
Expand Down Expand Up @@ -720,8 +716,7 @@ void hid_dump_input(struct hid_device *hdev, struct hid_usage *usage, __s32 valu
hid_debug_event(hdev, buf);

kfree(buf);
wake_up_interruptible(&hdev->debug_wait);

wake_up_interruptible(&hdev->debug_wait);
}
EXPORT_SYMBOL_GPL(hid_dump_input);

Expand Down Expand Up @@ -1086,8 +1081,8 @@ static int hid_debug_events_open(struct inode *inode, struct file *file)
goto out;
}

if (!(list->hid_debug_buf = kzalloc(sizeof(char) * HID_DEBUG_BUFSIZE, GFP_KERNEL))) {
err = -ENOMEM;
err = kfifo_alloc(&list->hid_debug_fifo, HID_DEBUG_FIFOSIZE, GFP_KERNEL);
if (err) {
kfree(list);
goto out;
}
Expand All @@ -1107,77 +1102,57 @@ static ssize_t hid_debug_events_read(struct file *file, char __user *buffer,
size_t count, loff_t *ppos)
{
struct hid_debug_list *list = file->private_data;
int ret = 0, len;
int ret = 0, copied;
DECLARE_WAITQUEUE(wait, current);

mutex_lock(&list->read_mutex);
while (ret == 0) {
if (list->head == list->tail) {
add_wait_queue(&list->hdev->debug_wait, &wait);
set_current_state(TASK_INTERRUPTIBLE);

while (list->head == list->tail) {
if (file->f_flags & O_NONBLOCK) {
ret = -EAGAIN;
break;
}
if (signal_pending(current)) {
ret = -ERESTARTSYS;
break;
}
if (kfifo_is_empty(&list->hid_debug_fifo)) {
add_wait_queue(&list->hdev->debug_wait, &wait);
set_current_state(TASK_INTERRUPTIBLE);

while (kfifo_is_empty(&list->hid_debug_fifo)) {
if (file->f_flags & O_NONBLOCK) {
ret = -EAGAIN;
break;
}

if (!list->hdev || !list->hdev->debug) {
ret = -EIO;
set_current_state(TASK_RUNNING);
goto out;
}
if (signal_pending(current)) {
ret = -ERESTARTSYS;
break;
}

/* allow O_NONBLOCK from other threads */
mutex_unlock(&list->read_mutex);
schedule();
mutex_lock(&list->read_mutex);
set_current_state(TASK_INTERRUPTIBLE);
/* if list->hdev is NULL we cannot remove_wait_queue().
* if list->hdev->debug is 0 then hid_debug_unregister()
* was already called and list->hdev is being destroyed.
* if we add remove_wait_queue() here we can hit a race.
*/
if (!list->hdev || !list->hdev->debug) {
ret = -EIO;
set_current_state(TASK_RUNNING);
goto out;
}

set_current_state(TASK_RUNNING);
remove_wait_queue(&list->hdev->debug_wait, &wait);
/* allow O_NONBLOCK from other threads */
mutex_unlock(&list->read_mutex);
schedule();
mutex_lock(&list->read_mutex);
set_current_state(TASK_INTERRUPTIBLE);
}

if (ret)
goto out;
__set_current_state(TASK_RUNNING);
remove_wait_queue(&list->hdev->debug_wait, &wait);

/* pass the ringbuffer contents to userspace */
copy_rest:
if (list->tail == list->head)
if (ret)
goto out;
if (list->tail > list->head) {
len = list->tail - list->head;
if (len > count)
len = count;

if (copy_to_user(buffer + ret, &list->hid_debug_buf[list->head], len)) {
ret = -EFAULT;
goto out;
}
ret += len;
list->head += len;
} else {
len = HID_DEBUG_BUFSIZE - list->head;
if (len > count)
len = count;

if (copy_to_user(buffer, &list->hid_debug_buf[list->head], len)) {
ret = -EFAULT;
goto out;
}
list->head = 0;
ret += len;
count -= len;
if (count > 0)
goto copy_rest;
}

}

/* pass the fifo content to userspace, locking is not needed with only
* one concurrent reader and one concurrent writer
*/
ret = kfifo_to_user(&list->hid_debug_fifo, buffer, count, &copied);
if (ret)
goto out;
ret = copied;
out:
mutex_unlock(&list->read_mutex);
return ret;
Expand All @@ -1188,7 +1163,7 @@ static unsigned int hid_debug_events_poll(struct file *file, poll_table *wait)
struct hid_debug_list *list = file->private_data;

poll_wait(file, &list->hdev->debug_wait, wait);
if (list->head != list->tail)
if (!kfifo_is_empty(&list->hid_debug_fifo))
return POLLIN | POLLRDNORM;
if (!list->hdev->debug)
return POLLERR | POLLHUP;
Expand All @@ -1203,7 +1178,7 @@ static int hid_debug_events_release(struct inode *inode, struct file *file)
spin_lock_irqsave(&list->hdev->debug_list_lock, flags);
list_del(&list->node);
spin_unlock_irqrestore(&list->hdev->debug_list_lock, flags);
kfree(list->hid_debug_buf);
kfifo_free(&list->hid_debug_fifo);
kfree(list);

return 0;
Expand Down Expand Up @@ -1254,4 +1229,3 @@ void hid_debug_exit(void)
{
debugfs_remove_recursive(hid_debug_root);
}

9 changes: 4 additions & 5 deletions include/linux/hid-debug.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,10 @@

#ifdef CONFIG_DEBUG_FS

#include <linux/kfifo.h>

#define HID_DEBUG_BUFSIZE 512
#define HID_DEBUG_FIFOSIZE 512

void hid_dump_input(struct hid_device *, struct hid_usage *, __s32);
void hid_dump_report(struct hid_device *, int , u8 *, int);
Expand All @@ -37,11 +40,8 @@ void hid_debug_init(void);
void hid_debug_exit(void);
void hid_debug_event(struct hid_device *, char *);


struct hid_debug_list {
char *hid_debug_buf;
int head;
int tail;
DECLARE_KFIFO_PTR(hid_debug_fifo, char);
struct fasync_struct *fasync;
struct hid_device *hdev;
struct list_head node;
Expand All @@ -64,4 +64,3 @@ struct hid_debug_list {
#endif

#endif

0 comments on commit 64a9f5f

Please sign in to comment.