* [PATCH v3] proc_sysctl: fix oops caused by incorrect command parameters. @ 2021-01-12 3:31 Xiaoming Ni 2021-01-12 4:33 ` Andrew Morton 0 siblings, 1 reply; 8+ messages in thread From: Xiaoming Ni @ 2021-01-12 3:31 UTC (permalink / raw) To: linux-kernel, mcgrof, keescook, yzaikin, adobriyan, linux-fsdevel, vbabka, akpm, mhocko, andy.shevchenko Cc: nixiaoming, wangle6 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", oops is triggered. The call stack is as follows: Kernel command line: .... hung_task_panic ...... Call trace: __pi_strlen+0x10/0x98 parse_args+0x278/0x344 do_sysctl_args+0x8c/0xfc kernel_init+0x5c/0xf4 ret_from_fork+0x10/0x30 To fix it, check whether "val" is empty when "phram" is a sysctl field. Error codes are returned in the failure branch, and error logs are generated by parse_args(). Fixes: 3db978d480e2843 ("kernel/sysctl: support setting sysctl parameters from kernel command line") Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com> --------- v3: Return -EINVAL, When phram is the sysctl field and val is empty. v2: https://lore.kernel.org/lkml/20210108023339.55917-1-nixiaoming@huawei.com/ 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 | 3 +++ 1 file changed, 3 insertions(+) diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c index 317899222d7f..d493a50058a5 100644 --- a/fs/proc/proc_sysctl.c +++ 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 -- 2.27.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v3] proc_sysctl: fix oops caused by incorrect command parameters. 2021-01-12 3:31 [PATCH v3] proc_sysctl: fix oops caused by incorrect command parameters Xiaoming Ni @ 2021-01-12 4:33 ` Andrew Morton 2021-01-12 6:24 ` Xiaoming Ni 0 siblings, 1 reply; 8+ messages in thread From: Andrew Morton @ 2021-01-12 4:33 UTC (permalink / raw) To: Xiaoming Ni Cc: linux-kernel, mcgrof, keescook, yzaikin, adobriyan, linux-fsdevel, vbabka, mhocko, andy.shevchenko, wangle6 On Tue, 12 Jan 2021 11:31:55 +0800 Xiaoming Ni <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. > > --- a/fs/proc/proc_sysctl.c > +++ 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; > + I think v2 (return 0) was preferable. Because all the other error-out cases in process_sysctl_arg() also do a `return 0'. If we're going to do a separate "patch: make process_sysctl_arg() return an errno instead of 0" then fine, we can discuss that. But it's conceptually a different work from fixing this situation. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] proc_sysctl: fix oops caused by incorrect command parameters. 2021-01-12 4:33 ` Andrew Morton @ 2021-01-12 6:24 ` Xiaoming Ni 2021-01-12 6:28 ` Andrew Morton 0 siblings, 1 reply; 8+ messages in thread From: Xiaoming Ni @ 2021-01-12 6:24 UTC (permalink / raw) To: Andrew Morton Cc: linux-kernel, mcgrof, keescook, yzaikin, adobriyan, linux-fsdevel, vbabka, mhocko, andy.shevchenko, wangle6 On 2021/1/12 12:33, Andrew Morton wrote: > On Tue, 12 Jan 2021 11:31:55 +0800 Xiaoming Ni <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. >> >> --- a/fs/proc/proc_sysctl.c >> +++ 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; >> + > > I think v2 (return 0) was preferable. Because all the other error-out > cases in process_sysctl_arg() also do a `return 0'. https://lore.kernel.org/lkml/bc098af4-c0cd-212e-d09d-46d617d0acab@huawei.com/ 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; Is this the version you're talking about? > > If we're going to do a separate "patch: make process_sysctl_arg() > return an errno instead of 0" then fine, we can discuss that. But it's > conceptually a different work from fixing this situation. > . > However, are the logs generated by process_sysctl_arg() clearer and more accurate than parse_args()? Should the logs generated by process_sysctl_arg() be deleted? Thanks Xiaoming Ni ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] proc_sysctl: fix oops caused by incorrect command parameters. 2021-01-12 6:24 ` Xiaoming Ni @ 2021-01-12 6:28 ` Andrew Morton 2021-01-12 7:24 ` Michal Hocko 0 siblings, 1 reply; 8+ messages in thread From: Andrew Morton @ 2021-01-12 6:28 UTC (permalink / raw) To: Xiaoming Ni Cc: linux-kernel, mcgrof, keescook, yzaikin, adobriyan, linux-fsdevel, vbabka, mhocko, andy.shevchenko, wangle6 On Tue, 12 Jan 2021 14:24:05 +0800 Xiaoming Ni <nixiaoming@huawei.com> wrote: > On 2021/1/12 12:33, Andrew Morton wrote: > > On Tue, 12 Jan 2021 11:31:55 +0800 Xiaoming Ni <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. > >> > >> --- a/fs/proc/proc_sysctl.c > >> +++ 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; > >> + > > > > I think v2 (return 0) was preferable. Because all the other error-out > > cases in process_sysctl_arg() also do a `return 0'. > > https://lore.kernel.org/lkml/bc098af4-c0cd-212e-d09d-46d617d0acab@huawei.com/ > > 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; > > Is this the version you're talking about? yes, but as a separate patch. The bugfix comes first. > > > > If we're going to do a separate "patch: make process_sysctl_arg() > > return an errno instead of 0" then fine, we can discuss that. But it's > > conceptually a different work from fixing this situation. > > . > > > However, are the logs generated by process_sysctl_arg() clearer and more > accurate than parse_args()? Should the logs generated by > process_sysctl_arg() be deleted? I think the individual logs are very useful and should be retained. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] proc_sysctl: fix oops caused by incorrect command parameters. 2021-01-12 6:28 ` Andrew Morton @ 2021-01-12 7:24 ` Michal Hocko 2021-01-12 11:42 ` Vlastimil Babka 0 siblings, 1 reply; 8+ messages in thread From: Michal Hocko @ 2021-01-12 7:24 UTC (permalink / raw) To: Andrew Morton Cc: Xiaoming Ni, linux-kernel, mcgrof, keescook, yzaikin, adobriyan, linux-fsdevel, vbabka, andy.shevchenko, wangle6 On Mon 11-01-21 22:28:45, Andrew Morton wrote: > On Tue, 12 Jan 2021 14:24:05 +0800 Xiaoming Ni <nixiaoming@huawei.com> wrote: > > > On 2021/1/12 12:33, Andrew Morton wrote: > > > On Tue, 12 Jan 2021 11:31:55 +0800 Xiaoming Ni <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. > > >> > > >> --- a/fs/proc/proc_sysctl.c > > >> +++ 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; > > >> + > > > > > > I think v2 (return 0) was preferable. Because all the other error-out > > > cases in process_sysctl_arg() also do a `return 0'. > > > > https://lore.kernel.org/lkml/bc098af4-c0cd-212e-d09d-46d617d0acab@huawei.com/ > > > > 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; > > > > Is this the version you're talking about? > > yes, but as a separate patch. The bugfix comes first. > > > > > > > If we're going to do a separate "patch: make process_sysctl_arg() > > > return an errno instead of 0" then fine, we can discuss that. But it's > > > conceptually a different work from fixing this situation. > > > . > > > > > However, are the logs generated by process_sysctl_arg() clearer and more > > accurate than parse_args()? Should the logs generated by > > process_sysctl_arg() be deleted? > > I think the individual logs are very useful and should be retained. Yes, other sysfs specific error messages are likely useful. I just fail to see why a missing value should be handled here when there is an existing handling in the caller. Not sure whether a complete shadow reporting in process_sysctl_arg is a deliberate decision or not. Vlastimil? Anyway one way or the other, all I care about is to have a reporting in place because this shouldn't be a silent failure. -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] proc_sysctl: fix oops caused by incorrect command parameters. 2021-01-12 7:24 ` Michal Hocko @ 2021-01-12 11:42 ` Vlastimil Babka 2021-01-17 2:59 ` Xiaoming Ni 0 siblings, 1 reply; 8+ messages in thread From: Vlastimil Babka @ 2021-01-12 11:42 UTC (permalink / raw) To: Michal Hocko, Andrew Morton Cc: Xiaoming Ni, linux-kernel, mcgrof, keescook, yzaikin, adobriyan, linux-fsdevel, andy.shevchenko, wangle6 On 1/12/21 8:24 AM, Michal Hocko wrote: >> > > >> > > If we're going to do a separate "patch: make process_sysctl_arg() >> > > return an errno instead of 0" then fine, we can discuss that. But it's >> > > conceptually a different work from fixing this situation. >> > > . >> > > >> > However, are the logs generated by process_sysctl_arg() clearer and more >> > accurate than parse_args()? Should the logs generated by >> > process_sysctl_arg() be deleted? >> >> I think the individual logs are very useful and should be retained. > > Yes, other sysfs specific error messages are likely useful. I just fail > to see why a missing value should be handled here when there is an > existing handling in the caller. Not sure whether a complete shadow > reporting in process_sysctl_arg is a deliberate decision or not. > Vlastimil? Yes, it's a way to have more useful sysctl-specific reports than the generic ones. And I think I was inspired by some other existing code, but don't remember exactly. The options are: 1) the current sysctl-specific reports, return 0 as the values are only consumed 2) be silent and return error, invent new error codes to have generic report be more useful for sysctl, but inevitably lose some nuances anyway 3) a mix where 2) is used for situations where generic report is sufficient enough, 1) where not Patch v2 went with option 1), v3 with option 3). I think it's down to preferences. I would personally go with v2 and message similar to the existing ones, i.e.: "Failed to set sysctl parameter '%s': no value given\n" Also we seem to be silently doing nothing when strlen(val) == 0, i.e. "hung_task_panic=" was passed. Worth reporting the same error. But v3 is fine with me as well. The generic error message works. We could just add "if (!len) return -EINVAL" below the strlen() call. Also please Cc: stable. > Anyway one way or the other, all I care about is to have a reporting in > place because this shouldn't be a silent failure. > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] proc_sysctl: fix oops caused by incorrect command parameters. 2021-01-12 11:42 ` Vlastimil Babka @ 2021-01-17 2:59 ` Xiaoming Ni 2021-01-18 10:50 ` Vlastimil Babka 0 siblings, 1 reply; 8+ messages in thread From: Xiaoming Ni @ 2021-01-17 2:59 UTC (permalink / raw) To: Vlastimil Babka, Michal Hocko, Andrew Morton, rdunlap, hkallweit1 Cc: linux-kernel, mcgrof, keescook, yzaikin, adobriyan, linux-fsdevel, andy.shevchenko, wangle6 On 2021/1/12 19:42, Vlastimil Babka wrote: > On 1/12/21 8:24 AM, Michal Hocko wrote: >>>>> >>>>> If we're going to do a separate "patch: make process_sysctl_arg() >>>>> return an errno instead of 0" then fine, we can discuss that. But it's >>>>> conceptually a different work from fixing this situation. >>>>> . >>>>> >>>> However, are the logs generated by process_sysctl_arg() clearer and more >>>> accurate than parse_args()? Should the logs generated by >>>> process_sysctl_arg() be deleted? >>> >>> I think the individual logs are very useful and should be retained. >> >> Yes, other sysfs specific error messages are likely useful. I just fail >> to see why a missing value should be handled here when there is an >> existing handling in the caller. Not sure whether a complete shadow >> reporting in process_sysctl_arg is a deliberate decision or not. >> Vlastimil? > > Yes, it's a way to have more useful sysctl-specific reports than the generic > ones. And I think I was inspired by some other existing code, but don't remember > exactly. The options are: > > 1) the current sysctl-specific reports, return 0 as the values are only consumed > 2) be silent and return error, invent new error codes to have generic report be > more useful for sysctl, but inevitably lose some nuances anyway > 3) a mix where 2) is used for situations where generic report is sufficient > enough, 1) where not > > Patch v2 went with option 1), v3 with option 3). I think it's down to > preferences. I would personally go with v2 and message similar to the existing > ones, i.e.: > > "Failed to set sysctl parameter '%s': no value given\n" > > Also we seem to be silently doing nothing when strlen(val) == 0, i.e. > "hung_task_panic=" was passed. Worth reporting the same error. > > But v3 is fine with me as well. The generic error message works. We could just > add "if (!len) return -EINVAL" below the strlen() call. > > Also please Cc: stable. > >> Anyway one way or the other, all I care about is to have a reporting in >> place because this shouldn't be a silent failure. >> The current v2 is already in the linux-next branch and throws a new error: https://lore.kernel.org/lkml/cb54e349-7147-0a1f-a349-1e16ba603fce@infradead.org/ This bug has been mentioned in the previous discussion and has been fixed in the current v3 patch. https://lore.kernel.org/linux-fsdevel/202101111149.20A58E1@keescook/ What am I supposed to do now? - Resend V3? - Rewrite a new fix patch based on the current code of linux-next. - Develop a new V4 patch: Use V2 to discuss how to use the Patch4 solution. https://lore.kernel.org/linux-fsdevel/bc098af4-c0cd-212e-d09d-46d617d0acab@huawei.com/#t Thanks Xiaoming Ni ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] proc_sysctl: fix oops caused by incorrect command parameters. 2021-01-17 2:59 ` Xiaoming Ni @ 2021-01-18 10:50 ` Vlastimil Babka 0 siblings, 0 replies; 8+ messages in thread From: Vlastimil Babka @ 2021-01-18 10:50 UTC (permalink / raw) To: Xiaoming Ni, Michal Hocko, Andrew Morton, rdunlap, hkallweit1 Cc: linux-kernel, mcgrof, keescook, yzaikin, adobriyan, linux-fsdevel, andy.shevchenko, wangle6 On 1/17/21 3:59 AM, Xiaoming Ni wrote: > On 2021/1/12 19:42, Vlastimil Babka wrote: >> On 1/12/21 8:24 AM, Michal Hocko wrote: >>>>>> >>>>>> If we're going to do a separate "patch: make process_sysctl_arg() >>>>>> return an errno instead of 0" then fine, we can discuss that. But it's >>>>>> conceptually a different work from fixing this situation. >>>>>> . >>>>>> >>>>> However, are the logs generated by process_sysctl_arg() clearer and more >>>>> accurate than parse_args()? Should the logs generated by >>>>> process_sysctl_arg() be deleted? >>>> >>>> I think the individual logs are very useful and should be retained. >>> >>> Yes, other sysfs specific error messages are likely useful. I just fail >>> to see why a missing value should be handled here when there is an >>> existing handling in the caller. Not sure whether a complete shadow >>> reporting in process_sysctl_arg is a deliberate decision or not. >>> Vlastimil? >> >> Yes, it's a way to have more useful sysctl-specific reports than the generic >> ones. And I think I was inspired by some other existing code, but don't remember >> exactly. The options are: >> >> 1) the current sysctl-specific reports, return 0 as the values are only consumed >> 2) be silent and return error, invent new error codes to have generic report be >> more useful for sysctl, but inevitably lose some nuances anyway >> 3) a mix where 2) is used for situations where generic report is sufficient >> enough, 1) where not >> >> Patch v2 went with option 1), v3 with option 3). I think it's down to >> preferences. I would personally go with v2 and message similar to the existing >> ones, i.e.: >> >> "Failed to set sysctl parameter '%s': no value given\n" >> >> Also we seem to be silently doing nothing when strlen(val) == 0, i.e. >> "hung_task_panic=" was passed. Worth reporting the same error. >> >> But v3 is fine with me as well. The generic error message works. We could just >> add "if (!len) return -EINVAL" below the strlen() call. >> >> Also please Cc: stable. >> >>> Anyway one way or the other, all I care about is to have a reporting in >>> place because this shouldn't be a silent failure. >>> > > > The current v2 is already in the linux-next branch and throws a new error: > https://lore.kernel.org/lkml/cb54e349-7147-0a1f-a349-1e16ba603fce@infradead.org/ > > This bug has been mentioned in the previous discussion and has been fixed in the > current v3 patch. > https://lore.kernel.org/linux-fsdevel/202101111149.20A58E1@keescook/ > > What am I supposed to do now? > - Resend V3? IMHO this. But also please handle also len == 0 like below. And add Cc: <stable@vger.kernel.org> > - Rewrite a new fix patch based on the current code of linux-next. AFAICS Andrew dropped the v2 already. Thanks. diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c index 317899222d7f..f424010d1a60 100644 --- a/fs/proc/proc_sysctl.c +++ b/fs/proc/proc_sysctl.c @@ -1770,6 +1770,12 @@ static int process_sysctl_arg(char *param, char *val, return 0; } + if (!val) + return -EINVAL; + len = strlen(val); + if (!len) + 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 @@ -1811,7 +1817,6 @@ static int process_sysctl_arg(char *param, char *val, file, param, val); goto out; } - len = strlen(val); wret = kernel_write(file, val, len, &pos); if (wret < 0) { err = wret; ^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-01-18 11:07 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-12 3:31 [PATCH v3] proc_sysctl: fix oops caused by incorrect command parameters Xiaoming Ni 2021-01-12 4:33 ` Andrew Morton 2021-01-12 6:24 ` Xiaoming Ni 2021-01-12 6:28 ` Andrew Morton 2021-01-12 7:24 ` Michal Hocko 2021-01-12 11:42 ` Vlastimil Babka 2021-01-17 2:59 ` Xiaoming Ni 2021-01-18 10:50 ` Vlastimil Babka
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).