Re: [PATCH v6 4/9] rtc: abx80x: properly handle shared IRQs

[sashiko-bot@kernel.org]

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

New issues:
- [High] Multi-step configuration key unlock and register write in `abx80x_probe` is unprotected by a mutex, leading to a race condition with concurrently registered userspace interfaces.

Pre-existing issues:
- [High] Read-modify-write of the W0C status register in `abx80x_ioctl` (`RTC_VL_CLR`) can silently clear and drop concurrently set interrupt flags like the Alarm Flag.
--

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

    rtc: abx80x: properly handle shared IRQs
    
    The abx80x RTC driver registers its interrupt handler with IRQF_SHARED,
    but the handler always returned IRQ_HANDLED and cleared the status
    register even when no interrupt source from the RTC was active.
    
    This could cause the driver to claim interrupts belonging to other
    devices sharing the same IRQ line and could also clear unrelated status
    bits in the RTC status register.

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

[ ... ]

> @@ -952,6 +965,41 @@ static int abx80x_probe(struct i2c_client *client)
>  	if (err)
>  		return err;
>  
> +	/* Disable unused interrupts */
> +	data = i2c_smbus_read_byte_data(client, ABX8XX_REG_IRQ);
> +	if (data < 0) {
> +		dev_err(&client->dev, "Unable to read irq register\n");
> +		return -EIO;
> +	}
> +
> +	err = i2c_smbus_write_byte_data(client, ABX8XX_REG_IRQ,
> +					data & ~(ABX8XX_IRQ_EX1E |
> +						 ABX8XX_IRQ_EX2E |
> +						 ABX8XX_IRQ_TIE |
> +						 ABX8XX_IRQ_BLIE));
> +	if (err < 0) {
> +		dev_err(&client->dev, "Unable to write irq register\n");
> +		return -EIO;
> +	}
> +
> +	data = i2c_smbus_read_byte_data(client, ABX8XX_REG_OSC);
> +	if (data < 0) {
> +		dev_err(&client->dev, "Unable to read Oscillator Control register\n");
> +		return -EIO;
> +	}
> +
> +	/* Unlock write access to Oscillator Control Register */
> +	if (abx80x_write_config_key(client, ABX8XX_CFG_KEY_OSC) < 0)
> +		return -EIO;
> +
> +	err = i2c_smbus_write_byte_data(client, ABX8XX_REG_OSC,
> +					data & ~(ABX8XX_OSC_ACIE |
> +						 ABX8XX_OSC_OFIE));

[Severity: High]
Since this unlocked two-step I2C transaction in abx80x_probe() occurs after
the watchdog is exposed to userspace, is it vulnerable to concurrent aborts?

If a concurrent userspace process opens and pings the watchdog, it
acquires priv->lock and issues an I2C write. This would interleave and
abort the configuration key sequence, causing the subsequent write to
ABX8XX_REG_OSC to be silently ignored.

This would leave unwanted hardware interrupts (ACIE, OFIE) enabled. If
these fire, the newly updated IRQ handler returns IRQ_NONE without
clearing them, causing an interrupt storm that forces the kernel to
permanently disable the RTC IRQ line.

> +	if (err < 0) {
> +		dev_err(&client->dev, "Unable to write Oscillator Control register\n");
> +		return -EIO;
> +	}
> +
>  	if (client->irq > 0) {
>  		dev_info(&client->dev, "IRQ %d supplied\n", client->irq);
>  		err = devm_request_threaded_irq(&client->dev, client->irq, NULL,


This isn't a bug introduced by this patch, but looking at the RTC_VL_CLR
handling in abx80x_ioctl():

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]
Can this read-modify-write pattern silently clear and drop concurrently
set interrupt flags like the Alarm Flag?

Because ABX8XX_REG_STATUS is a Write-0-to-Clear (W0C) register, any bit
that was 0 during the read is written back as 0. If the hardware asserts
a new flag in the window between the read and the write, the write of 0
will accidentally clear it before the IRQ handler can process it.

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