In get_guest_rtc_ns(), "s->base_rtc" is uint64_t, which multiplied by
"NANOSECONDS_PER_SECOND" may overflow the uint64_t type.

Fix it by avoiding adding s->base_rtc in get_guest_rtc_ns_offset(),
because get_guest_rtc_ns() is used either take the remainder of
NANOSECONDS_PER_SECOND or take the quotient of NANOSECONDS_PER_SECOND.

Fixes: 56038ef6234e ("RTC: Update the RTC clock only when reading it")
Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
 hw/rtc/mc146818rtc.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
index 8631386b9f..78bdfab8bd 100644
--- a/hw/rtc/mc146818rtc.c
+++ b/hw/rtc/mc146818rtc.c
@@ -79,10 +79,7 @@ static inline bool rtc_running(MC146818RtcState *s)
 
 static uint64_t get_guest_rtc_ns(MC146818RtcState *s)
 {
-    uint64_t guest_clock = qemu_clock_get_ns(rtc_clock);
-
-    return s->base_rtc * NANOSECONDS_PER_SECOND +
-        guest_clock - s->last_update + s->offset;
+    return qemu_clock_get_ns(rtc_clock) - s->last_update + s->offset;
 }
 
 static void rtc_coalesced_timer_update(MC146818RtcState *s)
@@ -623,10 +620,8 @@ static void rtc_update_time(MC146818RtcState *s)
 {
     struct tm ret;
     time_t guest_sec;
-    int64_t guest_nsec;
 
-    guest_nsec = get_guest_rtc_ns(s);
-    guest_sec = guest_nsec / NANOSECONDS_PER_SECOND;
+    guest_sec = s->base_rtc + get_guest_rtc_ns(s) / NANOSECONDS_PER_SECOND;
     gmtime_r(&guest_sec, &ret);
 
     /* Is SET flag of Register B disabled? */
@@ -637,7 +632,7 @@ static void rtc_update_time(MC146818RtcState *s)
 
 static int update_in_progress(MC146818RtcState *s)
 {
-    int64_t guest_nsec;
+    uint64_t guest_nsec;
 
     if (!rtc_running(s)) {
         return 0;
-- 
2.34.1

On Tue, 6 Jan 2026 at 06:22, Jinjie Ruan <ruanjinjie@huawei.com> wrote:
>
> In get_guest_rtc_ns(), "s->base_rtc" is uint64_t, which multiplied by
> "NANOSECONDS_PER_SECOND" may overflow the uint64_t type.
>
> Fix it by avoiding adding s->base_rtc in get_guest_rtc_ns_offset(),
> because get_guest_rtc_ns() is used either take the remainder of
> NANOSECONDS_PER_SECOND or take the quotient of NANOSECONDS_PER_SECOND.
>
> Fixes: 56038ef6234e ("RTC: Update the RTC clock only when reading it")
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> ---
>  hw/rtc/mc146818rtc.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
> index 8631386b9f..78bdfab8bd 100644
> --- a/hw/rtc/mc146818rtc.c
> +++ b/hw/rtc/mc146818rtc.c
> @@ -79,10 +79,7 @@ static inline bool rtc_running(MC146818RtcState *s)
>
>  static uint64_t get_guest_rtc_ns(MC146818RtcState *s)
>  {
> -    uint64_t guest_clock = qemu_clock_get_ns(rtc_clock);
> -
> -    return s->base_rtc * NANOSECONDS_PER_SECOND +
> -        guest_clock - s->last_update + s->offset;
> +    return qemu_clock_get_ns(rtc_clock) - s->last_update + s->offset;
>  }

This is all logically correct and avoids the overflow, but I
do wonder if this is setting us up for future bugs, because
now the function get_guest_rtc_ns() doesn't actually return
you the guest RTC value in nanoseconds...

I think at minimum it would be useful to have a comment noting
that this doesn't include the base_rtc seconds value and the
caller needs to handle that themselves.

(Maybe there's a neater function/set of functions that would
abstract things more obviously, but I can't think of one
right now.)

thanks
-- PMM

On 2026/1/13 22:22, Peter Maydell wrote:
> On Tue, 6 Jan 2026 at 06:22, Jinjie Ruan <ruanjinjie@huawei.com> wrote:
>>
>> In get_guest_rtc_ns(), "s->base_rtc" is uint64_t, which multiplied by
>> "NANOSECONDS_PER_SECOND" may overflow the uint64_t type.
>>
>> Fix it by avoiding adding s->base_rtc in get_guest_rtc_ns_offset(),
>> because get_guest_rtc_ns() is used either take the remainder of
>> NANOSECONDS_PER_SECOND or take the quotient of NANOSECONDS_PER_SECOND.
>>
>> Fixes: 56038ef6234e ("RTC: Update the RTC clock only when reading it")
>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
>> ---
>>  hw/rtc/mc146818rtc.c | 11 +++--------
>>  1 file changed, 3 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
>> index 8631386b9f..78bdfab8bd 100644
>> --- a/hw/rtc/mc146818rtc.c
>> +++ b/hw/rtc/mc146818rtc.c
>> @@ -79,10 +79,7 @@ static inline bool rtc_running(MC146818RtcState *s)
>>
>>  static uint64_t get_guest_rtc_ns(MC146818RtcState *s)
>>  {
>> -    uint64_t guest_clock = qemu_clock_get_ns(rtc_clock);
>> -
>> -    return s->base_rtc * NANOSECONDS_PER_SECOND +
>> -        guest_clock - s->last_update + s->offset;
>> +    return qemu_clock_get_ns(rtc_clock) - s->last_update + s->offset;
>>  }
> 
> This is all logically correct and avoids the overflow, but I
> do wonder if this is setting us up for future bugs, because
> now the function get_guest_rtc_ns() doesn't actually return
> you the guest RTC value in nanoseconds...
> 
> I think at minimum it would be useful to have a comment noting
> that this doesn't include the base_rtc seconds value and the
> caller needs to handle that themselves.

Thank you for the review.

> 
> (Maybe there's a neater function/set of functions that would
> abstract things more obviously, but I can't think of one
> right now.)
> 
> thanks
> -- PMM