linux-kernel - [PATCH] habanalabs: fix debugfs code

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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Message-Id: <20190504135608.176687-1-jannh@google.com>
Date:   Sat,  4 May 2019 15:56:08 +0200
From:   Jann Horn <jannh@...gle.com>
To:     Oded Gabbay <oded.gabbay@...il.com>, jannh@...gle.com
Cc:     linux-kernel@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Mike Rapoport <rppt@...ux.ibm.com>
Subject: [PATCH] habanalabs: fix debugfs code

This fixes multiple things in the habanalabs debugfs code, in particular:

 - mmu_write() was unnecessarily verbose, copying around between multiple
   buffers
 - mmu_write() could write a user-specified, unbounded amount of userspace
   memory into a kernel buffer (out-of-bounds write)
 - multiple debugfs read handlers ignored the user-supplied count,
   potentially corrupting out-of-bounds userspace data
 - hl_device_read() was unnecessarily verbose
 - hl_device_write() could read uninitialized stack memory
 - multiple debugfs read handlers copied terminating null characters to
   userspace

Signed-off-by: Jann Horn <jannh@...gle.com>
---
compile-tested only, so you might want to test this before applying the
patch

 drivers/misc/habanalabs/debugfs.c | 60 ++++++++++---------------------
 1 file changed, 18 insertions(+), 42 deletions(-)

diff --git a/drivers/misc/habanalabs/debugfs.c b/drivers/misc/habanalabs/debugfs.c
index 974a87789bd86..17ba26422b297 100644
--- a/drivers/misc/habanalabs/debugfs.c
+++ b/drivers/misc/habanalabs/debugfs.c
@@ -459,41 +459,31 @@ static ssize_t mmu_write(struct file *file, const char __user *buf,
 	struct hl_debugfs_entry *entry = s->private;
 	struct hl_dbg_device_entry *dev_entry = entry->dev_entry;
 	struct hl_device *hdev = dev_entry->hdev;
-	char kbuf[MMU_KBUF_SIZE], asid_kbuf[MMU_ASID_BUF_SIZE],
-		addr_kbuf[MMU_ADDR_BUF_SIZE];
+	char kbuf[MMU_KBUF_SIZE];
 	char *c;
 	ssize_t rc;
 
 	if (!hdev->mmu_enable)
 		return count;
 
-	memset(kbuf, 0, sizeof(kbuf));
-	memset(asid_kbuf, 0, sizeof(asid_kbuf));
-	memset(addr_kbuf, 0, sizeof(addr_kbuf));
-
+	if (count > sizeof(kbuf) - 1)
+		goto err;
 	if (copy_from_user(kbuf, buf, count))
 		goto err;
-
-	kbuf[MMU_KBUF_SIZE - 1] = 0;
+	kbuf[count] = 0;
 
 	c = strchr(kbuf, ' ');
 	if (!c)
 		goto err;
+	*c = '\0';
 
-	memcpy(asid_kbuf, kbuf, c - kbuf);
-
-	rc = kstrtouint(asid_kbuf, 10, &dev_entry->mmu_asid);
+	rc = kstrtouint(kbuf, 10, &dev_entry->mmu_asid);
 	if (rc)
 		goto err;
 
-	c = strstr(kbuf, " 0x");
-	if (!c)
+	if (strncmp(c+1, "0x", 2))
 		goto err;
-
-	c += 3;
-	memcpy(addr_kbuf, c, (kbuf + count) - c);
-
-	rc = kstrtoull(addr_kbuf, 16, &dev_entry->mmu_addr);
+	rc = kstrtoull(c+3, 16, &dev_entry->mmu_addr);
 	if (rc)
 		goto err;
 
@@ -525,10 +515,8 @@ static ssize_t hl_data_read32(struct file *f, char __user *buf,
 	}
 
 	sprintf(tmp_buf, "0x%08x\n", val);
-	rc = simple_read_from_buffer(buf, strlen(tmp_buf) + 1, ppos, tmp_buf,
-			strlen(tmp_buf) + 1);
-
-	return rc;
+	return simple_read_from_buffer(buf, count, ppos, tmp_buf,
+			strlen(tmp_buf));
 }
 
 static ssize_t hl_data_write32(struct file *f, const char __user *buf,
@@ -559,7 +547,6 @@ static ssize_t hl_get_power_state(struct file *f, char __user *buf,
 	struct hl_dbg_device_entry *entry = file_inode(f)->i_private;
 	struct hl_device *hdev = entry->hdev;
 	char tmp_buf[200];
-	ssize_t rc;
 	int i;
 
 	if (*ppos)
@@ -574,10 +561,8 @@ static ssize_t hl_get_power_state(struct file *f, char __user *buf,
 
 	sprintf(tmp_buf,
 		"current power state: %d\n1 - D0\n2 - D3hot\n3 - Unknown\n", i);
-	rc = simple_read_from_buffer(buf, strlen(tmp_buf) + 1, ppos, tmp_buf,
-			strlen(tmp_buf) + 1);
-
-	return rc;
+	return simple_read_from_buffer(buf, count, ppos, tmp_buf,
+			strlen(tmp_buf));
 }
 
 static ssize_t hl_set_power_state(struct file *f, const char __user *buf,
@@ -630,8 +615,8 @@ static ssize_t hl_i2c_data_read(struct file *f, char __user *buf,
 	}
 
 	sprintf(tmp_buf, "0x%02x\n", val);
-	rc = simple_read_from_buffer(buf, strlen(tmp_buf) + 1, ppos, tmp_buf,
-			strlen(tmp_buf) + 1);
+	rc = simple_read_from_buffer(buf, count, ppos, tmp_buf,
+			strlen(tmp_buf));
 
 	return rc;
 }
@@ -720,18 +705,9 @@ static ssize_t hl_led2_write(struct file *f, const char __user *buf,
 static ssize_t hl_device_read(struct file *f, char __user *buf,
 					size_t count, loff_t *ppos)
 {
-	char tmp_buf[200];
-	ssize_t rc;
-
-	if (*ppos)
-		return 0;
-
-	sprintf(tmp_buf,
-		"Valid values: disable, enable, suspend, resume, cpu_timeout\n");
-	rc = simple_read_from_buffer(buf, strlen(tmp_buf) + 1, ppos, tmp_buf,
-			strlen(tmp_buf) + 1);
-
-	return rc;
+	static const char *help =
+		"Valid values: disable, enable, suspend, resume, cpu_timeout\n";
+	return simple_read_from_buffer(buf, count, ppos, help, strlen(help));
 }
 
 static ssize_t hl_device_write(struct file *f, const char __user *buf,
@@ -739,7 +715,7 @@ static ssize_t hl_device_write(struct file *f, const char __user *buf,
 {
 	struct hl_dbg_device_entry *entry = file_inode(f)->i_private;
 	struct hl_device *hdev = entry->hdev;
-	char data[30];
+	char data[30] = {0};
 
 	/* don't allow partial writes */
 	if (*ppos != 0)
-- 
2.21.0.1020.gf2820cf01a-goog