| Message ID | 20190126170722.8035-1-vdronov@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] HID: debug: fix the ring buffer implementation | expand |
On Sat, Jan 26, 2019 at 6:07 PM Vladis Dronov <vdronov@redhat.com> wrote: > > 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 Thanks for the respin. v2 looks good to me. Oleg, can you provide some feedback before I push this? Cheers, Benjamin > > 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> > --- > drivers/hid/hid-debug.c | 116 ++++++++++++++------------------------ > include/linux/hid-debug.h | 9 ++- > 2 files changed, 47 insertions(+), 78 deletions(-) > > diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c > index c530476edba6..08870c909268 100644 > --- 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 (!list->hdev || !list->hdev->debug) { > - ret = -EIO; > - set_current_state(TASK_RUNNING); > - goto out; > - } > - > - /* allow O_NONBLOCK from other threads */ > - mutex_unlock(&list->read_mutex); > - schedule(); > - mutex_lock(&list->read_mutex); > - set_current_state(TASK_INTERRUPTIBLE); > - } > - > - set_current_state(TASK_RUNNING); > - remove_wait_queue(&list->hdev->debug_wait, &wait); > - } > - > - if (ret) > - goto out; > + 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 (signal_pending(current)) { > + ret = -ERESTARTSYS; > + break; > + } > + > + /* 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; > + } > + > + /* allow O_NONBLOCK from other threads */ > + mutex_unlock(&list->read_mutex); > + schedule(); > + mutex_lock(&list->read_mutex); > + set_current_state(TASK_INTERRUPTIBLE); > + } > + > + set_current_state(TASK_RUNNING); > + remove_wait_queue(&list->hdev->debug_wait, &wait); > + > + if (ret) > + goto out; > + } > > - /* pass the ringbuffer contents to userspace */ > -copy_rest: > - if (list->tail == list->head) > - 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 8663f216c563..e7a7c92aaf09 100644 > --- 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); > @@ -38,10 +41,7 @@ void hid_debug_event(struct hid_device *, char *); > 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 > -
On 01/28, Benjamin Tissoires wrote: > > Oleg, can you provide some feedback before I push this? Looks good to me, feel free to add Reviewed-by: Oleg Nesterov <oleg@redhat.com> > > + set_current_state(TASK_RUNNING); I still think that __set_current_state(TASK_RUNNING); will look a bit better, but this is really minor. Oleg.
Vlad, On Mon, Jan 28, 2019 at 12:18 PM Oleg Nesterov <oleg@redhat.com> wrote: > > On 01/28, Benjamin Tissoires wrote: > > > > Oleg, can you provide some feedback before I push this? > > Looks good to me, feel free to add > > Reviewed-by: Oleg Nesterov <oleg@redhat.com> > > > > + set_current_state(TASK_RUNNING); > > I still think that > > __set_current_state(TASK_RUNNING); > > will look a bit better, but this is really minor. Would you mind sending a v3 with this change? I'll apply it ASAP. Cheers, Benjamin
> > I still think that > > > > __set_current_state(TASK_RUNNING); > > > > will look a bit better, but this is really minor. > > Would you mind sending a v3 with this change? I'll apply it ASAP. Done, please, see inbox. Best regards, Vladis Dronov | Red Hat, Inc. | Product Security | Senior Software Engineer
diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c index c530476edba6..08870c909268 100644 --- 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 (!list->hdev || !list->hdev->debug) { - ret = -EIO; - set_current_state(TASK_RUNNING); - goto out; - } - - /* allow O_NONBLOCK from other threads */ - mutex_unlock(&list->read_mutex); - schedule(); - mutex_lock(&list->read_mutex); - set_current_state(TASK_INTERRUPTIBLE); - } - - set_current_state(TASK_RUNNING); - remove_wait_queue(&list->hdev->debug_wait, &wait); - } - - if (ret) - goto out; + 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 (signal_pending(current)) { + ret = -ERESTARTSYS; + break; + } + + /* 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; + } + + /* allow O_NONBLOCK from other threads */ + mutex_unlock(&list->read_mutex); + schedule(); + mutex_lock(&list->read_mutex); + set_current_state(TASK_INTERRUPTIBLE); + } + + set_current_state(TASK_RUNNING); + remove_wait_queue(&list->hdev->debug_wait, &wait); + + if (ret) + goto out; + } - /* pass the ringbuffer contents to userspace */ -copy_rest: - if (list->tail == list->head) - 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 8663f216c563..e7a7c92aaf09 100644 --- 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); @@ -38,10 +41,7 @@ void hid_debug_event(struct hid_device *, char *); 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 -
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 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> --- drivers/hid/hid-debug.c | 116 ++++++++++++++------------------------ include/linux/hid-debug.h | 9 ++- 2 files changed, 47 insertions(+), 78 deletions(-)