Message ID | 20210108023339.55917-1-nixiaoming@huawei.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [v2] proc_sysctl: fix oops caused by incorrect command parameters. | expand |
On Fri 08-01-21 10:33:39, Xiaoming Ni wrote: > The process_sysctl_arg() does not check whether val is empty before > invoking strlen(val). If the command line parameter () is incorrectly > configured and val is empty, oops is triggered. > > For example, "hung_task_panic=1" is incorrectly written as "hung_task_panic". > > log: > Kernel command line: .... hung_task_panic > .... > [000000000000000n] user address but active_mm is swapper > Internal error: Oops: 96000005 [#1] SMP > Modules linked in: > CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.10.1 #1 > Hardware name: linux,dummy-virt (DT) > pstate: 40000005 (nZcv daif -PAN -UAO -TCO BTYPE=--) > pc : __pi_strlen+0x10/0x98 > lr : process_sysctl_arg+0x1e4/0x2ac > sp : ffffffc01104bd40 > x29: ffffffc01104bd40 x28: 0000000000000000 > x27: ffffff80c0a4691e x26: ffffffc0102a7c8c > x25: 0000000000000000 x24: ffffffc01104be80 > x23: ffffff80c22f0b00 x22: ffffff80c02e28c0 > x21: ffffffc0109f9000 x20: 0000000000000000 > x19: ffffffc0107c08de x18: 0000000000000003 > x17: ffffffc01105d000 x16: 0000000000000054 > x15: ffffffffffffffff x14: 3030253078413830 > x13: 000000000000ffff x12: 0000000000000000 > x11: 0101010101010101 x10: 0000000000000005 > x9 : 0000000000000003 x8 : ffffff80c0980c08 > x7 : 0000000000000000 x6 : 0000000000000002 > x5 : ffffff80c0235000 x4 : ffffff810f7c7ee0 > x3 : 000000000000043a x2 : 00bdcc4ebacf1a54 > x1 : 0000000000000000 x0 : 0000000000000000 > Call trace: > __pi_strlen+0x10/0x98 > parse_args+0x278/0x344 > do_sysctl_args+0x8c/0xfc > kernel_init+0x5c/0xf4 > ret_from_fork+0x10/0x30 > Code: b200c3eb 927cec01 f2400c07 54000301 (a8c10c22) > > Fixes: 3db978d480e2843 ("kernel/sysctl: support setting sysctl parameters > from kernel command line") > Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com> Thanks for catching this! > --------- > v2: > Added log output of the failure branch based on the review comments of Kees Cook. > v1: https://lore.kernel.org/lkml/20201224074256.117413-1-nixiaoming@huawei.com/ > --------- > --- > fs/proc/proc_sysctl.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c > index 317899222d7f..dc1a56515e86 100644 > --- a/fs/proc/proc_sysctl.c > +++ b/fs/proc/proc_sysctl.c > @@ -1757,6 +1757,11 @@ static int process_sysctl_arg(char *param, char *val, > loff_t pos = 0; > ssize_t wret; > > + if (!val) { > + pr_err("Missing param value! Expected '%s=...value...'\n", param); > + return 0; > + } Shouldn't you return an error here? Also my understanding is that parse_args is responsible for reporting the error. > + > if (strncmp(param, "sysctl", sizeof("sysctl") - 1) == 0) { > param += sizeof("sysctl") - 1; > > -- > 2.27.0
On 2021/1/8 17:21, Michal Hocko wrote: > On Fri 08-01-21 10:33:39, Xiaoming Ni wrote: >> The process_sysctl_arg() does not check whether val is empty before >> invoking strlen(val). If the command line parameter () is incorrectly >> configured and val is empty, oops is triggered. >> >> For example, "hung_task_panic=1" is incorrectly written as "hung_task_panic". >> >> log: >> Kernel command line: .... hung_task_panic >> .... >> [000000000000000n] user address but active_mm is swapper >> Internal error: Oops: 96000005 [#1] SMP >> Modules linked in: >> CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.10.1 #1 >> Hardware name: linux,dummy-virt (DT) >> pstate: 40000005 (nZcv daif -PAN -UAO -TCO BTYPE=--) >> pc : __pi_strlen+0x10/0x98 >> lr : process_sysctl_arg+0x1e4/0x2ac >> sp : ffffffc01104bd40 >> x29: ffffffc01104bd40 x28: 0000000000000000 >> x27: ffffff80c0a4691e x26: ffffffc0102a7c8c >> x25: 0000000000000000 x24: ffffffc01104be80 >> x23: ffffff80c22f0b00 x22: ffffff80c02e28c0 >> x21: ffffffc0109f9000 x20: 0000000000000000 >> x19: ffffffc0107c08de x18: 0000000000000003 >> x17: ffffffc01105d000 x16: 0000000000000054 >> x15: ffffffffffffffff x14: 3030253078413830 >> x13: 000000000000ffff x12: 0000000000000000 >> x11: 0101010101010101 x10: 0000000000000005 >> x9 : 0000000000000003 x8 : ffffff80c0980c08 >> x7 : 0000000000000000 x6 : 0000000000000002 >> x5 : ffffff80c0235000 x4 : ffffff810f7c7ee0 >> x3 : 000000000000043a x2 : 00bdcc4ebacf1a54 >> x1 : 0000000000000000 x0 : 0000000000000000 >> Call trace: >> __pi_strlen+0x10/0x98 >> parse_args+0x278/0x344 >> do_sysctl_args+0x8c/0xfc >> kernel_init+0x5c/0xf4 >> ret_from_fork+0x10/0x30 >> Code: b200c3eb 927cec01 f2400c07 54000301 (a8c10c22) >> >> Fixes: 3db978d480e2843 ("kernel/sysctl: support setting sysctl parameters >> from kernel command line") >> Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com> > > Thanks for catching this! > >> --------- >> v2: >> Added log output of the failure branch based on the review comments of Kees Cook. >> v1: https://lore.kernel.org/lkml/20201224074256.117413-1-nixiaoming@huawei.com/ >> --------- >> --- >> fs/proc/proc_sysctl.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c >> index 317899222d7f..dc1a56515e86 100644 >> --- a/fs/proc/proc_sysctl.c >> +++ b/fs/proc/proc_sysctl.c >> @@ -1757,6 +1757,11 @@ static int process_sysctl_arg(char *param, char *val, >> loff_t pos = 0; >> ssize_t wret; >> >> + if (!val) { >> + pr_err("Missing param value! Expected '%s=...value...'\n", param); >> + return 0; I may need to move the validation code for val to the end of the validation code for param to prevent non-sysctl arguments from triggering the current print. Or delete the print and keep it silent for a little better performance. Which is better? >> + } > > Shouldn't you return an error here? Also my understanding is that > parse_args is responsible for reporting the error. > All exception branches in process_sysctl_arg record logs and return 0. Do I need to keep the same processing in the new branch? >> + >> if (strncmp(param, "sysctl", sizeof("sysctl") - 1) == 0) { >> param += sizeof("sysctl") - 1; >> >> -- >> 2.27.0 > Thanks Xiaoming Ni
On Fri 08-01-21 18:01:52, Xiaoming Ni wrote: > On 2021/1/8 17:21, Michal Hocko wrote: > > On Fri 08-01-21 10:33:39, Xiaoming Ni wrote: > > > The process_sysctl_arg() does not check whether val is empty before > > > invoking strlen(val). If the command line parameter () is incorrectly > > > configured and val is empty, oops is triggered. > > > > > > For example, "hung_task_panic=1" is incorrectly written as "hung_task_panic". > > > > > > log: > > > Kernel command line: .... hung_task_panic > > > .... > > > [000000000000000n] user address but active_mm is swapper > > > Internal error: Oops: 96000005 [#1] SMP > > > Modules linked in: > > > CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.10.1 #1 > > > Hardware name: linux,dummy-virt (DT) > > > pstate: 40000005 (nZcv daif -PAN -UAO -TCO BTYPE=--) > > > pc : __pi_strlen+0x10/0x98 > > > lr : process_sysctl_arg+0x1e4/0x2ac > > > sp : ffffffc01104bd40 > > > x29: ffffffc01104bd40 x28: 0000000000000000 > > > x27: ffffff80c0a4691e x26: ffffffc0102a7c8c > > > x25: 0000000000000000 x24: ffffffc01104be80 > > > x23: ffffff80c22f0b00 x22: ffffff80c02e28c0 > > > x21: ffffffc0109f9000 x20: 0000000000000000 > > > x19: ffffffc0107c08de x18: 0000000000000003 > > > x17: ffffffc01105d000 x16: 0000000000000054 > > > x15: ffffffffffffffff x14: 3030253078413830 > > > x13: 000000000000ffff x12: 0000000000000000 > > > x11: 0101010101010101 x10: 0000000000000005 > > > x9 : 0000000000000003 x8 : ffffff80c0980c08 > > > x7 : 0000000000000000 x6 : 0000000000000002 > > > x5 : ffffff80c0235000 x4 : ffffff810f7c7ee0 > > > x3 : 000000000000043a x2 : 00bdcc4ebacf1a54 > > > x1 : 0000000000000000 x0 : 0000000000000000 > > > Call trace: > > > __pi_strlen+0x10/0x98 > > > parse_args+0x278/0x344 > > > do_sysctl_args+0x8c/0xfc > > > kernel_init+0x5c/0xf4 > > > ret_from_fork+0x10/0x30 > > > Code: b200c3eb 927cec01 f2400c07 54000301 (a8c10c22) > > > > > > Fixes: 3db978d480e2843 ("kernel/sysctl: support setting sysctl parameters > > > from kernel command line") > > > Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com> > > > > Thanks for catching this! > > > > > --------- > > > v2: > > > Added log output of the failure branch based on the review comments of Kees Cook. > > > v1: https://lore.kernel.org/lkml/20201224074256.117413-1-nixiaoming@huawei.com/ > > > --------- > > > --- > > > fs/proc/proc_sysctl.c | 5 +++++ > > > 1 file changed, 5 insertions(+) > > > > > > diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c > > > index 317899222d7f..dc1a56515e86 100644 > > > --- a/fs/proc/proc_sysctl.c > > > +++ b/fs/proc/proc_sysctl.c > > > @@ -1757,6 +1757,11 @@ static int process_sysctl_arg(char *param, char *val, > > > loff_t pos = 0; > > > ssize_t wret; > > > + if (!val) { > > > + pr_err("Missing param value! Expected '%s=...value...'\n", param); > > > + return 0; > I may need to move the validation code for val to the end of the validation > code for param to prevent non-sysctl arguments from triggering the current > print. Why would that matter? A missing value is clearly a error path and it should be reported. > Or delete the print and keep it silent for a little better performance. > Which is better? I do not think there is a performance argument on the table. The generic code is returning EINVAL on a missing value where it is needed. Sysctl all require a value IIRC so EINVAL would be the right way to report this and let the generic code to complain.
On Fri, Jan 08, 2021 at 12:47:18PM +0100, Michal Hocko wrote: > On Fri 08-01-21 18:01:52, Xiaoming Ni wrote: > > On 2021/1/8 17:21, Michal Hocko wrote: > > > On Fri 08-01-21 10:33:39, Xiaoming Ni wrote: > > > > The process_sysctl_arg() does not check whether val is empty before > > > > invoking strlen(val). If the command line parameter () is incorrectly > > > > configured and val is empty, oops is triggered. > > > > > > > > For example, "hung_task_panic=1" is incorrectly written as "hung_task_panic". > > > > > > > > log: > > > > Kernel command line: .... hung_task_panic > > > > .... > > > > [000000000000000n] user address but active_mm is swapper > > > > Internal error: Oops: 96000005 [#1] SMP > > > > Modules linked in: > > > > CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.10.1 #1 > > > > Hardware name: linux,dummy-virt (DT) > > > > pstate: 40000005 (nZcv daif -PAN -UAO -TCO BTYPE=--) > > > > pc : __pi_strlen+0x10/0x98 > > > > lr : process_sysctl_arg+0x1e4/0x2ac > > > > sp : ffffffc01104bd40 > > > > x29: ffffffc01104bd40 x28: 0000000000000000 > > > > x27: ffffff80c0a4691e x26: ffffffc0102a7c8c > > > > x25: 0000000000000000 x24: ffffffc01104be80 > > > > x23: ffffff80c22f0b00 x22: ffffff80c02e28c0 > > > > x21: ffffffc0109f9000 x20: 0000000000000000 > > > > x19: ffffffc0107c08de x18: 0000000000000003 > > > > x17: ffffffc01105d000 x16: 0000000000000054 > > > > x15: ffffffffffffffff x14: 3030253078413830 > > > > x13: 000000000000ffff x12: 0000000000000000 > > > > x11: 0101010101010101 x10: 0000000000000005 > > > > x9 : 0000000000000003 x8 : ffffff80c0980c08 > > > > x7 : 0000000000000000 x6 : 0000000000000002 > > > > x5 : ffffff80c0235000 x4 : ffffff810f7c7ee0 > > > > x3 : 000000000000043a x2 : 00bdcc4ebacf1a54 > > > > x1 : 0000000000000000 x0 : 0000000000000000 > > > > Call trace: > > > > __pi_strlen+0x10/0x98 > > > > parse_args+0x278/0x344 > > > > do_sysctl_args+0x8c/0xfc > > > > kernel_init+0x5c/0xf4 > > > > ret_from_fork+0x10/0x30 > > > > Code: b200c3eb 927cec01 f2400c07 54000301 (a8c10c22) > > > > > > > > Fixes: 3db978d480e2843 ("kernel/sysctl: support setting sysctl parameters > > > > from kernel command line") > > > > Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com> > > > > > > Thanks for catching this! > > > > > > > --------- > > > > v2: > > > > Added log output of the failure branch based on the review comments of Kees Cook. > > > > v1: https://lore.kernel.org/lkml/20201224074256.117413-1-nixiaoming@huawei.com/ > > > > --------- > > > > --- > > > > fs/proc/proc_sysctl.c | 5 +++++ > > > > 1 file changed, 5 insertions(+) > > > > > > > > diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c > > > > index 317899222d7f..dc1a56515e86 100644 > > > > --- a/fs/proc/proc_sysctl.c > > > > +++ b/fs/proc/proc_sysctl.c > > > > @@ -1757,6 +1757,11 @@ static int process_sysctl_arg(char *param, char *val, > > > > loff_t pos = 0; > > > > ssize_t wret; > > > > + if (!val) { > > > > + pr_err("Missing param value! Expected '%s=...value...'\n", param); > > > > + return 0; > > I may need to move the validation code for val to the end of the validation > > code for param to prevent non-sysctl arguments from triggering the current > > print. > > Why would that matter? A missing value is clearly a error path and it > should be reported. This test is in the correct place. I think it's just a question of the return values. > > Or delete the print and keep it silent for a little better performance. > > Which is better? > > I do not think there is a performance argument on the table. The generic > code is returning EINVAL on a missing value where it is needed. Sysctl > all require a value IIRC so EINVAL would be the right way to report > this and let the generic code to complain. The reason the others do a "return 0" is because other error conditions will end up double-reporting: switch (ret) { case 0: continue; case -ENOENT: pr_err("%s: Unknown parameter `%s'\n", doing, param); break; case -ENOSPC: pr_err("%s: `%s' too large for parameter `%s'\n", doing, val ?: "", param); break; default: pr_err("%s: `%s' invalid for parameter `%s'\n", doing, val ?: "", param); break; } Also note that where the sysctl parsing happens, it calls parse_args() without checking return codes, so that doesn't matter either. It's possible that doing this would be sufficient, though: + if (!val) + return -EINVAL; Since that would hit the "default" error report which looks reasonable.
On Fri 08-01-21 11:56:33, Kees Cook wrote: > On Fri, Jan 08, 2021 at 12:47:18PM +0100, Michal Hocko wrote: > > On Fri 08-01-21 18:01:52, Xiaoming Ni wrote: > > > On 2021/1/8 17:21, Michal Hocko wrote: > > > > On Fri 08-01-21 10:33:39, Xiaoming Ni wrote: > > > > > The process_sysctl_arg() does not check whether val is empty before > > > > > invoking strlen(val). If the command line parameter () is incorrectly > > > > > configured and val is empty, oops is triggered. > > > > > > > > > > For example, "hung_task_panic=1" is incorrectly written as "hung_task_panic". > > > > > > > > > > log: > > > > > Kernel command line: .... hung_task_panic > > > > > .... > > > > > [000000000000000n] user address but active_mm is swapper > > > > > Internal error: Oops: 96000005 [#1] SMP > > > > > Modules linked in: > > > > > CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.10.1 #1 > > > > > Hardware name: linux,dummy-virt (DT) > > > > > pstate: 40000005 (nZcv daif -PAN -UAO -TCO BTYPE=--) > > > > > pc : __pi_strlen+0x10/0x98 > > > > > lr : process_sysctl_arg+0x1e4/0x2ac > > > > > sp : ffffffc01104bd40 > > > > > x29: ffffffc01104bd40 x28: 0000000000000000 > > > > > x27: ffffff80c0a4691e x26: ffffffc0102a7c8c > > > > > x25: 0000000000000000 x24: ffffffc01104be80 > > > > > x23: ffffff80c22f0b00 x22: ffffff80c02e28c0 > > > > > x21: ffffffc0109f9000 x20: 0000000000000000 > > > > > x19: ffffffc0107c08de x18: 0000000000000003 > > > > > x17: ffffffc01105d000 x16: 0000000000000054 > > > > > x15: ffffffffffffffff x14: 3030253078413830 > > > > > x13: 000000000000ffff x12: 0000000000000000 > > > > > x11: 0101010101010101 x10: 0000000000000005 > > > > > x9 : 0000000000000003 x8 : ffffff80c0980c08 > > > > > x7 : 0000000000000000 x6 : 0000000000000002 > > > > > x5 : ffffff80c0235000 x4 : ffffff810f7c7ee0 > > > > > x3 : 000000000000043a x2 : 00bdcc4ebacf1a54 > > > > > x1 : 0000000000000000 x0 : 0000000000000000 > > > > > Call trace: > > > > > __pi_strlen+0x10/0x98 > > > > > parse_args+0x278/0x344 > > > > > do_sysctl_args+0x8c/0xfc > > > > > kernel_init+0x5c/0xf4 > > > > > ret_from_fork+0x10/0x30 > > > > > Code: b200c3eb 927cec01 f2400c07 54000301 (a8c10c22) > > > > > > > > > > Fixes: 3db978d480e2843 ("kernel/sysctl: support setting sysctl parameters > > > > > from kernel command line") > > > > > Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com> > > > > > > > > Thanks for catching this! > > > > > > > > > --------- > > > > > v2: > > > > > Added log output of the failure branch based on the review comments of Kees Cook. > > > > > v1: https://lore.kernel.org/lkml/20201224074256.117413-1-nixiaoming@huawei.com/ > > > > > --------- > > > > > --- > > > > > fs/proc/proc_sysctl.c | 5 +++++ > > > > > 1 file changed, 5 insertions(+) > > > > > > > > > > diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c > > > > > index 317899222d7f..dc1a56515e86 100644 > > > > > --- a/fs/proc/proc_sysctl.c > > > > > +++ b/fs/proc/proc_sysctl.c > > > > > @@ -1757,6 +1757,11 @@ static int process_sysctl_arg(char *param, char *val, > > > > > loff_t pos = 0; > > > > > ssize_t wret; > > > > > + if (!val) { > > > > > + pr_err("Missing param value! Expected '%s=...value...'\n", param); > > > > > + return 0; > > > I may need to move the validation code for val to the end of the validation > > > code for param to prevent non-sysctl arguments from triggering the current > > > print. > > > > Why would that matter? A missing value is clearly a error path and it > > should be reported. > > This test is in the correct place. I think it's just a question of the > return values. I was probably not clear. The test for val is at the right place. I would just expect -EINVAL and have the generic code to report.
On Fri, 8 Jan 2021 21:10:25 +0100 Michal Hocko <mhocko@suse.com> wrote: > > > Why would that matter? A missing value is clearly a error path and it > > > should be reported. > > > > This test is in the correct place. I think it's just a question of the > > return values. > > I was probably not clear. The test for val is at the right place. I > would just expect -EINVAL and have the generic code to report. It does seem a bit screwy that process_sysctl_arg() returns zero in all situations (parse_args() is set up to handle an error return from it). But this patch is consistent with all the other error handling in process_sysctl_arg().
On 2021/1/9 17:10, Andy Shevchenko wrote: > > > On Friday, January 8, 2021, Xiaoming Ni <nixiaoming@huawei.com > <mailto:nixiaoming@huawei.com>> wrote: > > The process_sysctl_arg() does not check whether val is empty before > invoking strlen(val). If the command line parameter () is incorrectly > configured and val is empty, oops is triggered. > > For example, "hung_task_panic=1" is incorrectly written as > "hung_task_panic". > > log: > > > Can you drop redundant (not significant) lines from the log to avoid > noisy commit message? > ok, Thank you for your advice. I will update it in v3 patch. Thanks Xiaoming Ni.
On 2021/1/9 9:50, Andrew Morton wrote: > On Fri, 8 Jan 2021 21:10:25 +0100 Michal Hocko <mhocko@suse.com> wrote: > >>>> Why would that matter? A missing value is clearly a error path and it >>>> should be reported. >>> >>> This test is in the correct place. I think it's just a question of the >>> return values. >> >> I was probably not clear. The test for val is at the right place. I >> would just expect -EINVAL and have the generic code to report. > > It does seem a bit screwy that process_sysctl_arg() returns zero in all > situations (parse_args() is set up to handle an error return from it). > But this patch is consistent with all the other error handling in > process_sysctl_arg(). > . > Set the kernel startup parameter to "nosmp nokaslr hung_task_panic" and test the startup logs of different patches. patch1: +++ b/fs/proc/proc_sysctl.c @@ -1757,6 +1757,11 @@ static int process_sysctl_arg(char *param, char *val, loff_t pos = 0; ssize_t wret; + if (!val) { + pr_err("Missing param value! Expected '%s=...value...'\n", param); + return 0; + } + if (strncmp(param, "sysctl", sizeof("sysctl") - 1) == 0) { param += sizeof("sysctl") - 1; sysctl log for patch1: Missing param value! Expected 'nosmp=...value...' Missing param value! Expected 'nokaslr=...value...' Missing param value! Expected 'hung_task_panic=...value...' patch2: +++ b/fs/proc/proc_sysctl.c @@ -1756,6 +1756,8 @@ static int process_sysctl_arg(char *param, char *val, int err; loff_t pos = 0; ssize_t wret; + if (!val) + return -EINVAL; if (strncmp(param, "sysctl", sizeof("sysctl") - 1) == 0) { param += sizeof("sysctl") - 1; sysctl log for patch2: Setting sysctl args: `' invalid for parameter `nosmp' Setting sysctl args: `' invalid for parameter `nokaslr' Setting sysctl args: `' invalid for parameter `hung_task_panic' patch3: +++ b/fs/proc/proc_sysctl.c @@ -1770,6 +1770,9 @@ static int process_sysctl_arg(char *param, char *val, return 0; } + if (!val) + return -EINVAL; + /* * To set sysctl options, we use a temporary mount of proc, look up the * respective sys/ file and write to it. To avoid mounting it when no sysctl log for patch3: Setting sysctl args: `' invalid for parameter `hung_task_panic' patch4: +++ b/fs/proc/proc_sysctl.c @@ -1757,6 +1757,9 @@ static int process_sysctl_arg(char *param, char *val, loff_t pos = 0; ssize_t wret; + if (!val) + return 0; + if (strncmp(param, "sysctl", sizeof("sysctl") - 1) == 0) { param += sizeof("sysctl") - 1; sysctl log for patch3: no log When process_sysctl_arg() is called, the param parameter may not be the sysctl parameter. Patch3 or patch4, which is better? Thanks Xiaoming Ni
On Mon 11-01-21 11:48:19, Xiaoming Ni wrote: [...] > patch3: > +++ b/fs/proc/proc_sysctl.c > @@ -1770,6 +1770,9 @@ static int process_sysctl_arg(char *param, char *val, > return 0; > } > > + if (!val) > + return -EINVAL; > + > /* > * To set sysctl options, we use a temporary mount of proc, look up the > * respective sys/ file and write to it. To avoid mounting it when no > > sysctl log for patch3: > Setting sysctl args: `' invalid for parameter `hung_task_panic' [...] > When process_sysctl_arg() is called, the param parameter may not be the > sysctl parameter. > > Patch3 or patch4, which is better? Patch3
On Mon, Jan 11, 2021 at 03:21:31PM +0100, Michal Hocko wrote: > On Mon 11-01-21 11:48:19, Xiaoming Ni wrote: > [...] > > patch3: > > +++ b/fs/proc/proc_sysctl.c > > @@ -1770,6 +1770,9 @@ static int process_sysctl_arg(char *param, char *val, > > return 0; > > } > > > > + if (!val) > > + return -EINVAL; > > + > > /* > > * To set sysctl options, we use a temporary mount of proc, look up the > > * respective sys/ file and write to it. To avoid mounting it when no > > > > sysctl log for patch3: > > Setting sysctl args: `' invalid for parameter `hung_task_panic' > [...] > > When process_sysctl_arg() is called, the param parameter may not be the > > sysctl parameter. > > > > Patch3 or patch4, which is better? > > Patch3 Oh, I see the issue here -- I thought we were only calling process_sysctl_arg() with valid sysctl fields. It looks like we're not, which means it should silently ignore everything that isn't a sysctl field, and only return -EINVAL when it IS a sysctl but it lacks a value.
diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c index 317899222d7f..dc1a56515e86 100644 --- a/fs/proc/proc_sysctl.c +++ b/fs/proc/proc_sysctl.c @@ -1757,6 +1757,11 @@ static int process_sysctl_arg(char *param, char *val, loff_t pos = 0; ssize_t wret; + if (!val) { + pr_err("Missing param value! Expected '%s=...value...'\n", param); + return 0; + } + if (strncmp(param, "sysctl", sizeof("sysctl") - 1) == 0) { param += sizeof("sysctl") - 1;
The process_sysctl_arg() does not check whether val is empty before invoking strlen(val). If the command line parameter () is incorrectly configured and val is empty, oops is triggered. For example, "hung_task_panic=1" is incorrectly written as "hung_task_panic". log: Kernel command line: .... hung_task_panic .... [000000000000000n] user address but active_mm is swapper Internal error: Oops: 96000005 [#1] SMP Modules linked in: CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.10.1 #1 Hardware name: linux,dummy-virt (DT) pstate: 40000005 (nZcv daif -PAN -UAO -TCO BTYPE=--) pc : __pi_strlen+0x10/0x98 lr : process_sysctl_arg+0x1e4/0x2ac sp : ffffffc01104bd40 x29: ffffffc01104bd40 x28: 0000000000000000 x27: ffffff80c0a4691e x26: ffffffc0102a7c8c x25: 0000000000000000 x24: ffffffc01104be80 x23: ffffff80c22f0b00 x22: ffffff80c02e28c0 x21: ffffffc0109f9000 x20: 0000000000000000 x19: ffffffc0107c08de x18: 0000000000000003 x17: ffffffc01105d000 x16: 0000000000000054 x15: ffffffffffffffff x14: 3030253078413830 x13: 000000000000ffff x12: 0000000000000000 x11: 0101010101010101 x10: 0000000000000005 x9 : 0000000000000003 x8 : ffffff80c0980c08 x7 : 0000000000000000 x6 : 0000000000000002 x5 : ffffff80c0235000 x4 : ffffff810f7c7ee0 x3 : 000000000000043a x2 : 00bdcc4ebacf1a54 x1 : 0000000000000000 x0 : 0000000000000000 Call trace: __pi_strlen+0x10/0x98 parse_args+0x278/0x344 do_sysctl_args+0x8c/0xfc kernel_init+0x5c/0xf4 ret_from_fork+0x10/0x30 Code: b200c3eb 927cec01 f2400c07 54000301 (a8c10c22) Fixes: 3db978d480e2843 ("kernel/sysctl: support setting sysctl parameters from kernel command line") Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com> --------- v2: Added log output of the failure branch based on the review comments of Kees Cook. v1: https://lore.kernel.org/lkml/20201224074256.117413-1-nixiaoming@huawei.com/ --------- --- fs/proc/proc_sysctl.c | 5 +++++ 1 file changed, 5 insertions(+)