[Patch] sysctl: forbid too long numbers

[Patch] sysctl: forbid too long numbers

[Amerigo Wang]

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.

[I don't know whom I should Cc...]
Signed-off-by: WANG Cong <amwang@redhat.com>

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

Re: [Patch] sysctl: forbid too long numbers

[Andrew Morton]

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

Re: [Patch] sysctl: forbid too long numbers

[Lee Revell]

On Mon, Jun 29, 2009 at 6:01 PM, Andrew Morton<akpm@linux-foundation.org> wrote:
> 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.

This would break at least one existing setup that I know of.  Consider
an environment where shmmax is set to the value the biggest server
needs (well over 4GB) on all database servers to simplify management.
This change would cause the database to fail on the old 32 bit servers
used for testing and QA.

Lee

Re: [Patch] sysctl: forbid too long numbers

[Amerigo Wang]

Andrew Morton wrote:
> 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.
>
>   

Ah, I didn't consider this situation...
Hmm... but only taking the lower 32-bits really looks strange.

> 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.
>
>   

Yeah, maybe, but that is really tricky...
>
>   
>> 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()?
>
>   
Good points!
I agree, so maybe we only need to change this part?
Hmm, I need to check the callers of strict_strtol()...

Thank you!