[PATCH v2] panic: prevent panic_timeout * 1000 from overflow

[PATCH v2] panic: prevent panic_timeout * 1000 from overflow

[Changming]

From: Changming Liu <charley.ashbringer@gmail.com>

Since panic_timeout is an integer passed-in through sysctl,
the loop boundary panic_timeout * 1000 could overflow and
result in a zero-delay panic when panic_timeout is greater
than INT_MAX/1000.

Fix this by moving 1000 to the left, also in case i/1000
might never be greater than panic_timeout, change i to
long long so that it strictly has more bits.

Signed-off-by: Changming Liu <charley.ashbringer@gmail.com>
---
Changes in v2:
- change the loop in panic, instead of change the sysctl

 kernel/panic.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/panic.c b/kernel/panic.c
index e2157ca387c8..941848aac0ee 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -178,7 +178,8 @@ void panic(const char *fmt, ...)
 {
 	static char buf[1024];
 	va_list args;
-	long i, i_next = 0, len;
+	long long i;
+	long i_next = 0, len;
 	int state = 0;
 	int old_cpu, this_cpu;
 	bool _crash_kexec_post_notifiers = crash_kexec_post_notifiers;
@@ -315,7 +316,7 @@ void panic(const char *fmt, ...)
 		 */
 		pr_emerg("Rebooting in %d seconds..\n", panic_timeout);
 
-		for (i = 0; i < panic_timeout * 1000; i += PANIC_TIMER_STEP) {
+		for (i = 0; i / 1000 < panic_timeout; i += PANIC_TIMER_STEP) {
 			touch_nmi_watchdog();
 			if (i >= i_next) {
 				i += panic_blink(state ^= 1);
-- 
2.17.1

Re: [PATCH v2] panic: prevent panic_timeout * 1000 from overflow

[Andrew Morton]

On Sat, 11 Jul 2020 01:17:28 -0400 Changming <charley.ashbringer@gmail.com> wrote:

> From: Changming Liu <charley.ashbringer@gmail.com>
> 
> Since panic_timeout is an integer passed-in through sysctl,
> the loop boundary panic_timeout * 1000 could overflow and
> result in a zero-delay panic when panic_timeout is greater
> than INT_MAX/1000.
> 
> Fix this by moving 1000 to the left, also in case i/1000
> might never be greater than panic_timeout, change i to
> long long so that it strictly has more bits.
> 
> ...
>
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -178,7 +178,8 @@ void panic(const char *fmt, ...)
>  {
>  	static char buf[1024];
>  	va_list args;
> -	long i, i_next = 0, len;
> +	long long i;
> +	long i_next = 0, len;
>  	int state = 0;
>  	int old_cpu, this_cpu;
>  	bool _crash_kexec_post_notifiers = crash_kexec_post_notifiers;
> @@ -315,7 +316,7 @@ void panic(const char *fmt, ...)
>  		 */
>  		pr_emerg("Rebooting in %d seconds..\n", panic_timeout);
>  
> -		for (i = 0; i < panic_timeout * 1000; i += PANIC_TIMER_STEP) {
> +		for (i = 0; i / 1000 < panic_timeout; i += PANIC_TIMER_STEP) {

Problem is, 32-bit machines generally cannot perform 64-bit divides. 
So a call is emitted to the library function __divsi64() (I forget the exact
name) which Linux doesn't implement (because it's so slow, and we don't
want to be calling it by accident).

So a fix would be to call do_div() or something from
include/linux/div64.h but it's all a great mess.

However we can do native 64-bit multiplication on 32-bit!  So how about
something like

--- a/kernel/panic.c~a
+++ a/kernel/panic.c
@@ -313,13 +313,16 @@ void panic(const char *fmt, ...)
 		 * Delay timeout seconds before rebooting the machine.
 		 * We can't use the "normal" timers since we just panicked.
 		 */
+		u64 timeout = panic_timeout * 1000;	/* avoid overflow */
+		u64 timer;
+
 		pr_emerg("Rebooting in %d seconds..\n", panic_timeout);
 
-		for (i = 0; i < panic_timeout * 1000; i += PANIC_TIMER_STEP) {
+		for (timer = 0; timer < timeout; timer += PANIC_TIMER_STEP) {
 			touch_nmi_watchdog();
-			if (i >= i_next) {
-				i += panic_blink(state ^= 1);
-				i_next = i + 3600 / PANIC_BLINK_SPD;
+			if (timer >= i_next) {
+				timer += panic_blink(state ^= 1);
+				i_next = timer + 3600 / PANIC_BLINK_SPD;
 			}
 			mdelay(PANIC_TIMER_STEP);
 		}

(untested)

There's still the 3600/PANIC_BLINK_SPD in there, but a) that will be
done at compile-time and b) the 64-bit promotion should be done after
the division.

And... oh crap, i_next needs to be 64-bit as well.

Re: [PATCH v2] panic: prevent panic_timeout * 1000 from overflow

[Matthew Wilcox]

On Mon, Jul 13, 2020 at 06:57:39PM -0700, Andrew Morton wrote:
> +++ a/kernel/panic.c
> @@ -313,13 +313,16 @@ void panic(const char *fmt, ...)
>  		 * Delay timeout seconds before rebooting the machine.
>  		 * We can't use the "normal" timers since we just panicked.
>  		 */
> +		u64 timeout = panic_timeout * 1000;	/* avoid overflow */

1000ULL to not truncate before the assignment.

> +		u64 timer;

... as you implied later

		u64 timer, timer_next;

> (untested)

Well, there's the rub.  Testing requires 49 days (2^32 / 1000 seconds).

[PATCH v3] panic: prevent panic_timeout * 1000 from overflow

[Changming Liu]

Since panic_timeout is an s32 integer passed in through sysctl, the
loop boundary panic_timeout * 1000 could overflow and result in
a zero-delay panic when panic_timeout is greater than INT_MAX/1000.

Fix this by elevating the precision of the loop boundary via 
assigning the result to a u64 integer, also in case the loop 
counter i might never be greater than u64 timeout = panic_timeout*1000,
elevate its precision to u64(timer) as well. The same applies to
timer_next replacing i_next which is initialized to 0.

Signed-off-by: Changming Liu <charley.ashbringer@gmail.com>
---
Changes in v3:
- change the loop in panic, instead of change the sysctl
- avoid using 64-bit division, doing 64-bit mult instead

 kernel/panic.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/kernel/panic.c b/kernel/panic.c
index e2157ca..ef6cd57 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -313,13 +313,16 @@ void panic(const char *fmt, ...)
 		 * Delay timeout seconds before rebooting the machine.
 		 * We can't use the "normal" timers since we just panicked.
 		 */
+		u64 timeout = panic_timeout * 1000ULL;	/* avoid overflow */
+		u64 timer, timer_next = 0;
+
 		pr_emerg("Rebooting in %d seconds..\n", panic_timeout);
 
-		for (i = 0; i < panic_timeout * 1000; i += PANIC_TIMER_STEP) {
+		for (timer = 0; timer < timeout; timer += PANIC_TIMER_STEP) {
 			touch_nmi_watchdog();
-			if (i >= i_next) {
-				i += panic_blink(state ^= 1);
-				i_next = i + 3600 / PANIC_BLINK_SPD;
+			if (timer >= timer_next) {
+				timer += panic_blink(state ^= 1);
+				timer_next = timer + 3600 / PANIC_BLINK_SPD;
 			}
 			mdelay(PANIC_TIMER_STEP);
 		}
-- 
2.7.4

RE: [PATCH v2] panic: prevent panic_timeout * 1000 from overflow

[charley.ashbringer]

> > Since panic_timeout is an integer passed-in through sysctl,
> > the loop boundary panic_timeout * 1000 could overflow and
> > result in a zero-delay panic when panic_timeout is greater
> > than INT_MAX/1000.
> >
> > Fix this by moving 1000 to the left, also in case i/1000
> > might never be greater than panic_timeout, change i to
> > long long so that it strictly has more bits.
> >
> > ...
> >
> > --- a/kernel/panic.c
> > +++ b/kernel/panic.c
> > @@ -178,7 +178,8 @@ void panic(const char *fmt, ...)
> >  {
> >  	static char buf[1024];
> >  	va_list args;
> > -	long i, i_next = 0, len;
> > +	long long i;
> > +	long i_next = 0, len;
> >  	int state = 0;
> >  	int old_cpu, this_cpu;
> >  	bool _crash_kexec_post_notifiers = crash_kexec_post_notifiers;
> > @@ -315,7 +316,7 @@ void panic(const char *fmt, ...)
> >  		 */
> >  		pr_emerg("Rebooting in %d seconds..\n", panic_timeout);
> >
> > -		for (i = 0; i < panic_timeout * 1000; i += PANIC_TIMER_STEP)
{
> > +		for (i = 0; i / 1000 < panic_timeout; i += PANIC_TIMER_STEP)
{
> 
> Problem is, 32-bit machines generally cannot perform 64-bit divides.
> So a call is emitted to the library function __divsi64() (I forget the
exact
> name) which Linux doesn't implement (because it's so slow, and we don't
> want to be calling it by accident).
> 

It's good to know, thanks for letting me know why 64-bit division 
is slow, and 64-multiplication is fast, surely doing so many
64-bit division will drag a lot, and should be prevented.

> So a fix would be to call do_div() or something from
> include/linux/div64.h but it's all a great mess.
> 
> However we can do native 64-bit multiplication on 32-bit!  So how about
> something like
> 
> --- a/kernel/panic.c~a
> +++ a/kernel/panic.c
> @@ -313,13 +313,16 @@ void panic(const char *fmt, ...)
>  		 * Delay timeout seconds before rebooting the machine.
>  		 * We can't use the "normal" timers since we just panicked.
>  		 */
> +		u64 timeout = panic_timeout * 1000;	/* avoid overflow */
> +		u64 timer;
>		pr_emerg("Rebooting in %d seconds..\n", panic_timeout);
> -		for (i = 0; i < panic_timeout * 1000; i += PANIC_TIMER_STEP)
{
> +		for (timer = 0; timer < timeout; timer += PANIC_TIMER_STEP)
{

If using u64 as the loop boundary, would it be a problem if
panic_timeout is negative? Since in the current code, if
panic_timeout is negative, the loop will not be executed;
as in the patched code, the loop boundary will be a huge 
unsigned value. I guess s64 should do？

If it's not a problem, I'll submit another patch enforcing
the change, including the changes suggested by Matthew here:

> > +		u64 timeout = panic_timeout * 1000;	/* avoid overflow */
> 1000ULL to not truncate before the assignment.

> > +		u64 timer;
> ... as you implied lateru64 timer, timer_next;


Thank you guys so much for your valuable feedback, I learned a lot!

Best,
Changming