| 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
| ||
|
Date: Sun, 06 Dec 2020 22:46:14 +0100
From: Thomas Gleixner <tglx@...utronix.de>
To: LKML <linux-kernel@...r.kernel.org>
Cc: Alexandre Belloni <alexandre.belloni@...tlin.com>,
Jason Gunthorpe <jgg@...pe.ca>,
Miroslav Lichvar <mlichvar@...hat.com>,
John Stultz <john.stultz@...aro.org>,
Prarit Bhargava <prarit@...hat.com>,
Alessandro Zummo <a.zummo@...ertech.it>,
linux-rtc@...r.kernel.org, Peter Zijlstra <peterz@...radead.org>
Subject: [patch 1/8] rtc: mc146818: Prevent reading garbage
The MC146818 driver is prone to read garbage from the RTC. There are
several issues all related to the update cycle of the MC146818. The chip
increments seconds obviously once per second and indicates that by a bit in
a register. The bit goes high 244us before the actual update starts. During
the update the readout of the time values is undefined.
The code just checks whether the update in progress bit (UIP) is set before
reading the clock. If it's set it waits arbitrary 20ms before retrying,
which is ample because the maximum update time is ~2ms.
But this check does not guarantee that the UIP bit goes high and the actual
update happens during the readout. So the following can happen
0.997 UIP = False
-> Interrupt/NMI/preemption
0.998 UIP -> True
0.999 Readout <- Undefined
To prevent this rework the code so it checks UIP before and after the
readout and if set after the readout try again.
But that's not enough to cover the following:
0.997 UIP = False
Readout seconds
-> NMI (or vCPU scheduled out)
0.998 UIP -> True
update completes
UIP -> False
1.000 Readout minutes,....
UIP check succeeds
That can make the readout wrong up to 59 seconds.
To prevent this, read the seconds value before the first UIP check,
validate it after checking UIP and after reading out the rest.
It's amazing that the original i386 code had this actually correct and
the generic implementation of the MC146818 driver got it wrong in 2002 and
it stayed that way until today.
Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
---
drivers/rtc/rtc-mc146818-lib.c | 64 ++++++++++++++++++++++++-----------------
1 file changed, 39 insertions(+), 25 deletions(-)
--- a/drivers/rtc/rtc-mc146818-lib.c
+++ b/drivers/rtc/rtc-mc146818-lib.c
@@ -8,41 +8,41 @@
#include <linux/acpi.h>
#endif
-/*
- * Returns true if a clock update is in progress
- */
-static inline unsigned char mc146818_is_updating(void)
-{
- unsigned char uip;
- unsigned long flags;
-
- spin_lock_irqsave(&rtc_lock, flags);
- uip = (CMOS_READ(RTC_FREQ_SELECT) & RTC_UIP);
- spin_unlock_irqrestore(&rtc_lock, flags);
- return uip;
-}
-
unsigned int mc146818_get_time(struct rtc_time *time)
{
unsigned char ctrl;
unsigned long flags;
unsigned char century = 0;
+ bool retry;
#ifdef CONFIG_MACH_DECSTATION
unsigned int real_year;
#endif
+again:
+ spin_lock_irqsave(&rtc_lock, flags);
/*
- * read RTC once any update in progress is done. The update
- * can take just over 2ms. We wait 20ms. There is no need to
- * to poll-wait (up to 1s - eeccch) for the falling edge of RTC_UIP.
- * If you need to know *exactly* when a second has started, enable
- * periodic update complete interrupts, (via ioctl) and then
- * immediately read /dev/rtc which will block until you get the IRQ.
- * Once the read clears, read the RTC time (again via ioctl). Easy.
+ * Check whether there is an update in progress during which the
+ * readout is unspecified. The maximum update time is ~2ms. Poll
+ * every msec for completion.
+ *
+ * Store the second value before checking UIP so a long lasting NMI
+ * which happens to hit after the UIP check cannot make an update
+ * cycle invisible.
*/
- if (mc146818_is_updating())
- mdelay(20);
+ time->tm_sec = CMOS_READ(RTC_SECONDS);
+
+ if (CMOS_READ(RTC_FREQ_SELECT) & RTC_UIP) {
+ spin_unlock_irqrestore(&rtc_lock, flags);
+ mdelay(1);
+ goto again;
+ }
+
+ /* Revalidate the above readout */
+ if (time->tm_sec != CMOS_READ(RTC_SECONDS)) {
+ spin_unlock_irqrestore(&rtc_lock, flags);
+ goto again;
+ }
/*
* Only the values that we read from the RTC are set. We leave
@@ -50,8 +50,6 @@ unsigned int mc146818_get_time(struct rt
* RTC has RTC_DAY_OF_WEEK, we ignore it, as it is only updated
* by the RTC when initially set to a non-zero value.
*/
- spin_lock_irqsave(&rtc_lock, flags);
- time->tm_sec = CMOS_READ(RTC_SECONDS);
time->tm_min = CMOS_READ(RTC_MINUTES);
time->tm_hour = CMOS_READ(RTC_HOURS);
time->tm_mday = CMOS_READ(RTC_DAY_OF_MONTH);
@@ -66,8 +64,24 @@ unsigned int mc146818_get_time(struct rt
century = CMOS_READ(acpi_gbl_FADT.century);
#endif
ctrl = CMOS_READ(RTC_CONTROL);
+ /*
+ * Check for the UIP bit again. If it is set now then
+ * the above values may contain garbage.
+ */
+ retry = CMOS_READ(RTC_FREQ_SELECT) & RTC_UIP;
+ /*
+ * A NMI might have interrupted the above sequence so check whether
+ * the seconds value has changed which indicates that the NMI took
+ * longer than the UIP bit was set. Unlikely, but possible and
+ * there is also virt...
+ */
+ retry |= time->tm_sec != CMOS_READ(RTC_SECONDS);
+
spin_unlock_irqrestore(&rtc_lock, flags);
+ if (retry)
+ goto again;
+
if (!(ctrl & RTC_DM_BINARY) || RTC_ALWAYS_BCD)
{
time->tm_sec = bcd2bin(time->tm_sec);
Powered by blists - more mailing lists