Re: [PATCH] rtc: wilco-ec: Sanitize values received from RTC

[Nick Crews]

On Mon, Sep 16, 2019 at 2:02 AM Alexandre Belloni
<alexandre.belloni@xxxxxxxxxxx> 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@xxxxxxxxxxx> 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@xxxxxxxxxxxx>
> > > > ---
> > > >  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