[PATCH 0/3] sysctl: detect overflows when setting integers

[PATCH 0/3] sysctl: detect overflows when setting integers

[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

[PATCH 1/3] lib/kstrtox.c: functions returning end of string

[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

[PATCH 2/3] sysctl: detect overflows in proc_get_long

[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

[PATCH 3/3] sysctl: detect overflows when converting to int

[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

Fwd: [PATCH 3/3] sysctl: detect overflows when converting to int

[Heinrich Schuchardt]

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) {
> 

Re: [PATCH 3/3] sysctl: detect overflows when converting to int

[Andrew Morton]

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.

Re: [PATCH 3/3] sysctl: detect overflows when converting to int

[Heinrich Schuchardt]

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

Re: [PATCH 3/3] sysctl: detect overflows when converting to int

[Kees Cook]

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