* [PATCH v4 1/3] sysctl: handle overflow in proc_get_long
2019-02-10 20:39 [PATCH v4 0/3] sysctl: handle overflow for file-max Christian Brauner
@ 2019-02-10 20:39 ` Christian Brauner
2019-02-10 20:39 ` [PATCH v4 2/3] sysctl: handle overflow for file-max Christian Brauner
2019-02-10 20:39 ` [RFC PATCH v4 3/3] sysctl: return -EINVAL if val violates minmax Christian Brauner
2 siblings, 0 replies; 5+ messages in thread
From: Christian Brauner @ 2019-02-10 20:39 UTC (permalink / raw)
To: akpm, keescook, linux-kernel
Cc: ebiederm, mcgrof, joe.lawrence, longman, linux, viro, adobriyan,
linux-api, Christian Brauner
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.
Acked-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Christian Brauner <christian@brauner.io>
---
/* Changelog */
v3:
- (Kees) s/#include <../lib/kstrtox.h>/#include "../lib/kstrtox.h"/g
- (Kees) document strtoul_lenient()
v2:
- s/sysctl_cap_erange/sysctl_lenient/g
- consistenly fail on overflow
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 | 40 +++++++++++++++++++++++++++++++++++++++-
1 file changed, 39 insertions(+), 1 deletion(-)
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index ba4d9e85feb8..70581ade3555 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -67,6 +67,8 @@
#include <linux/bpf.h>
#include <linux/mount.h>
+#include "../lib/kstrtox.h"
+
#include <linux/uaccess.h>
#include <asm/processor.h>
@@ -2092,6 +2094,41 @@ static void proc_skip_char(char **buf, size_t *size, const char v)
}
}
+/**
+ * strtoul_lenient - parse an ASCII formatted integer from a buffer and only
+ * fail on overflow
+ *
+ * @cp: kernel buffer containing the string to parse
+ * @endp: pointer to store the trailing characters
+ * @base: the base to use
+ * @res: where the parsed integer will be stored
+ *
+ * In case of success 0 is returned and @res will contain the parsed integer,
+ * @endp will hold any trailing characters.
+ * This function will fail the parse on overflow. If there wasn't an overflow
+ * the function will defer the decision what characters count as invalid to the
+ * caller.
+ */
+static int strtoul_lenient(const char *cp, char **endp, unsigned int base,
+ unsigned long *res)
+{
+ 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))
+ return -ERANGE;
+
+ cp += rv;
+
+ if (endp)
+ *endp = (char *)cp;
+
+ *res = (unsigned long)result;
+ return 0;
+}
+
#define TMPBUFLEN 22
/**
* proc_get_long - reads an ASCII formatted integer from a user buffer
@@ -2135,7 +2172,8 @@ static int proc_get_long(char **buf, size_t *size,
if (!isdigit(*p))
return -EINVAL;
- *val = simple_strtoul(p, &p, 0);
+ if (strtoul_lenient(p, &p, 0, val))
+ return -EINVAL;
len = p - tmp;
--
2.20.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v4 2/3] sysctl: handle overflow for file-max
2019-02-10 20:39 [PATCH v4 0/3] sysctl: handle overflow for file-max Christian Brauner
2019-02-10 20:39 ` [PATCH v4 1/3] sysctl: handle overflow in proc_get_long Christian Brauner
@ 2019-02-10 20:39 ` Christian Brauner
2019-02-10 20:39 ` [RFC PATCH v4 3/3] sysctl: return -EINVAL if val violates minmax Christian Brauner
2 siblings, 0 replies; 5+ messages in thread
From: Christian Brauner @ 2019-02-10 20:39 UTC (permalink / raw)
To: akpm, keescook, linux-kernel
Cc: ebiederm, mcgrof, joe.lawrence, longman, linux, viro, adobriyan,
linux-api, Christian Brauner
Currently, when writing
echo 18446744073709551616 > /proc/sys/fs/file-max
/proc/sys/fs/file-max will overflow and be set to 0. That quickly
crashes the system.
This commit sets the max and min value for file-max. The max value is set
to long int. Any higher value cannot currently be used as the percpu
counters are long ints and not unsigned integers.
Note that the file-max value is ultimately parsed via
__do_proc_doulongvec_minmax(). This function does not report error when min
or max are exceeded. Which means if a value largen that long int is written
userspace will not receive an error instead the old value will be kept.
There is an argument to be made that this should be changed and
__do_proc_doulongvec_minmax() should return an error when a dedicated min
or max value are exceeded. However this has the potential to break
userspace so let's defer this to an RFC patch.
Cc: Dominik Brodowski <linux@dominikbrodowski.net>
Signed-off-by: Christian Brauner <christian@brauner.io>
Acked-by: Kees Cook <keescook@chromium.org>
---
v4:
- unchanged
The prior version of the patch contained a generic change affecting all
callers of __do_proc_doulongvec_minmax(). This part was split out into a
separate RFC patch as it nees a proper discussion and consideration
whether this would break userspace.
v3:
- unchanged
v1:
- consistenly fail on overflow
v1:
- if max value is < than ULONG_MAX use max as upper bound
- (Dominik) remove double "the" from commit message
---
kernel/sysctl.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 70581ade3555..c4a44b7ccb8a 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -129,6 +129,7 @@ static int __maybe_unused one = 1;
static int __maybe_unused two = 2;
static int __maybe_unused four = 4;
static unsigned long one_ul = 1;
+static unsigned long long_max = LONG_MAX;
static int one_hundred = 100;
static int one_thousand = 1000;
#ifdef CONFIG_PRINTK
@@ -1724,6 +1725,8 @@ static struct ctl_table fs_table[] = {
.maxlen = sizeof(files_stat.max_files),
.mode = 0644,
.proc_handler = proc_doulongvec_minmax,
+ .extra1 = &zero,
+ .extra2 = &long_max,
},
{
.procname = "nr_open",
--
2.20.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [RFC PATCH v4 3/3] sysctl: return -EINVAL if val violates minmax
2019-02-10 20:39 [PATCH v4 0/3] sysctl: handle overflow for file-max Christian Brauner
2019-02-10 20:39 ` [PATCH v4 1/3] sysctl: handle overflow in proc_get_long Christian Brauner
2019-02-10 20:39 ` [PATCH v4 2/3] sysctl: handle overflow for file-max Christian Brauner
@ 2019-02-10 20:39 ` Christian Brauner
2019-02-11 19:50 ` Luis Chamberlain
2 siblings, 1 reply; 5+ messages in thread
From: Christian Brauner @ 2019-02-10 20:39 UTC (permalink / raw)
To: akpm, keescook, linux-kernel
Cc: ebiederm, mcgrof, joe.lawrence, longman, linux, viro, adobriyan,
linux-api, Christian Brauner
Currently when userspace gives us a values that overflow e.g. file-max and
other callers of __do_proc_doulongvec_minmax() we simply
ignore the new value and leave the current value untouched. This can be
problematic as it gives the illusion that the limit has indeed be bumped
when in fact it failed.
This commit makes sure to return EINVAL when an overflow is detected.
Please note that this is a userspace facing change.
Signed-off-by: Christian Brauner <christian@brauner.io>
---
/* Changelog */
v4:
- patch introduced
v1-v3:
- patch not present
---
kernel/sysctl.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index c4a44b7ccb8a..516bc8a2812d 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2846,8 +2846,10 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
if (neg)
continue;
val = convmul * val / convdiv;
- if ((min && val < *min) || (max && val > *max))
- continue;
+ if ((min && val < *min) || (max && val > *max)) {
+ err = -EINVAL;
+ break;
+ }
*i = val;
} else {
val = convdiv * (*i) / convmul;
--
2.20.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [RFC PATCH v4 3/3] sysctl: return -EINVAL if val violates minmax
2019-02-10 20:39 ` [RFC PATCH v4 3/3] sysctl: return -EINVAL if val violates minmax Christian Brauner
@ 2019-02-11 19:50 ` Luis Chamberlain
0 siblings, 0 replies; 5+ messages in thread
From: Luis Chamberlain @ 2019-02-11 19:50 UTC (permalink / raw)
To: Christian Brauner
Cc: akpm, keescook, linux-kernel, ebiederm, joe.lawrence, longman,
linux, viro, adobriyan, linux-api
On Sun, Feb 10, 2019 at 09:39:43PM +0100, Christian Brauner wrote:
> Currently when userspace gives us a values that overflow e.g. file-max and
> other callers of __do_proc_doulongvec_minmax() we simply
> ignore the new value and leave the current value untouched. This can be
> problematic as it gives the illusion that the limit has indeed be bumped
> when in fact it failed.
> This commit makes sure to return EINVAL when an overflow is detected.
> Please note that this is a userspace facing change.
>
Acked-by: Luis Chamberlain <mcgrof@kernel.org>
Luis
> Signed-off-by: Christian Brauner <christian@brauner.io>
> ---
> /* Changelog */
> v4:
> - patch introduced
>
> v1-v3:
> - patch not present
> ---
> kernel/sysctl.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index c4a44b7ccb8a..516bc8a2812d 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -2846,8 +2846,10 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
> if (neg)
> continue;
> val = convmul * val / convdiv;
> - if ((min && val < *min) || (max && val > *max))
> - continue;
> + if ((min && val < *min) || (max && val > *max)) {
> + err = -EINVAL;
> + break;
> + }
> *i = val;
> } else {
> val = convdiv * (*i) / convmul;
> --
> 2.20.1
>
^ permalink raw reply [flat|nested] 5+ messages in thread