| Message ID | 032e024f-2b1b-a980-1b53-d903bc8db297@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [next] sysctl: add proc_dointvec_jiffies_minmax to limit the min/max write value | expand |
Friendly ping... > From: Zhiqiang Liu <liuzhiqiang26@huawei.com> > > In proc_dointvec_jiffies func, the write value is only checked > whether it is larger than INT_MAX. If the write value is less > than zero, it can also be successfully writen in the data. > > However, in some scenarios, users would adopt the data to > set timers or check whether time is expired. Generally, the data > will be cast to an unsigned type variable, then the negative data > becomes a very large unsigned value, which leads to long waits > or other unpredictable problems. > > Here, we add a new func, proc_dointvec_jiffies_minmax, to limit the > min/max write value, which is similar to the proc_dointvec_minmax func. > > Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com> > Reported-by: Qiang Ning <ningqiang1@huawei.com> > Reviewed-by: Jie Liu <liujie165@huawei.com> > --- > include/linux/sysctl.h | 2 ++ > kernel/sysctl.c | 44 +++++++++++++++++++++++++++++++++++++++++++- > 2 files changed, 45 insertions(+), 1 deletion(-) > > diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h > index b769ecf..8bde8a0 100644 > --- a/include/linux/sysctl.h > +++ b/include/linux/sysctl.h > @@ -53,6 +53,8 @@ extern int proc_douintvec_minmax(struct ctl_table *table, int write, > loff_t *ppos); > extern int proc_dointvec_jiffies(struct ctl_table *, int, > void __user *, size_t *, loff_t *); > +extern int proc_dointvec_jiffies_minmax(struct ctl_table *, int, > + void __user *, size_t *, loff_t *); > extern int proc_dointvec_userhz_jiffies(struct ctl_table *, int, > void __user *, size_t *, loff_t *); > extern int proc_dointvec_ms_jiffies(struct ctl_table *, int, > diff --git a/kernel/sysctl.c b/kernel/sysctl.c > index c9ec050..8e1eb59 100644 > --- a/kernel/sysctl.c > +++ b/kernel/sysctl.c > @@ -2967,10 +2967,15 @@ static int do_proc_dointvec_jiffies_conv(bool *negp, unsigned long *lvalp, > int *valp, > int write, void *data) > { > + struct do_proc_dointvec_minmax_conv_param *param = data; > + > if (write) { > if (*lvalp > INT_MAX / HZ) > return 1; > *valp = *negp ? -(*lvalp*HZ) : (*lvalp*HZ); > + if ((param->min && (*param->min)*HZ > *valp) || > + (param->max && (*param->max)*HZ < *valp)) > + return -EINVAL; > } else { > int val = *valp; > unsigned long lval; > @@ -3053,7 +3058,37 @@ int proc_dointvec_jiffies(struct ctl_table *table, int write, > void __user *buffer, size_t *lenp, loff_t *ppos) > { > return do_proc_dointvec(table,write,buffer,lenp,ppos, > - do_proc_dointvec_jiffies_conv,NULL); > + do_proc_dointvec_jiffies_conv, NULL); > +} > + > +/** > + * proc_dointvec_jiffies_minmax - read a vector of integers as seconds with min/max values > + * @table: the sysctl table > + * @write: %TRUE if this is a write to the sysctl file > + * @buffer: the user buffer > + * @lenp: the size of the user buffer > + * @ppos: file position > + * > + * Reads/writes up to table->maxlen/sizeof(unsigned int) integer > + * values from/to the user buffer, treated as an ASCII string. > + * The values read are assumed to be in seconds, and are converted into > + * jiffies. > + * > + * This routine will ensure the values are within the range specified by > + * table->extra1 (min) and table->extra2 (max). > + * > + * Returns 0 on success or -EINVAL on write when the range check fails. > + */ > +int proc_dointvec_jiffies_minmax(struct ctl_table *table, int write, > + void __user *buffer, size_t *lenp, loff_t *ppos) > +{ > + struct do_proc_dointvec_minmax_conv_param param = { > + .min = (int *) table->extra1, > + .max = (int *) table->extra2, > + }; > + > + return do_proc_dointvec(table, write, buffer, lenp, ppos, > + do_proc_dointvec_jiffies_conv, ¶m); > } > > /** > @@ -3301,6 +3336,12 @@ int proc_dointvec_jiffies(struct ctl_table *table, int write, > return -ENOSYS; > } > > +int proc_dointvec_jiffies_minmax(struct ctl_table *table, int write, > + void __user *buffer, size_t *lenp, loff_t *ppos) > +{ > + return -ENOSYS; > +} > + > int proc_dointvec_userhz_jiffies(struct ctl_table *table, int write, > void __user *buffer, size_t *lenp, loff_t *ppos) > { > @@ -3359,6 +3400,7 @@ static int proc_dointvec_minmax_bpf_stats(struct ctl_table *table, int write, > EXPORT_SYMBOL(proc_dointvec); > EXPORT_SYMBOL(proc_douintvec); > EXPORT_SYMBOL(proc_dointvec_jiffies); > +EXPORT_SYMBOL(proc_dointvec_jiffies_minmax); > EXPORT_SYMBOL(proc_dointvec_minmax); > EXPORT_SYMBOL_GPL(proc_douintvec_minmax); > EXPORT_SYMBOL(proc_dointvec_userhz_jiffies); >
Friendly ping... 在 2019/4/24 12:04, Zhiqiang Liu 写道: > > Friendly ping... > >> From: Zhiqiang Liu <liuzhiqiang26@huawei.com> >> >> In proc_dointvec_jiffies func, the write value is only checked >> whether it is larger than INT_MAX. If the write value is less >> than zero, it can also be successfully writen in the data. >> >> However, in some scenarios, users would adopt the data to >> set timers or check whether time is expired. Generally, the data >> will be cast to an unsigned type variable, then the negative data >> becomes a very large unsigned value, which leads to long waits >> or other unpredictable problems. >> >> Here, we add a new func, proc_dointvec_jiffies_minmax, to limit the >> min/max write value, which is similar to the proc_dointvec_minmax func. >> >> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com> >> Reported-by: Qiang Ning <ningqiang1@huawei.com> >> Reviewed-by: Jie Liu <liujie165@huawei.com> >> --- >> include/linux/sysctl.h | 2 ++ >> kernel/sysctl.c | 44 +++++++++++++++++++++++++++++++++++++++++++- >> 2 files changed, 45 insertions(+), 1 deletion(-) >> >> diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h >> index b769ecf..8bde8a0 100644 >> --- a/include/linux/sysctl.h >> +++ b/include/linux/sysctl.h >> @@ -53,6 +53,8 @@ extern int proc_douintvec_minmax(struct ctl_table *table, int write, >> loff_t *ppos); >> extern int proc_dointvec_jiffies(struct ctl_table *, int, >> void __user *, size_t *, loff_t *); >> +extern int proc_dointvec_jiffies_minmax(struct ctl_table *, int, >> + void __user *, size_t *, loff_t *); >> extern int proc_dointvec_userhz_jiffies(struct ctl_table *, int, >> void __user *, size_t *, loff_t *); >> extern int proc_dointvec_ms_jiffies(struct ctl_table *, int, >> diff --git a/kernel/sysctl.c b/kernel/sysctl.c >> index c9ec050..8e1eb59 100644 >> --- a/kernel/sysctl.c >> +++ b/kernel/sysctl.c >> @@ -2967,10 +2967,15 @@ static int do_proc_dointvec_jiffies_conv(bool *negp, unsigned long *lvalp, >> int *valp, >> int write, void *data) >> { >> + struct do_proc_dointvec_minmax_conv_param *param = data; >> + >> if (write) { >> if (*lvalp > INT_MAX / HZ) >> return 1; >> *valp = *negp ? -(*lvalp*HZ) : (*lvalp*HZ); >> + if ((param->min && (*param->min)*HZ > *valp) || >> + (param->max && (*param->max)*HZ < *valp)) >> + return -EINVAL; >> } else { >> int val = *valp; >> unsigned long lval; >> @@ -3053,7 +3058,37 @@ int proc_dointvec_jiffies(struct ctl_table *table, int write, >> void __user *buffer, size_t *lenp, loff_t *ppos) >> { >> return do_proc_dointvec(table,write,buffer,lenp,ppos, >> - do_proc_dointvec_jiffies_conv,NULL); >> + do_proc_dointvec_jiffies_conv, NULL); >> +} >> + >> +/** >> + * proc_dointvec_jiffies_minmax - read a vector of integers as seconds with min/max values >> + * @table: the sysctl table >> + * @write: %TRUE if this is a write to the sysctl file >> + * @buffer: the user buffer >> + * @lenp: the size of the user buffer >> + * @ppos: file position >> + * >> + * Reads/writes up to table->maxlen/sizeof(unsigned int) integer >> + * values from/to the user buffer, treated as an ASCII string. >> + * The values read are assumed to be in seconds, and are converted into >> + * jiffies. >> + * >> + * This routine will ensure the values are within the range specified by >> + * table->extra1 (min) and table->extra2 (max). >> + * >> + * Returns 0 on success or -EINVAL on write when the range check fails. >> + */ >> +int proc_dointvec_jiffies_minmax(struct ctl_table *table, int write, >> + void __user *buffer, size_t *lenp, loff_t *ppos) >> +{ >> + struct do_proc_dointvec_minmax_conv_param param = { >> + .min = (int *) table->extra1, >> + .max = (int *) table->extra2, >> + }; >> + >> + return do_proc_dointvec(table, write, buffer, lenp, ppos, >> + do_proc_dointvec_jiffies_conv, ¶m); >> } >> >> /** >> @@ -3301,6 +3336,12 @@ int proc_dointvec_jiffies(struct ctl_table *table, int write, >> return -ENOSYS; >> } >> >> +int proc_dointvec_jiffies_minmax(struct ctl_table *table, int write, >> + void __user *buffer, size_t *lenp, loff_t *ppos) >> +{ >> + return -ENOSYS; >> +} >> + >> int proc_dointvec_userhz_jiffies(struct ctl_table *table, int write, >> void __user *buffer, size_t *lenp, loff_t *ppos) >> { >> @@ -3359,6 +3400,7 @@ static int proc_dointvec_minmax_bpf_stats(struct ctl_table *table, int write, >> EXPORT_SYMBOL(proc_dointvec); >> EXPORT_SYMBOL(proc_douintvec); >> EXPORT_SYMBOL(proc_dointvec_jiffies); >> +EXPORT_SYMBOL(proc_dointvec_jiffies_minmax); >> EXPORT_SYMBOL(proc_dointvec_minmax); >> EXPORT_SYMBOL_GPL(proc_douintvec_minmax); >> EXPORT_SYMBOL(proc_dointvec_userhz_jiffies); >> > > > . >
On Wed, May 15, 2019 at 10:53:55PM +0800, Zhiqiang Liu wrote: > Friendly ping... > > 在 2019/4/24 12:04, Zhiqiang Liu 写道: > > > > Friendly ping... Hi! (Please include akpm on CC for next versions of this, as he's likely the person to take this patch.) > > > >> From: Zhiqiang Liu <liuzhiqiang26@huawei.com> > >> > >> In proc_dointvec_jiffies func, the write value is only checked > >> whether it is larger than INT_MAX. If the write value is less > >> than zero, it can also be successfully writen in the data. This appears to be "be design", but I see many "unsigned int" users that might be tricked into giant values... (for example, see net/netfilter/nf_conntrack_standalone.c) Should proc_dointvec_jiffies() just be fixed to disallow negative values entirely? Looking at the implementation, it seems to be very intentional about accepting negative values. However, when I looked through a handful of proc_dointvec_jiffies() users, it looks like they're all expecting a positive value. Many in the networking subsystem are, in fact, writing to unsigned long variables, as I mentioned. Are there real-world cases of wanting to set a negative jiffie value via proc_dointvec_jiffies()? > >> > >> However, in some scenarios, users would adopt the data to > >> set timers or check whether time is expired. Generally, the data > >> will be cast to an unsigned type variable, then the negative data > >> becomes a very large unsigned value, which leads to long waits > >> or other unpredictable problems. > >> > >> Here, we add a new func, proc_dointvec_jiffies_minmax, to limit the > >> min/max write value, which is similar to the proc_dointvec_minmax func. > >> > >> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com> > >> Reported-by: Qiang Ning <ningqiang1@huawei.com> > >> Reviewed-by: Jie Liu <liujie165@huawei.com> If proc_dointvec_jiffies() can't just be fixed, where will the new function get used? It seems all the "unsigned int" users could benefit.
> On Wed, May 15, 2019 at 10:53:55PM +0800, Zhiqiang Liu wrote: > > (Please include akpm on CC for next versions of this, as he's likely > the person to take this patch.) Thanks for your advice. And sorry to reply you so late. >>>> In proc_dointvec_jiffies func, the write value is only checked >>>> whether it is larger than INT_MAX. If the write value is less >>>> than zero, it can also be successfully writen in the data. > > This appears to be "be design", but I see many "unsigned int" users > that might be tricked into giant values... (for example, see > net/netfilter/nf_conntrack_standalone.c) > > Should proc_dointvec_jiffies() just be fixed to disallow negative values > entirely? Looking at the implementation, it seems to be very intentional > about accepting negative values. > > However, when I looked through a handful of proc_dointvec_jiffies() > users, it looks like they're all expecting a positive value. Many in the > networking subsystem are, in fact, writing to unsigned long variables, > as I mentioned. > I totally agree with you. And I also cannot find an scenario that expects negative values. Consideing the "negative" scenario may be exist, I add the proc_dointvec_jiffies_minmax like proc_dointvec_minmax. > Are there real-world cases of wanting to set a negative jiffie value > via proc_dointvec_jiffies()? Until now, I do not find such cases. >>>> >>>> Here, we add a new func, proc_dointvec_jiffies_minmax, to limit the >>>> min/max write value, which is similar to the proc_dointvec_minmax func. >>>> > > If proc_dointvec_jiffies() can't just be fixed, where will the new > function get used? It seems all the "unsigned int" users could benefit. > I tend to add the proc_dointvec_jiffies_minmax func to provide more choices and not change the previous use of proc_dointvec_jiffies func. Thanks for your reply again.
friendly ping ... On 2019/6/4 23:27, Zhiqiang Liu wrote: >> On Wed, May 15, 2019 at 10:53:55PM +0800, Zhiqiang Liu wrote: >> >> (Please include akpm on CC for next versions of this, as he's likely >> the person to take this patch.) > Thanks for your advice. And sorry to reply you so late. > >>>>> In proc_dointvec_jiffies func, the write value is only checked >>>>> whether it is larger than INT_MAX. If the write value is less >>>>> than zero, it can also be successfully writen in the data. >> >> This appears to be "be design", but I see many "unsigned int" users >> that might be tricked into giant values... (for example, see >> net/netfilter/nf_conntrack_standalone.c) >> >> Should proc_dointvec_jiffies() just be fixed to disallow negative values >> entirely? Looking at the implementation, it seems to be very intentional >> about accepting negative values. >> >> However, when I looked through a handful of proc_dointvec_jiffies() >> users, it looks like they're all expecting a positive value. Many in the >> networking subsystem are, in fact, writing to unsigned long variables, >> as I mentioned. >> > I totally agree with you. And I also cannot find an scenario that expects > negative values. Consideing the "negative" scenario may be exist, I add the > proc_dointvec_jiffies_minmax like proc_dointvec_minmax. > >> Are there real-world cases of wanting to set a negative jiffie value >> via proc_dointvec_jiffies()? > Until now, I do not find such cases. > >>>>> >>>>> Here, we add a new func, proc_dointvec_jiffies_minmax, to limit the >>>>> min/max write value, which is similar to the proc_dointvec_minmax func. >>>>> >> >> If proc_dointvec_jiffies() can't just be fixed, where will the new >> function get used? It seems all the "unsigned int" users could benefit. >> > I tend to add the proc_dointvec_jiffies_minmax func to provide more choices and > not change the previous use of proc_dointvec_jiffies func. > > Thanks for your reply again. > > > . >
On Tue, Jun 04, 2019 at 11:27:51PM +0800, Zhiqiang Liu wrote: > > On Wed, May 15, 2019 at 10:53:55PM +0800, Zhiqiang Liu wrote: > >>>> In proc_dointvec_jiffies func, the write value is only checked > >>>> whether it is larger than INT_MAX. If the write value is less > >>>> than zero, it can also be successfully writen in the data. > > > > This appears to be "be design", but I see many "unsigned int" users > > that might be tricked into giant values... (for example, see > > net/netfilter/nf_conntrack_standalone.c) > > > > Should proc_dointvec_jiffies() just be fixed to disallow negative values > > entirely? Looking at the implementation, it seems to be very intentional > > about accepting negative values. > > > > However, when I looked through a handful of proc_dointvec_jiffies() > > users, it looks like they're all expecting a positive value. Many in the > > networking subsystem are, in fact, writing to unsigned long variables, > > as I mentioned. > > > I totally agree with you. And I also cannot find an scenario that expects > negative values. Consideing the "negative" scenario may be exist, I add the > proc_dointvec_jiffies_minmax like proc_dointvec_minmax. If no negative values exist, and there is no real point to it, then just rename the existing one and update the docs. Luis
diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h index b769ecf..8bde8a0 100644 --- a/include/linux/sysctl.h +++ b/include/linux/sysctl.h @@ -53,6 +53,8 @@ extern int proc_douintvec_minmax(struct ctl_table *table, int write, loff_t *ppos); extern int proc_dointvec_jiffies(struct ctl_table *, int, void __user *, size_t *, loff_t *); +extern int proc_dointvec_jiffies_minmax(struct ctl_table *, int, + void __user *, size_t *, loff_t *); extern int proc_dointvec_userhz_jiffies(struct ctl_table *, int, void __user *, size_t *, loff_t *); extern int proc_dointvec_ms_jiffies(struct ctl_table *, int, diff --git a/kernel/sysctl.c b/kernel/sysctl.c index c9ec050..8e1eb59 100644 --- a/kernel/sysctl.c +++ b/kernel/sysctl.c @@ -2967,10 +2967,15 @@ static int do_proc_dointvec_jiffies_conv(bool *negp, unsigned long *lvalp, int *valp, int write, void *data) { + struct do_proc_dointvec_minmax_conv_param *param = data; + if (write) { if (*lvalp > INT_MAX / HZ) return 1; *valp = *negp ? -(*lvalp*HZ) : (*lvalp*HZ); + if ((param->min && (*param->min)*HZ > *valp) || + (param->max && (*param->max)*HZ < *valp)) + return -EINVAL; } else { int val = *valp; unsigned long lval; @@ -3053,7 +3058,37 @@ int proc_dointvec_jiffies(struct ctl_table *table, int write, void __user *buffer, size_t *lenp, loff_t *ppos) { return do_proc_dointvec(table,write,buffer,lenp,ppos, - do_proc_dointvec_jiffies_conv,NULL); + do_proc_dointvec_jiffies_conv, NULL); +} + +/** + * proc_dointvec_jiffies_minmax - read a vector of integers as seconds with min/max values + * @table: the sysctl table + * @write: %TRUE if this is a write to the sysctl file + * @buffer: the user buffer + * @lenp: the size of the user buffer + * @ppos: file position + * + * Reads/writes up to table->maxlen/sizeof(unsigned int) integer + * values from/to the user buffer, treated as an ASCII string. + * The values read are assumed to be in seconds, and are converted into + * jiffies. + * + * This routine will ensure the values are within the range specified by + * table->extra1 (min) and table->extra2 (max). + * + * Returns 0 on success or -EINVAL on write when the range check fails. + */ +int proc_dointvec_jiffies_minmax(struct ctl_table *table, int write, + void __user *buffer, size_t *lenp, loff_t *ppos) +{ + struct do_proc_dointvec_minmax_conv_param param = { + .min = (int *) table->extra1, + .max = (int *) table->extra2, + }; + + return do_proc_dointvec(table, write, buffer, lenp, ppos, + do_proc_dointvec_jiffies_conv, ¶m); } /** @@ -3301,6 +3336,12 @@ int proc_dointvec_jiffies(struct ctl_table *table, int write, return -ENOSYS; } +int proc_dointvec_jiffies_minmax(struct ctl_table *table, int write, + void __user *buffer, size_t *lenp, loff_t *ppos) +{ + return -ENOSYS; +} + int proc_dointvec_userhz_jiffies(struct ctl_table *table, int write, void __user *buffer, size_t *lenp, loff_t *ppos) { @@ -3359,6 +3400,7 @@ static int proc_dointvec_minmax_bpf_stats(struct ctl_table *table, int write, EXPORT_SYMBOL(proc_dointvec); EXPORT_SYMBOL(proc_douintvec); EXPORT_SYMBOL(proc_dointvec_jiffies); +EXPORT_SYMBOL(proc_dointvec_jiffies_minmax); EXPORT_SYMBOL(proc_dointvec_minmax); EXPORT_SYMBOL_GPL(proc_douintvec_minmax); EXPORT_SYMBOL(proc_dointvec_userhz_jiffies);