On Tue, Oct 16, 2018 at 12:53 PM, 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
> 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
Can you add this as kerndoc above strtoul_lenient()? (New people
reading this code should be able to tell quickly what it's lenient
about, and how it behaves without have to know the internals of
kstrtox.h.)
> 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.
Yup -- I think this makes a lot of sense.
>
> Cc: Kees Cook <keescook@xxxxxxxxxxxx>
> Signed-off-by: Christian Brauner <christian@xxxxxxxxxx>
> ---
> v1->v2:
> - s/sysctl_cap_erange/sysctl_lenient/g
> - consistenly fail on overflow
> 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 | 24 +++++++++++++++++++++++-
> 1 file changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index cc02050fd0c4..7d98e02e5d72 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>
Should this be "../lib/kstrtox.h" instead of <>?
>
> #include <linux/uaccess.h>
> #include <asm/processor.h>
> @@ -2065,6 +2066,26 @@ static void proc_skip_char(char **buf, size_t *size, const char v)
> }
> }
>
> +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
> @@ -2108,7 +2129,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.17.1
>
With kerndoc added, please consider this acked by me:
Acked-by: Kees Cook <keescook@xxxxxxxxxxxx>
-Kees
--
Kees Cook
Pixel Security
