HID: debug: fix the ring buffer implementation

Ring buffer implementation in hid_debug_event() and hid_debug_events_read()
is strange allowing lost or corrupted data. After commit 717adfdaf147
("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()

Link: https://bugzilla.redhat.com/show_bug.cgi?id=1669187
Cc: stable@vger.kernel.org # v4.18+
Fixes: cd667ce24796 ("HID: use debugfs for events/reports dumping")
Fixes: 717adfdaf147 ("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>

diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
index c530476edba62b7804b892e47a675396dfe1c3c8..ac9fda1b5a7233c227cd5517dcf6331a29483a60 100644 (file)
--- a/drivers/hid/hid-debug.c
+++ b/drivers/hid/hid-debug.c
@@ -30,6 +30,7 @@
 
 #include <linux/debugfs.h>
 #include <linux/seq_file.h>
+#include <linux/kfifo.h>
 #include <linux/sched/signal.h>
 #include <linux/export.h>
 #include <linux/slab.h>
@@ -661,17 +662,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);
@@ -722,8 +718,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);
 
@@ -1083,8 +1078,8 @@ static int hid_debug_events_open(struct inode *inode, struct file *file)
                goto out;
        }
 
-       if (!(list->hid_debug_buf = kzalloc(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;
        }
@@ -1104,77 +1099,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;
@@ -1185,7 +1160,7 @@ static __poll_t 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 EPOLLIN | EPOLLRDNORM;
        if (!list->hdev->debug)
                return EPOLLERR | EPOLLHUP;
@@ -1200,7 +1175,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;
@@ -1246,4 +1221,3 @@ void hid_debug_exit(void)
 {
        debugfs_remove_recursive(hid_debug_root);
 }
-

diff --git a/include/linux/hid-debug.h b/include/linux/hid-debug.h
index 8663f216c563e9023df067c83ec9b6e5fbcbe02b..2d6100edf2049d6195b13f03729ac1165f8c192b 100644 (file)
--- a/include/linux/hid-debug.h
+++ b/include/linux/hid-debug.h
@@ -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);
@@ -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;
@@ -64,4 +64,3 @@ struct hid_debug_list {
 #endif
 
 #endif
-