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@xxxxxxxxxx> 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