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

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

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]

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
> 

[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