Re: [PATCH v6 5/9] rtc: abx80x: add irq to struct abx80x_priv

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unprotected I2C reads in `abx80x_rtc_read_time`, `abx80x_read_alarm`, and sysfs show functions can interleave with and break multi-transaction I2C sequences protected by `priv->lock`.
- [High] `abx80x_ioctl(RTC_VL_CLR)` is vulnerable to a TOCTOU race with the hardware that can unintentionally clear unrelated interrupts like `ABX8XX_STATUS_AF`.
--

commit b245a6cf28d590ef87d1389ee95de3f0f3c0348b
Author: Antoni Pokusinski <apokusinski01@gmail.com>

rtc: abx80x: add irq to struct abx80x_priv

This patch adds the irq number to struct abx80x_priv so it is not tied to
the i2c_client. This allows the irq to be easily accessed also in the case
of ABX81X SPI clocks.

> diff --git a/drivers/rtc/rtc-abx80x.c b/drivers/rtc/rtc-abx80x.c
> index 8f9211bfb7e24..529f63e024f8d 100644
> --- a/drivers/rtc/rtc-abx80x.c
> +++ b/drivers/rtc/rtc-abx80x.c

[ ... ]

> @@ -305,11 +306,12 @@ static irqreturn_t abx80x_handle_irq(int irq, void *dev_id)
>  static int abx80x_read_alarm(struct device *dev, struct rtc_wkalrm *t)
>  {
>  	struct i2c_client *client = to_i2c_client(dev);
> +	struct abx80x_priv *priv = i2c_get_clientdata(client);
>  	unsigned char buf[7];
>  
>  	int irq_mask, err;
>  
> -	if (client->irq <= 0)
> +	if (priv->irq <= 0)
>  		return -EINVAL;
>  
>  	err = i2c_smbus_read_i2c_block_data(client, ABX8XX_REG_ASC,

[Severity: High]
This isn't a bug introduced by this patch, but this pre-existing issue
performs I2C reads here in abx80x_read_alarm() without acquiring priv->lock.

Could concurrent execution of these lockless I2C reads via userspace ioctls
interleave with multi-transaction I2C sequences protected by priv->lock?

For example, if this read interrupts the sequence between writing a config
key and a target register in functions like abx80x_rtc_set_autocalibration()
or oscillator_store(), would it reset the hardware config key and cause the
subsequent register write to fail silently?

A similar issue also exists in abx80x_rtc_read_time() and the sysfs show
functions.

[ ... ]


While reviewing this driver, I noticed another pre-existing issue in the
unmodified abx80x_ioctl() code:

drivers/rtc/rtc-abx80x.c:abx80x_ioctl() {
    ...
	case RTC_VL_CLR:
		scoped_guard(mutex, &priv->lock) {
			status = i2c_smbus_read_byte_data(client, ABX8XX_REG_STATUS);
			if (status < 0)
				return status;

			status &= ~ABX8XX_STATUS_BLF;

			tmp = i2c_smbus_write_byte_data(client, ABX8XX_REG_STATUS,
							status);
    ...
}

[Severity: High]
This is a pre-existing issue, but does this read-modify-write on a
Write-0-to-Clear (W0C) register create a Time-Of-Check to Time-Of-Use
(TOCTOU) race?

If the hardware asserts an unrelated interrupt (like ABX8XX_STATUS_AF)
between the read and the write, it seems this write will place a 0 in those
bits and unintentionally clear the new interrupts, causing permanent loss
of alarm or watchdog events.

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