linux-kernel - Re: [PATCH] rtc: st-lpc: remove unnecessary check

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: <1556663479.31309.36.camel@impinj.com>
Date:   Tue, 30 Apr 2019 22:31:19 +0000
From:   Trent Piepho <tpiepho@...inj.com>
To:     "patrice.chotard@...com" <patrice.chotard@...com>,
        "linux-rtc@...r.kernel.org" <linux-rtc@...r.kernel.org>,
        "alexandre.belloni@...tlin.com" <alexandre.belloni@...tlin.com>
CC:     "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] rtc: st-lpc: remove unnecessary check

On Tue, 2019-04-30 at 22:18 +0200, Alexandre Belloni wrote:
> The RTC core already ensures the alarm is set to a time in the future, it
> is not necessary to check again in the driver.

My reading of the rtc core code is that it checks if the alarm is in
the future *twice* before handing off the set call to the driver, which
possibly checks a 3rd time (as seen here).

However, all these checks are done *before* setting the alarm.  It
still possible to have a race and set the alarm after the time has
already passed, in which case the alarm will never fire.

The way to fix the race would be to have the driver check the alarm
*after* setting it.  In precisely this order, do these steps:

1. Set alarm in RTC, to Talarm
2. Get time from RTC, as Tcurrent
3. Get alarm status from RTC

If Talarm < Tcurrent, alarm was set to future time, no error
Else
  If status == fired, alarm was set and has since fired, no error
  Else status == not fired, alarm was set in past, EINVAL

This should be race free.

>  
> -	/* Invalid alarm time */
> -	if (now_secs > alarm_secs)
> -		return -EINVAL;
> -
>  	memcpy(&rtc->alarm, t, sizeof(struct rtc_wkalrm));
>  
>  	/* Now many secs to fire */
        alarm_secs -= now_secs;
        lpa = (unsigned long long)alarm_secs * rtc->clkrate;

While it's true the time wouldn't normally be in past, it still races,
as describe above. In that case, the math here underflows alarm_secs,
so it probably still makes sense to check.