linux-rtc.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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).