| Message ID | 20190910151929.780-1-ncrews@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | rtc: wilco-ec: Sanitize values received from RTC | expand |
Hi Nick, On 10/09/2019 16:19:29+0100, Nick Crews wrote: > Check that the time received from the RTC HW is valid, > otherwise the computation of rtc_year_days() in the next > line could, and sometimes does, crash the kernel. > > While we're at it, fix the license to plain "GPL". > > Signed-off-by: Nick Crews <ncrews@chromium.org> > --- > drivers/rtc/rtc-wilco-ec.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/rtc/rtc-wilco-ec.c b/drivers/rtc/rtc-wilco-ec.c > index 8ad4c4e6d557..0ccbf2dce832 100644 > --- a/drivers/rtc/rtc-wilco-ec.c > +++ b/drivers/rtc/rtc-wilco-ec.c > @@ -110,8 +110,16 @@ static int wilco_ec_rtc_read(struct device *dev, struct rtc_time *tm) > tm->tm_mday = rtc.day; > tm->tm_mon = rtc.month - 1; > tm->tm_year = rtc.year + (rtc.century * 100) - 1900; > - tm->tm_yday = rtc_year_days(tm->tm_mday, tm->tm_mon, tm->tm_year); If your driver doesn't care about yday, userspace doesn't either. You can simply not set it. > > + if (rtc_valid_tm(tm)) { > + dev_warn(dev, > + "Time computed from EC RTC is invalid: sec=%d, min=%d, hour=%d, mday=%d, mon=%d, year=%d", > + tm->tm_sec, tm->tm_min, tm->tm_hour, tm->mday, > + tm->mon, tm->year); > + return -EIO; > + } > + > + tm->tm_yday = rtc_year_days(tm->tm_mday, tm->tm_mon, tm->tm_year); > /* Don't compute day of week, we don't need it. */ > tm->tm_wday = -1; > > @@ -188,5 +196,5 @@ module_platform_driver(wilco_ec_rtc_driver); > > MODULE_ALIAS("platform:rtc-wilco-ec"); > MODULE_AUTHOR("Nick Crews <ncrews@chromium.org>"); > -MODULE_LICENSE("GPL v2"); > +MODULE_LICENSE("GPL"); This should be in a separate patch. > MODULE_DESCRIPTION("Wilco EC RTC driver"); > -- > 2.11.0 >
Hi Alexandre, thanks for the thoughts. On Thu, Sep 12, 2019 at 9:09 AM Alexandre Belloni <alexandre.belloni@bootlin.com> wrote: > > Hi Nick, > > On 10/09/2019 16:19:29+0100, Nick Crews wrote: > > Check that the time received from the RTC HW is valid, > > otherwise the computation of rtc_year_days() in the next > > line could, and sometimes does, crash the kernel. > > > > While we're at it, fix the license to plain "GPL". > > > > Signed-off-by: Nick Crews <ncrews@chromium.org> > > --- > > drivers/rtc/rtc-wilco-ec.c | 12 ++++++++++-- > > 1 file changed, 10 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/rtc/rtc-wilco-ec.c b/drivers/rtc/rtc-wilco-ec.c > > index 8ad4c4e6d557..0ccbf2dce832 100644 > > --- a/drivers/rtc/rtc-wilco-ec.c > > +++ b/drivers/rtc/rtc-wilco-ec.c > > @@ -110,8 +110,16 @@ static int wilco_ec_rtc_read(struct device *dev, struct rtc_time *tm) > > tm->tm_mday = rtc.day; > > tm->tm_mon = rtc.month - 1; > > tm->tm_year = rtc.year + (rtc.century * 100) - 1900; > > - tm->tm_yday = rtc_year_days(tm->tm_mday, tm->tm_mon, tm->tm_year); > > If your driver doesn't care about yday, userspace doesn't either. You > can simply not set it. This driver indeed does not care about yday, so it sounds good to me to simply not set it. However, I do still want to worry about the HW returning some bogus time. It could be indicative of some other problem, and I don't want to pass on this illegal time further up the stack. I can't think of a reason why an illegal time should exist at all, we should just deal with it as soon as possible. How about I remove setting yday as you suggest, but still keep the rtc_valid_tm() check? > > > > > + if (rtc_valid_tm(tm)) { > > + dev_warn(dev, > > + "Time computed from EC RTC is invalid: sec=%d, min=%d, hour=%d, mday=%d, mon=%d, year=%d", > > + tm->tm_sec, tm->tm_min, tm->tm_hour, tm->mday, > > + tm->mon, tm->year); > > + return -EIO; > > + } > > + > > + tm->tm_yday = rtc_year_days(tm->tm_mday, tm->tm_mon, tm->tm_year); > > /* Don't compute day of week, we don't need it. */ > > tm->tm_wday = -1; Following our discussion, perhaps I'll just remove this too since the RTC core inits this to -1 already? > > > > @@ -188,5 +196,5 @@ module_platform_driver(wilco_ec_rtc_driver); > > > > MODULE_ALIAS("platform:rtc-wilco-ec"); > > MODULE_AUTHOR("Nick Crews <ncrews@chromium.org>"); > > -MODULE_LICENSE("GPL v2"); > > +MODULE_LICENSE("GPL"); > > This should be in a separate patch. OK, I'll separate this out :) > > > MODULE_DESCRIPTION("Wilco EC RTC driver"); > > -- > > 2.11.0 > > > > -- > Alexandre Belloni, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
On 15/09/2019 23:44:03+0100, Nick Crews wrote: > Hi Alexandre, thanks for the thoughts. > > On Thu, Sep 12, 2019 at 9:09 AM Alexandre Belloni > <alexandre.belloni@bootlin.com> wrote: > > > > Hi Nick, > > > > On 10/09/2019 16:19:29+0100, Nick Crews wrote: > > > Check that the time received from the RTC HW is valid, > > > otherwise the computation of rtc_year_days() in the next > > > line could, and sometimes does, crash the kernel. > > > > > > While we're at it, fix the license to plain "GPL". > > > > > > Signed-off-by: Nick Crews <ncrews@chromium.org> > > > --- > > > drivers/rtc/rtc-wilco-ec.c | 12 ++++++++++-- > > > 1 file changed, 10 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/rtc/rtc-wilco-ec.c b/drivers/rtc/rtc-wilco-ec.c > > > index 8ad4c4e6d557..0ccbf2dce832 100644 > > > --- a/drivers/rtc/rtc-wilco-ec.c > > > +++ b/drivers/rtc/rtc-wilco-ec.c > > > @@ -110,8 +110,16 @@ static int wilco_ec_rtc_read(struct device *dev, struct rtc_time *tm) > > > tm->tm_mday = rtc.day; > > > tm->tm_mon = rtc.month - 1; > > > tm->tm_year = rtc.year + (rtc.century * 100) - 1900; > > > - tm->tm_yday = rtc_year_days(tm->tm_mday, tm->tm_mon, tm->tm_year); > > > > If your driver doesn't care about yday, userspace doesn't either. You > > can simply not set it. > > This driver indeed does not care about yday, so it sounds good to me > to simply not set it. However, I do still want to worry about the HW > returning some bogus time. It could be indicative of some other problem, > and I don't want to pass on this illegal time further up the stack. I can't > think of a reason why an illegal time should exist at all, we should just > deal with it as soon as possible. How about I remove setting yday as > you suggest, but still keep the rtc_valid_tm() check? > As rtc_valid_tm would be the last thing the driver does in the callback and the first thing the core does after returning from the callback, you'd get two calls back to back which is not useful. > > > > > > > > + if (rtc_valid_tm(tm)) { > > > + dev_warn(dev, > > > + "Time computed from EC RTC is invalid: sec=%d, min=%d, hour=%d, mday=%d, mon=%d, year=%d", > > > + tm->tm_sec, tm->tm_min, tm->tm_hour, tm->mday, > > > + tm->mon, tm->year); > > > + return -EIO; > > > + } > > > + > > > + tm->tm_yday = rtc_year_days(tm->tm_mday, tm->tm_mon, tm->tm_year); > > > /* Don't compute day of week, we don't need it. */ > > > tm->tm_wday = -1; > > Following our discussion, perhaps I'll just remove this too since the > RTC core inits this to -1 already? > You can remove it.
On Mon, Sep 16, 2019 at 2:02 AM Alexandre Belloni <alexandre.belloni@bootlin.com> wrote: > > On 15/09/2019 23:44:03+0100, Nick Crews wrote: > > Hi Alexandre, thanks for the thoughts. > > > > On Thu, Sep 12, 2019 at 9:09 AM Alexandre Belloni > > <alexandre.belloni@bootlin.com> wrote: > > > > > > Hi Nick, > > > > > > On 10/09/2019 16:19:29+0100, Nick Crews wrote: > > > > Check that the time received from the RTC HW is valid, > > > > otherwise the computation of rtc_year_days() in the next > > > > line could, and sometimes does, crash the kernel. > > > > > > > > While we're at it, fix the license to plain "GPL". > > > > > > > > Signed-off-by: Nick Crews <ncrews@chromium.org> > > > > --- > > > > drivers/rtc/rtc-wilco-ec.c | 12 ++++++++++-- > > > > 1 file changed, 10 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/rtc/rtc-wilco-ec.c b/drivers/rtc/rtc-wilco-ec.c > > > > index 8ad4c4e6d557..0ccbf2dce832 100644 > > > > --- a/drivers/rtc/rtc-wilco-ec.c > > > > +++ b/drivers/rtc/rtc-wilco-ec.c > > > > @@ -110,8 +110,16 @@ static int wilco_ec_rtc_read(struct device *dev, struct rtc_time *tm) > > > > tm->tm_mday = rtc.day; > > > > tm->tm_mon = rtc.month - 1; > > > > tm->tm_year = rtc.year + (rtc.century * 100) - 1900; > > > > - tm->tm_yday = rtc_year_days(tm->tm_mday, tm->tm_mon, tm->tm_year); > > > > > > If your driver doesn't care about yday, userspace doesn't either. You > > > can simply not set it. > > > > This driver indeed does not care about yday, so it sounds good to me > > to simply not set it. However, I do still want to worry about the HW > > returning some bogus time. It could be indicative of some other problem, > > and I don't want to pass on this illegal time further up the stack. I can't > > think of a reason why an illegal time should exist at all, we should just > > deal with it as soon as possible. How about I remove setting yday as > > you suggest, but still keep the rtc_valid_tm() check? > > > > As rtc_valid_tm would be the last thing the driver does in the callback > and the first thing the core does after returning from the callback, > you'd get two calls back to back which is not useful. Ah, I see now where the core checks that the time is valid in interface.c. Sounds good, I'll skip the check then. > > > > > > > > > > > > + if (rtc_valid_tm(tm)) { > > > > + dev_warn(dev, > > > > + "Time computed from EC RTC is invalid: sec=%d, min=%d, hour=%d, mday=%d, mon=%d, year=%d", > > > > + tm->tm_sec, tm->tm_min, tm->tm_hour, tm->mday, > > > > + tm->mon, tm->year); > > > > + return -EIO; > > > > + } > > > > + > > > > + tm->tm_yday = rtc_year_days(tm->tm_mday, tm->tm_mon, tm->tm_year); > > > > /* Don't compute day of week, we don't need it. */ > > > > tm->tm_wday = -1; > > > > Following our discussion, perhaps I'll just remove this too since the > > RTC core inits this to -1 already? > > > > You can remove it. Will do! > > > -- > Alexandre Belloni, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
diff --git a/drivers/rtc/rtc-wilco-ec.c b/drivers/rtc/rtc-wilco-ec.c index 8ad4c4e6d557..0ccbf2dce832 100644 --- a/drivers/rtc/rtc-wilco-ec.c +++ b/drivers/rtc/rtc-wilco-ec.c @@ -110,8 +110,16 @@ static int wilco_ec_rtc_read(struct device *dev, struct rtc_time *tm) tm->tm_mday = rtc.day; tm->tm_mon = rtc.month - 1; tm->tm_year = rtc.year + (rtc.century * 100) - 1900; - tm->tm_yday = rtc_year_days(tm->tm_mday, tm->tm_mon, tm->tm_year); + if (rtc_valid_tm(tm)) { + dev_warn(dev, + "Time computed from EC RTC is invalid: sec=%d, min=%d, hour=%d, mday=%d, mon=%d, year=%d", + tm->tm_sec, tm->tm_min, tm->tm_hour, tm->mday, + tm->mon, tm->year); + return -EIO; + } + + tm->tm_yday = rtc_year_days(tm->tm_mday, tm->tm_mon, tm->tm_year); /* Don't compute day of week, we don't need it. */ tm->tm_wday = -1; @@ -188,5 +196,5 @@ module_platform_driver(wilco_ec_rtc_driver); MODULE_ALIAS("platform:rtc-wilco-ec"); MODULE_AUTHOR("Nick Crews <ncrews@chromium.org>"); -MODULE_LICENSE("GPL v2"); +MODULE_LICENSE("GPL"); MODULE_DESCRIPTION("Wilco EC RTC driver");
Check that the time received from the RTC HW is valid, otherwise the computation of rtc_year_days() in the next line could, and sometimes does, crash the kernel. While we're at it, fix the license to plain "GPL". Signed-off-by: Nick Crews <ncrews@chromium.org> --- drivers/rtc/rtc-wilco-ec.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)