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

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@gmail.com>
> Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> ---
>  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

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

[Chen, Yu C]

Hi Rafael,

> -----Original Message-----
> From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> Sent: Tuesday, July 19, 2016 6:02 AM
> To: Chen, Yu C
> Cc: John Stultz; Thomas Gleixner; linux-kernel@vger.kernel.org; Linux PM list
> Subject: Re: [PATCH] timekeeping: Fix memory overwrite of sleep_time_bin
> array
> 
> 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@gmail.com>
> > Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> > ---
> >  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?
Yes, we can check the maximal value for tv_sec, and don't have to extend sleep_time_bin array.

thanks,
Yu