* [PATCH 0/3] sysctl: detect overflows when setting integers @ 2015-03-29 19:28 Heinrich Schuchardt 2015-03-29 19:28 ` [PATCH 1/3] lib/kstrtox.c: functions returning end of string Heinrich Schuchardt ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: Heinrich Schuchardt @ 2015-03-29 19:28 UTC (permalink / raw) To: Andrew Morton Cc: Michal Nazarewicz, Ingo Molnar, Steven Rostedt, Peter Zijlstra, Joe Perches, Josh Hunt, Rasmus Villemoes, Rusty Russell, Daniel Walter, David Rientjes, Kees Cook, David S. Miller, Johannes Weiner, Aaron Tomlin, Prarit Bhargava, Eric B Munson, Paul E. McKenney, Sam Ravnborg, linux-kernel, Heinrich Schuchardt This patch series addresses undetected overflows when writing to the sysctl file system. E.g. echo 0x800001234 > /proc/sys/kernel/threads-max has the same effect as echo 0x1234 > /proc/sys/kernel/threads-max The first type of overflow occurs when converting from string to unsigned long. The second type of overflow occurs when converting from unsigned long to int. The first patch provide new functions kstrtoul_e and kstrtoull_e that can be used to replace deprecated simple_strtoul and simple_strtoull. The second patch replaces a call to simple_strtoul by kstrtoul_e. This is necessary to detect overflows when converting from string to unsigned long. The third patch adds checks when converting form unsigned long to int. Heinrich Schuchardt (3): lib/kstrtox.c: functions returning end of string sysctl: detect overflows in proc_get_long sysctl: detect overflows when converting to int include/linux/kernel.h | 4 +++ kernel/sysctl.c | 13 +++++++-- lib/kstrtox.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++--- 3 files changed, 83 insertions(+), 5 deletions(-) -- 2.1.4 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/3] lib/kstrtox.c: functions returning end of string 2015-03-29 19:28 [PATCH 0/3] sysctl: detect overflows when setting integers Heinrich Schuchardt @ 2015-03-29 19:28 ` Heinrich Schuchardt 2015-03-29 19:28 ` [PATCH 2/3] sysctl: detect overflows in proc_get_long Heinrich Schuchardt 2015-03-29 19:28 ` [PATCH 3/3] sysctl: detect overflows when converting to int Heinrich Schuchardt 2 siblings, 0 replies; 8+ messages in thread From: Heinrich Schuchardt @ 2015-03-29 19:28 UTC (permalink / raw) To: Andrew Morton Cc: Michal Nazarewicz, Ingo Molnar, Steven Rostedt, Peter Zijlstra, Joe Perches, Josh Hunt, Rasmus Villemoes, Rusty Russell, Daniel Walter, David Rientjes, Kees Cook, David S. Miller, Johannes Weiner, Aaron Tomlin, Prarit Bhargava, Eric B Munson, Paul E. McKenney, Sam Ravnborg, linux-kernel, Heinrich Schuchardt Functions simple_strtoul and simple_strtoull have been deprecated as they cannot return overflows. See https://lkml.org/lkml/2011/2/26/52 Unfortunately functions simple_strtoul and simple_strtoull cannot be replaced by kstrtoul and kstrtoull in some places, because they expect a zero terminated string instead of returning a pointer to the character after the last digit. This patch introduces two new functions kstrtoul_e and kstrtoull_e which fill this gap. Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> --- include/linux/kernel.h | 4 +++ lib/kstrtox.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 72 insertions(+), 3 deletions(-) diff --git a/include/linux/kernel.h b/include/linux/kernel.h index d6d630d..580212e 100644 --- a/include/linux/kernel.h +++ b/include/linux/kernel.h @@ -255,6 +255,10 @@ void complete_and_exit(struct completion *, long) int __must_check _kstrtoul(const char *s, unsigned int base, unsigned long *res); int __must_check _kstrtol(const char *s, unsigned int base, long *res); +int __must_check kstrtoull_e(const char *s, char **ends, unsigned int base, + unsigned long long *res); +int __must_check kstrtoul_e(const char *s, char **ends, unsigned int base, + unsigned long *res); int __must_check kstrtoull(const char *s, unsigned int base, unsigned long long *res); int __must_check kstrtoll(const char *s, unsigned int base, long long *res); diff --git a/lib/kstrtox.c b/lib/kstrtox.c index ec8da78..e2b8618 100644 --- a/lib/kstrtox.c +++ b/lib/kstrtox.c @@ -83,7 +83,8 @@ unsigned int _parse_integer(const char *s, unsigned int base, unsigned long long return rv; } -static int _kstrtoull(const char *s, unsigned int base, unsigned long long *res) +static int _kstrtoull_e(const char *s, char **ends, unsigned int base, + unsigned long long *res) { unsigned long long _res; unsigned int rv; @@ -95,15 +96,79 @@ static int _kstrtoull(const char *s, unsigned int base, unsigned long long *res) if (rv == 0) return -EINVAL; s += rv; + + if (ends) { + *ends = (char *) s; + goto out_ok; + } + if (*s == '\n') s++; if (*s) return -EINVAL; + +out_ok: *res = _res; return 0; } /** + * kstrtoull_e - convert a string to an unsigned long long + * @s: The start of the string. + * @ends: Returns a pointer to the character after the last digit. + * @base: The number base to use. The maximum supported base is 16. If base is + * given as 0, then the base of the string is automatically detected with the + * conventional semantics - If it begins with 0x the number will be parsed as a + * hexadecimal (case insensitive), if it otherwise begins with 0, it will be + * parsed as an octal number. Otherwise it will be parsed as a decimal. + * @res: Where to write the result of the conversion on success. + * + * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error. + * Used as a replacement for the obsolete simple_strtoull. Return code must + * be checked. + */ +int kstrtoull_e(const char *s, char **ends, unsigned int base, + unsigned long long *res) +{ + if (!ends) + return -EINVAL; + + return _kstrtoull_e(s, ends, base, res); +} +EXPORT_SYMBOL(kstrtoull_e); + +/** + * kstrtoul_e - convert a string to an unsigned long + * @s: The start of the string. + * @ends: Returns a pointer to the character after the last digit. + * @base: The number base to use. The maximum supported base is 16. If base is + * given as 0, then the base of the string is automatically detected with the + * conventional semantics - If it begins with 0x the number will be parsed as a + * hexadecimal (case insensitive), if it otherwise begins with 0, it will be + * parsed as an octal number. Otherwise it will be parsed as a decimal. + * @res: Where to write the result of the conversion on success. + * + * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error. + * Used as a replacement for the obsolete simple_strtoul. Return code must + * be checked. + */ +int kstrtoul_e(const char *s, char **ends, unsigned int base, + unsigned long *res) +{ + unsigned long long value; + int rv; + + rv = kstrtoull_e(s, ends, base, &value); + if (rv < 0) + return rv; + if (value != (unsigned long long) (unsigned long) value) + return -ERANGE; + *res = value; + return 0; +} +EXPORT_SYMBOL(kstrtoul_e); + +/** * kstrtoull - convert a string to an unsigned long long * @s: The start of the string. The string must be null-terminated, and may also * include a single newline before its terminating null. The first character @@ -123,7 +188,7 @@ int kstrtoull(const char *s, unsigned int base, unsigned long long *res) { if (s[0] == '+') s++; - return _kstrtoull(s, base, res); + return _kstrtoull_e(s, NULL, base, res); } EXPORT_SYMBOL(kstrtoull); @@ -149,7 +214,7 @@ int kstrtoll(const char *s, unsigned int base, long long *res) int rv; if (s[0] == '-') { - rv = _kstrtoull(s + 1, base, &tmp); + rv = _kstrtoull_e(s + 1, NULL, base, &tmp); if (rv < 0) return rv; if ((long long)(-tmp) >= 0) -- 2.1.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/3] sysctl: detect overflows in proc_get_long 2015-03-29 19:28 [PATCH 0/3] sysctl: detect overflows when setting integers Heinrich Schuchardt 2015-03-29 19:28 ` [PATCH 1/3] lib/kstrtox.c: functions returning end of string Heinrich Schuchardt @ 2015-03-29 19:28 ` Heinrich Schuchardt 2015-03-29 19:28 ` [PATCH 3/3] sysctl: detect overflows when converting to int Heinrich Schuchardt 2 siblings, 0 replies; 8+ messages in thread From: Heinrich Schuchardt @ 2015-03-29 19:28 UTC (permalink / raw) To: Andrew Morton Cc: Michal Nazarewicz, Ingo Molnar, Steven Rostedt, Peter Zijlstra, Joe Perches, Josh Hunt, Rasmus Villemoes, Rusty Russell, Daniel Walter, David Rientjes, Kees Cook, David S. Miller, Johannes Weiner, Aaron Tomlin, Prarit Bhargava, Eric B Munson, Paul E. McKenney, Sam Ravnborg, linux-kernel, Heinrich Schuchardt When converting strings to unsigned long overflows may occur. These currently are not detected. E.g. on a 32bit system echo 0x800001234 > /proc/sys/kernel/threads-max has the same effect as echo 0x1234 > /proc/sys/kernel/threads-max The patch replaces the call to deprecated simple_strtoul by a call to kstrtoul_e. Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> --- kernel/sysctl.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/kernel/sysctl.c b/kernel/sysctl.c index 88ea2d6..4d9d139 100644 --- a/kernel/sysctl.c +++ b/kernel/sysctl.c @@ -1884,7 +1884,8 @@ static int proc_get_long(char **buf, size_t *size, if (!isdigit(*p)) return -EINVAL; - *val = simple_strtoul(p, &p, 0); + if (kstrtoul_e(p, &p, 0, val) < 0) + return -EINVAL; len = p - tmp; -- 2.1.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 3/3] sysctl: detect overflows when converting to int 2015-03-29 19:28 [PATCH 0/3] sysctl: detect overflows when setting integers Heinrich Schuchardt 2015-03-29 19:28 ` [PATCH 1/3] lib/kstrtox.c: functions returning end of string Heinrich Schuchardt 2015-03-29 19:28 ` [PATCH 2/3] sysctl: detect overflows in proc_get_long Heinrich Schuchardt @ 2015-03-29 19:28 ` Heinrich Schuchardt 2015-03-31 17:12 ` Fwd: " Heinrich Schuchardt 2015-03-31 22:45 ` Andrew Morton 2 siblings, 2 replies; 8+ messages in thread From: Heinrich Schuchardt @ 2015-03-29 19:28 UTC (permalink / raw) To: Andrew Morton Cc: Michal Nazarewicz, Ingo Molnar, Steven Rostedt, Peter Zijlstra, Joe Perches, Josh Hunt, Rasmus Villemoes, Rusty Russell, Daniel Walter, David Rientjes, Kees Cook, David S. Miller, Johannes Weiner, Aaron Tomlin, Prarit Bhargava, Eric B Munson, Paul E. McKenney, Sam Ravnborg, linux-kernel, Heinrich Schuchardt When converting unsigned long to int overflows may occur. These currently are not detected when writing to the sysctl file system. E.g. on a system where int has 32 bits and long has 64 bits echo 0x800001234 > /proc/sys/kernel/threads-max has the same effect as echo 0x1234 > /proc/sys/kernel/threads-max The patch adds the missing check in do_proc_dointvec_conv. With the patch an overflow will result in an error EINVAL when writing to the the sysctl file system. Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> --- kernel/sysctl.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/kernel/sysctl.c b/kernel/sysctl.c index 4d9d139..2fd6b82 100644 --- a/kernel/sysctl.c +++ b/kernel/sysctl.c @@ -1953,7 +1953,15 @@ static int do_proc_dointvec_conv(bool *negp, unsigned long *lvalp, int write, void *data) { if (write) { - *valp = *negp ? -*lvalp : *lvalp; + if (*negp) { + if (*lvalp > (unsigned long) INT_MAX + 1) + return -EINVAL; + *valp = -*lvalp; + } else { + if (*lvalp > (unsigned long) INT_MAX) + return -EINVAL; + *valp = *lvalp; + } } else { int val = *valp; if (val < 0) { -- 2.1.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Fwd: [PATCH 3/3] sysctl: detect overflows when converting to int 2015-03-29 19:28 ` [PATCH 3/3] sysctl: detect overflows when converting to int Heinrich Schuchardt @ 2015-03-31 17:12 ` Heinrich Schuchardt 2015-03-31 22:45 ` Andrew Morton 1 sibling, 0 replies; 8+ messages in thread From: Heinrich Schuchardt @ 2015-03-31 17:12 UTC (permalink / raw) To: Andrew Morton Cc: Michal Nazarewicz, Ingo Molnar, Steven Rostedt, Peter Zijlstra, Joe Perches, Josh Hunt, Rasmus Villemoes, Rusty Russell, Daniel Walter, David Rientjes, Kees Cook, David S. Miller, Johannes Weiner, Aaron Tomlin, Prarit Bhargava, Eric B Munson, Paul E. McKenney, Sam Ravnborg, linux-kernel, Alexey Dobriyan Hello Andrew, could you, please, propagate the patch in https://lkml.org/lkml/2015/3/29/189 to the MM tree. It is independent of the other two patches in the patch series. Rework for the other patches was requested by Alexey Dobriyan in https://lkml.org/lkml/2015/3/31/251 Best regards Heinrich Schuchardt On 29.03.2015 21:28, Heinrich Schuchardt wrote: > When converting unsigned long to int overflows may occur. > These currently are not detected when writing to the sysctl > file system. > > E.g. on a system where int has 32 bits and long has 64 bits > echo 0x800001234 > /proc/sys/kernel/threads-max > has the same effect as > echo 0x1234 > /proc/sys/kernel/threads-max > > The patch adds the missing check in do_proc_dointvec_conv. > > With the patch an overflow will result in an error EINVAL when > writing to the the sysctl file system. > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > --- > kernel/sysctl.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/kernel/sysctl.c b/kernel/sysctl.c > index 4d9d139..2fd6b82 100644 > --- a/kernel/sysctl.c > +++ b/kernel/sysctl.c > @@ -1953,7 +1953,15 @@ static int do_proc_dointvec_conv(bool *negp, unsigned long *lvalp, > int write, void *data) > { > if (write) { > - *valp = *negp ? -*lvalp : *lvalp; > + if (*negp) { > + if (*lvalp > (unsigned long) INT_MAX + 1) > + return -EINVAL; > + *valp = -*lvalp; > + } else { > + if (*lvalp > (unsigned long) INT_MAX) > + return -EINVAL; > + *valp = *lvalp; > + } > } else { > int val = *valp; > if (val < 0) { > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] sysctl: detect overflows when converting to int 2015-03-29 19:28 ` [PATCH 3/3] sysctl: detect overflows when converting to int Heinrich Schuchardt 2015-03-31 17:12 ` Fwd: " Heinrich Schuchardt @ 2015-03-31 22:45 ` Andrew Morton 2015-04-01 17:31 ` Heinrich Schuchardt 1 sibling, 1 reply; 8+ messages in thread From: Andrew Morton @ 2015-03-31 22:45 UTC (permalink / raw) To: Heinrich Schuchardt Cc: Michal Nazarewicz, Ingo Molnar, Steven Rostedt, Peter Zijlstra, Joe Perches, Josh Hunt, Rasmus Villemoes, Rusty Russell, Daniel Walter, David Rientjes, Kees Cook, David S. Miller, Johannes Weiner, Aaron Tomlin, Prarit Bhargava, Eric B Munson, Paul E. McKenney, Sam Ravnborg, linux-kernel On Sun, 29 Mar 2015 21:28:29 +0200 Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > When converting unsigned long to int overflows may occur. > These currently are not detected when writing to the sysctl > file system. > > E.g. on a system where int has 32 bits and long has 64 bits > echo 0x800001234 > /proc/sys/kernel/threads-max > has the same effect as > echo 0x1234 > /proc/sys/kernel/threads-max > > The patch adds the missing check in do_proc_dointvec_conv. > > With the patch an overflow will result in an error EINVAL when > writing to the the sysctl file system. hm, why fix this? There's a small risk of breaking accidentally-working userspace, but I expect we can live with that. But how big a problem is this, really? This behaviour is quite expected, after all. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] sysctl: detect overflows when converting to int 2015-03-31 22:45 ` Andrew Morton @ 2015-04-01 17:31 ` Heinrich Schuchardt 2015-06-08 22:41 ` Kees Cook 0 siblings, 1 reply; 8+ messages in thread From: Heinrich Schuchardt @ 2015-04-01 17:31 UTC (permalink / raw) To: Andrew Morton Cc: Michal Nazarewicz, Ingo Molnar, Steven Rostedt, Peter Zijlstra, Joe Perches, Josh Hunt, Rasmus Villemoes, Rusty Russell, Daniel Walter, David Rientjes, Kees Cook, David S. Miller, Johannes Weiner, Aaron Tomlin, Prarit Bhargava, Eric B Munson, Paul E. McKenney, Sam Ravnborg, linux-kernel On 01.04.2015 00:45, Andrew Morton wrote: > On Sun, 29 Mar 2015 21:28:29 +0200 Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > >> When converting unsigned long to int overflows may occur. >> These currently are not detected when writing to the sysctl >> file system. >> >> E.g. on a system where int has 32 bits and long has 64 bits >> echo 0x800001234 > /proc/sys/kernel/threads-max >> has the same effect as >> echo 0x1234 > /proc/sys/kernel/threads-max >> >> The patch adds the missing check in do_proc_dointvec_conv. >> >> With the patch an overflow will result in an error EINVAL when >> writing to the the sysctl file system. > > hm, why fix this? There's a small risk of breaking > accidentally-working userspace, but I expect we can live with that. > > But how big a problem is this, really? This behaviour is quite > expected, after all. > The typical user of a Linux system has never read the Kernel code and possibly has limited programming experience. Furthermore in Documentation/sysctl/kernel.txt there is no hint that only 32-bit integers can be used. So why should this typical user expect that on a 64-bit system +3000000000 is considered a negative number? Now that we know this is a bug why shouldn't we fix it? Best regards Heinrich ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] sysctl: detect overflows when converting to int 2015-04-01 17:31 ` Heinrich Schuchardt @ 2015-06-08 22:41 ` Kees Cook 0 siblings, 0 replies; 8+ messages in thread From: Kees Cook @ 2015-06-08 22:41 UTC (permalink / raw) To: Heinrich Schuchardt Cc: Andrew Morton, Michal Nazarewicz, Ingo Molnar, Steven Rostedt, Peter Zijlstra, Joe Perches, Josh Hunt, Rasmus Villemoes, Rusty Russell, Daniel Walter, David Rientjes, David S. Miller, Johannes Weiner, Aaron Tomlin, Prarit Bhargava, Eric B Munson, Paul E. McKenney, Sam Ravnborg, LKML On Wed, Apr 1, 2015 at 10:31 AM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > On 01.04.2015 00:45, Andrew Morton wrote: >> On Sun, 29 Mar 2015 21:28:29 +0200 Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >> >>> When converting unsigned long to int overflows may occur. >>> These currently are not detected when writing to the sysctl >>> file system. >>> >>> E.g. on a system where int has 32 bits and long has 64 bits >>> echo 0x800001234 > /proc/sys/kernel/threads-max >>> has the same effect as >>> echo 0x1234 > /proc/sys/kernel/threads-max >>> >>> The patch adds the missing check in do_proc_dointvec_conv. >>> >>> With the patch an overflow will result in an error EINVAL when >>> writing to the the sysctl file system. >> >> hm, why fix this? There's a small risk of breaking >> accidentally-working userspace, but I expect we can live with that. >> >> But how big a problem is this, really? This behaviour is quite >> expected, after all. >> > > The typical user of a Linux system has never read the Kernel code and > possibly has limited programming experience. > Furthermore in Documentation/sysctl/kernel.txt there is no hint that > only 32-bit integers can be used. > So why should this typical user expect that on a 64-bit system > +3000000000 is considered a negative number? > > Now that we know this is a bug why shouldn't we fix it? I think this is worth fixing. It is, from a certain perspective, "unexpected behavior". At the very least we could tie it to the sysctl_writes_strict flag? Anything depending on an overflow to get "correct" results seems extremely unlikely to me. -Kees -- Kees Cook Chrome OS Security ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2015-06-08 22:41 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-03-29 19:28 [PATCH 0/3] sysctl: detect overflows when setting integers Heinrich Schuchardt 2015-03-29 19:28 ` [PATCH 1/3] lib/kstrtox.c: functions returning end of string Heinrich Schuchardt 2015-03-29 19:28 ` [PATCH 2/3] sysctl: detect overflows in proc_get_long Heinrich Schuchardt 2015-03-29 19:28 ` [PATCH 3/3] sysctl: detect overflows when converting to int Heinrich Schuchardt 2015-03-31 17:12 ` Fwd: " Heinrich Schuchardt 2015-03-31 22:45 ` Andrew Morton 2015-04-01 17:31 ` Heinrich Schuchardt 2015-06-08 22:41 ` Kees Cook
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).