[PATCH v3 3/5] rtc: Fix the RTC time comparison issues adding cast

[Geert Uytterhoeven]

Hi Uwe,

On Tue, 15 Apr 2025 at 00:30, Uwe Kleine-König
<u.kleine-koenig at baylibre.com> wrote:
> On Fri, Apr 11, 2025 at 02:35:56PM +0200, Alexandre Mergnat wrote:
> > The RTC subsystem was experiencing comparison issues between signed and
> > unsigned time values. When comparing time64_t variables (signed) with
> > potentially unsigned range values, incorrect results could occur leading
> > to runtime errors.
> >
> > Adds explicit type casts to time64_t for critical RTC time comparisons
> > in both class.c and interface.c files. The changes ensure proper
> > handling of negative time values during range validation and offset
> > calculations, particularly when dealing with timestamps before 1970.
> >
> > The previous implementation might incorrectly interpret negative values
> > as extremely large positive values, causing unexpected behavior in the
> > RTC hardware abstraction logic.
> >
> > Signed-off-by: Alexandre Mergnat <amergnat at baylibre.com>
> > ---
> >  drivers/rtc/class.c     | 6 +++---
> >  drivers/rtc/interface.c | 8 ++++----
> >  2 files changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c
> > index e31fa0ad127e9..1ee3f609f92ea 100644
> > --- a/drivers/rtc/class.c
> > +++ b/drivers/rtc/class.c
> > @@ -282,7 +282,7 @@ static void rtc_device_get_offset(struct rtc_device *rtc)
> >        * then we can not expand the RTC range by adding or subtracting one
> >        * offset.
> >        */
> > -     if (rtc->range_min == rtc->range_max)
> > +     if (rtc->range_min == (time64_t)rtc->range_max)
> >               return;
>
> For which values of range_min and range_max does this change result in a
> different semantic?
>
> Trying to answer that question myself I wrote two functions:
>
>         #include <stdint.h>
>
>         int compare_unsigned(uint64_t a, int64_t b)
>         {
>                 return a == b;
>         }
>
>         int compare_signed(uint64_t a, int64_t b)
>         {
>                 return (int64_t)a == b;
>         }
>
> When I compile this (with gcc -Os) the assembly for both functions is
> the same (tested for x86_64 and arm32).
>
> >       ret = device_property_read_u32(rtc->dev.parent, "start-year",
> > @@ -299,7 +299,7 @@ static void rtc_device_get_offset(struct rtc_device *rtc)
> >       if (!rtc->set_start_time)
> >               return;
> >
> > -     range_secs = rtc->range_max - rtc->range_min + 1;
> > +     range_secs = (time64_t)rtc->range_max - rtc->range_min + 1;
>
> In the case where no overflow (or underflow) happens, the result is the
> same, isn't it? If there is an overflow, the unsigned variant is
> probably the better choice because overflow for signed variables is
> undefined behaviour (UB).
>
> Respective demo program looks as follows:
>
>         #include <stdint.h>
>
>         int test_unsigned(uint64_t a)
>         {
>                 return a + 3 > a;
>         }
>
>         int test_signed(int64_t a)
>         {
>                 return a + 3 > a;
>         }
>
> Using again `gcc -Os`, the signed variant is compiled to a function that
> returns true unconditionally while the unsigned one implements the
> expected semantic.

Hence that is why the kernel is compiled with -fwrapv...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds