Re: [PATCH v4 03/15] rtc: rzn1: Fix weekday underflow when alarm crosses month boundary

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

Pre-existing issues:
- [High] Truncation of 64-bit `time64_t` alarm timestamps to 32-bit `unsigned long` and misuse of `time_after()` allows users to bypass the 1-week hardware alarm limit on 32-bit systems for alarms requested > 68 years in the future.
- [Medium] Missing validation for an uninitialized `wday` hardware register in `rzn1_rtc_read_alarm` produces bogus alarm dates.
--

--- Patch [3]: [PATCH v4 03/15] rtc: rzn1: Fix weekday underflow when alarm crosses month boundary ---
commit d19190c56b59d2960781807a89bdaa97caa58aa4
Author: Lad Prabhakar <[email protected]>

rtc: rzn1: Fix weekday underflow when alarm crosses month boundary

This patch fixes an underflow issue in rzn1_rtc_set_alarm() when
calculating the alarm weekday across month boundaries. It directly uses
the tm_wday field from the pre-populated rtc_time struct instead of
recalculating it from the days ahead.

> diff --git a/drivers/rtc/rtc-rzn1.c b/drivers/rtc/rtc-rzn1.c
> index aa27ad7f5941a..8c70dbf8c7ecd 100644
> --- a/drivers/rtc/rtc-rzn1.c
> +++ b/drivers/rtc/rtc-rzn1.c

[ ... ]

> @@ -273,13 +272,9 @@ static int rzn1_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
>  	if (time_after(alarm, farest))
>  		return -ERANGE;

[Severity: High]
This is a pre-existing issue, but does this time_after() comparison
allow bypassing the 1-week hardware limit on 32-bit systems?

Since rtc_tm_to_time64() returns a 64-bit value that gets truncated
into the 32-bit unsigned long alarm and farest variables on 32-bit
architectures, requesting an alarm > 68 years in the future (e.g. 2099)
could cause the difference farest - alarm to exceed INT_MAX.

This subtraction would overflow in two's complement and evaluate as a
positive integer, causing time_after(alarm, farest) to evaluate to
false, bypassing the -ERANGE check and configuring an alarm that fires at
the wrong time.


While reviewing this file, I also noticed another pre-existing issue in
rzn1_rtc_read_alarm() that might need attention:

drivers/rtc/rtc-rzn1.c:rzn1_rtc_read_alarm() {
    ...
    wday = readl(rtc->base + RZN1_RTC_ALW);
    ...
    delta_days = ((fls(wday) - 1) - tm->tm_wday + 7) % 7;
    tm->tm_wday = fls(wday) - 1;
    ...
}

[Severity: Medium]
This is a pre-existing issue, but what happens if the RZN1_RTC_ALW register
is uninitialized and wday is 0?

Calling fls(0) - 1 would evaluate to -1. If the current time is a Saturday
(tm_now.tm_wday == 6), delta_days evaluates to 0, which bypasses
the if (delta_days) block and leaves tm->tm_wday = -1.

Since the core function rtc_valid_tm() does not check tm_wday, would this
propagate a bogus alarm date to the caller?


> -	/* Convert alarm day into week day */
> -	days_ahead = tm->tm_mday - tm_now.tm_mday;
> -	wday = (tm_now.tm_wday + days_ahead) % 7;
> -
>  	writel(bin2bcd(tm->tm_min), rtc->base + RZN1_RTC_ALM);
>  	writel(bin2bcd(tm->tm_hour), rtc->base + RZN1_RTC_ALH);
> -	writel(BIT(wday), rtc->base + RZN1_RTC_ALW);
> +	writel(BIT(tm->tm_wday), rtc->base + RZN1_RTC_ALW);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3