Re: [PATCH] rtc: ftrtc010: fix negative offset handling and smatch warning

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] rtc: rtc-ftrtc010: s32 cast limits positive offset to 68 years but range_max allows 136 years
- [Medium] rtc: ftrtc010: unprotected sequential hardware counter reads cause time skew
- [Medium] rtc: ftrtc010: range_min blocks setting negative offsets
--

commit 36a9e88aad02944c1e1579ef1c2b0dd3b5ceff14
Author: Liu Dalin <liudalin@kylinsec.com.cn>

rtc: ftrtc010: fix negative offset handling and smatch warning

This commit addresses negative offset sign-extension and 32-bit arithmetic
overflows by explicitly casting the offset to an s32 before computing the
time in 64-bit space.

> diff --git a/drivers/rtc/rtc-ftrtc010.c b/drivers/rtc/rtc-ftrtc010.c
> index b29c96be40f4a..65fe1e872239e 100644
> --- a/drivers/rtc/rtc-ftrtc010.c
> +++ b/drivers/rtc/rtc-ftrtc010.c
> @@ -72,7 +72,8 @@ 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: Medium]
This problem wasn't introduced by this patch, but does computing the time
from these sequential MMIO reads risk corrupting the result if the hardware
rolls over a counter during the reads?

For example, if sec is read as 59, and then a hardware tick rolls it to 0
before min is read, min will be incremented. The combined calculation from:

	sec  = readl(rtc->rtc_base + FTRTC010_RTC_SECOND);
	min  = readl(rtc->rtc_base + FTRTC010_RTC_MINUTE);
	hour = readl(rtc->rtc_base + FTRTC010_RTC_HOUR);
	days = readl(rtc->rtc_base + FTRTC010_RTC_DAYS);

will yield a time skewed by a full minute. Does this sequence need a retry
loop or explicit latching to prevent skew in ftrtc010_rtc_read_time() and
ftrtc010_rtc_set_time()?

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

[Severity: High]
Does this explicit s32 cast limit the maximum positive offset to roughly
68 years while the driver continues to advertise a range of 136 years?

Looking at ftrtc010_rtc_probe(), rtc_dev->range_max is set to U32_MAX:

	rtc_dev->range_max = U32_MAX + rtc_dev->range_min;

If a time is set more than 68 years into the future, ftrtc010_rtc_set_time()
will compute a 32-bit offset with the most significant bit set.

When read back here in ftrtc010_rtc_read_time(), the new (s32) cast will
incorrectly interpret that large positive offset as a negative offset,
causing the time to jump backwards. Should range_max be updated to use
S32_MAX?

[Severity: Medium]
Does the current rtc_dev->range_min configuration prevent setting the
negative offsets that this patch intends to handle?

In ftrtc010_rtc_probe(), range_min is statically initialized to the exact
hardware time at the moment of driver probe:

	rtc_dev->range_min = (u64)days * 86400 + hour * 3600 + min * 60 + sec;

Because the RTC core's rtc_valid_range() checks this bound, it will return
-ERANGE for any attempt to set a time older than the hardware counters at
probe.

This blocks configuring valid negative offsets. Does range_min need to be
decreased by S32_MAX to allow setting times in the past?

>  	rtc_time64_to_tm(time, tm);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/0C8AEB5636132B0C+20261008113732.581825-1-liudalin@kylinsec.com.cn?part=1