| 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, 4 Oct 2020 03:43:23 +0200
From: Jonathan Neuschäfer <j.neuschaefer@....net>
To: Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
Cc: Jonathan Neuschäfer <j.neuschaefer@....net>,
linux-kernel@...r.kernel.org,
Alexandre Belloni <alexandre.belloni@...tlin.com>,
Heiko Stuebner <heiko@...ech.de>, linux-pwm@...r.kernel.org,
Linus Walleij <linus.walleij@...aro.org>,
Thierry Reding <thierry.reding@...il.com>,
Fabio Estevam <festevam@...il.com>, linux-rtc@...r.kernel.org,
Arnd Bergmann <arnd@...db.de>,
Mauro Carvalho Chehab <mchehab+huawei@...nel.org>,
Sam Ravnborg <sam@...nborg.org>,
Daniel Palmer <daniel@...f.com>,
Andy Shevchenko <andy.shevchenko@...il.com>,
Andreas Kemnade <andreas@...nade.info>,
NXP Linux Team <linux-imx@....com>,
devicetree@...r.kernel.org, Stephan Gerhold <stephan@...hold.net>,
allen <allen.chen@....com.tw>,
Sascha Hauer <s.hauer@...gutronix.de>,
Lubomir Rintel <lkundrak@...sk>,
Rob Herring <robh+dt@...nel.org>,
Lee Jones <lee.jones@...aro.org>,
linux-arm-kernel@...ts.infradead.org,
Alessandro Zummo <a.zummo@...ertech.it>,
Mark Brown <broonie@...nel.org>,
Pengutronix Kernel Team <kernel@...gutronix.de>,
Heiko Stuebner <heiko.stuebner@...obroma-systems.com>,
Josua Mayer <josua.mayer@....eu>,
Shawn Guo <shawnguo@...nel.org>,
"David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH v3 5/7] rtc: New driver for RTC in Netronix embedded
controller
On Fri, Sep 25, 2020 at 07:44:24AM +0200, Uwe Kleine-König wrote:
> Hello Jonathan,
>
> On Thu, Sep 24, 2020 at 09:24:53PM +0200, Jonathan Neuschäfer wrote:
> > +#define NTXEC_REG_WRITE_YEAR 0x10
> > +#define NTXEC_REG_WRITE_MONTH 0x11
> > +#define NTXEC_REG_WRITE_DAY 0x12
> > +#define NTXEC_REG_WRITE_HOUR 0x13
> > +#define NTXEC_REG_WRITE_MINUTE 0x14
> > +#define NTXEC_REG_WRITE_SECOND 0x15
> > +
> > +#define NTXEC_REG_READ_YM 0x20
> > +#define NTXEC_REG_READ_DH 0x21
> > +#define NTXEC_REG_READ_MS 0x23
>
> Is this an official naming? I think at least ..._MS is a poor name.
> Maybe consider ..._MINSEC instead and make the other two names a bit longer
> for consistency?
It's inofficial (the vendor kernel uses 0x10 etc. directly).
I'll pick longer names.
> > +static int ntxec_read_time(struct device *dev, struct rtc_time *tm)
> > +{
> > + struct ntxec_rtc *rtc = dev_get_drvdata(dev);
> > + unsigned int value;
> > + int res;
> > +
> > + res = regmap_read(rtc->ec->regmap, NTXEC_REG_READ_YM, &value);
> > + if (res < 0)
> > + return res;
> > +
> > + tm->tm_year = (value >> 8) + 100;
> > + tm->tm_mon = (value & 0xff) - 1;
> > +
> > + res = regmap_read(rtc->ec->regmap, NTXEC_REG_READ_DH, &value);
> > + if (res < 0)
> > + return res;
> > +
> > + tm->tm_mday = value >> 8;
> > + tm->tm_hour = value & 0xff;
> > +
> > + res = regmap_read(rtc->ec->regmap, NTXEC_REG_READ_MS, &value);
> > + if (res < 0)
> > + return res;
> > +
> > + tm->tm_min = value >> 8;
> > + tm->tm_sec = value & 0xff;
> > +
> > + return 0;
> > +}
> > +
> > +static int ntxec_set_time(struct device *dev, struct rtc_time *tm)
> > +{
> > + struct ntxec_rtc *rtc = dev_get_drvdata(dev);
> > + int res = 0;
> > +
> > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_YEAR, ntxec_reg8(tm->tm_year - 100));
> > + if (res)
> > + return res;
> > +
> > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MONTH, ntxec_reg8(tm->tm_mon + 1));
> > + if (res)
> > + return res;
> > +
> > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_DAY, ntxec_reg8(tm->tm_mday));
> > + if (res)
> > + return res;
> > +
> > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_HOUR, ntxec_reg8(tm->tm_hour));
> > + if (res)
> > + return res;
> > +
> > + res = regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MINUTE, ntxec_reg8(tm->tm_min));
> > + if (res)
> > + return res;
> > +
> > + return regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_SECOND, ntxec_reg8(tm->tm_sec));
>
> I wonder: Is this racy? If you write minute, does the seconds reset to
> zero or something like that? Or can it happen, that after writing the
> minute register and before writing the second register the seconds
> overflow and you end up with the time set to a minute later than
> intended? If so it might be worth to set the seconds to 0 at the start
> of the function (with an explaining comment).
The setting the minutes does not reset the seconds, so I think this race
condition is possible. I'll add the workaround.
> .read_time has a similar race. What happens if minutes overflow between
> reading NTXEC_REG_READ_DH and NTXEC_REG_READ_MS?
Yes, we get read tearing in that case. It could even propagate all the
way to the year/month field, for example when the following time rolls
over:
A | B | C
2020-10-31 23:59:59
2020-11-01 00:00:00
- If the increment happens after reading C, we get 2020-10-31 23:59:59
- If the increment happens between reading B and C, we get 2020-10-31 23:00:00
- If the increment happens between reading A and B, we get 2020-10-01 00:00:00
- If the increment happens before reading A, we get 2020-11-01 00:00:00
... both of which are far from correct.
To mitigate this issue, I think something like the following is needed:
- Read year/month
- Read day/hour
- Read minute/second
- Read day/hour, compare with previously read value, restart on mismatch
- Read year/month, compare with previously read value, restart on mismatch
The order of the last two steps doesn't matter, as far as I can see, but
if I remove one of them, I can't catch all cases of read tearing.
> > +static struct platform_driver ntxec_rtc_driver = {
> > + .driver = {
> > + .name = "ntxec-rtc",
> > + },
> > + .probe = ntxec_rtc_probe,
>
> No .remove function?
I don't think it would serve a purpose in this driver. There are no
device-specific resources to release (no clocks to unprepare, for
example).
Thanks,
Jonathan Neuschäfer
Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)
Powered by blists - more mailing lists