| Message ID | 1594351343-11811-1-git-send-email-charley.ashbringer@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sysctl: add bound to panic_timeout to prevent overflow | expand |
On 7/9/20 8:22 PM, Changming Liu wrote: > Function panic() in kernel/panic.c will use panic_timeout > multiplying 1000 as a loop boundery. So this multiplication boundary. > can overflow when panic_timeout is greater than (INT_MAX/1000). > And this results in a zero-delay panic, instead of a huge > timeout as the user intends. > > Fix this by adding bound check to make it no bigger than > (INT_MAX/1000). > > Signed-off-by: Changming Liu <charley.ashbringer@gmail.com> > --- > kernel/sysctl.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/kernel/sysctl.c b/kernel/sysctl.c > index db1ce7a..e60cf04 100644 > --- a/kernel/sysctl.c > +++ b/kernel/sysctl.c > @@ -137,6 +137,9 @@ static int minolduid; > static int ngroups_max = NGROUPS_MAX; > static const int cap_last_cap = CAP_LAST_CAP; > > +/* this is needed for setting boundery for panic_timeout to prevent it from overflow*/ boundary (or max value) overflow */ > +static int panic_time_max = INT_MAX / 1000; > + > /* > * This is needed for proc_doulongvec_minmax of sysctl_hung_task_timeout_secs > * and hung_task_check_interval_secs > @@ -1857,7 +1860,8 @@ static struct ctl_table kern_table[] = { > .data = &panic_timeout, > .maxlen = sizeof(int), > .mode = 0644, > - .proc_handler = proc_dointvec, > + .proc_handler = proc_dointvec_minmax, > + .extra2 = &panic_time_max, > }, > #ifdef CONFIG_COREDUMP > { > thanks.
On Thu, Jul 09, 2020 at 08:31:39PM -0700, Randy Dunlap wrote: > > +/* this is needed for setting boundery for panic_timeout to prevent it from overflow*/ > > boundary (or max value) overflow */ > > > +static int panic_time_max = INT_MAX / 1000; Or just simplify the comment. /* Prevent overflow in panic() */ Or perhaps better, fix panic() to not overflow. - for (i = 0; i < panic_timeout * 1000; i += PANIC_TIMER_STEP) { + for (i = 0; i / 1000 < panic_timeout; i += PANIC_TIMER_STEP) { you probably also want to change i to be a long long or the loop may never terminate.
> On Thu, Jul 09, 2020 at 08:31:39PM -0700, Randy Dunlap wrote: > > > +/* this is needed for setting boundery for panic_timeout to prevent > > > +it from overflow*/ > > > > boundary (or max value) overflow */ > > > > > +static int panic_time_max = INT_MAX / 1000; > > Or just simplify the comment. > > /* Prevent overflow in panic() */ > > Or perhaps better, fix panic() to not overflow. > > - for (i = 0; i < panic_timeout * 1000; i += PANIC_TIMER_STEP) { > + for (i = 0; i / 1000 < panic_timeout; i += PANIC_TIMER_STEP) { > > you probably also want to change i to be a long long or the loop may never > terminate. Thanks for the feedback, I too agree this should be better than modifying the sysctl, considering how localized and neat this change is. It's also more readable. Setting a bound in sysctl.c which is dependent on the constant value in panic.c is not a very good idea. I agree changing i from long to long long is necessary. I'll submit a v2 patch enforcing this shortly. Cheers, Changming
diff --git a/kernel/sysctl.c b/kernel/sysctl.c index db1ce7a..e60cf04 100644 --- a/kernel/sysctl.c +++ b/kernel/sysctl.c @@ -137,6 +137,9 @@ static int minolduid; static int ngroups_max = NGROUPS_MAX; static const int cap_last_cap = CAP_LAST_CAP; +/* this is needed for setting boundery for panic_timeout to prevent it from overflow*/ +static int panic_time_max = INT_MAX / 1000; + /* * This is needed for proc_doulongvec_minmax of sysctl_hung_task_timeout_secs * and hung_task_check_interval_secs @@ -1857,7 +1860,8 @@ static struct ctl_table kern_table[] = { .data = &panic_timeout, .maxlen = sizeof(int), .mode = 0644, - .proc_handler = proc_dointvec, + .proc_handler = proc_dointvec_minmax, + .extra2 = &panic_time_max, }, #ifdef CONFIG_COREDUMP {
Function panic() in kernel/panic.c will use panic_timeout multiplying 1000 as a loop boundery. So this multiplication can overflow when panic_timeout is greater than (INT_MAX/1000). And this results in a zero-delay panic, instead of a huge timeout as the user intends. Fix this by adding bound check to make it no bigger than (INT_MAX/1000). Signed-off-by: Changming Liu <charley.ashbringer@gmail.com> --- kernel/sysctl.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)