[PATCH] system/rtc: Silent overflow in qemu_timedate_diff()

[PATCH] system/rtc: Silent overflow in qemu_timedate_diff()

[Philippe Mathieu-Daudé]

Per ctime(3) man-page:

  A negative value for tm_isdst causes the mktime() function to
  attempt to divine whether summer time is in effect for the
  specified time.  The tm_isdst and tm_gmtoff members are forced
  to zero by timegm().

  The mktime() function returns the specified calendar time; if
  the calendar time cannot be represented, it returns -1;

Coverity reports (CID 1547724 Overflowed return value) the
qemu_timedate_diff() method doesn't handle this error path.

Since this method was added in commit f650305967f ("Unify RTCs
that use host time, fix M48t59 alarm") in 2008, and there is no
open issue related to it, keep ignoring this unlikely case, but
add an assertion to make Coverity happy.

Fixes: CID 1547724
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 system/rtc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/system/rtc.c b/system/rtc.c
index 56951288c40..070b99fe6ad 100644
--- a/system/rtc.c
+++ b/system/rtc.c
@@ -98,6 +98,7 @@ time_t qemu_timedate_diff(struct tm *tm)
         struct tm tmp = *tm;
         tmp.tm_isdst = -1; /* use timezone to figure it out */
         seconds = mktime(&tmp);
+        assert(seconds >= 0);
         break;
     }
     default:
-- 
2.49.0

Re: [PATCH] system/rtc: Silent overflow in qemu_timedate_diff()

[Philippe Mathieu-Daudé]

ping?

On 11/8/25 12:08, Philippe Mathieu-Daudé wrote:
> Per ctime(3) man-page:
> 
>    A negative value for tm_isdst causes the mktime() function to
>    attempt to divine whether summer time is in effect for the
>    specified time.  The tm_isdst and tm_gmtoff members are forced
>    to zero by timegm().
> 
>    The mktime() function returns the specified calendar time; if
>    the calendar time cannot be represented, it returns -1;
> 
> Coverity reports (CID 1547724 Overflowed return value) the
> qemu_timedate_diff() method doesn't handle this error path.
> 
> Since this method was added in commit f650305967f ("Unify RTCs
> that use host time, fix M48t59 alarm") in 2008, and there is no
> open issue related to it, keep ignoring this unlikely case, but
> add an assertion to make Coverity happy.
> 
> Fixes: CID 1547724
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   system/rtc.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/system/rtc.c b/system/rtc.c
> index 56951288c40..070b99fe6ad 100644
> --- a/system/rtc.c
> +++ b/system/rtc.c
> @@ -98,6 +98,7 @@ time_t qemu_timedate_diff(struct tm *tm)
>           struct tm tmp = *tm;
>           tmp.tm_isdst = -1; /* use timezone to figure it out */
>           seconds = mktime(&tmp);
> +        assert(seconds >= 0);
>           break;
>       }
>       default:

Re: [PATCH] system/rtc: Silent overflow in qemu_timedate_diff()

[Peter Maydell]

On Mon, 11 Aug 2025 at 11:08, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Per ctime(3) man-page:
>
>   A negative value for tm_isdst causes the mktime() function to
>   attempt to divine whether summer time is in effect for the
>   specified time.  The tm_isdst and tm_gmtoff members are forced
>   to zero by timegm().
>
>   The mktime() function returns the specified calendar time; if
>   the calendar time cannot be represented, it returns -1;
>
> Coverity reports (CID 1547724 Overflowed return value) the
> qemu_timedate_diff() method doesn't handle this error path.
>
> Since this method was added in commit f650305967f ("Unify RTCs
> that use host time, fix M48t59 alarm") in 2008, and there is no
> open issue related to it, keep ignoring this unlikely case, but
> add an assertion to make Coverity happy.
>
> Fixes: CID 1547724
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  system/rtc.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/system/rtc.c b/system/rtc.c
> index 56951288c40..070b99fe6ad 100644
> --- a/system/rtc.c
> +++ b/system/rtc.c
> @@ -98,6 +98,7 @@ time_t qemu_timedate_diff(struct tm *tm)
>          struct tm tmp = *tm;
>          tmp.tm_isdst = -1; /* use timezone to figure it out */
>          seconds = mktime(&tmp);
> +        assert(seconds >= 0);
>          break;
>      }
>      default:

Generally the struct tm that we call this function
on is filled in using information from the guest.
So I think that a silly guest could probably program
a device in a way that causes us to call qemu_timedate_diff()
on an invalid time and make mktime() fail.

Also, if the device is programmable to a date earlier
than the Unix epoch, this will also make mktime() return
a negative number, e.g:

  tm_sec   = 0
  tm_min   = 0
  tm_hour  = 0
  tm_mday  = 1
  tm_mon   = 0
  tm_year  = 70
  tm_wday  = 0
  tm_yday  = 0
  tm_isdst = 0

(Jan 1st 1970) makes mktime return -3600.

The safe way to check for mktime() failure is to clear errno
before calling it and then check for it returning -1 and
setting errno.

I think that if we care about trying to handle errors here
(and plausibly we don't care enough to go to the effort)
we would want to:
 * update the API of this function to allow it to indicate
   failure (in some better way than raw mktime() so we
   don't have the "-1 is ambiguous" problem)
 * have all the callers handle an error in whatever way
   makes sense (which might be assert if they can guarantee
   the fields to be in-bounds, or might be something else,
   e.g. for mt48t59.c where it wants to set the alarm
   timer "alarm time out of range" should be handled as
   "don't set the timer" because the requested time will
   be either in the past or else so far in the future it
   will never arrive)

thanks
-- PMM