Re: [PATCH v4 2/9] rtc: abx80x: fix error check after i2c_smbus_read in read_alarm()

[sashiko-bot@kernel.org]

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

New issues:
- [High] The relaxed error check allows short I2C reads to bypass validation, resulting in uninitialized stack memory being parsed and used.

Pre-existing issues:
- [High] The patch fails to properly fix the issue it claims to address because it returns a positive byte count on success, which the RTC subsystem treats as an error.
- [High] Concurrent sysfs accesses can cause data corruption in the hardware oscillator control register.
- [High] Concurrent NVMEM accesses cause data corruption due to unprotected page register updates.
--

--- Patch [2]: [PATCH v4 2/9] rtc: abx80x: fix error check after i2c_smbus_read in read_alarm() ---
commit 0f39bb1c012fd3e70a85b6d2103b11a6c2ce0801
Author: Antoni Pokusinski <apokusinski01@gmail.com>

    rtc: abx80x: fix error check after i2c_smbus_read in read_alarm()
    
    i2c_smbus_read_i2c_block_data() returns the number of bytes read on
    success and a negative error code on failure. The existing code treated
    any non-zero return value as an error.
    
    Fix the error handling by checking only for negative return values.

> diff --git a/drivers/rtc/rtc-abx80x.c b/drivers/rtc/rtc-abx80x.c
> index 5486d9d0b1e5d..fdf9664ebb6f0 100644
> --- a/drivers/rtc/rtc-abx80x.c
> +++ b/drivers/rtc/rtc-abx80x.c
> @@ -294,7 +294,7 @@ static int abx80x_read_alarm(struct device *dev, struct rtc_wkalrm *t)
>  
>  	err = i2c_smbus_read_i2c_block_data(client, ABX8XX_REG_ASC,
>  					    sizeof(buf), buf);
> -	if (err)
> +	if (err < 0)

[Severity: High]
Does this relaxed error check allow short I2C reads to bypass validation?

If i2c_smbus_read_i2c_block_data() returns a positive byte count less than
sizeof(buf), this check will pass and execution continues. The subsequent code
will then parse uninitialized stack memory from buf to populate the alarm
configuration, which could cause undefined behavior.

>  		return err;
>  
>  	irq_mask = i2c_smbus_read_byte_data(client, ABX8XX_REG_IRQ);


[Severity: High]
This isn't a bug introduced by this patch, but since err contains the positive
number of bytes read on success, does returning it unmodified at the end of
abx80x_read_alarm() cause the RTC core to treat the success case as a failure?

drivers/rtc/rtc-abx80x.c:abx80x_read_alarm() {
...
	t->enabled = !!(irq_mask & ABX8XX_IRQ_AIE);
	t->pending = (buf[6] & ABX8XX_STATUS_AF) && t->enabled;

	return err;
}

Since the caller will still see a positive return value, the alarm feature 
remains broken. Should this be updated to return 0 on success?


[Severity: High]
This is a pre-existing issue, but does abx80x_rtc_read_time() also suffer from
the same missing length check pattern?

drivers/rtc/rtc-abx80x.c:abx80x_rtc_read_time() {
...
	err = i2c_smbus_read_i2c_block_data(client, ABX8XX_REG_HTH,
					    sizeof(buf), buf);
	if (err < 0) {
		dev_err(&client->dev, "Unable to read date\n");
		return -EIO;
	}

	tm->tm_sec = bcd2bin(buf[ABX8XX_REG_SC] & 0x7F);
...
}

If a short read occurs, this allows uninitialized stack memory to be used when
setting the time.


[Severity: High]
This is a pre-existing issue, but can concurrent sysfs accesses cause data
corruption in the hardware oscillator control register?

The operations in abx80x_rtc_set_autocalibration() and oscillator_store() both
perform lockless read-modify-write on ABX8XX_REG_OSC:

drivers/rtc/rtc-abx80x.c:abx80x_rtc_set_autocalibration() {
...
	flags = i2c_smbus_read_byte_data(client, ABX8XX_REG_OSC);
...
	retval = i2c_smbus_write_byte_data(client, ABX8XX_REG_OSC, flags);
...
}

drivers/rtc/rtc-abx80x.c:oscillator_store() {
...
	flags =  i2c_smbus_read_byte_data(client, ABX8XX_REG_OSC);
...
	retval = i2c_smbus_write_byte_data(client, ABX8XX_REG_OSC, flags);
...
}

Because these sysfs operations do not serialize through the RTC core's ops_lock,
is it possible for one thread's modification to be clobbered by another?


[Severity: High]
This is a pre-existing issue, but do concurrent NVMEM accesses cause data
corruption due to unprotected page register updates?

In abx80x_nvmem_xfer(), the driver writes to the page register before
performing an I2C block operation:

drivers/rtc/rtc-abx80x.c:abx80x_nvmem_xfer() {
...
		ret = i2c_smbus_write_byte_data(priv->client, ABX8XX_REG_EXTRAM,
						extram);
		if (ret)
			return ret;

		if (write) {
			ret = i2c_smbus_write_i2c_block_data(priv->client, reg,
							     len, val);
...
}

Since the NVMEM core does not serialize accesses and the driver provides no
internal locking, could concurrent reads or writes from userspace interleave,
causing operations to target the incorrect memory page?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260810193826.210683-1-apokusinski01@gmail.com?part=2