linux-kernel - [PATCH 3/6] proc: allocate count + 1 for our read buffer

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-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20200813210411.905010-4-josef@toxicpanda.com>
Date:   Thu, 13 Aug 2020 17:04:08 -0400
From:   Josef Bacik <josef@...icpanda.com>
To:     hch@....de, viro@...IV.linux.org.uk, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org, willy@...radead.org,
        kernel-team@...com
Subject: [PATCH 3/6] proc: allocate count + 1 for our read buffer

Al suggested that if we allocate enough space to add in the '\0'
character at the end of our strings, we could just use scnprintf() in
our ->proc_handler functions without having to be fancy about keeping
track of space.  There are a lot of these handlers, so the follow ups
will be separate, but start with allocating the extra byte to handle the
null termination of strings.

Signed-off-by: Josef Bacik <josef@...icpanda.com>
---
 fs/proc/proc_sysctl.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 8e19bad83b45..446e7a949025 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -548,6 +548,7 @@ static ssize_t proc_sys_call_handler(struct file *filp, void __user *ubuf,
 	struct ctl_table *table = PROC_I(inode)->sysctl_entry;
 	void *kbuf;
 	ssize_t error;
+	size_t orig_count = count;
 
 	if (IS_ERR(head))
 		return PTR_ERR(head);
@@ -577,9 +578,23 @@ static ssize_t proc_sys_call_handler(struct file *filp, void __user *ubuf,
 			goto out;
 		}
 	} else {
-		kbuf = kvzalloc(count, GFP_KERNEL);
+		/*
+		 * To make our lives easier in ->proc_handler, we allocate an
+		 * extra byte to allow us to use scnprintf() for handling the
+		 * buffer output.  This works properly because scnprintf() will
+		 * only return the number of bytes that it was able to write
+		 * out, _NOT_ including the NULL byte.  This means the handler's
+		 * will only ever return a maximum of count as what they've
+		 * copied.
+		 *
+		 * HOWEVER, we do not assume that ->proc_handlers are without
+		 * bugs, so further down we'll do an extra check to make sure
+		 * that count isn't larger than the orig_count.
+		 */
+		kbuf = kvzalloc(count + 1, GFP_KERNEL);
 		if (!kbuf)
 			goto out;
+		count += 1;
 	}
 
 	error = BPF_CGROUP_RUN_PROG_SYSCTL(head, table, write, &kbuf, &count,
@@ -593,6 +608,13 @@ static ssize_t proc_sys_call_handler(struct file *filp, void __user *ubuf,
 		goto out_free_buf;
 
 	if (!write) {
+		/*
+		 * This shouldn't happen, but those are the last words before
+		 * somebody adds a security vulnerability, so just make sure
+		 * that count isn't larger than orig_count.
+		 */
+		if (count > orig_count)
+			count = orig_count;
 		error = -EFAULT;
 		if (copy_to_user(ubuf, kbuf, count))
 			goto out_free_buf;
-- 
2.24.1