Re: [PATCH] timekeeping: Fix memory overwrite of sleep_time_bin array

[Rafael J. Wysocki]

On Monday, July 18, 2016 09:42:19 AM Chen Yu wrote:
> It is reported the hibernation fails at 2nd attempt, which
> 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 shows that, the problem is caused by setting
> /sys/power/pm_trace to 1 before the 1st hibernation, since once
> pm_trace is enabled, the rtc becomes an unmeaningful value after resumed,
> which might bring a significant long sleep time in timekeeping_resume,
> thus in tk_debug_account_sleep_time, the delta of timespec64 might
> exceed 32bit after commit 7d489d15ce4b ("timekeeping: Convert timekeeping
> core to use timespec64s"), thus if the bit31 happened set to 1, the
> fls might return 32 and then we add 1 to sleep_time_bin[32], which
> caused a memory overwritten. As System.map shows:
> 
> ffffffff81c9d080 b sleep_time_bin
> ffffffff81c9d100 B dma_spin_lock
> 
> Thus set the dma_spin_lock.val to 1, which caused this problem.

Nice catch!

> This patch fixes this issue by extending sleep_time_bin to 64, and
> use __fls to be fit for timespec64.
> 
> Fixes: 7d489d15ce4b ("timekeeping: Convert timekeeping core to use timespec64s")
> Reported-and-tested-by: Janek Kozicki <cosurgi@xxxxxxxxx>
> Signed-off-by: Chen Yu <yu.c.chen@xxxxxxxxx>
> ---
>  kernel/time/timekeeping_debug.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/time/timekeeping_debug.c b/kernel/time/timekeeping_debug.c
> index f6bd652..12b07d5 100644
> --- a/kernel/time/timekeeping_debug.c
> +++ b/kernel/time/timekeeping_debug.c
> @@ -23,14 +23,14 @@
>  
>  #include "timekeeping_internal.h"
>  
> -static unsigned int sleep_time_bin[32] = {0};
> +static unsigned int sleep_time_bin[64] = {0};
>  
>  static int tk_debug_show_sleep_time(struct seq_file *s, void *data)
>  {
>  	unsigned int bin;
>  	seq_puts(s, "      time (secs)        count\n");
>  	seq_puts(s, "------------------------------\n");
> -	for (bin = 0; bin < 32; bin++) {
> +	for (bin = 0; bin < 64; bin++) {
>  		if (sleep_time_bin[bin] == 0)
>  			continue;
>  		seq_printf(s, "%10u - %-10u %4u\n",
> @@ -69,6 +69,7 @@ late_initcall(tk_debug_sleep_time_init);
>  
>  void tk_debug_account_sleep_time(struct timespec64 *t)
>  {
> -	sleep_time_bin[fls(t->tv_sec)]++;
> +	if (t->tv_sec > 0)
> +		sleep_time_bin[__fls(t->tv_sec)]++;

But you could simply validate t->tv_sec here without extending sleeo_time_bin[]
and switching over to __fls(), couldn't you?

Thanks,
Rafael