* [PATCH] rtc: wilco-ec: Sanitize values received from RTC @ 2019-09-10 15:19 Nick Crews 2019-09-12 8:09 ` Alexandre Belloni 0 siblings, 1 reply; 5+ messages in thread From: Nick Crews @ 2019-09-10 15:19 UTC (permalink / raw) To: Alexandre Belloni, Alessandro Zummo Cc: linux-rtc, linux-kernel, Duncan Laurie, Enric Balletbo i Serra, Nick Crews 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 (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"); -- 2.11.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] rtc: wilco-ec: Sanitize values received from RTC 2019-09-10 15:19 [PATCH] rtc: wilco-ec: Sanitize values received from RTC Nick Crews @ 2019-09-12 8:09 ` Alexandre Belloni 2019-09-15 22:44 ` Nick Crews 0 siblings, 1 reply; 5+ messages in thread From: Alexandre Belloni @ 2019-09-12 8:09 UTC (permalink / raw) To: Nick Crews Cc: Alessandro Zummo, linux-rtc, linux-kernel, Duncan Laurie, Enric Balletbo i Serra 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 > -- Alexandre Belloni, Bootlin Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] rtc: wilco-ec: Sanitize values received from RTC 2019-09-12 8:09 ` Alexandre Belloni @ 2019-09-15 22:44 ` Nick Crews 2019-09-16 8:02 ` Alexandre Belloni 0 siblings, 1 reply; 5+ messages in thread From: Nick Crews @ 2019-09-15 22:44 UTC (permalink / raw) To: Alexandre Belloni Cc: Alessandro Zummo, linux-rtc, linux-kernel, Duncan Laurie, Enric Balletbo i Serra 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] rtc: wilco-ec: Sanitize values received from RTC 2019-09-15 22:44 ` Nick Crews @ 2019-09-16 8:02 ` Alexandre Belloni 2019-09-16 17:56 ` Nick Crews 0 siblings, 1 reply; 5+ messages in thread From: Alexandre Belloni @ 2019-09-16 8:02 UTC (permalink / raw) To: Nick Crews Cc: Alessandro Zummo, linux-rtc, linux-kernel, Duncan Laurie, Enric Balletbo i Serra 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. -- Alexandre Belloni, Bootlin Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] rtc: wilco-ec: Sanitize values received from RTC 2019-09-16 8:02 ` Alexandre Belloni @ 2019-09-16 17:56 ` Nick Crews 0 siblings, 0 replies; 5+ messages in thread From: Nick Crews @ 2019-09-16 17:56 UTC (permalink / raw) To: Alexandre Belloni Cc: Alessandro Zummo, linux-rtc, linux-kernel, Duncan Laurie, Enric Balletbo i Serra 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-09-16 17:57 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-10 15:19 [PATCH] rtc: wilco-ec: Sanitize values received from RTC Nick Crews 2019-09-12 8:09 ` Alexandre Belloni 2019-09-15 22:44 ` Nick Crews 2019-09-16 8:02 ` Alexandre Belloni 2019-09-16 17:56 ` Nick Crews
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).