linux-kernel - Re: [RFC][PATCH 1/2] time: alarmtimer: Fix erroneous case of using 0 as an "invalid" initialization value

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: <CAOf5uwncu5Otb+aQJiOL=twv6tPhNxb9-_pKQEkwe1TZX3gNNQ@mail.gmail.com>
Date:   Sat, 18 Feb 2023 15:51:40 +0100
From:   Michael Nazzareno Trimarchi <michael@...rulasolutions.com>
To:     John Stultz <jstultz@...gle.com>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Stephen Boyd <sboyd@...nel.org>, Arnd Bergmann <arnd@...db.de>,
        Michael <michael@...isi.de>, kernel-team@...roid.com
Subject: Re: [RFC][PATCH 1/2] time: alarmtimer: Fix erroneous case of using 0
 as an "invalid" initialization value

On Sat, Feb 11, 2023 at 7:45 AM John Stultz <jstultz@...gle.com> wrote:
>
> Michael reported seeing an error where alarmtimers would
> occasionally not wake the system up.
>
> It was found that in alarmtimer_suspend() it was exiting via
> the:
>     if (min == 0)
>         return 0;
> check. This logic was from one of the early versions of the
> original alarmtimer patch, where we initialized min to 0, and
> then this check would exit early if we found no timers to expire
> (leaving min still at 0).
>
> However, its possible for an alarmtimer to expire as we are
> checking it, leaving the calculated delta to be zero, and thus
> setting min to zero.
>
> This is the result of my using 0 as an invalid time value which
> is clearly erroneous. Instead KTIME_MAX should have been used.
>
> This patch, split out from a change originally suggested by
> Thomas Gleixner, changes the logic to instead use KTIME_MAX.
>
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Cc: Stephen Boyd <sboyd@...nel.org>
> Cc: Arnd Bergmann <arnd@...db.de>
> Cc: Michael <michael@...isi.de>
> Cc: Michael Trimarchi <michael@...rulasolutions.com>
> Cc: kernel-team@...roid.com
> Reported-by: Michael <michael@...isi.de>
> Reported-by: Michael Trimarchi <michael@...rulasolutions.com>
> Fixes: ff3ead96d17f ("timers: Introduce in-kernel alarm-timer interface")
> Originally-by: Thomas Gleixner <tglx@...utronix.de>
> Link: https://lore.kernel.org/lkml/alpine.DEB.2.21.1909021247250.3955@nanos.tec.linutronix.de/
> [jstultz: Forward ported to 6.2-rc, and split out just the
>           KTIME_MAX change]
> Signed-off-by: John Stultz <jstultz@...gle.com>
> ---
>  kernel/time/alarmtimer.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
> index 5897828b9d7e..f7b2128f64e2 100644
> --- a/kernel/time/alarmtimer.c
> +++ b/kernel/time/alarmtimer.c
> @@ -251,7 +251,7 @@ static int alarmtimer_suspend(struct device *dev)
>         min = freezer_delta;
>         expires = freezer_expires;
>         type = freezer_alarmtype;
> -       freezer_delta = 0;
> +       freezer_delta = KTIME_MAX;
>         spin_unlock_irqrestore(&freezer_delta_lock, flags);
>
>         rtc = alarmtimer_get_rtcdev();
> @@ -271,13 +271,14 @@ static int alarmtimer_suspend(struct device *dev)
>                 if (!next)
>                         continue;
>                 delta = ktime_sub(next->expires, base->get_ktime());
> -               if (!min || (delta < min)) {
> +               if (delta < min) {
>                         expires = next->expires;
>                         min = delta;
>                         type = i;
>                 }
>         }
> -       if (min == 0)
> +       /* No timers to expire */
> +       if (min == KTIME_MAX)
>                 return 0;
>
>         if (ktime_to_ns(min) < 2 * NSEC_PER_SEC) {
> @@ -503,7 +504,7 @@ static void alarmtimer_freezerset(ktime_t absexp, enum alarmtimer_type type)
>         delta = ktime_sub(absexp, base->get_ktime());
>
>         spin_lock_irqsave(&freezer_delta_lock, flags);
> -       if (!freezer_delta || (delta < freezer_delta)) {
> +       if (delta < freezer_delta) {
>                 freezer_delta = delta;
>                 freezer_expires = absexp;
>                 freezer_alarmtype = type;
> --
> 2.39.1.581.gbfd45094c4-goog
>

Tested-by: Michael Trimarchi <michael@...rulasolutions.com>

I don't find regression on this

Michael
-- 
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
michael@...rulasolutions.com
__________________________________

Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
info@...rulasolutions.com
www.amarulasolutions.com