[PATCH] sysctl: min-max range check is broken

[PATCH] sysctl: min-max range check is broken

[Shakesh Jain, ShakeshJain]

do_proc_dointvec_minmax_conv() which gets callled from
proc_dointvec_minmax proc_handler doesn't increment the pointer to
the 'min' (extra1) and 'max' (extra2) after each range check which
results in doing the check against same set of min and max values.

This breaks the range checking for those sysctl's where you can
write multiple values to /proc with each variable having its own range
specification.

It seems to be implemented for the sysctl() system call strategy in
sysctl_intvec() where min and max are treated as arrays.

Signed-off-by: Shakesh Jain <shjain@akamai.com>
---
 kernel/sysctl.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
========================================================================
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 368d163..50bffcd 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2377,8 +2377,8 @@ static int do_proc_dointvec_minmax_conv(int *negp, unsigned long *lvalp,
 	struct do_proc_dointvec_minmax_conv_param *param = data;
 	if (write) {
 		int val = *negp ? -*lvalp : *lvalp;
-		if ((param->min && *param->min > val) ||
-		    (param->max && *param->max < val))
+		if ((param->min && *(param->min++) > val) ||
+		    (param->max && *(param->max++) < val))
 			return -EINVAL;
 		*valp = val;
 	} else {

Re: [PATCH] sysctl: min-max range check is broken

[Andrew Morton]

On Wed, 4 Feb 2009 00:40:22 -0800
Shakesh Jain <shjain@akamai.com>, ShakeshJain@akamai.com wrote:

> do_proc_dointvec_minmax_conv() which gets callled from
> proc_dointvec_minmax proc_handler doesn't increment the pointer to
> the 'min' (extra1) and 'max' (extra2) after each range check which
> results in doing the check against same set of min and max values.
> 
> This breaks the range checking for those sysctl's where you can
> write multiple values to /proc with each variable having its own range
> specification.
> 
> It seems to be implemented for the sysctl() system call strategy in
> sysctl_intvec() where min and max are treated as arrays.
> 
> Signed-off-by: Shakesh Jain <shjain@akamai.com>
> ---
>  kernel/sysctl.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> ========================================================================
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 368d163..50bffcd 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -2377,8 +2377,8 @@ static int do_proc_dointvec_minmax_conv(int *negp, unsigned long *lvalp,
>  	struct do_proc_dointvec_minmax_conv_param *param = data;
>  	if (write) {
>  		int val = *negp ? -*lvalp : *lvalp;
> -		if ((param->min && *param->min > val) ||
> -		    (param->max && *param->max < val))
> +		if ((param->min && *(param->min++) > val) ||
> +		    (param->max && *(param->max++) < val))
>  			return -EINVAL;
>  		*valp = val;
>  	} else {

Scary code.

It will unconditionally increment param->min.

But it will only increment param->max if the (*param->min > val) test
succeeded.

Is this really the intended and correct behaviour?  It seems odd.

Even if it _is_ correct, can the code be rearranged to be less scary?

Re: [PATCH] sysctl: min-max range check is broken

[Eric W. Biederman]

Andrew Morton <akpm@linux-foundation.org> writes:

> On Wed, 4 Feb 2009 00:40:22 -0800
> Shakesh Jain <shjain@akamai.com>, ShakeshJain@akamai.com wrote:
>
>> do_proc_dointvec_minmax_conv() which gets callled from
>> proc_dointvec_minmax proc_handler doesn't increment the pointer to
>> the 'min' (extra1) and 'max' (extra2) after each range check which
>> results in doing the check against same set of min and max values.
>> 
>> This breaks the range checking for those sysctl's where you can
>> write multiple values to /proc with each variable having its own range
>> specification.

I just did a quick grep for .extra1 and I don't see anywhere we use
the code as described.

>> It seems to be implemented for the sysctl() system call strategy in
>> sysctl_intvec() where min and max are treated as arrays.

Yep.  There is an inconsistency here.  Given how sysctl is used and
tested, and the fact I could not find where it appears that anyone is
passing an array into min/max I would say that the proc version is
correct and the sysctl version is wrong.

The untested patch below looks like it will fix the this.

I don't know if there are any cases where we use minmax with an array
of integers but I don't see the point of using an array of minmax
values at this point.  For the original sysctl design it may have made
some sense.  New code should be one value per file.

Eric



diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 790f9d7..4050ce1 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2916,9 +2916,9 @@ int sysctl_intvec(struct ctl_table *table,
 			int value;
 			if (get_user(value, vec + i))
 				return -EFAULT;
-			if (min && value < min[i])
+			if (min && value < *min)
 				return -EINVAL;
-			if (max && value > max[i])
+			if (max && value > *max)
 				return -EINVAL;
 		}
 	}

Re: [PATCH] sysctl: min-max range check is broken

[Alexey Dobriyan]

On Thu, Feb 05, 2009 at 12:55:56PM -0800, Eric W. Biederman wrote:
> Andrew Morton <akpm@linux-foundation.org> writes:
> 
> > On Wed, 4 Feb 2009 00:40:22 -0800
> > Shakesh Jain <shjain@akamai.com>, ShakeshJain@akamai.com wrote:
> >
> >> do_proc_dointvec_minmax_conv() which gets callled from
> >> proc_dointvec_minmax proc_handler doesn't increment the pointer to
> >> the 'min' (extra1) and 'max' (extra2) after each range check which
> >> results in doing the check against same set of min and max values.
> >> 
> >> This breaks the range checking for those sysctl's where you can
> >> write multiple values to /proc with each variable having its own range
> >> specification.
> 
> I just did a quick grep for .extra1 and I don't see anywhere we use
> the code as described.
> 
> >> It seems to be implemented for the sysctl() system call strategy in
> >> sysctl_intvec() where min and max are treated as arrays.
> 
> Yep.  There is an inconsistency here.  Given how sysctl is used and
> tested, and the fact I could not find where it appears that anyone is
> passing an array into min/max I would say that the proc version is
> correct and the sysctl version is wrong.
> 
> The untested patch below looks like it will fix the this.
> 
> I don't know if there are any cases where we use minmax with an array
> of integers but I don't see the point of using an array of minmax
> values at this point.

Multiple values, each one is bounded by it's own set of values.

> For the original sysctl design it may have made
> some sense.  New code should be one value per file.

So leave them alone.

> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -2916,9 +2916,9 @@ int sysctl_intvec(struct ctl_table *table,
>  			int value;
>  			if (get_user(value, vec + i))
>  				return -EFAULT;
> -			if (min && value < min[i])
> +			if (min && value < *min)
>  				return -EINVAL;
> -			if (max && value > max[i])
> +			if (max && value > *max)
>  				return -EINVAL;

Correct way is:

	min[i] <= val[i] <= max[i]

This is what sysctl_intvec() does.

Re: [PATCH] sysctl: min-max range check is broken

[Eric W. Biederman]

Alexey Dobriyan <adobriyan@gmail.com> writes:

>> Yep.  There is an inconsistency here.  Given how sysctl is used and
>> tested, and the fact I could not find where it appears that anyone is
>> passing an array into min/max I would say that the proc version is
>> correct and the sysctl version is wrong.
>> 
>> The untested patch below looks like it will fix the this.
>> 
>> I don't know if there are any cases where we use minmax with an array
>> of integers but I don't see the point of using an array of minmax
>> values at this point.
>
> Multiple values, each one is bounded by it's own set of values.

Nack.  That is a change to the users of sysctl.   An array
of minmax values appears to be a brand new extension therefore
we should not make this change.

If we want new behavior we should introduce new helpers.

>> For the original sysctl design it may have made
>> some sense.  New code should be one value per file.
>
> So leave them alone.

Sure. My point is that we don't care about this for new code.
Old code was tested and used with the proc version.

NO ONE uses sys_sysctl NO ONE.

>> --- a/kernel/sysctl.c
>> +++ b/kernel/sysctl.c
>> @@ -2916,9 +2916,9 @@ int sysctl_intvec(struct ctl_table *table,
>>  			int value;
>>  			if (get_user(value, vec + i))
>>  				return -EFAULT;
>> -			if (min && value < min[i])
>> +			if (min && value < *min)
>>  				return -EINVAL;
>> -			if (max && value > max[i])
>> +			if (max && value > *max)
>>  				return -EINVAL;
>
> Correct way is:
>
> 	min[i] <= val[i] <= max[i]
>
> This is what sysctl_intvec() does.

Therefore sysctl_intvec is wrong because it does not match
proc_dointvec_minmax.  proc_dointvec_minmax is definitive as that what
people use and test.  sysctl_intvec is a vestigial appendage that
was never really used, and it is already deprecated and scheduled for
remove because of this.

The only way to support the case of arrays of minmax values is to
show code that cares.  I did not see anyone passing in an array
in .extra one.  So you are advocating code that will walk around
and read random values from the kernels .data section.  Which
is VERY broken.

Eric