Messages in this thread | | | Date | Fri, 6 Feb 2009 00:19:34 +0300 | From | Alexey Dobriyan <> | Subject | Re: [PATCH] sysctl: min-max range check is broken |
| |
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.
| |