[PATCH] proc/sysctl: fix return error for proc_doulongvec_minmax

[PATCH] proc/sysctl: fix return error for proc_doulongvec_minmax

[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

Re: [PATCH] proc/sysctl: fix return error for proc_doulongvec_minmax

[Luis Chamberlain]

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
> 

Re: Re: [PATCH] proc/sysctl: fix return error for proc_doulongvec_minmax

[Luis Chamberlain]

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

Re: Re: Re: [PATCH] proc/sysctl: fix return error forproc_doulongvec_minmax

[Luis Chamberlain]

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.

Re: Re: [PATCH] proc/sysctl: fix return error for proc_doulongvec_minmax

[Kees Cook]

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

Re: [PATCH] proc/sysctl: fix return error for proc_doulongvec_minmax

[Kees Cook]

On Thu, Dec 6, 2018 at 12:52 AM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> On Thu, Dec 06, 2018 at 03:36:15PM +0800, Cheng Lin wrote:
> > If the number of input parameters is less than the total
> > parameters, an EINVAL error will be returned.
> >
> > e.g.
> > We use proc_doulongvec_minmax to pass up to two parameters
> > with kern_table.
> >
> > {
> >       .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 ~]#
> >
> > There are three processing functions dealing with digital parameters,
> > __do_proc_dointvec/__do_proc_douintvec/__do_proc_doulongvec_minmax.
> >
> > 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.
> >
> > Signed-off-by: Cheng Lin <cheng.lin130@zte.com.cn>
>
> Thanks for fixing up the commit log.
>
> Acked-by: Luis Chamberlain <mcgrof@kernel.org>

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

>
> I think we can live with this outside of stable. So stable is not
> needed. But I would not be surprised if autosel algorithm will end
> up picking it up. And if so.. well, it cannot hurt.
>
>   Luis



-- 
Kees Cook

Re: [PATCH] proc/sysctl: fix return error for proc_doulongvec_minmax

[Luis Chamberlain]

On Thu, Dec 06, 2018 at 03:36:15PM +0800, Cheng Lin wrote:
> If the number of input parameters is less than the total
> parameters, an EINVAL error will be returned.
> 
> e.g.
> We use proc_doulongvec_minmax to pass up to two parameters
> with kern_table.
> 
> {
> 	.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 ~]#
> 
> There are three processing functions dealing with digital parameters,
> __do_proc_dointvec/__do_proc_douintvec/__do_proc_doulongvec_minmax.
> 
> 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.
> 
> Signed-off-by: Cheng Lin <cheng.lin130@zte.com.cn>

Thanks for fixing up the commit log.

Acked-by: Luis Chamberlain <mcgrof@kernel.org>

I think we can live with this outside of stable. So stable is not
needed. But I would not be surprised if autosel algorithm will end
up picking it up. And if so.. well, it cannot hurt.

  Luis

[PATCH] proc/sysctl: fix return error for proc_doulongvec_minmax

[Cheng Lin]

If the number of input parameters is less than the total
parameters, an EINVAL error will be returned.

e.g.
We use proc_doulongvec_minmax to pass up to two parameters
with kern_table.

{
	.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 ~]#

There are three processing functions dealing with digital parameters,
__do_proc_dointvec/__do_proc_douintvec/__do_proc_doulongvec_minmax.

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.

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