Skip to content

Commit 7f2923c

Browse files
braunertorvalds
authored andcommitted
sysctl: handle overflow 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 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_lenient() which: - fails on overflow with -ERANGE - returns the trailing characters to the caller The reason why we should fail on ERANGE is that we already do a partial fail on overflow right now. Namely, when the TMPBUFLEN is exceeded. So we already reject values such as 184467440737095516160 (21 chars) but accept values such as 18446744073709551616 (20 chars) but both are overflows. So we should just always reject 64bit overflows and not special-case this based on the number of chars. Link: http://lkml.kernel.org/r/[email protected] Signed-off-by: Christian Brauner <[email protected]> Acked-by: Kees Cook <[email protected]> Cc: "Eric W. Biederman" <[email protected]> Cc: Luis Chamberlain <[email protected]> Cc: Joe Lawrence <[email protected]> Cc: Waiman Long <[email protected]> Cc: Dominik Brodowski <[email protected]> Cc: Al Viro <[email protected]> Cc: Alexey Dobriyan <[email protected]> Signed-off-by: Andrew Morton <[email protected]> Signed-off-by: Linus Torvalds <[email protected]>
1 parent 92bf501 commit 7f2923c

File tree

1 file changed

+39
-1
lines changed

1 file changed

+39
-1
lines changed

kernel/sysctl.c

Lines changed: 39 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,8 @@
6767
#include <linux/bpf.h>
6868
#include <linux/mount.h>
6969

70+
#include "../lib/kstrtox.h"
71+
7072
#include <linux/uaccess.h>
7173
#include <asm/processor.h>
7274

@@ -2117,6 +2119,41 @@ static void proc_skip_char(char **buf, size_t *size, const char v)
21172119
}
21182120
}
21192121

2122+
/**
2123+
* strtoul_lenient - parse an ASCII formatted integer from a buffer and only
2124+
* fail on overflow
2125+
*
2126+
* @cp: kernel buffer containing the string to parse
2127+
* @endp: pointer to store the trailing characters
2128+
* @base: the base to use
2129+
* @res: where the parsed integer will be stored
2130+
*
2131+
* In case of success 0 is returned and @res will contain the parsed integer,
2132+
* @endp will hold any trailing characters.
2133+
* This function will fail the parse on overflow. If there wasn't an overflow
2134+
* the function will defer the decision what characters count as invalid to the
2135+
* caller.
2136+
*/
2137+
static int strtoul_lenient(const char *cp, char **endp, unsigned int base,
2138+
unsigned long *res)
2139+
{
2140+
unsigned long long result;
2141+
unsigned int rv;
2142+
2143+
cp = _parse_integer_fixup_radix(cp, &base);
2144+
rv = _parse_integer(cp, base, &result);
2145+
if ((rv & KSTRTOX_OVERFLOW) || (result != (unsigned long)result))
2146+
return -ERANGE;
2147+
2148+
cp += rv;
2149+
2150+
if (endp)
2151+
*endp = (char *)cp;
2152+
2153+
*res = (unsigned long)result;
2154+
return 0;
2155+
}
2156+
21202157
#define TMPBUFLEN 22
21212158
/**
21222159
* proc_get_long - reads an ASCII formatted integer from a user buffer
@@ -2160,7 +2197,8 @@ static int proc_get_long(char **buf, size_t *size,
21602197
if (!isdigit(*p))
21612198
return -EINVAL;
21622199

2163-
*val = simple_strtoul(p, &p, 0);
2200+
if (strtoul_lenient(p, &p, 0, val))
2201+
return -EINVAL;
21642202

21652203
len = p - tmp;
21662204

0 commit comments

Comments
 (0)