linux-kernel - [PATCH v1 1/2] sysctl: cap to ULONG_MAX in proc_get

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: <20181015105544.4395-2-christian@brauner.io>
Date:   Mon, 15 Oct 2018 12:55:43 +0200
From:   Christian Brauner <christian@...uner.io>
To:     keescook@...omium.org, linux-kernel@...r.kernel.org
Cc:     ebiederm@...ssion.com, mcgrof@...nel.org,
        akpm@...ux-foundation.org, joe.lawrence@...hat.com,
        longman@...hat.com, linux@...inikbrodowski.net,
        viro@...iv.linux.org.uk, Christian Brauner <christian@...uner.io>
Subject: [PATCH v1 1/2] sysctl: cap to ULONG_MAX in proc_get_long()

proc_get_long() is a funny function. It uses simple_strtoul() and for a
good reason. proc_get_long() wants to always succeed the parse and return
the maybe incorrect value and the trailing characters to check against a
pre-defined list of acceptable trailing values.
However, simple_strtoul() explicitly ignores overflows which can cause
funny things like the following to happen:

echo 18446744073709551616 > /proc/sys/fs/file-max
cat /proc/sys/fs/file-max
0

(Which will cause your system to silently die behind your back.)

On the other hand kstrtoul() does do overflow detection but fails the parse
in this case, does not return the trailing characters, and also fails the
parse when anything other than '\n' is a trailing character whereas
proc_get_long() wants to be more lenient.

Now, before adding another kstrtoul() function let's simply add a static
parse strtoul_cap_erange() which does:
- returns ULONG_MAX on ERANGE
- returns the trailing characters to the caller
This guarantees that we don't regress userspace in any way but also caps
any parsed value to ULONG_MAX and prevents things like file-max to become 0
on overflow.

Cc: Kees Cook <keescook@...omium.org>
Signed-off-by: Christian Brauner <christian@...uner.io>
---
v0->v1:
- s/sysctl_strtoul_lenient/strtoul_cap_erange/g
- (Al) remove bool overflow return argument from strtoul_cap_erange
- (Al) return ULONG_MAX on ERANGE from strtoul_cap_erange
- (Dominik) fix spelling in commit message
---
 kernel/sysctl.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index cc02050fd0c4..97551eb42946 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -67,6 +67,7 @@
 #include <linux/bpf.h>
 #include <linux/mount.h>
 #include <linux/pipe_fs_i.h>
+#include <../lib/kstrtox.h>
 
 #include <linux/uaccess.h>
 #include <asm/processor.h>
@@ -2065,6 +2066,25 @@ static void proc_skip_char(char **buf, size_t *size, const char v)
 	}
 }
 
+static unsigned long strtoul_cap_erange(const char *cp, char **endp,
+					unsigned int base)
+{
+	unsigned long long result;
+	unsigned int rv;
+
+	cp = _parse_integer_fixup_radix(cp, &base);
+	rv = _parse_integer(cp, base, &result);
+	if ((rv & KSTRTOX_OVERFLOW) || (result != (unsigned long)result))
+		result = ULONG_MAX;
+
+	cp += (rv & ~KSTRTOX_OVERFLOW);
+
+	if (endp)
+		*endp = (char *)cp;
+
+	return result;
+}
+
 #define TMPBUFLEN 22
 /**
  * proc_get_long - reads an ASCII formatted integer from a user buffer
@@ -2108,7 +2128,7 @@ static int proc_get_long(char **buf, size_t *size,
 	if (!isdigit(*p))
 		return -EINVAL;
 
-	*val = simple_strtoul(p, &p, 0);
+	*val = strtoul_cap_erange(p, &p, 0);
 
 	len = p - tmp;
 
-- 
2.17.1