Re: [PATCH v6 4/6] rtc: s35390a: Read 24-hour mode on access

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Uninitialized stack variable usage due to missing error check for negative return values from s35390a_read_status.
--

--- Patch [4]: [PATCH v6 4/6] rtc: s35390a: Read 24-hour mode on access ---
commit d45935347291ea27eb3558322c8f56d965fd15e6
Author: Markus Probst <markus.probst@posteo.de>

rtc: s35390a: Read 24-hour mode on access

This commit reads the 24-hour mode on access instead of only on probe. It
fixes time corruption when resetting the chip in 12-hour mode, as
s35390a_init did not update the cached value.

> diff --git a/drivers/rtc/rtc-s35390a.c b/drivers/rtc/rtc-s35390a.c
> index 575bb256eb258..8e3616c65d2c0 100644
> --- a/drivers/rtc/rtc-s35390a.c
> +++ b/drivers/rtc/rtc-s35390a.c
[ ... ]
> @@ -256,7 +258,7 @@ static int s35390a_rtc_read_time(struct device *dev, struct rtc_time *tm)
>  
>  	tm->tm_sec = bcd2bin(buf[S35390A_BYTE_SECS]);
>  	tm->tm_min = bcd2bin(buf[S35390A_BYTE_MINS]);
> -	tm->tm_hour = s35390a_reg2hr(s35390a, buf[S35390A_BYTE_HOURS]);
> +	tm->tm_hour = s35390a_reg2hr(buf[S35390A_BYTE_HOURS], status & S35390A_FLAG_24H);

[Severity: High]
Earlier in s35390a_rtc_read_time(), the code calls
s35390a_read_status(s35390a, &status).

If this call fails with a negative error code (e.g. -EIO due to a transient
I2C error), the check if (s35390a_read_status(s35390a, &status) == 1) evaluates
to false, and the function proceeds without returning an error.

Will this cause the uninitialized status stack variable to be evaluated here,
potentially corrupting the time calculation?

[ ... ]
> @@ -345,14 +347,17 @@ static int s35390a_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alm)
>  {
>  	struct i2c_client *client = to_i2c_client(dev);
>  	struct s35390a *s35390a = i2c_get_clientdata(client);
> -	u8 buf[3], sts;
> +	u8 buf[3], status1, status2;
>  	int i, err;
>  
> -	err = s35390a_get_reg(s35390a, S35390A_CMD_STATUS2, &sts, sizeof(sts));
> +	if (s35390a_read_status(s35390a, &status1) == 1)
> +		return -EINVAL;
> +
> +	err = s35390a_get_reg(s35390a, S35390A_CMD_STATUS2, &status2, sizeof(status2));
>  	if (err < 0)
>  		return err;

[ ... ]

> @@ -381,8 +386,8 @@ static int s35390a_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alm)
>  
>  	if (buf[S35390A_ALRM_BYTE_HOURS] & 0x80)
>  		alm->time.tm_hour =
> -			s35390a_reg2hr(s35390a,
> -				       buf[S35390A_ALRM_BYTE_HOURS] & ~0x80);
> +			s35390a_reg2hr(buf[S35390A_ALRM_BYTE_HOURS] & ~0x80,
> +				       status1 & S35390A_FLAG_24H);

[Severity: High]
Similarly to s35390a_rtc_read_time(), if s35390a_read_status() returns a
negative error code, the check above will pass and the status1 stack variable
will remain uninitialized.

Could this allow the uninitialized status1 variable to be used here to determine
the alarm hour calculations?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260824-rtc_s35390a_int1-v6-0-df96569e3412@posteo.de?part=4