linux-kernel - Re: [PATCH] timekeeping: Cap array access in timekeeping_debug to protect against invalid sleep times

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]

Date:	Thu, 18 Aug 2016 03:09:29 +0200
From:	"Rafael J. Wysocki" <rjw@...ysocki.net>
To:	John Stultz <john.stultz@...aro.org>
Cc:	Chen Yu <yu.c.chen@...el.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Janek Kozicki <cosurgi@...il.com>,
	Xunlei Pang <xpang@...hat.com>,
	Zhang Rui <rui.zhang@...el.com>, linux-kernel@...r.kernel.org,
	linux-pm@...r.kernel.org
Subject: Re: [PATCH] timekeeping: Cap array access in timekeeping_debug to protect against invalid sleep times

On Wednesday, August 17, 2016 12:18:50 PM John Stultz wrote:
> It was reported that hibernation could fail on the 2nd attempt,
> where the system hangs at hibernate() -> syscore_resume() ->
> i8237A_resume() -> claim_dma_lock(), because the lock has
> already been taken.
> 
> However there is actually no other process would like to grab
> this lock on that problematic platform.
> 
> Further investigation showed that the problem is triggered by
> setting /sys/power/pm_trace to 1 before the 1st hibernation.
> 
> Since once pm_trace is enabled, the rtc becomes unmeaningful
> after suspend, and meanwhile some BIOSes would like to adjust
> the 'invalid' tsc(e.g, smaller than 1970) to the release date
> of that motherboard during POST stage, thus after resumed, it
> may seem that the system had a significant long sleep time might
> due to meaningless tsc or RTC delta.
> 
> Then in timekeeping_resume -> tk_debug_account_sleep_time, if
> the bit31 of the sleep time happened to be set to 1, the fls
> returns 32 and then we add 1 to sleep_time_bin[32], which
> caused a memory overwritten.
> 
> As depicted by System.map:
> ffffffff81c9d080 b sleep_time_bin
> ffffffff81c9d100 B dma_spin_lock
> the dma_spin_lock.val is set to 1, which caused this problem.
> 
> This patch adds a sanity check in tk_debug_account_sleep_time()
> to ensure we don't index past the sleep_time_bin array.
> 
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Cc: Rafael J. Wysocki <rjw@...ysocki.net>
> Cc: Janek Kozicki <cosurgi@...il.com>
> Cc: Chen Yu <yu.c.chen@...el.com>
> Cc: Xunlei Pang <xpang@...hat.com>
> Cc: Zhang Rui <rui.zhang@...el.com>
> Cc: linux-kernel@...r.kernel.org
> Cc: linux-pm@...r.kernel.org
> Reported-by: Janek Kozicki <cosurgi@...il.com>
> Reported-by: Chen Yu <yu.c.chen@...el.com>
> [jstultz: Problem diagnosed and original patch by Chen Yu, I've
>  solved the issue slightly differently, but borrowed his excelent
>  explanation of of the issue here.]
> Signed-off-by: John Stultz <john.stultz@...aro.org>
> ---
>  kernel/time/timekeeping_debug.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/time/timekeeping_debug.c b/kernel/time/timekeeping_debug.c
> index f6bd652..107310a6 100644
> --- a/kernel/time/timekeeping_debug.c
> +++ b/kernel/time/timekeeping_debug.c
> @@ -23,7 +23,9 @@
>  
>  #include "timekeeping_internal.h"
>  
> -static unsigned int sleep_time_bin[32] = {0};
> +#define NUM_BINS 32
> +
> +static unsigned int sleep_time_bin[NUM_BINS] = {0};
>  
>  static int tk_debug_show_sleep_time(struct seq_file *s, void *data)
>  {
> @@ -69,6 +71,9 @@ late_initcall(tk_debug_sleep_time_init);
>  
>  void tk_debug_account_sleep_time(struct timespec64 *t)
>  {
> -	sleep_time_bin[fls(t->tv_sec)]++;
> +	/* Cap bin index so we don't overflow the array */
> +	int bin = min(fls(t->tv_sec), NUM_BINS-1);
> +
> +	sleep_time_bin[bin]++;
>  }
>  
> 


If pm_trace_enabled is set, we can (or maybe even should) just skip
timekeeping_inject_sleeptime() entirely in rtc_resume() at least, because
sleep_time is almost certainly bogus in that case, even if it doesn't
overflow.

Of course, the above is still needed then.

Thanks,
Rafael