[PATCH v1 0/2] sysctl: cap file-max value at ULONG_MAX

[PATCH v1 0/2] sysctl: cap file-max value at ULONG_MAX

[Christian Brauner]

Hey,

Here is v1 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. Let's detect the overflow and set to ULONG_MAX in
this case effectively capping the value.

The patch tries to ensure that there is no other user visible change in
behavior for other values. Only when a maximum value is set for a
specific sysctl will it be capped on overflow. The details are outlined
in the commit message of the first commit.

(This patchset is in reference to https://lkml.org/lkml/2018/10/11/585.)

Thanks!
Christian


Christian Brauner (2):
  sysctl: cap to ULONG_MAX in proc_get_long()
  sysctl: handle overflow for file-max

 kernel/sysctl.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

-- 
2.17.1

[PATCH v1 1/2] sysctl: cap to ULONG_MAX in proc_get_long()

[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 fails the parse
in this case, 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_cap_erange() which does:
- returns ULONG_MAX on ERANGE
- returns the trailing characters to the caller
This guarantees that we don't regress userspace in any way but also caps
any parsed value to ULONG_MAX and prevents things like file-max to become 0
on overflow.

Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Christian Brauner <christian@brauner.io>
---
v0->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 | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index cc02050fd0c4..97551eb42946 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -67,6 +67,7 @@
 #include <linux/bpf.h>
 #include <linux/mount.h>
 #include <linux/pipe_fs_i.h>
+#include <../lib/kstrtox.h>
 
 #include <linux/uaccess.h>
 #include <asm/processor.h>
@@ -2065,6 +2066,25 @@ static void proc_skip_char(char **buf, size_t *size, const char v)
 	}
 }
 
+static unsigned long strtoul_cap_erange(const char *cp, char **endp,
+					unsigned int base)
+{
+	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))
+		result = ULONG_MAX;
+
+	cp += (rv & ~KSTRTOX_OVERFLOW);
+
+	if (endp)
+		*endp = (char *)cp;
+
+	return result;
+}
+
 #define TMPBUFLEN 22
 /**
  * proc_get_long - reads an ASCII formatted integer from a user buffer
@@ -2108,7 +2128,7 @@ static int proc_get_long(char **buf, size_t *size,
 	if (!isdigit(*p))
 		return -EINVAL;
 
-	*val = simple_strtoul(p, &p, 0);
+	*val = strtoul_cap_erange(p, &p, 0);
 
 	len = p - tmp;
 
-- 
2.17.1

Re: [PATCH v1 1/2] sysctl: cap to ULONG_MAX in proc_get_long()

[Kees Cook]

On Mon, Oct 15, 2018 at 3:55 AM, Christian Brauner <christian@brauner.io> wrote:
> 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

What depends on simple_strtoul() ignoring overflows? Can we just cap
it to ULONG_MAX instead?

I note that both simple_strtoul() and simple_strtoull() are marked as
obsolete (more below).

> 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 fails the parse
> in this case, 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.

This parsing strictness difference makes it seem like the simple_*()
shouldn't be considered obsolete...

and it's still very heavily used:

$ git grep -E 'simple_strtoull?\(' | wc -l
745

> Now, before adding another kstrtoul() function let's simply add a static
> parse strtoul_cap_erange() which does:
> - returns ULONG_MAX on ERANGE
> - returns the trailing characters to the caller
> This guarantees that we don't regress userspace in any way but also caps
> any parsed value to ULONG_MAX and prevents things like file-max to become 0
> on overflow.

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v1 1/2] sysctl: cap to ULONG_MAX in proc_get_long()

[Christian Brauner]

On Mon, Oct 15, 2018 at 09:18:40AM -0700, Kees Cook wrote:
> On Mon, Oct 15, 2018 at 3:55 AM, Christian Brauner <christian@brauner.io> wrote:
> > 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
> 
> What depends on simple_strtoul() ignoring overflows? Can we just cap
> it to ULONG_MAX instead?
> 
> I note that both simple_strtoul() and simple_strtoull() are marked as
> obsolete (more below).
> 
> > 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 fails the parse
> > in this case, 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.
> 
> This parsing strictness difference makes it seem like the simple_*()
> shouldn't be considered obsolete...
> 
> and it's still very heavily used:
> 
> $ git grep -E 'simple_strtoull?\(' | wc -l
> 745

Maybe, but the intention is probably to fade it out and to not use it in
new code because it doesn't handle overflow.
Tbh, I'm weary to change that to suddenly return a ULONG_MAX on overflow
instead of what it is doing now. I have absolutely no idea what this
might break given how much it is still used in the kernel...

> 
> > Now, before adding another kstrtoul() function let's simply add a static
> > parse strtoul_cap_erange() which does:
> > - returns ULONG_MAX on ERANGE
> > - returns the trailing characters to the caller
> > This guarantees that we don't regress userspace in any way but also caps
> > any parsed value to ULONG_MAX and prevents things like file-max to become 0
> > on overflow.
> 
> -Kees
> 
> -- 
> Kees Cook
> Pixel Security

Re: [PATCH v1 1/2] sysctl: cap to ULONG_MAX in proc_get_long()

[Christian Brauner]

On Mon, Oct 15, 2018 at 6:30 PM Christian Brauner <christian@brauner.io> wrote:
>
> On Mon, Oct 15, 2018 at 09:18:40AM -0700, Kees Cook wrote:
> > On Mon, Oct 15, 2018 at 3:55 AM, Christian Brauner <christian@brauner.io> wrote:
> > > 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
> >
> > What depends on simple_strtoul() ignoring overflows? Can we just cap
> > it to ULONG_MAX instead?
> >
> > I note that both simple_strtoul() and simple_strtoull() are marked as
> > obsolete (more below).
> >
> > > 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 fails the parse
> > > in this case, 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.
> >
> > This parsing strictness difference makes it seem like the simple_*()
> > shouldn't be considered obsolete...

So what I would prefer in this case - if people agree that such a function
is useful - is to add a new function to lib/kstrtox.c and include/linux/kernel.h
e.g.:

int kstrtoul_bounded(const char *s, unsigned int base, char
**trailing, unsigned long long *res)

which aligns with the other kstrto*() functions, caps at <TYPE>_MAX,
doesn't fail the parse,
and returns the trailing characters in char **trailing. Then people
can start switching users of
simple_strtoul() over to this function without regressing anything.

Christian

> >
> > and it's still very heavily used:
> >
> > $ git grep -E 'simple_strtoull?\(' | wc -l
> > 745
>
> Maybe, but the intention is probably to fade it out and to not use it in
> new code because it doesn't handle overflow.
> Tbh, I'm weary to change that to suddenly return a ULONG_MAX on overflow
> instead of what it is doing now. I have absolutely no idea what this
> might break given how much it is still used in the kernel...
>
> >
> > > Now, before adding another kstrtoul() function let's simply add a static
> > > parse strtoul_cap_erange() which does:
> > > - returns ULONG_MAX on ERANGE
> > > - returns the trailing characters to the caller
> > > This guarantees that we don't regress userspace in any way but also caps
> > > any parsed value to ULONG_MAX and prevents things like file-max to become 0
> > > on overflow.
> >
> > -Kees
> >
> > --
> > Kees Cook
> > Pixel Security

[PATCH v1 2/2] sysctl: handle overflow for file-max

[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 explicitly caps the value for file-max to ULONG_MAX.

Note, this isn't technically necessary since proc_get_long() will already
return ULONG_MAX. However, two reason why we still should do this:
1. it makes it explicit what the upper bound of file-max is instead of
   making readers of the code infer it from proc_get_long() themselves
2. other tunebles than file-max may want to set a lower max value than
   ULONG_MAX and we need to enable __do_proc_doulongvec_minmax() to handle
   such cases too

Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Christian Brauner <christian@brauner.io>
---
v0->v1:
- if max value is < than ULONG_MAX use max as upper bound
- (Dominik) remove double "the" from commit message
---
 kernel/sysctl.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 97551eb42946..226d4eaf4b0e 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -127,6 +127,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 ulong_max = ULONG_MAX;
 static int one_hundred = 100;
 static int one_thousand = 1000;
 #ifdef CONFIG_PRINTK
@@ -1696,6 +1697,7 @@ static struct ctl_table fs_table[] = {
 		.maxlen		= sizeof(files_stat.max_files),
 		.mode		= 0644,
 		.proc_handler	= proc_doulongvec_minmax,
+		.extra2		= &ulong_max,
 	},
 	{
 		.procname	= "nr_open",
@@ -2795,6 +2797,8 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
 				break;
 			if (neg)
 				continue;
+			if (max && val > *max)
+				val = *max;
 			val = convmul * val / convdiv;
 			if ((min && val < *min) || (max && val > *max))
 				continue;
-- 
2.17.1

Re: [PATCH v1 2/2] sysctl: handle overflow for file-max

[Kees Cook]

On Mon, Oct 15, 2018 at 3:55 AM, Christian Brauner <christian@brauner.io> wrote:
> 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 explicitly caps the value for file-max to ULONG_MAX.
>
> Note, this isn't technically necessary since proc_get_long() will already
> return ULONG_MAX. However, two reason why we still should do this:
> 1. it makes it explicit what the upper bound of file-max is instead of
>    making readers of the code infer it from proc_get_long() themselves
> 2. other tunebles than file-max may want to set a lower max value than
>    ULONG_MAX and we need to enable __do_proc_doulongvec_minmax() to handle
>    such cases too
>
> Cc: Kees Cook <keescook@chromium.org>
> Signed-off-by: Christian Brauner <christian@brauner.io>
> ---
> v0->v1:
> - if max value is < than ULONG_MAX use max as upper bound
> - (Dominik) remove double "the" from commit message
> ---
>  kernel/sysctl.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 97551eb42946..226d4eaf4b0e 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -127,6 +127,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 ulong_max = ULONG_MAX;
>  static int one_hundred = 100;
>  static int one_thousand = 1000;
>  #ifdef CONFIG_PRINTK
> @@ -1696,6 +1697,7 @@ static struct ctl_table fs_table[] = {
>                 .maxlen         = sizeof(files_stat.max_files),
>                 .mode           = 0644,
>                 .proc_handler   = proc_doulongvec_minmax,
> +               .extra2         = &ulong_max,

Don't we want this capped lower? The percpu comparisons, for example,
are all signed long. And there is at least this test, which could
overflow:

        if (atomic_long_read(&unix_nr_socks) > 2 * get_max_files())
                goto out;

Seems like max-files should be  SLONG_MAX / 2 or something instead?

>         },
>         {
>                 .procname       = "nr_open",
> @@ -2795,6 +2797,8 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
>                                 break;
>                         if (neg)
>                                 continue;
> +                       if (max && val > *max)
> +                               val = *max;
>                         val = convmul * val / convdiv;
>                         if ((min && val < *min) || (max && val > *max))
>                                 continue;
> --
> 2.17.1
>

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v1 2/2] sysctl: handle overflow for file-max

[Christian Brauner]

On Mon, Oct 15, 2018 at 09:11:51AM -0700, Kees Cook wrote:
> On Mon, Oct 15, 2018 at 3:55 AM, Christian Brauner <christian@brauner.io> wrote:
> > 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 explicitly caps the value for file-max to ULONG_MAX.
> >
> > Note, this isn't technically necessary since proc_get_long() will already
> > return ULONG_MAX. However, two reason why we still should do this:
> > 1. it makes it explicit what the upper bound of file-max is instead of
> >    making readers of the code infer it from proc_get_long() themselves
> > 2. other tunebles than file-max may want to set a lower max value than
> >    ULONG_MAX and we need to enable __do_proc_doulongvec_minmax() to handle
> >    such cases too
> >
> > Cc: Kees Cook <keescook@chromium.org>
> > Signed-off-by: Christian Brauner <christian@brauner.io>
> > ---
> > v0->v1:
> > - if max value is < than ULONG_MAX use max as upper bound
> > - (Dominik) remove double "the" from commit message
> > ---
> >  kernel/sysctl.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> > index 97551eb42946..226d4eaf4b0e 100644
> > --- a/kernel/sysctl.c
> > +++ b/kernel/sysctl.c
> > @@ -127,6 +127,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 ulong_max = ULONG_MAX;
> >  static int one_hundred = 100;
> >  static int one_thousand = 1000;
> >  #ifdef CONFIG_PRINTK
> > @@ -1696,6 +1697,7 @@ static struct ctl_table fs_table[] = {
> >                 .maxlen         = sizeof(files_stat.max_files),
> >                 .mode           = 0644,
> >                 .proc_handler   = proc_doulongvec_minmax,
> > +               .extra2         = &ulong_max,
> 
> Don't we want this capped lower? The percpu comparisons, for example,
> are all signed long. And there is at least this test, which could
> overflow:
> 
>         if (atomic_long_read(&unix_nr_socks) > 2 * get_max_files())
>                 goto out;

Does that check even make sense? 
Commit 518de9b39e854542de59bfb8b9f61c8f7ecf808b made get_max_files()
return a long to bump the number of allowed files to more than 2^31.

But assuming a platform where an unsigned long is 64bit which is what
get_max_files() returns and atomic_long_read() is 64bit too this is
guaranteed to overflow, no?  So I'm not clear what this is trying to do.
Seems this should simply be:

if (atomic_long_read(&unix_nr_socks) > get_max_files())
        goto out;

or am I missing a crucial point?

> 
> Seems like max-files should be  SLONG_MAX / 2 or something instead?

Hm. Isn't that a bit low? Iiuc, this would mean cutting the maximum
number of open files in half? If at all shouldn't it be LONG_MAX?

> 
> >         },
> >         {
> >                 .procname       = "nr_open",
> > @@ -2795,6 +2797,8 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
> >                                 break;
> >                         if (neg)
> >                                 continue;
> > +                       if (max && val > *max)
> > +                               val = *max;
> >                         val = convmul * val / convdiv;
> >                         if ((min && val < *min) || (max && val > *max))
> >                                 continue;
> > --
> > 2.17.1
> >
> 
> -Kees
> 
> -- 
> Kees Cook
> Pixel Security

Re: [PATCH v1 2/2] sysctl: handle overflow for file-max

[Kees Cook]

On Mon, Oct 15, 2018 at 9:28 AM, Christian Brauner <christian@brauner.io> wrote:
> On Mon, Oct 15, 2018 at 09:11:51AM -0700, Kees Cook wrote:
>> On Mon, Oct 15, 2018 at 3:55 AM, Christian Brauner <christian@brauner.io> wrote:
>> > 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 explicitly caps the value for file-max to ULONG_MAX.
>> >
>> > Note, this isn't technically necessary since proc_get_long() will already
>> > return ULONG_MAX. However, two reason why we still should do this:
>> > 1. it makes it explicit what the upper bound of file-max is instead of
>> >    making readers of the code infer it from proc_get_long() themselves
>> > 2. other tunebles than file-max may want to set a lower max value than
>> >    ULONG_MAX and we need to enable __do_proc_doulongvec_minmax() to handle
>> >    such cases too
>> >
>> > Cc: Kees Cook <keescook@chromium.org>
>> > Signed-off-by: Christian Brauner <christian@brauner.io>
>> > ---
>> > v0->v1:
>> > - if max value is < than ULONG_MAX use max as upper bound
>> > - (Dominik) remove double "the" from commit message
>> > ---
>> >  kernel/sysctl.c | 4 ++++
>> >  1 file changed, 4 insertions(+)
>> >
>> > diff --git a/kernel/sysctl.c b/kernel/sysctl.c
>> > index 97551eb42946..226d4eaf4b0e 100644
>> > --- a/kernel/sysctl.c
>> > +++ b/kernel/sysctl.c
>> > @@ -127,6 +127,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 ulong_max = ULONG_MAX;
>> >  static int one_hundred = 100;
>> >  static int one_thousand = 1000;
>> >  #ifdef CONFIG_PRINTK
>> > @@ -1696,6 +1697,7 @@ static struct ctl_table fs_table[] = {
>> >                 .maxlen         = sizeof(files_stat.max_files),
>> >                 .mode           = 0644,
>> >                 .proc_handler   = proc_doulongvec_minmax,
>> > +               .extra2         = &ulong_max,
>>
>> Don't we want this capped lower? The percpu comparisons, for example,
>> are all signed long. And there is at least this test, which could
>> overflow:
>>
>>         if (atomic_long_read(&unix_nr_socks) > 2 * get_max_files())
>>                 goto out;
>
> Does that check even make sense?
> Commit 518de9b39e854542de59bfb8b9f61c8f7ecf808b made get_max_files()
> return a long to bump the number of allowed files to more than 2^31.
>
> But assuming a platform where an unsigned long is 64bit which is what
> get_max_files() returns and atomic_long_read() is 64bit too this is
> guaranteed to overflow, no?  So I'm not clear what this is trying to do.
> Seems this should simply be:
>
> if (atomic_long_read(&unix_nr_socks) > get_max_files())
>         goto out;
>
> or am I missing a crucial point?
>
>>
>> Seems like max-files should be  SLONG_MAX / 2 or something instead?
>
> Hm. Isn't that a bit low? Iiuc, this would mean cutting the maximum
> number of open files in half? If at all shouldn't it be LONG_MAX?

LONG_MAX would align us with the values in the percpu stuff. I'm
really not sure what's happening in the sock check, but it's prone to
an unsigned multiplication overflow, if I'm reading it right. Probably
should just be a separate bug fix:

-         if (atomic_long_read(&unix_nr_socks) > 2 * get_max_files())
+         if (atomic_long_read(&unix_nr_socks) / 2 > get_max_files())

-Kees

>
>>
>> >         },
>> >         {
>> >                 .procname       = "nr_open",
>> > @@ -2795,6 +2797,8 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
>> >                                 break;
>> >                         if (neg)
>> >                                 continue;
>> > +                       if (max && val > *max)
>> > +                               val = *max;
>> >                         val = convmul * val / convdiv;
>> >                         if ((min && val < *min) || (max && val > *max))
>> >                                 continue;
>> > --
>> > 2.17.1
>> >
>>
>> -Kees
>>
>> --
>> Kees Cook
>> Pixel Security



-- 
Kees Cook
Pixel Security

Re: [PATCH v1 2/2] sysctl: handle overflow for file-max

[Christian Brauner]

On Mon, Oct 15, 2018 at 02:20:15PM -0700, Kees Cook wrote:
> On Mon, Oct 15, 2018 at 9:28 AM, Christian Brauner <christian@brauner.io> wrote:
> > On Mon, Oct 15, 2018 at 09:11:51AM -0700, Kees Cook wrote:
> >> On Mon, Oct 15, 2018 at 3:55 AM, Christian Brauner <christian@brauner.io> wrote:
> >> > 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 explicitly caps the value for file-max to ULONG_MAX.
> >> >
> >> > Note, this isn't technically necessary since proc_get_long() will already
> >> > return ULONG_MAX. However, two reason why we still should do this:
> >> > 1. it makes it explicit what the upper bound of file-max is instead of
> >> >    making readers of the code infer it from proc_get_long() themselves
> >> > 2. other tunebles than file-max may want to set a lower max value than
> >> >    ULONG_MAX and we need to enable __do_proc_doulongvec_minmax() to handle
> >> >    such cases too
> >> >
> >> > Cc: Kees Cook <keescook@chromium.org>
> >> > Signed-off-by: Christian Brauner <christian@brauner.io>
> >> > ---
> >> > v0->v1:
> >> > - if max value is < than ULONG_MAX use max as upper bound
> >> > - (Dominik) remove double "the" from commit message
> >> > ---
> >> >  kernel/sysctl.c | 4 ++++
> >> >  1 file changed, 4 insertions(+)
> >> >
> >> > diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> >> > index 97551eb42946..226d4eaf4b0e 100644
> >> > --- a/kernel/sysctl.c
> >> > +++ b/kernel/sysctl.c
> >> > @@ -127,6 +127,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 ulong_max = ULONG_MAX;
> >> >  static int one_hundred = 100;
> >> >  static int one_thousand = 1000;
> >> >  #ifdef CONFIG_PRINTK
> >> > @@ -1696,6 +1697,7 @@ static struct ctl_table fs_table[] = {
> >> >                 .maxlen         = sizeof(files_stat.max_files),
> >> >                 .mode           = 0644,
> >> >                 .proc_handler   = proc_doulongvec_minmax,
> >> > +               .extra2         = &ulong_max,
> >>
> >> Don't we want this capped lower? The percpu comparisons, for example,
> >> are all signed long. And there is at least this test, which could
> >> overflow:
> >>
> >>         if (atomic_long_read(&unix_nr_socks) > 2 * get_max_files())
> >>                 goto out;
> >
> > Does that check even make sense?
> > Commit 518de9b39e854542de59bfb8b9f61c8f7ecf808b made get_max_files()
> > return a long to bump the number of allowed files to more than 2^31.
> >
> > But assuming a platform where an unsigned long is 64bit which is what
> > get_max_files() returns and atomic_long_read() is 64bit too this is
> > guaranteed to overflow, no?  So I'm not clear what this is trying to do.
> > Seems this should simply be:
> >
> > if (atomic_long_read(&unix_nr_socks) > get_max_files())
> >         goto out;
> >
> > or am I missing a crucial point?
> >
> >>
> >> Seems like max-files should be  SLONG_MAX / 2 or something instead?
> >
> > Hm. Isn't that a bit low? Iiuc, this would mean cutting the maximum
> > number of open files in half? If at all shouldn't it be LONG_MAX?
> 
> LONG_MAX would align us with the values in the percpu stuff. I'm
> really not sure what's happening in the sock check, but it's prone to
> an unsigned multiplication overflow, if I'm reading it right. Probably
> should just be a separate bug fix:
> 
> -         if (atomic_long_read(&unix_nr_socks) > 2 * get_max_files())
> +         if (atomic_long_read(&unix_nr_socks) / 2 > get_max_files())

Yeah, as I said before this is just waiting for an overflow and it seems
this bug has existed since the switch to git. The intention apparently
was indeed to allow the number of sockets to be double the open file
limit. However, as it is right now this just overflows. I'll send a
separate patch for this but I'd sugest to do something like:


diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index d1edfa3cad61..7e461f4f0466 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -754,9 +754,12 @@ static struct sock *unix_create1(struct net *net, struct socket *sock, int kern)
 {
 	struct sock *sk = NULL;
 	struct unix_sock *u;
+	static atomic_long_t tmp;
 
 	atomic_long_inc(&unix_nr_socks);
-	if (atomic_long_read(&unix_nr_socks) > 2 * get_max_files())
+	tmp = atomic_long_read(&unix_nr_socks);
+	do_div(tmp, 2);
+	if (tmp > get_max_files())
 		goto out;
 
 	sk = sk_alloc(net, PF_UNIX, GFP_KERNEL, &unix_proto, kern);

> 
> -Kees
> 
> >
> >>
> >> >         },
> >> >         {
> >> >                 .procname       = "nr_open",
> >> > @@ -2795,6 +2797,8 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
> >> >                                 break;
> >> >                         if (neg)
> >> >                                 continue;
> >> > +                       if (max && val > *max)
> >> > +                               val = *max;
> >> >                         val = convmul * val / convdiv;
> >> >                         if ((min && val < *min) || (max && val > *max))
> >> >                                 continue;
> >> > --
> >> > 2.17.1
> >> >
> >>
> >> -Kees
> >>
> >> --
> >> Kees Cook
> >> Pixel Security
> 
> 
> 
> -- 
> Kees Cook
> Pixel Security

Re: [PATCH v1 2/2] sysctl: handle overflow for file-max

[Christian Brauner]

On Tue, Oct 16, 2018 at 03:16:21PM +0200, Christian Brauner wrote:
> On Mon, Oct 15, 2018 at 02:20:15PM -0700, Kees Cook wrote:
> > On Mon, Oct 15, 2018 at 9:28 AM, Christian Brauner <christian@brauner.io> wrote:
> > > On Mon, Oct 15, 2018 at 09:11:51AM -0700, Kees Cook wrote:
> > >> On Mon, Oct 15, 2018 at 3:55 AM, Christian Brauner <christian@brauner.io> wrote:
> > >> > 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 explicitly caps the value for file-max to ULONG_MAX.
> > >> >
> > >> > Note, this isn't technically necessary since proc_get_long() will already
> > >> > return ULONG_MAX. However, two reason why we still should do this:
> > >> > 1. it makes it explicit what the upper bound of file-max is instead of
> > >> >    making readers of the code infer it from proc_get_long() themselves
> > >> > 2. other tunebles than file-max may want to set a lower max value than
> > >> >    ULONG_MAX and we need to enable __do_proc_doulongvec_minmax() to handle
> > >> >    such cases too
> > >> >
> > >> > Cc: Kees Cook <keescook@chromium.org>
> > >> > Signed-off-by: Christian Brauner <christian@brauner.io>
> > >> > ---
> > >> > v0->v1:
> > >> > - if max value is < than ULONG_MAX use max as upper bound
> > >> > - (Dominik) remove double "the" from commit message
> > >> > ---
> > >> >  kernel/sysctl.c | 4 ++++
> > >> >  1 file changed, 4 insertions(+)
> > >> >
> > >> > diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> > >> > index 97551eb42946..226d4eaf4b0e 100644
> > >> > --- a/kernel/sysctl.c
> > >> > +++ b/kernel/sysctl.c
> > >> > @@ -127,6 +127,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 ulong_max = ULONG_MAX;
> > >> >  static int one_hundred = 100;
> > >> >  static int one_thousand = 1000;
> > >> >  #ifdef CONFIG_PRINTK
> > >> > @@ -1696,6 +1697,7 @@ static struct ctl_table fs_table[] = {
> > >> >                 .maxlen         = sizeof(files_stat.max_files),
> > >> >                 .mode           = 0644,
> > >> >                 .proc_handler   = proc_doulongvec_minmax,
> > >> > +               .extra2         = &ulong_max,
> > >>
> > >> Don't we want this capped lower? The percpu comparisons, for example,
> > >> are all signed long. And there is at least this test, which could
> > >> overflow:
> > >>
> > >>         if (atomic_long_read(&unix_nr_socks) > 2 * get_max_files())
> > >>                 goto out;
> > >
> > > Does that check even make sense?
> > > Commit 518de9b39e854542de59bfb8b9f61c8f7ecf808b made get_max_files()
> > > return a long to bump the number of allowed files to more than 2^31.
> > >
> > > But assuming a platform where an unsigned long is 64bit which is what
> > > get_max_files() returns and atomic_long_read() is 64bit too this is
> > > guaranteed to overflow, no?  So I'm not clear what this is trying to do.
> > > Seems this should simply be:
> > >
> > > if (atomic_long_read(&unix_nr_socks) > get_max_files())
> > >         goto out;
> > >
> > > or am I missing a crucial point?
> > >
> > >>
> > >> Seems like max-files should be  SLONG_MAX / 2 or something instead?
> > >
> > > Hm. Isn't that a bit low? Iiuc, this would mean cutting the maximum
> > > number of open files in half? If at all shouldn't it be LONG_MAX?
> > 
> > LONG_MAX would align us with the values in the percpu stuff. I'm
> > really not sure what's happening in the sock check, but it's prone to
> > an unsigned multiplication overflow, if I'm reading it right. Probably
> > should just be a separate bug fix:
> > 
> > -         if (atomic_long_read(&unix_nr_socks) > 2 * get_max_files())
> > +         if (atomic_long_read(&unix_nr_socks) / 2 > get_max_files())
> 
> Yeah, as I said before this is just waiting for an overflow and it seems
> this bug has existed since the switch to git. The intention apparently
> was indeed to allow the number of sockets to be double the open file
> limit. However, as it is right now this just overflows. I'll send a
> separate patch for this but I'd sugest to do something like:
> 

Forget that. I'm obviously overcomplicating things that are supposed to
be simple.

> 
> > 
> > -Kees
> > 
> > >
> > >>
> > >> >         },
> > >> >         {
> > >> >                 .procname       = "nr_open",
> > >> > @@ -2795,6 +2797,8 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
> > >> >                                 break;
> > >> >                         if (neg)
> > >> >                                 continue;
> > >> > +                       if (max && val > *max)
> > >> > +                               val = *max;
> > >> >                         val = convmul * val / convdiv;
> > >> >                         if ((min && val < *min) || (max && val > *max))
> > >> >                                 continue;
> > >> > --
> > >> > 2.17.1
> > >> >
> > >>
> > >> -Kees
> > >>
> > >> --
> > >> Kees Cook
> > >> Pixel Security
> > 
> > 
> > 
> > -- 
> > Kees Cook
> > Pixel Security

Re: [PATCH v1 2/2] sysctl: handle overflow for file-max

[Waiman Long]

On 10/15/2018 06:55 AM, Christian Brauner wrote:
> 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 explicitly caps the value for file-max to ULONG_MAX.
>
> Note, this isn't technically necessary since proc_get_long() will already
> return ULONG_MAX. However, two reason why we still should do this:
> 1. it makes it explicit what the upper bound of file-max is instead of
>    making readers of the code infer it from proc_get_long() themselves
> 2. other tunebles than file-max may want to set a lower max value than
>    ULONG_MAX and we need to enable __do_proc_doulongvec_minmax() to handle
>    such cases too
>
> Cc: Kees Cook <keescook@chromium.org>
> Signed-off-by: Christian Brauner <christian@brauner.io>
> ---
> v0->v1:
> - if max value is < than ULONG_MAX use max as upper bound
> - (Dominik) remove double "the" from commit message
> ---
>  kernel/sysctl.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 97551eb42946..226d4eaf4b0e 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -127,6 +127,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 ulong_max = ULONG_MAX;
>  static int one_hundred = 100;
>  static int one_thousand = 1000;
>  #ifdef CONFIG_PRINTK
> @@ -1696,6 +1697,7 @@ static struct ctl_table fs_table[] = {
>  		.maxlen		= sizeof(files_stat.max_files),
>  		.mode		= 0644,
>  		.proc_handler	= proc_doulongvec_minmax,
> +		.extra2		= &ulong_max,

What is the point of having a maximum value of ULONG_MAX anyway? No
value you can put into a ulong type can be bigger than that.

>  	},
>  	{
>  		.procname	= "nr_open",
> @@ -2795,6 +2797,8 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
>  				break;
>  			if (neg)
>  				continue;
> +			if (max && val > *max)
> +				val = *max;
>  			val = convmul * val / convdiv;
>  			if ((min && val < *min) || (max && val > *max))
>  				continue;

This does introduce a change in behavior. Previously the out-of-bound
value is ignored, now it is capped at its maximum. This is a
user-visible change.

Cheers,
Longman

Re: [PATCH v1 2/2] sysctl: handle overflow for file-max

[Christian Brauner]

On Tue, Oct 16, 2018 at 11:13:28AM -0400, Waiman Long wrote:
> On 10/15/2018 06:55 AM, Christian Brauner wrote:
> > 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 explicitly caps the value for file-max to ULONG_MAX.
> >
> > Note, this isn't technically necessary since proc_get_long() will already
> > return ULONG_MAX. However, two reason why we still should do this:
> > 1. it makes it explicit what the upper bound of file-max is instead of
> >    making readers of the code infer it from proc_get_long() themselves
> > 2. other tunebles than file-max may want to set a lower max value than
> >    ULONG_MAX and we need to enable __do_proc_doulongvec_minmax() to handle
> >    such cases too
> >
> > Cc: Kees Cook <keescook@chromium.org>
> > Signed-off-by: Christian Brauner <christian@brauner.io>
> > ---
> > v0->v1:
> > - if max value is < than ULONG_MAX use max as upper bound
> > - (Dominik) remove double "the" from commit message
> > ---
> >  kernel/sysctl.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> > index 97551eb42946..226d4eaf4b0e 100644
> > --- a/kernel/sysctl.c
> > +++ b/kernel/sysctl.c
> > @@ -127,6 +127,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 ulong_max = ULONG_MAX;
> >  static int one_hundred = 100;
> >  static int one_thousand = 1000;
> >  #ifdef CONFIG_PRINTK
> > @@ -1696,6 +1697,7 @@ static struct ctl_table fs_table[] = {
> >  		.maxlen		= sizeof(files_stat.max_files),
> >  		.mode		= 0644,
> >  		.proc_handler	= proc_doulongvec_minmax,
> > +		.extra2		= &ulong_max,
> 
> What is the point of having a maximum value of ULONG_MAX anyway? No
> value you can put into a ulong type can be bigger than that.

This is changed in the new code to LONG_MAX. See the full thread for
context. There's also an additional explantion in the commit message.

> 
> >  	},
> >  	{
> >  		.procname	= "nr_open",
> > @@ -2795,6 +2797,8 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
> >  				break;
> >  			if (neg)
> >  				continue;
> > +			if (max && val > *max)
> > +				val = *max;
> >  			val = convmul * val / convdiv;
> >  			if ((min && val < *min) || (max && val > *max))
> >  				continue;
> 
> This does introduce a change in behavior. Previously the out-of-bound
> value is ignored, now it is capped at its maximum. This is a
> user-visible change.

Not completely true though. Try

echo 18446744073709551616 > /proc/sys/fs/file-max

on a system you find acceptable loosing.
So this is an acceptable user-visible change I'd say. But I'm open to
other suggestions.

> 
> Cheers,
> Longman
> 
> 

Re: [PATCH v1 2/2] sysctl: handle overflow for file-max

[Waiman Long]

On 10/16/2018 11:21 AM, Christian Brauner wrote:
> On Tue, Oct 16, 2018 at 11:13:28AM -0400, Waiman Long wrote:
>> On 10/15/2018 06:55 AM, Christian Brauner wrote:
>>> 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 explicitly caps the value for file-max to ULONG_MAX.
>>>
>>> Note, this isn't technically necessary since proc_get_long() will already
>>> return ULONG_MAX. However, two reason why we still should do this:
>>> 1. it makes it explicit what the upper bound of file-max is instead of
>>>    making readers of the code infer it from proc_get_long() themselves
>>> 2. other tunebles than file-max may want to set a lower max value than
>>>    ULONG_MAX and we need to enable __do_proc_doulongvec_minmax() to handle
>>>    such cases too
>>>
>>> Cc: Kees Cook <keescook@chromium.org>
>>> Signed-off-by: Christian Brauner <christian@brauner.io>
>>> ---
>>> v0->v1:
>>> - if max value is < than ULONG_MAX use max as upper bound
>>> - (Dominik) remove double "the" from commit message
>>> ---
>>>  kernel/sysctl.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
>>> index 97551eb42946..226d4eaf4b0e 100644
>>> --- a/kernel/sysctl.c
>>> +++ b/kernel/sysctl.c
>>> @@ -127,6 +127,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 ulong_max = ULONG_MAX;
>>>  static int one_hundred = 100;
>>>  static int one_thousand = 1000;
>>>  #ifdef CONFIG_PRINTK
>>> @@ -1696,6 +1697,7 @@ static struct ctl_table fs_table[] = {
>>>  		.maxlen		= sizeof(files_stat.max_files),
>>>  		.mode		= 0644,
>>>  		.proc_handler	= proc_doulongvec_minmax,
>>> +		.extra2		= &ulong_max,
>> What is the point of having a maximum value of ULONG_MAX anyway? No
>> value you can put into a ulong type can be bigger than that.
> This is changed in the new code to LONG_MAX. See the full thread for
> context. There's also an additional explantion in the commit message.
>
>>>  	},
>>>  	{
>>>  		.procname	= "nr_open",
>>> @@ -2795,6 +2797,8 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
>>>  				break;
>>>  			if (neg)
>>>  				continue;
>>> +			if (max && val > *max)
>>> +				val = *max;
>>>  			val = convmul * val / convdiv;
>>>  			if ((min && val < *min) || (max && val > *max))
>>>  				continue;
>> This does introduce a change in behavior. Previously the out-of-bound
>> value is ignored, now it is capped at its maximum. This is a
>> user-visible change.
> Not completely true though. Try
>
> echo 18446744073709551616 > /proc/sys/fs/file-max
>
> on a system you find acceptable loosing.
> So this is an acceptable user-visible change I'd say. But I'm open to
> other suggestions.

I am not saying this is unacceptable. I just say this is a user-visible
change and so should be documented somehow. BTW, you cap the max value,
but not the min value. So there is inconsistency. I would say you either
do both, or none of them.

Cheers,
Longman

Re: [PATCH v1 2/2] sysctl: handle overflow for file-max

[Christian Brauner]

On Tue, Oct 16, 2018 at 11:25:42AM -0400, Waiman Long wrote:
> On 10/16/2018 11:21 AM, Christian Brauner wrote:
> > On Tue, Oct 16, 2018 at 11:13:28AM -0400, Waiman Long wrote:
> >> On 10/15/2018 06:55 AM, Christian Brauner wrote:
> >>> 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 explicitly caps the value for file-max to ULONG_MAX.
> >>>
> >>> Note, this isn't technically necessary since proc_get_long() will already
> >>> return ULONG_MAX. However, two reason why we still should do this:
> >>> 1. it makes it explicit what the upper bound of file-max is instead of
> >>>    making readers of the code infer it from proc_get_long() themselves
> >>> 2. other tunebles than file-max may want to set a lower max value than
> >>>    ULONG_MAX and we need to enable __do_proc_doulongvec_minmax() to handle
> >>>    such cases too
> >>>
> >>> Cc: Kees Cook <keescook@chromium.org>
> >>> Signed-off-by: Christian Brauner <christian@brauner.io>
> >>> ---
> >>> v0->v1:
> >>> - if max value is < than ULONG_MAX use max as upper bound
> >>> - (Dominik) remove double "the" from commit message
> >>> ---
> >>>  kernel/sysctl.c | 4 ++++
> >>>  1 file changed, 4 insertions(+)
> >>>
> >>> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> >>> index 97551eb42946..226d4eaf4b0e 100644
> >>> --- a/kernel/sysctl.c
> >>> +++ b/kernel/sysctl.c
> >>> @@ -127,6 +127,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 ulong_max = ULONG_MAX;
> >>>  static int one_hundred = 100;
> >>>  static int one_thousand = 1000;
> >>>  #ifdef CONFIG_PRINTK
> >>> @@ -1696,6 +1697,7 @@ static struct ctl_table fs_table[] = {
> >>>  		.maxlen		= sizeof(files_stat.max_files),
> >>>  		.mode		= 0644,
> >>>  		.proc_handler	= proc_doulongvec_minmax,
> >>> +		.extra2		= &ulong_max,
> >> What is the point of having a maximum value of ULONG_MAX anyway? No
> >> value you can put into a ulong type can be bigger than that.
> > This is changed in the new code to LONG_MAX. See the full thread for
> > context. There's also an additional explantion in the commit message.
> >
> >>>  	},
> >>>  	{
> >>>  		.procname	= "nr_open",
> >>> @@ -2795,6 +2797,8 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
> >>>  				break;
> >>>  			if (neg)
> >>>  				continue;
> >>> +			if (max && val > *max)
> >>> +				val = *max;
> >>>  			val = convmul * val / convdiv;
> >>>  			if ((min && val < *min) || (max && val > *max))
> >>>  				continue;
> >> This does introduce a change in behavior. Previously the out-of-bound
> >> value is ignored, now it is capped at its maximum. This is a
> >> user-visible change.
> > Not completely true though. Try
> >
> > echo 18446744073709551616 > /proc/sys/fs/file-max
> >
> > on a system you find acceptable loosing.
> > So this is an acceptable user-visible change I'd say. But I'm open to
> > other suggestions.
> 
> I am not saying this is unacceptable. I just say this is a user-visible
> change and so should be documented somehow. BTW, you cap the max value,

Sure, I'll update linux manpages and I can CC stable on the next round.

> but not the min value. So there is inconsistency. I would say you either
> do both, or none of them.

The min value is 0. I don't think it needs to be set explicitly. I just
kept the max value because it is != ULONG_MAX but LONG_MAX for file-max.

Re: [PATCH v1 2/2] sysctl: handle overflow for file-max

[Christian Brauner]

On Tue, Oct 16, 2018 at 05:29:55PM +0200, Christian Brauner wrote:
> On Tue, Oct 16, 2018 at 11:25:42AM -0400, Waiman Long wrote:
> > On 10/16/2018 11:21 AM, Christian Brauner wrote:
> > > On Tue, Oct 16, 2018 at 11:13:28AM -0400, Waiman Long wrote:
> > >> On 10/15/2018 06:55 AM, Christian Brauner wrote:
> > >>> 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 explicitly caps the value for file-max to ULONG_MAX.
> > >>>
> > >>> Note, this isn't technically necessary since proc_get_long() will already
> > >>> return ULONG_MAX. However, two reason why we still should do this:
> > >>> 1. it makes it explicit what the upper bound of file-max is instead of
> > >>>    making readers of the code infer it from proc_get_long() themselves
> > >>> 2. other tunebles than file-max may want to set a lower max value than
> > >>>    ULONG_MAX and we need to enable __do_proc_doulongvec_minmax() to handle
> > >>>    such cases too
> > >>>
> > >>> Cc: Kees Cook <keescook@chromium.org>
> > >>> Signed-off-by: Christian Brauner <christian@brauner.io>
> > >>> ---
> > >>> v0->v1:
> > >>> - if max value is < than ULONG_MAX use max as upper bound
> > >>> - (Dominik) remove double "the" from commit message
> > >>> ---
> > >>>  kernel/sysctl.c | 4 ++++
> > >>>  1 file changed, 4 insertions(+)
> > >>>
> > >>> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> > >>> index 97551eb42946..226d4eaf4b0e 100644
> > >>> --- a/kernel/sysctl.c
> > >>> +++ b/kernel/sysctl.c
> > >>> @@ -127,6 +127,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 ulong_max = ULONG_MAX;
> > >>>  static int one_hundred = 100;
> > >>>  static int one_thousand = 1000;
> > >>>  #ifdef CONFIG_PRINTK
> > >>> @@ -1696,6 +1697,7 @@ static struct ctl_table fs_table[] = {
> > >>>  		.maxlen		= sizeof(files_stat.max_files),
> > >>>  		.mode		= 0644,
> > >>>  		.proc_handler	= proc_doulongvec_minmax,
> > >>> +		.extra2		= &ulong_max,
> > >> What is the point of having a maximum value of ULONG_MAX anyway? No
> > >> value you can put into a ulong type can be bigger than that.
> > > This is changed in the new code to LONG_MAX. See the full thread for
> > > context. There's also an additional explantion in the commit message.
> > >
> > >>>  	},
> > >>>  	{
> > >>>  		.procname	= "nr_open",
> > >>> @@ -2795,6 +2797,8 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
> > >>>  				break;
> > >>>  			if (neg)
> > >>>  				continue;
> > >>> +			if (max && val > *max)
> > >>> +				val = *max;
> > >>>  			val = convmul * val / convdiv;
> > >>>  			if ((min && val < *min) || (max && val > *max))
> > >>>  				continue;
> > >> This does introduce a change in behavior. Previously the out-of-bound
> > >> value is ignored, now it is capped at its maximum. This is a
> > >> user-visible change.
> > > Not completely true though. Try
> > >
> > > echo 18446744073709551616 > /proc/sys/fs/file-max
> > >
> > > on a system you find acceptable loosing.
> > > So this is an acceptable user-visible change I'd say. But I'm open to
> > > other suggestions.
> > 
> > I am not saying this is unacceptable. I just say this is a user-visible
> > change and so should be documented somehow. BTW, you cap the max value,
> 
> Sure, I'll update linux manpages and I can CC stable on the next round.

Sorry, s/stable/linux-api@vger.kernel.org/

> 
> > but not the min value. So there is inconsistency. I would say you either
> > do both, or none of them.
> 
> The min value is 0. I don't think it needs to be set explicitly. I just
> kept the max value because it is != ULONG_MAX but LONG_MAX for file-max.

Re: [PATCH v1 2/2] sysctl: handle overflow for file-max

[Waiman Long]

On 10/16/2018 11:29 AM, Christian Brauner wrote:
> On Tue, Oct 16, 2018 at 11:25:42AM -0400, Waiman Long wrote:
>> On 10/16/2018 11:21 AM, Christian Brauner wrote:
>>> On Tue, Oct 16, 2018 at 11:13:28AM -0400, Waiman Long wrote:
>>>> On 10/15/2018 06:55 AM, Christian Brauner wrote:
>>>>> 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 explicitly caps the value for file-max to ULONG_MAX.
>>>>>
>>>>> Note, this isn't technically necessary since proc_get_long() will already
>>>>> return ULONG_MAX. However, two reason why we still should do this:
>>>>> 1. it makes it explicit what the upper bound of file-max is instead of
>>>>>    making readers of the code infer it from proc_get_long() themselves
>>>>> 2. other tunebles than file-max may want to set a lower max value than
>>>>>    ULONG_MAX and we need to enable __do_proc_doulongvec_minmax() to handle
>>>>>    such cases too
>>>>>
>>>>> Cc: Kees Cook <keescook@chromium.org>
>>>>> Signed-off-by: Christian Brauner <christian@brauner.io>
>>>>> ---
>>>>> v0->v1:
>>>>> - if max value is < than ULONG_MAX use max as upper bound
>>>>> - (Dominik) remove double "the" from commit message
>>>>> ---
>>>>>  kernel/sysctl.c | 4 ++++
>>>>>  1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
>>>>> index 97551eb42946..226d4eaf4b0e 100644
>>>>> --- a/kernel/sysctl.c
>>>>> +++ b/kernel/sysctl.c
>>>>> @@ -127,6 +127,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 ulong_max = ULONG_MAX;
>>>>>  static int one_hundred = 100;
>>>>>  static int one_thousand = 1000;
>>>>>  #ifdef CONFIG_PRINTK
>>>>> @@ -1696,6 +1697,7 @@ static struct ctl_table fs_table[] = {
>>>>>  		.maxlen		= sizeof(files_stat.max_files),
>>>>>  		.mode		= 0644,
>>>>>  		.proc_handler	= proc_doulongvec_minmax,
>>>>> +		.extra2		= &ulong_max,
>>>> What is the point of having a maximum value of ULONG_MAX anyway? No
>>>> value you can put into a ulong type can be bigger than that.
>>> This is changed in the new code to LONG_MAX. See the full thread for
>>> context. There's also an additional explantion in the commit message.
>>>
>>>>>  	},
>>>>>  	{
>>>>>  		.procname	= "nr_open",
>>>>> @@ -2795,6 +2797,8 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
>>>>>  				break;
>>>>>  			if (neg)
>>>>>  				continue;
>>>>> +			if (max && val > *max)
>>>>> +				val = *max;
>>>>>  			val = convmul * val / convdiv;
>>>>>  			if ((min && val < *min) || (max && val > *max))
>>>>>  				continue;
>>>> This does introduce a change in behavior. Previously the out-of-bound
>>>> value is ignored, now it is capped at its maximum. This is a
>>>> user-visible change.
>>> Not completely true though. Try
>>>
>>> echo 18446744073709551616 > /proc/sys/fs/file-max
>>>
>>> on a system you find acceptable loosing.
>>> So this is an acceptable user-visible change I'd say. But I'm open to
>>> other suggestions.
>> I am not saying this is unacceptable. I just say this is a user-visible
>> change and so should be documented somehow. BTW, you cap the max value,
> Sure, I'll update linux manpages and I can CC stable on the next round.
>
>> but not the min value. So there is inconsistency. I would say you either
>> do both, or none of them.
> The min value is 0. I don't think it needs to be set explicitly. I just
> kept the max value because it is != ULONG_MAX but LONG_MAX for file-max.

I think you are making the change with just one use case in mind. This
is a generic function that can be used by many different callers. So any
change you make has to be applicable to all use cases. You just can't
assume min is always 0 in all the other use cases.

Cheers,
Longman

Re: [PATCH v1 2/2] sysctl: handle overflow for file-max

[Christian Brauner]

On Tue, Oct 16, 2018 at 11:34:07AM -0400, Waiman Long wrote:
> On 10/16/2018 11:29 AM, Christian Brauner wrote:
> > On Tue, Oct 16, 2018 at 11:25:42AM -0400, Waiman Long wrote:
> >> On 10/16/2018 11:21 AM, Christian Brauner wrote:
> >>> On Tue, Oct 16, 2018 at 11:13:28AM -0400, Waiman Long wrote:
> >>>> On 10/15/2018 06:55 AM, Christian Brauner wrote:
> >>>>> 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 explicitly caps the value for file-max to ULONG_MAX.
> >>>>>
> >>>>> Note, this isn't technically necessary since proc_get_long() will already
> >>>>> return ULONG_MAX. However, two reason why we still should do this:
> >>>>> 1. it makes it explicit what the upper bound of file-max is instead of
> >>>>>    making readers of the code infer it from proc_get_long() themselves
> >>>>> 2. other tunebles than file-max may want to set a lower max value than
> >>>>>    ULONG_MAX and we need to enable __do_proc_doulongvec_minmax() to handle
> >>>>>    such cases too
> >>>>>
> >>>>> Cc: Kees Cook <keescook@chromium.org>
> >>>>> Signed-off-by: Christian Brauner <christian@brauner.io>
> >>>>> ---
> >>>>> v0->v1:
> >>>>> - if max value is < than ULONG_MAX use max as upper bound
> >>>>> - (Dominik) remove double "the" from commit message
> >>>>> ---
> >>>>>  kernel/sysctl.c | 4 ++++
> >>>>>  1 file changed, 4 insertions(+)
> >>>>>
> >>>>> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> >>>>> index 97551eb42946..226d4eaf4b0e 100644
> >>>>> --- a/kernel/sysctl.c
> >>>>> +++ b/kernel/sysctl.c
> >>>>> @@ -127,6 +127,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 ulong_max = ULONG_MAX;
> >>>>>  static int one_hundred = 100;
> >>>>>  static int one_thousand = 1000;
> >>>>>  #ifdef CONFIG_PRINTK
> >>>>> @@ -1696,6 +1697,7 @@ static struct ctl_table fs_table[] = {
> >>>>>  		.maxlen		= sizeof(files_stat.max_files),
> >>>>>  		.mode		= 0644,
> >>>>>  		.proc_handler	= proc_doulongvec_minmax,
> >>>>> +		.extra2		= &ulong_max,
> >>>> What is the point of having a maximum value of ULONG_MAX anyway? No
> >>>> value you can put into a ulong type can be bigger than that.
> >>> This is changed in the new code to LONG_MAX. See the full thread for
> >>> context. There's also an additional explantion in the commit message.
> >>>
> >>>>>  	},
> >>>>>  	{
> >>>>>  		.procname	= "nr_open",
> >>>>> @@ -2795,6 +2797,8 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
> >>>>>  				break;
> >>>>>  			if (neg)
> >>>>>  				continue;
> >>>>> +			if (max && val > *max)
> >>>>> +				val = *max;
> >>>>>  			val = convmul * val / convdiv;
> >>>>>  			if ((min && val < *min) || (max && val > *max))
> >>>>>  				continue;
> >>>> This does introduce a change in behavior. Previously the out-of-bound
> >>>> value is ignored, now it is capped at its maximum. This is a
> >>>> user-visible change.
> >>> Not completely true though. Try
> >>>
> >>> echo 18446744073709551616 > /proc/sys/fs/file-max
> >>>
> >>> on a system you find acceptable loosing.
> >>> So this is an acceptable user-visible change I'd say. But I'm open to
> >>> other suggestions.
> >> I am not saying this is unacceptable. I just say this is a user-visible
> >> change and so should be documented somehow. BTW, you cap the max value,
> > Sure, I'll update linux manpages and I can CC stable on the next round.
> >
> >> but not the min value. So there is inconsistency. I would say you either
> >> do both, or none of them.
> > The min value is 0. I don't think it needs to be set explicitly. I just
> > kept the max value because it is != ULONG_MAX but LONG_MAX for file-max.
> 
> I think you are making the change with just one use case in mind. This
> is a generic function that can be used by many different callers. So any
> change you make has to be applicable to all use cases. You just can't
> assume min is always 0 in all the other use cases.

So, any caller that calls {do_}proc_doulongvec_minmax() must want an
unsigned long lest they are calling the wrong function.
The smallest value for an unsigned long is 0. So if any caller wants to
get into a situation where the caller needs to be capped they need to be
able to set the value to lower than 0 which they can't since they are
requesting an unsigned. So I'm not sure it makes sense.

Christian

Re: [PATCH v1 2/2] sysctl: handle overflow for file-max

[Waiman Long]

On 10/16/2018 11:40 AM, Christian Brauner wrote:
> On Tue, Oct 16, 2018 at 11:34:07AM -0400, Waiman Long wrote:
>> On 10/16/2018 11:29 AM, Christian Brauner wrote:
>>> On Tue, Oct 16, 2018 at 11:25:42AM -0400, Waiman Long wrote:
>>>> On 10/16/2018 11:21 AM, Christian Brauner wrote:
>>>>> On Tue, Oct 16, 2018 at 11:13:28AM -0400, Waiman Long wrote:
>>>>>> On 10/15/2018 06:55 AM, Christian Brauner wrote:
>>>>>>> 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 explicitly caps the value for file-max to ULONG_MAX.
>>>>>>>
>>>>>>> Note, this isn't technically necessary since proc_get_long() will already
>>>>>>> return ULONG_MAX. However, two reason why we still should do this:
>>>>>>> 1. it makes it explicit what the upper bound of file-max is instead of
>>>>>>>    making readers of the code infer it from proc_get_long() themselves
>>>>>>> 2. other tunebles than file-max may want to set a lower max value than
>>>>>>>    ULONG_MAX and we need to enable __do_proc_doulongvec_minmax() to handle
>>>>>>>    such cases too
>>>>>>>
>>>>>>> Cc: Kees Cook <keescook@chromium.org>
>>>>>>> Signed-off-by: Christian Brauner <christian@brauner.io>
>>>>>>> ---
>>>>>>> v0->v1:
>>>>>>> - if max value is < than ULONG_MAX use max as upper bound
>>>>>>> - (Dominik) remove double "the" from commit message
>>>>>>> ---
>>>>>>>  kernel/sysctl.c | 4 ++++
>>>>>>>  1 file changed, 4 insertions(+)
>>>>>>>
>>>>>>> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
>>>>>>> index 97551eb42946..226d4eaf4b0e 100644
>>>>>>> --- a/kernel/sysctl.c
>>>>>>> +++ b/kernel/sysctl.c
>>>>>>> @@ -127,6 +127,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 ulong_max = ULONG_MAX;
>>>>>>>  static int one_hundred = 100;
>>>>>>>  static int one_thousand = 1000;
>>>>>>>  #ifdef CONFIG_PRINTK
>>>>>>> @@ -1696,6 +1697,7 @@ static struct ctl_table fs_table[] = {
>>>>>>>  		.maxlen		= sizeof(files_stat.max_files),
>>>>>>>  		.mode		= 0644,
>>>>>>>  		.proc_handler	= proc_doulongvec_minmax,
>>>>>>> +		.extra2		= &ulong_max,
>>>>>> What is the point of having a maximum value of ULONG_MAX anyway? No
>>>>>> value you can put into a ulong type can be bigger than that.
>>>>> This is changed in the new code to LONG_MAX. See the full thread for
>>>>> context. There's also an additional explantion in the commit message.
>>>>>
>>>>>>>  	},
>>>>>>>  	{
>>>>>>>  		.procname	= "nr_open",
>>>>>>> @@ -2795,6 +2797,8 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
>>>>>>>  				break;
>>>>>>>  			if (neg)
>>>>>>>  				continue;
>>>>>>> +			if (max && val > *max)
>>>>>>> +				val = *max;
>>>>>>>  			val = convmul * val / convdiv;
>>>>>>>  			if ((min && val < *min) || (max && val > *max))
>>>>>>>  				continue;
>>>>>> This does introduce a change in behavior. Previously the out-of-bound
>>>>>> value is ignored, now it is capped at its maximum. This is a
>>>>>> user-visible change.
>>>>> Not completely true though. Try
>>>>>
>>>>> echo 18446744073709551616 > /proc/sys/fs/file-max
>>>>>
>>>>> on a system you find acceptable loosing.
>>>>> So this is an acceptable user-visible change I'd say. But I'm open to
>>>>> other suggestions.
>>>> I am not saying this is unacceptable. I just say this is a user-visible
>>>> change and so should be documented somehow. BTW, you cap the max value,
>>> Sure, I'll update linux manpages and I can CC stable on the next round.
>>>
>>>> but not the min value. So there is inconsistency. I would say you either
>>>> do both, or none of them.
>>> The min value is 0. I don't think it needs to be set explicitly. I just
>>> kept the max value because it is != ULONG_MAX but LONG_MAX for file-max.
>> I think you are making the change with just one use case in mind. This
>> is a generic function that can be used by many different callers. So any
>> change you make has to be applicable to all use cases. You just can't
>> assume min is always 0 in all the other use cases.
> So, any caller that calls {do_}proc_doulongvec_minmax() must want an
> unsigned long lest they are calling the wrong function.
> The smallest value for an unsigned long is 0. So if any caller wants to
> get into a situation where the caller needs to be capped they need to be
> able to set the value to lower than 0 which they can't since they are
> requesting an unsigned. So I'm not sure it makes sense.
>
> Christian

There may be use cases where the developer may want a min value that is
bigger than 0. As I said, you can't just make an assumption here.
Otherwise, what is the point of the following check:

	if ((min && val < *min) || (max && val > *max))
 		continue;

Cheers,
Longman

Re: [PATCH v1 2/2] sysctl: handle overflow for file-max

[Christian Brauner]

On Tue, Oct 16, 2018 at 11:44:40AM -0400, Waiman Long wrote:
> On 10/16/2018 11:40 AM, Christian Brauner wrote:
> > On Tue, Oct 16, 2018 at 11:34:07AM -0400, Waiman Long wrote:
> >> On 10/16/2018 11:29 AM, Christian Brauner wrote:
> >>> On Tue, Oct 16, 2018 at 11:25:42AM -0400, Waiman Long wrote:
> >>>> On 10/16/2018 11:21 AM, Christian Brauner wrote:
> >>>>> On Tue, Oct 16, 2018 at 11:13:28AM -0400, Waiman Long wrote:
> >>>>>> On 10/15/2018 06:55 AM, Christian Brauner wrote:
> >>>>>>> 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 explicitly caps the value for file-max to ULONG_MAX.
> >>>>>>>
> >>>>>>> Note, this isn't technically necessary since proc_get_long() will already
> >>>>>>> return ULONG_MAX. However, two reason why we still should do this:
> >>>>>>> 1. it makes it explicit what the upper bound of file-max is instead of
> >>>>>>>    making readers of the code infer it from proc_get_long() themselves
> >>>>>>> 2. other tunebles than file-max may want to set a lower max value than
> >>>>>>>    ULONG_MAX and we need to enable __do_proc_doulongvec_minmax() to handle
> >>>>>>>    such cases too
> >>>>>>>
> >>>>>>> Cc: Kees Cook <keescook@chromium.org>
> >>>>>>> Signed-off-by: Christian Brauner <christian@brauner.io>
> >>>>>>> ---
> >>>>>>> v0->v1:
> >>>>>>> - if max value is < than ULONG_MAX use max as upper bound
> >>>>>>> - (Dominik) remove double "the" from commit message
> >>>>>>> ---
> >>>>>>>  kernel/sysctl.c | 4 ++++
> >>>>>>>  1 file changed, 4 insertions(+)
> >>>>>>>
> >>>>>>> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> >>>>>>> index 97551eb42946..226d4eaf4b0e 100644
> >>>>>>> --- a/kernel/sysctl.c
> >>>>>>> +++ b/kernel/sysctl.c
> >>>>>>> @@ -127,6 +127,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 ulong_max = ULONG_MAX;
> >>>>>>>  static int one_hundred = 100;
> >>>>>>>  static int one_thousand = 1000;
> >>>>>>>  #ifdef CONFIG_PRINTK
> >>>>>>> @@ -1696,6 +1697,7 @@ static struct ctl_table fs_table[] = {
> >>>>>>>  		.maxlen		= sizeof(files_stat.max_files),
> >>>>>>>  		.mode		= 0644,
> >>>>>>>  		.proc_handler	= proc_doulongvec_minmax,
> >>>>>>> +		.extra2		= &ulong_max,
> >>>>>> What is the point of having a maximum value of ULONG_MAX anyway? No
> >>>>>> value you can put into a ulong type can be bigger than that.
> >>>>> This is changed in the new code to LONG_MAX. See the full thread for
> >>>>> context. There's also an additional explantion in the commit message.
> >>>>>
> >>>>>>>  	},
> >>>>>>>  	{
> >>>>>>>  		.procname	= "nr_open",
> >>>>>>> @@ -2795,6 +2797,8 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
> >>>>>>>  				break;
> >>>>>>>  			if (neg)
> >>>>>>>  				continue;
> >>>>>>> +			if (max && val > *max)
> >>>>>>> +				val = *max;
> >>>>>>>  			val = convmul * val / convdiv;
> >>>>>>>  			if ((min && val < *min) || (max && val > *max))
> >>>>>>>  				continue;
> >>>>>> This does introduce a change in behavior. Previously the out-of-bound
> >>>>>> value is ignored, now it is capped at its maximum. This is a
> >>>>>> user-visible change.
> >>>>> Not completely true though. Try
> >>>>>
> >>>>> echo 18446744073709551616 > /proc/sys/fs/file-max
> >>>>>
> >>>>> on a system you find acceptable loosing.
> >>>>> So this is an acceptable user-visible change I'd say. But I'm open to
> >>>>> other suggestions.
> >>>> I am not saying this is unacceptable. I just say this is a user-visible
> >>>> change and so should be documented somehow. BTW, you cap the max value,
> >>> Sure, I'll update linux manpages and I can CC stable on the next round.
> >>>
> >>>> but not the min value. So there is inconsistency. I would say you either
> >>>> do both, or none of them.
> >>> The min value is 0. I don't think it needs to be set explicitly. I just
> >>> kept the max value because it is != ULONG_MAX but LONG_MAX for file-max.
> >> I think you are making the change with just one use case in mind. This
> >> is a generic function that can be used by many different callers. So any
> >> change you make has to be applicable to all use cases. You just can't
> >> assume min is always 0 in all the other use cases.
> > So, any caller that calls {do_}proc_doulongvec_minmax() must want an
> > unsigned long lest they are calling the wrong function.
> > The smallest value for an unsigned long is 0. So if any caller wants to
> > get into a situation where the caller needs to be capped they need to be
> > able to set the value to lower than 0 which they can't since they are
> > requesting an unsigned. So I'm not sure it makes sense.
> >
> > Christian
> 
> There may be use cases where the developer may want a min value that is
> bigger than 0. As I said, you can't just make an assumption here.

Sorry, I'm not following your argument anymore. Why exactly is this
function not already handling this case and what exactly does break with
my changs?  And what exactly do we gain from setting file-max to extra1
= 0?

> Otherwise, what is the point of the following check:
> 
> 	if ((min && val < *min) || (max && val > *max))
>  		continue;
> 
> Cheers,
> Longman
> 

Re: [PATCH v1 2/2] sysctl: handle overflow for file-max

[Waiman Long]

On 10/16/2018 11:47 AM, Christian Brauner wrote:
> On Tue, Oct 16, 2018 at 11:44:40AM -0400, Waiman Long wrote:
>> On 10/16/2018 11:40 AM, Christian Brauner wrote:
>>> On Tue, Oct 16, 2018 at 11:34:07AM -0400, Waiman Long wrote:
>>>> On 10/16/2018 11:29 AM, Christian Brauner wrote:
>>>>> On Tue, Oct 16, 2018 at 11:25:42AM -0400, Waiman Long wrote:
>>>>>> On 10/16/2018 11:21 AM, Christian Brauner wrote:
>>>>>>> On Tue, Oct 16, 2018 at 11:13:28AM -0400, Waiman Long wrote:
>>>>>>>> On 10/15/2018 06:55 AM, Christian Brauner wrote:
>>>>>>>>> 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 explicitly caps the value for file-max to ULONG_MAX.
>>>>>>>>>
>>>>>>>>> Note, this isn't technically necessary since proc_get_long() will already
>>>>>>>>> return ULONG_MAX. However, two reason why we still should do this:
>>>>>>>>> 1. it makes it explicit what the upper bound of file-max is instead of
>>>>>>>>>    making readers of the code infer it from proc_get_long() themselves
>>>>>>>>> 2. other tunebles than file-max may want to set a lower max value than
>>>>>>>>>    ULONG_MAX and we need to enable __do_proc_doulongvec_minmax() to handle
>>>>>>>>>    such cases too
>>>>>>>>>
>>>>>>>>> Cc: Kees Cook <keescook@chromium.org>
>>>>>>>>> Signed-off-by: Christian Brauner <christian@brauner.io>
>>>>>>>>> ---
>>>>>>>>> v0->v1:
>>>>>>>>> - if max value is < than ULONG_MAX use max as upper bound
>>>>>>>>> - (Dominik) remove double "the" from commit message
>>>>>>>>> ---
>>>>>>>>>  kernel/sysctl.c | 4 ++++
>>>>>>>>>  1 file changed, 4 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
>>>>>>>>> index 97551eb42946..226d4eaf4b0e 100644
>>>>>>>>> --- a/kernel/sysctl.c
>>>>>>>>> +++ b/kernel/sysctl.c
>>>>>>>>> @@ -127,6 +127,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 ulong_max = ULONG_MAX;
>>>>>>>>>  static int one_hundred = 100;
>>>>>>>>>  static int one_thousand = 1000;
>>>>>>>>>  #ifdef CONFIG_PRINTK
>>>>>>>>> @@ -1696,6 +1697,7 @@ static struct ctl_table fs_table[] = {
>>>>>>>>>  		.maxlen		= sizeof(files_stat.max_files),
>>>>>>>>>  		.mode		= 0644,
>>>>>>>>>  		.proc_handler	= proc_doulongvec_minmax,
>>>>>>>>> +		.extra2		= &ulong_max,
>>>>>>>> What is the point of having a maximum value of ULONG_MAX anyway? No
>>>>>>>> value you can put into a ulong type can be bigger than that.
>>>>>>> This is changed in the new code to LONG_MAX. See the full thread for
>>>>>>> context. There's also an additional explantion in the commit message.
>>>>>>>
>>>>>>>>>  	},
>>>>>>>>>  	{
>>>>>>>>>  		.procname	= "nr_open",
>>>>>>>>> @@ -2795,6 +2797,8 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
>>>>>>>>>  				break;
>>>>>>>>>  			if (neg)
>>>>>>>>>  				continue;
>>>>>>>>> +			if (max && val > *max)
>>>>>>>>> +				val = *max;
>>>>>>>>>  			val = convmul * val / convdiv;
>>>>>>>>>  			if ((min && val < *min) || (max && val > *max))
>>>>>>>>>  				continue;
>>>>>>>> This does introduce a change in behavior. Previously the out-of-bound
>>>>>>>> value is ignored, now it is capped at its maximum. This is a
>>>>>>>> user-visible change.
>>>>>>> Not completely true though. Try
>>>>>>>
>>>>>>> echo 18446744073709551616 > /proc/sys/fs/file-max
>>>>>>>
>>>>>>> on a system you find acceptable loosing.
>>>>>>> So this is an acceptable user-visible change I'd say. But I'm open to
>>>>>>> other suggestions.
>>>>>> I am not saying this is unacceptable. I just say this is a user-visible
>>>>>> change and so should be documented somehow. BTW, you cap the max value,
>>>>> Sure, I'll update linux manpages and I can CC stable on the next round.
>>>>>
>>>>>> but not the min value. So there is inconsistency. I would say you either
>>>>>> do both, or none of them.
>>>>> The min value is 0. I don't think it needs to be set explicitly. I just
>>>>> kept the max value because it is != ULONG_MAX but LONG_MAX for file-max.
>>>> I think you are making the change with just one use case in mind. This
>>>> is a generic function that can be used by many different callers. So any
>>>> change you make has to be applicable to all use cases. You just can't
>>>> assume min is always 0 in all the other use cases.
>>> So, any caller that calls {do_}proc_doulongvec_minmax() must want an
>>> unsigned long lest they are calling the wrong function.
>>> The smallest value for an unsigned long is 0. So if any caller wants to
>>> get into a situation where the caller needs to be capped they need to be
>>> able to set the value to lower than 0 which they can't since they are
>>> requesting an unsigned. So I'm not sure it makes sense.
>>>
>>> Christian
>> There may be use cases where the developer may want a min value that is
>> bigger than 0. As I said, you can't just make an assumption here.
> Sorry, I'm not following your argument anymore. Why exactly is this
> function not already handling this case and what exactly does break with
> my changs?  And what exactly do we gain from setting file-max to extra1
> = 0?

OK, I am not talking about the file-max case. Your patch caps the
maximum, but not the minimum. This is inconsistent as users will see
different behavior depending on whether the input value is bigger than
the maximum or smaller than the minimum.

Cheers,
Longman

Re: [PATCH v1 2/2] sysctl: handle overflow for file-max

[Christian Brauner]

On Tue, Oct 16, 2018 at 11:53:53AM -0400, Waiman Long wrote:
> On 10/16/2018 11:47 AM, Christian Brauner wrote:
> > On Tue, Oct 16, 2018 at 11:44:40AM -0400, Waiman Long wrote:
> >> On 10/16/2018 11:40 AM, Christian Brauner wrote:
> >>> On Tue, Oct 16, 2018 at 11:34:07AM -0400, Waiman Long wrote:
> >>>> On 10/16/2018 11:29 AM, Christian Brauner wrote:
> >>>>> On Tue, Oct 16, 2018 at 11:25:42AM -0400, Waiman Long wrote:
> >>>>>> On 10/16/2018 11:21 AM, Christian Brauner wrote:
> >>>>>>> On Tue, Oct 16, 2018 at 11:13:28AM -0400, Waiman Long wrote:
> >>>>>>>> On 10/15/2018 06:55 AM, Christian Brauner wrote:
> >>>>>>>>> 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 explicitly caps the value for file-max to ULONG_MAX.
> >>>>>>>>>
> >>>>>>>>> Note, this isn't technically necessary since proc_get_long() will already
> >>>>>>>>> return ULONG_MAX. However, two reason why we still should do this:
> >>>>>>>>> 1. it makes it explicit what the upper bound of file-max is instead of
> >>>>>>>>>    making readers of the code infer it from proc_get_long() themselves
> >>>>>>>>> 2. other tunebles than file-max may want to set a lower max value than
> >>>>>>>>>    ULONG_MAX and we need to enable __do_proc_doulongvec_minmax() to handle
> >>>>>>>>>    such cases too
> >>>>>>>>>
> >>>>>>>>> Cc: Kees Cook <keescook@chromium.org>
> >>>>>>>>> Signed-off-by: Christian Brauner <christian@brauner.io>
> >>>>>>>>> ---
> >>>>>>>>> v0->v1:
> >>>>>>>>> - if max value is < than ULONG_MAX use max as upper bound
> >>>>>>>>> - (Dominik) remove double "the" from commit message
> >>>>>>>>> ---
> >>>>>>>>>  kernel/sysctl.c | 4 ++++
> >>>>>>>>>  1 file changed, 4 insertions(+)
> >>>>>>>>>
> >>>>>>>>> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> >>>>>>>>> index 97551eb42946..226d4eaf4b0e 100644
> >>>>>>>>> --- a/kernel/sysctl.c
> >>>>>>>>> +++ b/kernel/sysctl.c
> >>>>>>>>> @@ -127,6 +127,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 ulong_max = ULONG_MAX;
> >>>>>>>>>  static int one_hundred = 100;
> >>>>>>>>>  static int one_thousand = 1000;
> >>>>>>>>>  #ifdef CONFIG_PRINTK
> >>>>>>>>> @@ -1696,6 +1697,7 @@ static struct ctl_table fs_table[] = {
> >>>>>>>>>  		.maxlen		= sizeof(files_stat.max_files),
> >>>>>>>>>  		.mode		= 0644,
> >>>>>>>>>  		.proc_handler	= proc_doulongvec_minmax,
> >>>>>>>>> +		.extra2		= &ulong_max,
> >>>>>>>> What is the point of having a maximum value of ULONG_MAX anyway? No
> >>>>>>>> value you can put into a ulong type can be bigger than that.
> >>>>>>> This is changed in the new code to LONG_MAX. See the full thread for
> >>>>>>> context. There's also an additional explantion in the commit message.
> >>>>>>>
> >>>>>>>>>  	},
> >>>>>>>>>  	{
> >>>>>>>>>  		.procname	= "nr_open",
> >>>>>>>>> @@ -2795,6 +2797,8 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int
> >>>>>>>>>  				break;
> >>>>>>>>>  			if (neg)
> >>>>>>>>>  				continue;
> >>>>>>>>> +			if (max && val > *max)
> >>>>>>>>> +				val = *max;
> >>>>>>>>>  			val = convmul * val / convdiv;
> >>>>>>>>>  			if ((min && val < *min) || (max && val > *max))
> >>>>>>>>>  				continue;
> >>>>>>>> This does introduce a change in behavior. Previously the out-of-bound
> >>>>>>>> value is ignored, now it is capped at its maximum. This is a
> >>>>>>>> user-visible change.
> >>>>>>> Not completely true though. Try
> >>>>>>>
> >>>>>>> echo 18446744073709551616 > /proc/sys/fs/file-max
> >>>>>>>
> >>>>>>> on a system you find acceptable loosing.
> >>>>>>> So this is an acceptable user-visible change I'd say. But I'm open to
> >>>>>>> other suggestions.
> >>>>>> I am not saying this is unacceptable. I just say this is a user-visible
> >>>>>> change and so should be documented somehow. BTW, you cap the max value,
> >>>>> Sure, I'll update linux manpages and I can CC stable on the next round.
> >>>>>
> >>>>>> but not the min value. So there is inconsistency. I would say you either
> >>>>>> do both, or none of them.
> >>>>> The min value is 0. I don't think it needs to be set explicitly. I just
> >>>>> kept the max value because it is != ULONG_MAX but LONG_MAX for file-max.
> >>>> I think you are making the change with just one use case in mind. This
> >>>> is a generic function that can be used by many different callers. So any
> >>>> change you make has to be applicable to all use cases. You just can't
> >>>> assume min is always 0 in all the other use cases.
> >>> So, any caller that calls {do_}proc_doulongvec_minmax() must want an
> >>> unsigned long lest they are calling the wrong function.
> >>> The smallest value for an unsigned long is 0. So if any caller wants to
> >>> get into a situation where the caller needs to be capped they need to be
> >>> able to set the value to lower than 0 which they can't since they are
> >>> requesting an unsigned. So I'm not sure it makes sense.
> >>>
> >>> Christian
> >> There may be use cases where the developer may want a min value that is
> >> bigger than 0. As I said, you can't just make an assumption here.
> > Sorry, I'm not following your argument anymore. Why exactly is this
> > function not already handling this case and what exactly does break with
> > my changs?  And what exactly do we gain from setting file-max to extra1
> > = 0?
> 
> OK, I am not talking about the file-max case. Your patch caps the
> maximum, but not the minimum. This is inconsistent as users will see
> different behavior depending on whether the input value is bigger than
> the maximum or smaller than the minimum.

So the difference for me is that without the max cap we run into
overflow issues because you can in fact overflow since some callers are
long int but doulongvec is unsigned long but we can't really run into
underflow issues since it is an unsigned long and proc_get_long() is
handling the case where there's a leading '-' correctly.
So there's no need to cap and change behavior there was well.
But I'll send out another version and we'll see what other people think
of this. We can always make this change in the next version.

> 
> Cheers,
> Longman