Re: [PATCH 07/12] rtc: rzn1: fix alarm range check truncation on 32-bit systems

[Wolfram Sang <wsa+renesas@sang-engineering.com>]

[-- Attachment #1: Type: text/plain, Size: 4683 bytes --]

Hi,

Sashiko found issues, not introduced by your patch, but still.. I
curated the list and left only those which I think are valid. Do you
have time to look at these issues?

Thank you and happy hacking,

   Wolfram

> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Incorrect hardware alarm weekday assignment due to unsigned underflow when the alarm crosses a month boundary.
> - [High] Unconfigured alarms in `rzn1_rtc_read_alarm` can return an invalid `tm_wday` value of `-1`, risking out-of-bounds array access in userspace.
> - [High] Potential spurious interrupt when sequentially updating active hardware alarm registers.
> --
> 
> commit d278abd6309a65fff235820bea9a8e8631da47be
> Author: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Subject: rtc: rzn1: fix alarm range check truncation on 32-bit systems
> 
> This commit fixes a silent truncation issue on 32-bit systems by changing
> the alarm and farest variables from unsigned long to time64_t. It also
> replaces the time_after() macro with a direct signed comparison.
> 
> > diff --git a/drivers/rtc/rtc-rzn1.c b/drivers/rtc/rtc-rzn1.c
> > index fe21fa6242881..06339adae71f6 100644
> > --- a/drivers/rtc/rtc-rzn1.c
> > +++ b/drivers/rtc/rtc-rzn1.c
> [ ... ]
> > @@ -265,8 +266,8 @@ static int rzn1_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> >  {
> >  	struct rzn1_rtc *rtc = dev_get_drvdata(dev);
> >  	struct rtc_time *tm = &alrm->time, tm_now;
> > -	unsigned long alarm, farest;
> >  	unsigned int days_ahead, wday;
> > +	time64_t alarm, farest;
> >  	int ret;
> >  
> >  	ret = rzn1_rtc_read_time(dev, &tm_now);
> > @@ -276,7 +277,7 @@ static int rzn1_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> >  	/* We cannot set alarms more than one week ahead */
> >  	farest = rtc_tm_to_time64(&tm_now) + rtc->rtcdev->alarm_offset_max;
> >  	alarm = rtc_tm_to_time64(tm);
> > -	if (time_after(alarm, farest))
> > +	if (alarm > farest)
> >  		return -ERANGE;
> >  
> >  	/* Convert alarm day into week day */
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does the calculation for
> days_ahead underflow if the alarm crosses a month boundary?
> 
> In rzn1_rtc_set_alarm(), days_ahead is typed as an unsigned int and is
> calculated like this:
> 
>     days_ahead = tm->tm_mday - tm_now.tm_mday;
>     wday = (tm_now.tm_wday + days_ahead) % 7;
> 
> If the alarm is set for the 1st of the next month, and today is the 31st,
> the subtraction results in -30. When assigned to an unsigned int, this
> underflows. Because 2^32 % 7 evaluates to 4, the resulting wday calculation
> shifts the weekday by exactly 4 days, causing the hardware alarm register
> to be programmed for the wrong day.
> 
> [Severity: High]
> This is a pre-existing issue, but is there a risk of a spurious interrupt
> when updating the active hardware alarm registers?
> 
> Just below this hunk in rzn1_rtc_set_alarm(), the match registers are
> updated sequentially:
> 
>     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);
> 
> The RTC core can invoke this function without first disabling the hardware
> alarm if replacing an existing timer. If the hardware alarm interrupt is
> active, modifying these registers sequentially could cause the intermediate
> hardware state to momentarily match the current time, triggering a spurious
> interrupt and prematurely expiring the next timer in the queue.
> 
> [Severity: High]
> This is a pre-existing issue, but what happens in rzn1_rtc_read_alarm() if
> the hardware alarm is unconfigured?
> 
> If the hardware alarm register reads 0, fls(0) - 1 evaluates to -1:
> 
> 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;
>     ...
> }
> 
> If the current weekday is Saturday (6), delta_days evaluates to 0:
> ((-1 - 6 + 7) % 7) == 0.
> 
> Because delta_days is 0, the rtc_time64_to_tm() block is skipped, and the
> function returns with tm->tm_wday = -1. Since rtc_valid_tm() does not
> bound-check tm_wday, this -1 is passed to userspace where tools commonly
> use it as an array index, which could cause an out-of-bounds memory read.
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260615154805.1619693-1-prabhakar.mahadev-lad.rj@bp.renesas.com?part=7

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]