Stable Archive on lore.kernel.org
 help / Atom feed
* [PATCH v4.14.y] HID: debug: fix the ring buffer implementation
@ 2019-02-11 17:26 Guenter Roeck
  2019-02-13 14:18 ` Greg Kroah-Hartman
  0 siblings, 1 reply; 3+ messages in thread
From: Guenter Roeck @ 2019-02-11 17:26 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: stable, Vladis Dronov, Oleg Nesterov, Benjamin Tissoires, Guenter Roeck

From: Vladis Dronov <vdronov@redhat.com>

commit 13054abbaa4f1fd4e6f3b4b63439ec033b4c8035 upstream.

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
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>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
[groeck: backport to v4.14.y]
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
This patch is marked v4.18+, but commit 717adfdaf147 is marked for stable
and found its way into all stable releases. Therefore, this patch is needed
in older stable releases as well. This patch only applies to v4.14.y;
backport to v4.9.y will follow.

Copying patch author and reviewers to make sure I didn't miss anything.

 drivers/hid/hid-debug.c   | 121 ++++++++++++++++++----------------------------
 include/linux/hid-debug.h |   9 ++--
 2 files changed, 51 insertions(+), 79 deletions(-)

diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
index ae8c8e66a6c4..b4de6656f65f 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);
 
@@ -1088,8 +1083,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;
 	}
@@ -1109,77 +1104,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;
@@ -1189,8 +1164,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;
@@ -1205,7 +1179,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;
@@ -1256,4 +1230,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..2d6100edf204 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);
@@ -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
-
-- 
2.7.4


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v4.14.y] HID: debug: fix the ring buffer implementation
  2019-02-11 17:26 [PATCH v4.14.y] HID: debug: fix the ring buffer implementation Guenter Roeck
@ 2019-02-13 14:18 ` Greg Kroah-Hartman
  2019-02-13 15:04   ` Guenter Roeck
  0 siblings, 1 reply; 3+ messages in thread
From: Greg Kroah-Hartman @ 2019-02-13 14:18 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: stable, Vladis Dronov, Oleg Nesterov, Benjamin Tissoires

On Mon, Feb 11, 2019 at 09:26:25AM -0800, Guenter Roeck wrote:
> From: Vladis Dronov <vdronov@redhat.com>
> 
> commit 13054abbaa4f1fd4e6f3b4b63439ec033b4c8035 upstream.
> 
> 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
> 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>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> [groeck: backport to v4.14.y]
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> This patch is marked v4.18+, but commit 717adfdaf147 is marked for stable
> and found its way into all stable releases. Therefore, this patch is needed
> in older stable releases as well. This patch only applies to v4.14.y;
> backport to v4.9.y will follow.
> 
> Copying patch author and reviewers to make sure I didn't miss anything.
> 
>  drivers/hid/hid-debug.c   | 121 ++++++++++++++++++----------------------------
>  include/linux/hid-debug.h |   9 ++--
>  2 files changed, 51 insertions(+), 79 deletions(-)

Vladis sent backports that are a bit different from yours, so I'll go
with his now :)

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v4.14.y] HID: debug: fix the ring buffer implementation
  2019-02-13 14:18 ` Greg Kroah-Hartman
@ 2019-02-13 15:04   ` Guenter Roeck
  0 siblings, 0 replies; 3+ messages in thread
From: Guenter Roeck @ 2019-02-13 15:04 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: stable, Vladis Dronov, Oleg Nesterov, Benjamin Tissoires

On 2/13/19 6:18 AM, Greg Kroah-Hartman wrote:
> On Mon, Feb 11, 2019 at 09:26:25AM -0800, Guenter Roeck wrote:
>> From: Vladis Dronov <vdronov@redhat.com>
>>
>> commit 13054abbaa4f1fd4e6f3b4b63439ec033b4c8035 upstream.
>>
>> 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
>> 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>
>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> [groeck: backport to v4.14.y]
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>> This patch is marked v4.18+, but commit 717adfdaf147 is marked for stable
>> and found its way into all stable releases. Therefore, this patch is needed
>> in older stable releases as well. This patch only applies to v4.14.y;
>> backport to v4.9.y will follow.
>>
>> Copying patch author and reviewers to make sure I didn't miss anything.
>>
>>   drivers/hid/hid-debug.c   | 121 ++++++++++++++++++----------------------------
>>   include/linux/hid-debug.h |   9 ++--
>>   2 files changed, 51 insertions(+), 79 deletions(-)
> 
> Vladis sent backports that are a bit different from yours, so I'll go
> with his now :)
> 

NP, and preferred, as long as the problem is getting fixed ...

Guenter

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-11 17:26 [PATCH v4.14.y] HID: debug: fix the ring buffer implementation Guenter Roeck
2019-02-13 14:18 ` Greg Kroah-Hartman
2019-02-13 15:04   ` Guenter Roeck

Stable Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/stable/0 stable/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 stable stable/ https://lore.kernel.org/stable \
		stable@vger.kernel.org stable@archiver.kernel.org
	public-inbox-index stable


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.stable


AGPL code for this site: git clone https://public-inbox.org/ public-inbox