* [PATCH] proc/sysctl: fix return error for proc_doulongvec_minmax @ 2018-11-30 6:35 Cheng Lin 2018-11-30 19:14 ` Luis Chamberlain 0 siblings, 1 reply; 5+ messages in thread From: Cheng Lin @ 2018-11-30 6:35 UTC (permalink / raw) To: mcgrof Cc: keescook, linux-kernel, linux-fsdevel, zhong.weidong, wang.yi59, Cheng Lin If the number of input parameters is less than the total parameters, an INVAL error will be returned. This patch ensure no error returned in this condition, just like other interfaces do. Signed-off-by: Cheng Lin <cheng.lin130@zte.com.cn> --- kernel/sysctl.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/kernel/sysctl.c b/kernel/sysctl.c index 5fc724e..9ee261f 100644 --- a/kernel/sysctl.c +++ b/kernel/sysctl.c @@ -2779,6 +2779,8 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int bool neg; left -= proc_skip_spaces(&p); + if (!left) + break; err = proc_get_long(&p, &left, &val, &neg, proc_wspace_sep, -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] proc/sysctl: fix return error for proc_doulongvec_minmax 2018-11-30 6:35 [PATCH] proc/sysctl: fix return error for proc_doulongvec_minmax Cheng Lin @ 2018-11-30 19:14 ` Luis Chamberlain [not found] ` <201812031312398404610@zte.com.cn> 0 siblings, 1 reply; 5+ messages in thread From: Luis Chamberlain @ 2018-11-30 19:14 UTC (permalink / raw) To: Cheng Lin; +Cc: keescook, linux-kernel, linux-fsdevel, zhong.weidong, wang.yi59 Cheng, thanks for the patch! On Fri, Nov 30, 2018 at 02:35:17PM +0800, Cheng Lin wrote: > If the number of input parameters is less than the total > parameters, an INVAL error will be returned. Do you mean EINVAL? > This patch ensure no error returned in this condition, just > like other interfaces do. Have an actual example to reproduce? Luis > Signed-off-by: Cheng Lin <cheng.lin130@zte.com.cn> > --- > kernel/sysctl.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/kernel/sysctl.c b/kernel/sysctl.c > index 5fc724e..9ee261f 100644 > --- a/kernel/sysctl.c > +++ b/kernel/sysctl.c > @@ -2779,6 +2779,8 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int > bool neg; > > left -= proc_skip_spaces(&p); > + if (!left) > + break; > > err = proc_get_long(&p, &left, &val, &neg, > proc_wspace_sep, > -- > 1.8.3.1 > ^ permalink raw reply [flat|nested] 5+ messages in thread
[parent not found: <201812031312398404610@zte.com.cn>]
* Re: Re: [PATCH] proc/sysctl: fix return error for proc_doulongvec_minmax [not found] ` <201812031312398404610@zte.com.cn> @ 2018-12-03 20:14 ` Luis Chamberlain [not found] ` <201812051510071985717@zte.com.cn> 2018-12-05 23:30 ` Re: [PATCH] proc/sysctl: fix return error for proc_doulongvec_minmax Kees Cook 0 siblings, 2 replies; 5+ messages in thread From: Luis Chamberlain @ 2018-12-03 20:14 UTC (permalink / raw) To: cheng.lin130, keescook, akpm, ebiederm Cc: linux-kernel, linux-fsdevel, zhong.weidong, wang.yi59 On Mon, Dec 03, 2018 at 01:12:39PM +0800, cheng.lin130@zte.com.cn wrote: > >Cheng, thanks for the patch! > > > >On Fri, Nov 30, 2018 at 02:35:17PM +0800, Cheng Lin wrote: > >> If the number of input parameters is less than the total > >> parameters, an INVAL error will be returned. > > > >Do you mean EINVAL? > > > Yes, it's EINVAL. Please adjust the commit log. > >> This patch ensure no error returned in this condition, just > >> like other interfaces do. > > > >Have an actual example to reproduce? > > > >Luis > > > We use proc_doulongvec_minmax to pass up to two parameters with kern_table. > e.g. > { > .procname = "monitor_signals", > .data = &monitor_sigs, > .maxlen = 2*sizeof(unsigned long), > .mode = 0644, > .proc_handler = proc_doulongvec_minmax, > }, > > Reproduce: > When passing two parameters, it's work normal. But passing only one parameter, an error "Invalid argument"(EINVAL) is returned. > [root@cl150 ~]# echo 1 2 > /proc/sys/kernel/monitor_signals > [root@cl150 ~]# cat /proc/sys/kernel/monitor_signals > 1 2 > [root@cl150 ~]# echo 3 > /proc/sys/kernel/monitor_signals > -bash: echo: write error: Invalid argument > [root@cl150 ~]# echo $? > 1 > [root@cl150 ~]# cat /proc/sys/kernel/monitor_signals > 3 2 > [root@cl150 ~]# > > The following is the result after apply this patch. No error is returned when the number of input parameters is less than the total parameters. > [root@cl150 ~]# echo 1 2 > /proc/sys/kernel/monitor_signals > [root@cl150 ~]# cat /proc/sys/kernel/monitor_signals > 1 2 > [root@cl150 ~]# echo 3 > /proc/sys/kernel/monitor_signals > [root@cl150 ~]# echo $? > 0 > [root@cl150 ~]# cat /proc/sys/kernel/monitor_signals > 3 2 > [root@cl150 ~]# This would be good to have in the commit log as well. But your patch only addresses one of the proc users, there are a few other checks like this that would also need to be expanded for this. So please expand your patch to cover the other cases as well. Since this worked before I do agree that we need to keep it working now, and I can't think of an issue with returning 0 now. Since this is about semantics though I'd like a bit more review from at last one more person. Kees, Eric, Andrew? Luis > Cheng > > >> Signed-off-by: Cheng Lin <cheng.lin130@zte.com.cn> > >> --- > >> kernel/sysctl.c | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >> diff --git a/kernel/sysctl.c b/kernel/sysctl.c > >> index 5fc724e..9ee261f 100644 > >> --- a/kernel/sysctl.c > >> +++ b/kernel/sysctl.c > >> @@ -2779,6 +2779,8 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int > >> bool neg; > >> > >> left -= proc_skip_spaces(&p); > >> + if (!left) > >> + break; > >> > >> err = proc_get_long(&p, &left, &val, &neg, > >> proc_wspace_sep, > >> -- > >> 1.8.3.1 > >> ^ permalink raw reply [flat|nested] 5+ messages in thread
[parent not found: <201812051510071985717@zte.com.cn>]
* Re: Re: Re: [PATCH] proc/sysctl: fix return error forproc_doulongvec_minmax [not found] ` <201812051510071985717@zte.com.cn> @ 2018-12-05 18:08 ` Luis Chamberlain 0 siblings, 0 replies; 5+ messages in thread From: Luis Chamberlain @ 2018-12-05 18:08 UTC (permalink / raw) To: cheng.lin130 Cc: keescook, akpm, ebiederm, linux-kernel, linux-fsdevel, zhong.weidong, wang.yi59 On Wed, Dec 05, 2018 at 03:10:07PM +0800, cheng.lin130@zte.com.cn wrote: > > On Mon, Dec 03, 2018 at 01:12:39PM +0800, cheng.lin130@zte.com.cn wrote: > > > >Cheng, thanks for the patch! > > > > > > > >On Fri, Nov 30, 2018 at 02:35:17PM +0800, Cheng Lin wrote: > > > >> If the number of input parameters is less than the total > > > >> parameters, an INVAL error will be returned. > > > > > > > >Do you mean EINVAL? > > > > > > > Yes, it's EINVAL. > > > > Please adjust the commit log. > > > > > >> This patch ensure no error returned in this condition, just > > > >> like other interfaces do. > > > > > > > >Have an actual example to reproduce? > > > > > > > >Luis > > > > > > > We use proc_doulongvec_minmax to pass up to two parameters with kern_table. > > > e.g. > > { > > > .procname = "monitor_signals", > > > .data = &monitor_sigs, > > > .maxlen = 2*sizeof(unsigned long), > > > .mode = 0644, > > > .proc_handler = proc_doulongvec_minmax, > > > }, > > > > > > Reproduce: > > > When passing two parameters, it's work normal. But passing only one parameter, an error "Invalid argument"(EINVAL) is returned. > > > [root@cl150 ~]# echo 1 2 > /proc/sys/kernel/monitor_signals > > > [root@cl150 ~]# cat /proc/sys/kernel/monitor_signals > > > 1 2 > > > [root@cl150 ~]# echo 3 > /proc/sys/kernel/monitor_signals > > > -bash: echo: write error: Invalid argument > > > [root@cl150 ~]# echo $? > > > 1 > > > [root@cl150 ~]# cat /proc/sys/kernel/monitor_signals > > > 3 2 > > > [root@cl150 ~]# > > > > > > The following is the result after apply this patch. No error is returned when the number of input parameters is less than the total parameters. > > > [root@cl150 ~]# echo 1 2 > /proc/sys/kernel/monitor_signals > > > [root@cl150 ~]# cat /proc/sys/kernel/monitor_signals > > > 1 2 > > > [root@cl150 ~]# echo 3 > /proc/sys/kernel/monitor_signals > > > [root@cl150 ~]# echo $? > > > 0 > > > [root@cl150 ~]# cat /proc/sys/kernel/monitor_signals > > > 3 2 > > > [root@cl150 ~]# > > > > This would be good to have in the commit log as well. But your patch > > only addresses one of the proc users, there are a few other checks like > > this that would also need to be expanded for this. So please expand > > your patch to cover the other cases as well. > > I have done the check for the interfaces exported in kernel/sysctl.c. > EXPORT_SYMBOL(proc_dointvec); > EXPORT_SYMBOL(proc_douintvec); > EXPORT_SYMBOL(proc_dointvec_jiffies); > EXPORT_SYMBOL(proc_dointvec_minmax); > EXPORT_SYMBOL_GPL(proc_douintvec_minmax); > EXPORT_SYMBOL(proc_dointvec_userhz_jiffies); > EXPORT_SYMBOL(proc_dointvec_ms_jiffies); > EXPORT_SYMBOL(proc_dostring); > EXPORT_SYMBOL(proc_doulongvec_minmax); > EXPORT_SYMBOL(proc_doulongvec_ms_jiffies_minmax); > > The function call relationship is as follows. There are three processing functions dealing with digital parameters, __do_proc_dointvec/__do_proc_douintvec/__do_proc_doulongvec_minmax. > > proc_dointvec------------------------------| > proc_dointvec_jiffies----------------------| > proc_dointvec_minmax------------------| > proc_dointvec_userhz_jiffies------------| > proc_dointvec_ms_jiffies-----------------|-> do_proc_dointvec----|-> __do_proc_dointvec > > proc_douintvec-----------------------------| > proc_douintvec_minmax-----------------|-> do_proc_douintvec---|-> __do_proc_douintvec > > proc_doulongvec_minmax---------------| > proc_doulongvec_ms_jiffies_minmax--|-> do_proc_doulongvec_minmax----|-> __do_proc_doulongvec_minmax OK > This patch deals with __do_proc_doulongvec_minmax, just as > __do_proc_dointvec does, adding a check for parameters 'left'. In > __do_proc_douintvec, its code implementation explicitly does not > support multiple inputs. static int __do_proc_douintvec(...){ > ... > /* > * Arrays are not supported, keep this simple. *Do not* add > * support for them. > */ > if (vleft != 1) { > *lenp = 0; > return -EINVAL; > ... > } > > > So, just __do_proc_doulongvec_minmax has the problem. And most use of > proc_doulongvec_minmax/proc_doulongvec_ms_jiffies_minmax just have one > parameter. The above text, up to my OK, is useful information for the commit log, please add that. > It's well hidden. You mean that the issue is not widely spread? If so please add that comment to the commit log, and resubmit a v2. Luis > Typical multi-parameter applications for > proc_dointvec, such as /proc/sys/kernel/printk. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Re: [PATCH] proc/sysctl: fix return error for proc_doulongvec_minmax 2018-12-03 20:14 ` Luis Chamberlain [not found] ` <201812051510071985717@zte.com.cn> @ 2018-12-05 23:30 ` Kees Cook 1 sibling, 0 replies; 5+ messages in thread From: Kees Cook @ 2018-12-05 23:30 UTC (permalink / raw) To: Luis R. Rodriguez, cheng.lin130 Cc: Andrew Morton, Eric W. Biederman, LKML, linux-fsdevel, zhong.weidong, wang.yi59, Michael Kerrisk On Mon, Dec 3, 2018 at 12:14 PM Luis Chamberlain <mcgrof@kernel.org> wrote: > Since this worked before I do agree that we need to keep it working now, > and I can't think of an issue with returning 0 now. Since this is about > semantics though I'd like a bit more review from at last one more > person. > > Kees, Eric, Andrew? This is a weird one: it would return an error _AND_ still perform the write. :( I think this patch is right, and I struggle to imagine a case where removing the failure is a problem. A quick question, though, do we want to instead do the reverse? (Not update, and keep the error?) Are there any examples of doing partial writes like this in real software? The proposed change is the safest change, though... -- Kees Cook ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-12-05 23:31 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-30 6:35 [PATCH] proc/sysctl: fix return error for proc_doulongvec_minmax Cheng Lin
2018-11-30 19:14 ` Luis Chamberlain
[not found] ` <201812031312398404610@zte.com.cn>
2018-12-03 20:14 ` Luis Chamberlain
[not found] ` <201812051510071985717@zte.com.cn>
2018-12-05 18:08 ` Re: Re: [PATCH] proc/sysctl: fix return error forproc_doulongvec_minmax Luis Chamberlain
2018-12-05 23:30 ` Re: [PATCH] proc/sysctl: fix return error for proc_doulongvec_minmax Kees Cook
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).