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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
>
On Mon, Dec 03, 2018 at 01:12:39PM +0800, [email protected] 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 <[email protected]>
> >> ---
> >> 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
> >>
On Mon, Dec 3, 2018 at 12:14 PM Luis Chamberlain <[email protected]> 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