Date: Mon, 29 Jun 2009 15:01:23 -0700
From: Andrew Morton <akpm@linux-foundation.org>
To: Amerigo Wang <amwang@redhat.com>
Cc: linux-kernel@vger.kernel.org, amwang@redhat.com
Subject: Re: [Patch] sysctl: forbid too long numbers
Message-Id: <20090629150123.43f79091.akpm@linux-foundation.org>
In-Reply-To: <20090619072212.6519.10915.sendpatchset@localhost.localdomain>
References: <20090619072212.6519.10915.sendpatchset@localhost.localdomain>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3308
Lines: 106

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()?


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/