linux-kernel - [PATCH 1/3] HID: debug: avoid infinite loop and corrupting data

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20181003171936.11271-2-vdronov@redhat.com>
Date:   Wed,  3 Oct 2018 19:19:34 +0200
From:   Vladis Dronov <vdronov@...hat.com>
To:     Jiri Kosina <jikos@...nel.org>,
        Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        linux-input@...r.kernel.org, linux-kernel@...r.kernel.org
Cc:     Vladis Dronov <vdronov@...hat.com>
Subject: [PATCH 1/3] HID: debug: avoid infinite loop and corrupting data

hid_debug_events_read() does not properly handle the case when tail < head
and count < HID_DEBUG_BUFSIZE - head and returns corrupted data. Also,
after commit 717adfdaf147 ("HID: debug: check length before
copy_to_user()") it can enter an infinite loop if called with count == 0.
Fix this by properly handling this case and adding a check.

Also, the function has "while (ret == 0)" loop which is not needed, remove
it.

Signed-off-by: Vladis Dronov <vdronov@...hat.com>
---
 drivers/hid/hid-debug.c | 109 ++++++++++++++++++++++++----------------
 1 file changed, 65 insertions(+), 44 deletions(-)

diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
index b48100236df8..20580871b0ec 100644
--- a/drivers/hid/hid-debug.c
+++ b/drivers/hid/hid-debug.c
@@ -722,7 +722,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);
@@ -1112,73 +1112,95 @@ static ssize_t hid_debug_events_read(struct file *file, char __user *buffer,
 	int ret = 0, len;
 	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);
+	if (!count)
+		return 0;
 
-			while (list->head == list->tail) {
-				if (file->f_flags & O_NONBLOCK) {
-					ret = -EAGAIN;
-					break;
-				}
-				if (signal_pending(current)) {
-					ret = -ERESTARTSYS;
-					break;
-				}
+	mutex_lock(&list->read_mutex);
+	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 (!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 || !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);
+	}
+
+	if (ret)
+		goto out;
 
-		/* pass the ringbuffer contents to userspace */
+	/* pass the ringbuffer content 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 (list->tail == list->head)
+		goto out;
+
+	/* the data from the head to the tail in the buffer is linear */
+	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)) {
+		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;
+
+		/* the data is split in 2 pieces: from the head to the end of
+		 * the ring buffer and from the start of the ring buffer to the
+		 *  tail. check if we need to copy out only the part between the
+		 * head and the end.
+		 */
+		if (len > count) {
+			len = count;
+
+			if (copy_to_user(buffer,
+			      &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)) {
+			if (copy_to_user(buffer,
+			      &list->hid_debug_buf[list->head], len)) {
 				ret = -EFAULT;
 				goto out;
 			}
 			list->head = 0;
 			ret += len;
 			count -= len;
+
+			/* check for a corner case when len == count */
 			if (count > 0)
 				goto copy_rest;
 		}
-
 	}
 out:
 	mutex_unlock(&list->read_mutex);
@@ -1256,4 +1278,3 @@ void hid_debug_exit(void)
 {
 	debugfs_remove_recursive(hid_debug_root);
 }
-
-- 
2.19.0