Re: [Patch] sysctl: forbid too long numbers

Andrew Morton
2009-06-29T22:01:46Z

On Fri, 19 Jun 2009 03:19:57 -0400
Amerigo Wang <amwang@redhat.com> wrote:

>
> For some file under /proc/sys/kernel, the valid values for them
> are < ULONG_MAX if they don't have they own max value defined. Thus,
> numbers longer than are invalid.
>
> So use strict_strtoul() instead of simple_strtoul(), and
> make strict_strtoul() more strict.
>

The changelog is a bit puzzling.

I think that what you're saying is that we can do

echo 9999999999999999999999999999999999999999999 > /proc/whatever

and that the kernel will take the lower 32 (or 64) bits of that number
and will accept this without error?

If so then I expect there are zillions of procfs/sysfs/debugfs/etc
files which have the same problem?

Also, fixing this is a non-backward-compatible change which could break
existing userspace. Although the chances of this seem fairly small.

Or are they? One could imagine a script which was tested and developed
on a 64-bit system, which writes a >4G number into a pseudo file. That
script happens to work on 32-bit systems (it might not work _well_, but
it'll work). With this change, the write will fail on the 32-bit
system and the entire application could bale out or something.

I'm not saying that this is a reason to avoid making the change, but
it's all a worry and needs consideration.

> ---
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index ab462b9..bc27e00 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -2331,7 +2331,8 @@ static int __do_proc_dointvec(void *tbl_data, struct ctl_table *table,
> if (*p < '0' || *p > '9')
> break;
>
> - lval = simple_strtoul(p, &p, 0);
> + if (strict_strtoul(p, 0, &lval))
> + return -EINVAL;
>
> len = p-buf;
> if ((len < left) && *p && !isspace(*p))

The other worrisome thing about this change is that there may well be
existing userspace which does

echo 42foo > /proc/whatever

and the conversion to strict_strtoul() will cause that script to
newly fail.

And the chances that there are scripts which do this are pretty darned
good - it's fairly easy to accidentally leave junk like this in strings
when hacking stuff together in scripting languages.

> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 756ccaf..ff2ca5c 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -163,11 +163,14 @@ int strict_strtoul(const char *cp, unsigned int base, unsigned long *res)
> char *tail;
> unsigned long val;
> size_t len;
> + char tmp[32];
>
> *res = 0;
> len = strlen(cp);
> if (len == 0)
> return -EINVAL;
> + if (len > snprintf(tmp, "%ld", ULONG_MAX))
> + return -EINVAL;
>
> val = simple_strtoul(cp, &tail, base);
> if (tail == cp)

And here we're doing a check for that overflow, yes?

- We don't need tmp[]. vsnprintf(NULL, ...) can be used to query the
length of an sprintf. See how kvasprintf() does this.

- The strict_strtoul() documentation should be updated?

- The above change affects strict_strtol() too.

- The same change should be made to strict_strtoull() and hence
strict_strtoll()?