* [RESEND PATCH v3 0/2] sysctl: handle overflow for file-max @ 2019-01-07 22:26 Christian Brauner 2019-01-07 22:26 ` [RESEND PATCH v3 1/2] sysctl: handle overflow in proc_get_long Christian Brauner ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Christian Brauner @ 2019-01-07 22:26 UTC (permalink / raw) To: akpm, keescook, linux-kernel Cc: ebiederm, mcgrof, joe.lawrence, longman, linux, viro, adobriyan, linux-api, Christian Brauner Hey, Here is v3 of this patchset. Changelogs are in the individual commits. 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. The first version of this patch intended to detect the overflow and cap at ULONG_MAX. However, we should not do this and rather return EINVAL on overflow. The reasons are: - this aligns with other sysctl handlers that simply reject overflows (cf. [1], [2], and a bunch of others) - 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. (This patchset is in reference to https://lkml.org/lkml/2018/10/11/585.) Thanks! Christian [1]: fb910c42cceb ("sysctl: check for UINT_MAX before unsigned int min/max") [2]: 196851bed522 ("s390/topology: correct topology mode proc handler") Christian Brauner (2): sysctl: handle overflow in proc_get_long sysctl: handle overflow for file-max kernel/sysctl.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 46 insertions(+), 1 deletion(-) -- 2.19.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [RESEND PATCH v3 1/2] sysctl: handle overflow in proc_get_long 2019-01-07 22:26 [RESEND PATCH v3 0/2] sysctl: handle overflow for file-max Christian Brauner @ 2019-01-07 22:26 ` Christian Brauner 2019-01-07 22:27 ` [RESEND PATCH v3 2/2] sysctl: handle overflow for file-max Christian Brauner 2019-01-07 23:16 ` [RESEND PATCH v3 0/2] " Kees Cook 2 siblings, 0 replies; 9+ messages in thread From: Christian Brauner @ 2019-01-07 22:26 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 1825f712e73b..06df9ef138e3 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> @@ -2085,6 +2087,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 @@ -2128,7 +2165,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.19.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [RESEND PATCH v3 2/2] sysctl: handle overflow for file-max 2019-01-07 22:26 [RESEND PATCH v3 0/2] sysctl: handle overflow for file-max Christian Brauner 2019-01-07 22:26 ` [RESEND PATCH v3 1/2] sysctl: handle overflow in proc_get_long Christian Brauner @ 2019-01-07 22:27 ` Christian Brauner 2019-01-08 7:01 ` Dominik Brodowski 2019-01-07 23:16 ` [RESEND PATCH v3 0/2] " Kees Cook 2 siblings, 1 reply; 9+ messages in thread From: Christian Brauner @ 2019-01-07 22:27 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 and returns -EINVAL when a long int is exceeded. Any higher value cannot currently be used as the percpu counters are long ints and not unsigned integers. This behavior also aligns with other tuneables that return -EINVAL when their range is exceeded. See e.g. [1], [2] and others. [1]: fb910c42cceb ("sysctl: check for UINT_MAX before unsigned int min/max") [2]: 196851bed522 ("s390/topology: correct topology mode proc handler") Acked-by: Kees Cook <keescook@chromium.org> Signed-off-by: Christian Brauner <christian@brauner.io> --- 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 | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/kernel/sysctl.c b/kernel/sysctl.c index 06df9ef138e3..11378a59af4b 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 @@ -1717,6 +1718,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", @@ -2833,6 +2836,10 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int break; if (neg) continue; + if ((max && val > *max) || (min && val < *min)) { + err = -EINVAL; + break; + } val = convmul * val / convdiv; if ((min && val < *min) || (max && val > *max)) continue; -- 2.19.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [RESEND PATCH v3 2/2] sysctl: handle overflow for file-max 2019-01-07 22:27 ` [RESEND PATCH v3 2/2] sysctl: handle overflow for file-max Christian Brauner @ 2019-01-08 7:01 ` Dominik Brodowski 2019-01-10 14:50 ` Christian Brauner 0 siblings, 1 reply; 9+ messages in thread From: Dominik Brodowski @ 2019-01-08 7:01 UTC (permalink / raw) To: Christian Brauner Cc: akpm, keescook, linux-kernel, ebiederm, mcgrof, joe.lawrence, longman, viro, adobriyan, linux-api On Mon, Jan 07, 2019 at 11:27:00PM +0100, Christian Brauner wrote: > @@ -2833,6 +2836,10 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int > break; > if (neg) > continue; > + if ((max && val > *max) || (min && val < *min)) { > + err = -EINVAL; > + break; > + } > val = convmul * val / convdiv; > if ((min && val < *min) || (max && val > *max)) > continue; This is a generic change which affects all users of do_proc_doulongvec_minmax() that have extra1 or extra2 set. In sysctl.c, I do not see another user of proc_doulongvec_minmax() that has extra1 or extra2 set. However, have you verified whether your patch changes the behaviour for other files that make use of proc_doulongvec_minmax() or proc_doulongvec_ms_jiffies_minmax(), and not only of the file-max sysctl? Thanks, Dominik ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RESEND PATCH v3 2/2] sysctl: handle overflow for file-max 2019-01-08 7:01 ` Dominik Brodowski @ 2019-01-10 14:50 ` Christian Brauner 2019-01-10 14:55 ` Dominik Brodowski 0 siblings, 1 reply; 9+ messages in thread From: Christian Brauner @ 2019-01-10 14:50 UTC (permalink / raw) To: Dominik Brodowski Cc: akpm, keescook, linux-kernel, ebiederm, mcgrof, joe.lawrence, longman, viro, adobriyan, linux-api On Tue, Jan 08, 2019 at 08:01:10AM +0100, Dominik Brodowski wrote: > On Mon, Jan 07, 2019 at 11:27:00PM +0100, Christian Brauner wrote: > > @@ -2833,6 +2836,10 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int > > break; > > if (neg) > > continue; > > + if ((max && val > *max) || (min && val < *min)) { > > + err = -EINVAL; > > + break; > > + } > > val = convmul * val / convdiv; > > if ((min && val < *min) || (max && val > *max)) > > continue; > > This is a generic change which affects all users of > do_proc_doulongvec_minmax() that have extra1 or extra2 set. In sysctl.c, I > do not see another user of proc_doulongvec_minmax() that has extra1 or > extra2 set. However, have you verified whether your patch changes the > behaviour for other files that make use of proc_doulongvec_minmax() or > proc_doulongvec_ms_jiffies_minmax(), and not only of the file-max sysctl? Sorry for the delayed reply. I did look at the callers. The functions that are of interest afaict are: proc_doulongvec_ms_jiffies_minmax proc_doulongvec_minmax So this could be visible when users write values that would overflow the type used in the kernel. I guess your point is whether we are venturing into userspace break territory. Hm... We should probably make sure that we're not regressing anyone else! What do you think if instead of the above patch we did: diff --git a/kernel/sysctl.c b/kernel/sysctl.c index ba4d9e85feb8..37727b4c7a97 100644 --- a/kernel/sysctl.c +++ b/kernel/sysctl.c @@ -1721,7 +1721,7 @@ static struct ctl_table fs_table[] = { .data = &files_stat.max_files, .maxlen = sizeof(files_stat.max_files), .mode = 0644, - .proc_handler = proc_doulongvec_minmax, + .proc_handler = proc_file_max, }, { .procname = "nr_open", @@ -2758,7 +2758,7 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int void __user *buffer, size_t *lenp, loff_t *ppos, unsigned long convmul, - unsigned long convdiv) + unsigned long convdiv, bool strict) { unsigned long *i, *min, *max; int vleft, first = 1, err = 0; @@ -2806,7 +2806,12 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int continue; val = convmul * val / convdiv; if ((min && val < *min) || (max && val > *max)) - continue; + if (strict) { + err = -EINVAL; + break; + } else { + continue; + } *i = val; } else { val = convdiv * (*i) / convmul; @@ -2843,7 +2848,15 @@ static int do_proc_doulongvec_minmax(struct ctl_table *table, int write, unsigned long convdiv) { return __do_proc_doulongvec_minmax(table->data, table, write, - buffer, lenp, ppos, convmul, convdiv); + buffer, lenp, ppos, convmul, convdiv, false); +} + +static int proc_file_max(struct ctl_table *table, int write, + void __user *buffer, size_t *lenp, loff_t *ppos, + unsigned long convmul, unsigned long convdiv) +{ + return __do_proc_doulongvec_minmax(table->data, table, write, buffer, + lenp, ppos, convmul, convdiv, true); } /** @@ -2865,7 +2878,8 @@ static int do_proc_doulongvec_minmax(struct ctl_table *table, int write, int proc_doulongvec_minmax(struct ctl_table *table, int write, void __user *buffer, size_t *lenp, loff_t *ppos) { - return do_proc_doulongvec_minmax(table, write, buffer, lenp, ppos, 1l, 1l); + return do_proc_doulongvec_minmax(table, write, buffer, lenp, ppos, 1l, + 1l, false); } /** @@ -2890,7 +2904,7 @@ int proc_doulongvec_ms_jiffies_minmax(struct ctl_table *table, int write, size_t *lenp, loff_t *ppos) { return do_proc_doulongvec_minmax(table, write, buffer, - lenp, ppos, HZ, 1000l); + lenp, ppos, HZ, 1000l, false); } ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [RESEND PATCH v3 2/2] sysctl: handle overflow for file-max 2019-01-10 14:50 ` Christian Brauner @ 2019-01-10 14:55 ` Dominik Brodowski 2019-01-10 15:00 ` Christian Brauner 2019-01-11 14:51 ` Christian Brauner 0 siblings, 2 replies; 9+ messages in thread From: Dominik Brodowski @ 2019-01-10 14:55 UTC (permalink / raw) To: Christian Brauner Cc: akpm, keescook, linux-kernel, ebiederm, mcgrof, joe.lawrence, longman, viro, adobriyan, linux-api On Thu, Jan 10, 2019 at 03:50:05PM +0100, Christian Brauner wrote: > On Tue, Jan 08, 2019 at 08:01:10AM +0100, Dominik Brodowski wrote: > > On Mon, Jan 07, 2019 at 11:27:00PM +0100, Christian Brauner wrote: > > > @@ -2833,6 +2836,10 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int > > > break; > > > if (neg) > > > continue; > > > + if ((max && val > *max) || (min && val < *min)) { > > > + err = -EINVAL; > > > + break; > > > + } > > > val = convmul * val / convdiv; > > > if ((min && val < *min) || (max && val > *max)) > > > continue; > > > > This is a generic change which affects all users of > > do_proc_doulongvec_minmax() that have extra1 or extra2 set. In sysctl.c, I > > do not see another user of proc_doulongvec_minmax() that has extra1 or > > extra2 set. However, have you verified whether your patch changes the > > behaviour for other files that make use of proc_doulongvec_minmax() or > > proc_doulongvec_ms_jiffies_minmax(), and not only of the file-max sysctl? > > Sorry for the delayed reply. I did look at the callers. The functions > that are of interest afaict are: > > proc_doulongvec_ms_jiffies_minmax > proc_doulongvec_minmax > > So this could be visible when users write values that would overflow the > type used in the kernel. > > I guess your point is whether we are venturing into userspace break > territory. Hm... We should probably make sure that we're not regressing > anyone else! What do you think if instead of the above patch we did: Hm, I prefer the original patch -- as the same (valid) reasons which apply for the file-max sysctl might also apply to other users of this function where extra1 and/or2 extra2 are set. If there are no other users of this function where extra1 or extra2 are set, just add a comment in the commit message: While this changes the behaviour of __do_proc_doulongvec_minmax(), no other existing users in the kernel are affected by this change. If there are other users of this function where extra1 or extra2 are set, you would need to generalize the commit message overall. Thanks, Dominik ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RESEND PATCH v3 2/2] sysctl: handle overflow for file-max 2019-01-10 14:55 ` Dominik Brodowski @ 2019-01-10 15:00 ` Christian Brauner 2019-01-11 14:51 ` Christian Brauner 1 sibling, 0 replies; 9+ messages in thread From: Christian Brauner @ 2019-01-10 15:00 UTC (permalink / raw) To: Dominik Brodowski Cc: akpm, keescook, linux-kernel, ebiederm, mcgrof, joe.lawrence, longman, viro, adobriyan, linux-api On Thu, Jan 10, 2019 at 03:55:59PM +0100, Dominik Brodowski wrote: > On Thu, Jan 10, 2019 at 03:50:05PM +0100, Christian Brauner wrote: > > On Tue, Jan 08, 2019 at 08:01:10AM +0100, Dominik Brodowski wrote: > > > On Mon, Jan 07, 2019 at 11:27:00PM +0100, Christian Brauner wrote: > > > > @@ -2833,6 +2836,10 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int > > > > break; > > > > if (neg) > > > > continue; > > > > + if ((max && val > *max) || (min && val < *min)) { > > > > + err = -EINVAL; > > > > + break; > > > > + } > > > > val = convmul * val / convdiv; > > > > if ((min && val < *min) || (max && val > *max)) > > > > continue; > > > > > > This is a generic change which affects all users of > > > do_proc_doulongvec_minmax() that have extra1 or extra2 set. In sysctl.c, I > > > do not see another user of proc_doulongvec_minmax() that has extra1 or > > > extra2 set. However, have you verified whether your patch changes the > > > behaviour for other files that make use of proc_doulongvec_minmax() or > > > proc_doulongvec_ms_jiffies_minmax(), and not only of the file-max sysctl? > > > > Sorry for the delayed reply. I did look at the callers. The functions > > that are of interest afaict are: > > > > proc_doulongvec_ms_jiffies_minmax > > proc_doulongvec_minmax > > > > So this could be visible when users write values that would overflow the > > type used in the kernel. > > > > I guess your point is whether we are venturing into userspace break > > territory. Hm... We should probably make sure that we're not regressing > > anyone else! What do you think if instead of the above patch we did: > > Hm, I prefer the original patch -- as the same (valid) reasons which apply > for the file-max sysctl might also apply to other users of this function > where extra1 and/or2 extra2 are set. In that case we should erorr out on: val = convmul * val / convdiv; if ((min && val < *min) || (max && val > *max)) { err = -EINVAL; break; } I fear that erroring out before might break *_jiffies since they are the only caller that request a convmul/convdiv value that is not 1l. Christian ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RESEND PATCH v3 2/2] sysctl: handle overflow for file-max 2019-01-10 14:55 ` Dominik Brodowski 2019-01-10 15:00 ` Christian Brauner @ 2019-01-11 14:51 ` Christian Brauner 1 sibling, 0 replies; 9+ messages in thread From: Christian Brauner @ 2019-01-11 14:51 UTC (permalink / raw) To: Dominik Brodowski, akpm Cc: keescook, linux-kernel, ebiederm, mcgrof, joe.lawrence, longman, viro, adobriyan, linux-api On Thu, Jan 10, 2019 at 03:55:59PM +0100, Dominik Brodowski wrote: > On Thu, Jan 10, 2019 at 03:50:05PM +0100, Christian Brauner wrote: > > On Tue, Jan 08, 2019 at 08:01:10AM +0100, Dominik Brodowski wrote: > > > On Mon, Jan 07, 2019 at 11:27:00PM +0100, Christian Brauner wrote: > > > > @@ -2833,6 +2836,10 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int > > > > break; > > > > if (neg) > > > > continue; > > > > + if ((max && val > *max) || (min && val < *min)) { > > > > + err = -EINVAL; > > > > + break; > > > > + } > > > > val = convmul * val / convdiv; > > > > if ((min && val < *min) || (max && val > *max)) > > > > continue; > > > > > > This is a generic change which affects all users of > > > do_proc_doulongvec_minmax() that have extra1 or extra2 set. In sysctl.c, I > > > do not see another user of proc_doulongvec_minmax() that has extra1 or > > > extra2 set. However, have you verified whether your patch changes the > > > behaviour for other files that make use of proc_doulongvec_minmax() or > > > proc_doulongvec_ms_jiffies_minmax(), and not only of the file-max sysctl? > > > > Sorry for the delayed reply. I did look at the callers. The functions > > that are of interest afaict are: > > > > proc_doulongvec_ms_jiffies_minmax > > proc_doulongvec_minmax > > > > So this could be visible when users write values that would overflow the > > type used in the kernel. > > > > I guess your point is whether we are venturing into userspace break > > territory. Hm... We should probably make sure that we're not regressing > > anyone else! What do you think if instead of the above patch we did: > > Hm, I prefer the original patch -- as the same (valid) reasons which apply > for the file-max sysctl might also apply to other users of this function > where extra1 and/or2 extra2 are set. > > If there are no other users of this function where extra1 or extra2 are set, > just add a comment in the commit message: > > While this changes the behaviour of __do_proc_doulongvec_minmax(), > no other existing users in the kernel are affected by this change. > > If there are other users of this function where extra1 or extra2 are set, > you would need to generalize the commit message overall. Andrew, can you please drop this patch [RESEND PATCH v3 2/2] sysctl: handle overflow for file-max from your tree (It should be located at [1] from what I can gather.). I'll resend it based on Dominik's observation and will generalize the commit message and also error out *after* the conversion has been done and not before. The first patch 1/2 is correct and can be kept. Thanks! Christian [1]: https://www.ozlabs.org/~akpm/mmots/broken-out/sysctl-handle-overflow-for-file-max.patch ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RESEND PATCH v3 0/2] sysctl: handle overflow for file-max 2019-01-07 22:26 [RESEND PATCH v3 0/2] sysctl: handle overflow for file-max Christian Brauner 2019-01-07 22:26 ` [RESEND PATCH v3 1/2] sysctl: handle overflow in proc_get_long Christian Brauner 2019-01-07 22:27 ` [RESEND PATCH v3 2/2] sysctl: handle overflow for file-max Christian Brauner @ 2019-01-07 23:16 ` Kees Cook 2 siblings, 0 replies; 9+ messages in thread From: Kees Cook @ 2019-01-07 23:16 UTC (permalink / raw) To: Christian Brauner Cc: Andrew Morton, LKML, Eric W. Biederman, Luis R. Rodriguez, Joe Lawrence, Waiman Long, Dominik Brodowski, Al Viro, Alexey Dobriyan, Linux API On Mon, Jan 7, 2019 at 2:27 PM Christian Brauner <christian@brauner.io> wrote: > > Hey, > > Here is v3 of this patchset. Changelogs are in the individual commits. Andrew, can you take these? I've already Acked them. -Kees > > 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. > > The first version of this patch intended to detect the overflow and cap > at ULONG_MAX. However, we should not do this and rather return EINVAL on > overflow. The reasons are: > - this aligns with other sysctl handlers that simply reject overflows > (cf. [1], [2], and a bunch of others) > - 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. > > (This patchset is in reference to https://lkml.org/lkml/2018/10/11/585.) > > Thanks! > Christian > > [1]: fb910c42cceb ("sysctl: check for UINT_MAX before unsigned int min/max") > [2]: 196851bed522 ("s390/topology: correct topology mode proc handler") > > Christian Brauner (2): > sysctl: handle overflow in proc_get_long > sysctl: handle overflow for file-max > > kernel/sysctl.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 46 insertions(+), 1 deletion(-) > > -- > 2.19.1 > -- Kees Cook ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-01-11 14:51 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-01-07 22:26 [RESEND PATCH v3 0/2] sysctl: handle overflow for file-max Christian Brauner 2019-01-07 22:26 ` [RESEND PATCH v3 1/2] sysctl: handle overflow in proc_get_long Christian Brauner 2019-01-07 22:27 ` [RESEND PATCH v3 2/2] sysctl: handle overflow for file-max Christian Brauner 2019-01-08 7:01 ` Dominik Brodowski 2019-01-10 14:50 ` Christian Brauner 2019-01-10 14:55 ` Dominik Brodowski 2019-01-10 15:00 ` Christian Brauner 2019-01-11 14:51 ` Christian Brauner 2019-01-07 23:16 ` [RESEND PATCH v3 0/2] " 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).