linux-kernel - Re: [PATCH v2] panic: prevent panic

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20200713185739.03d576cca0dc9f618ea76d67@linux-foundation.org>
Date:   Mon, 13 Jul 2020 18:57:39 -0700
From:   Andrew Morton <akpm@...ux-foundation.org>
To:     Changming <charley.ashbringer@...il.com>
Cc:     willy@...radead.org, rdunlap@...radead.org, keescook@...omium.org,
        mcgrof@...nel.org, yzaikin@...gle.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] panic: prevent panic_timeout * 1000 from overflow

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

> From: Changming Liu <charley.ashbringer@...il.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.