Re: [PATCH v2 2/3] rtc: ds1307: Fix off-by-one issue with wday for rx8130

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Undefined Behavior via out-of-bounds shift in ds1307_set_time()
- [High] Hardware configuration corruption in rx8130_set_alarm()
--

commit 3f83bdbd74c118b742ad3d44c6daf3ae8e8ffcbe
Author: Fredrik M Olsson <fredrik.m.olsson@axis.com>

rtc: ds1307: Fix off-by-one issue with wday for rx8130

The RTC represent each weekday with a individual bit set in the WDAY
register, where the 0th bit represent the first day of the week and the
6th bit represents the last day of the week.

> diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
> index 7205c59ff7294..bf42c250ea7d1 100644
> --- a/drivers/rtc/rtc-ds1307.c
> +++ b/drivers/rtc/rtc-ds1307.c
> @@ -308,7 +308,7 @@ static int ds1307_get_time(struct device *dev, struct rtc_time *t)
>  	t->tm_hour = bcd2bin(tmp);
>  	/* rx8130 is bit position, not BCD */
>  	if (ds1307->type == rx_8130)
> -		t->tm_wday = fls(regs[DS1307_REG_WDAY] & 0x7f);
> +		t->tm_wday = fls(regs[DS1307_REG_WDAY] & 0x7f) - 1;

[Severity: High]
This isn't a bug introduced by this patch, but does this introduce a new
non-malicious trigger for a pre-existing undefined behavior?

The kernel's rtc_valid_tm() omits validation of tm_wday. A malicious userspace
program with CAP_SYS_TIME could already pass a negative value to RTC_SET_TIME,
triggering an out-of-bounds shift in ds1307_set_time():

drivers/rtc/rtc-ds1307.c:ds1307_set_time() {
    ...
	if (ds1307->type == rx_8130)
		regs[DS1307_REG_WDAY] = 1 << t->tm_wday;
    ...
}

With this patch, if the hardware WDAY register is zeroed, fls(0) returns 0,
making t->tm_wday equal to -1. A benign read-modify-write cycle by standard
tools (like hwclock) will now naturally trigger the undefined behavior.

>  	else
>  		t->tm_wday = bcd2bin(regs[DS1307_REG_WDAY] & 0x07) - 1;
>  	t->tm_mday = bcd2bin(regs[DS1307_REG_MDAY] & 0x3f);

[Severity: High]
This is a pre-existing issue, but does rx8130_set_alarm() corrupt the RTC's
hardware configuration?

drivers/rtc/rtc-ds1307.c:rx8130_set_alarm() {
    ...
	/* Read control registers. */
	ret = regmap_bulk_read(ds1307->regmap, RX8130_REG_EXTENSION, ctl,
			       sizeof(ctl));
	if (ret < 0)
		return ret;

	ctl[0] &= RX8130_REG_EXTENSION_WADA;
	ctl[1] &= ~RX8130_REG_FLAG_AF;
	ctl[2] &= ~RX8130_REG_CONTROL0_AIE;
    ...
}

Because RX8130_REG_EXTENSION_WADA is defined as BIT(3), this operation masks
out and clears all other configuration bits in the register (such as timer
enables and frequency selection), while preserving the WADA bit if it was set
instead of clearing it.

Does this break other features every time an alarm is set?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260520-ds1307-rx8901-add-v2-0-e069ea32e1db@axis.com?part=2