[PATCH] sysctl: add bound to panic_timeout to prevent overflow

[PATCH] sysctl: add bound to panic_timeout to prevent overflow

[Changming Liu]

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(-)

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
 	{
-- 
2.7.4

Re: [PATCH] sysctl: add bound to panic_timeout to prevent overflow

[Randy Dunlap]

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.
-- 
~Randy

Re: [PATCH] sysctl: add bound to panic_timeout to prevent overflow

[Matthew Wilcox]

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.

RE: [PATCH] sysctl: add bound to panic_timeout to prevent overflow

[charley.ashbringer]

> 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