Re: [PATCH v2 3/3] rtc: ftrtc010: fix integer overflow in time calculation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] The patch claims to fix the Y2106 integer overflow, but mathematically computes the exact same 32-bit truncated values as the original code, making it a completely ineffective placebo.

Pre-existing issues:
- [High] Unsynchronized sequential reads of the RTC time registers lack a latching or retry mechanism, causing hardware tearing and persistent time corruption.
- [High] Unconditional IRQ_HANDLED return in an IRQF_SHARED interrupt handler breaks the kernel's spurious interrupt detector, causing a system hard lockup.
--

commit 9a9a270ad0bf60c8a0b5860cccb3bb2994f4adcf
Author: Liu Dalin <liudalin@kylinsec.com.cn>

rtc: ftrtc010: fix integer overflow in time calculation

This patch modifies the time calculation in the ftrtc010 driver to cast
variables to timeu64_t before calculating offsets and days. This is
intended to prevent 32-bit arithmetic wrapping for dates beyond year 2106.

> diff --git a/drivers/rtc/rtc-ftrtc010.c b/drivers/rtc/rtc-ftrtc010.c
> index 2f610a0fc6e9b..c989574b1fbc6 100644
> --- a/drivers/rtc/rtc-ftrtc010.c
> +++ b/drivers/rtc/rtc-ftrtc010.c
> @@ -73,7 +73,7 @@ static int ftrtc010_rtc_read_time(struct device *dev, struct rtc_time *tm)
>  	days = readl(rtc->rtc_base + FTRTC010_RTC_DAYS);
>  	offset = readl(rtc->rtc_base + FTRTC010_RTC_RECORD);

[Severity: High]
This is a pre-existing issue, but do the sequential reads in
ftrtc010_rtc_read_time() lack a software retry loop?

If an internal hardware counter rolls over while the registers are being
read (e.g., SECOND from 59 to 0), it seems the driver could acquire an
inconsistent temporal state. Would this expose the time computation to
transient corruption?

> -	time = offset + days * 86400 + hour * 3600 + min * 60 + sec;
> +	time = (timeu64_t)(offset + days * 86400 + hour * 3600 + min * 60 + sec);

[Severity: High]
Does this explicit cast to timeu64_t in ftrtc010_rtc_read_time() actually
prevent the integer overflow?

Since offset and all counter variables are u32, this addition will be
evaluated in 32-bit arithmetic and wrap around (modulo 2^32) before the
result is cast to 64-bit. Because of this, it appears the final returned
time can never exceed 0xFFFFFFFF when reading an RTC time beyond year
2106, leaving the issue unfixed.

>  	rtc_time64_to_tm(time, tm);
>  
[ ... ]
> @@ -93,7 +93,8 @@ static int ftrtc010_rtc_set_time(struct device *dev, struct rtc_time *tm)
>  	hour = readl(rtc->rtc_base + FTRTC010_RTC_HOUR);
>  	day = readl(rtc->rtc_base + FTRTC010_RTC_DAYS);

[Severity: High]
This is a pre-existing issue, but similar to ftrtc010_rtc_read_time(),
could a hardware tear occur during these sequential unlatched reads in
ftrtc010_rtc_set_time()?

If reading the hardware registers concurrently with an internal hardware
counter rollover, the torn state would be used to compute the epoch
offset. This offset is then permanently committed to the hardware RTC
record, corrupting the system RTC base.

> -	offset = time - (day * 86400 + hour * 3600 + min * 60 + sec);
> +	offset = time - ((timeu64_t)day * 86400 + (timeu64_t)hour * 3600 +
> +			 (timeu64_t)min * 60 + (timeu64_t)sec);

[Severity: High]
Does casting the operands to timeu64_t in ftrtc010_rtc_set_time()
successfully fix the truncation issue here?

The offset variable is declared as a u32. Assigning the 64-bit difference
to u32 offset instantly discards the upper 32 bits, making it
mathematically identical to the original 32-bit arithmetic. This seems to
prevent the driver from successfully setting an RTC time beyond year 2106.

>  	writel(offset, rtc->rtc_base + FTRTC010_RTC_RECORD);
>  	writel(0x01, rtc->rtc_base + FTRTC010_RTC_CR);

While reviewing this file, I noticed another pre-existing issue:

drivers/rtc/rtc-ftrtc010.c:ftrtc010_rtc_interrupt() {
    static irqreturn_t ftrtc010_rtc_interrupt(int irq, void *dev)
    {
        return IRQ_HANDLED;
    }
}

[Severity: High]
This isn't a bug introduced by this patch, but does this unconditional
return of IRQ_HANDLED in an IRQF_SHARED interrupt handler break the
kernel's spurious interrupt detector?

By unconditionally claiming ownership of unhandled interrupts generated by
other devices on the same shared line, this could prevent the kernel from
disabling a malfunctioning IRQ line. Would this potentially lead to an
infinite interrupt loop and a system hard lockup?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827080320.3351155-1-liudalin@kylinsec.com.cn?part=3