Re: [PATCH v6 8/9] iio: accel: mma8452: Fix use-after-free bug in error error path

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The device is left in ACTIVE mode on probe failure paths, potentially draining power and asserting interrupts indefinitely.
- [High] The driver leaks the PM runtime usage count on multiple error paths when I2C reads/writes fail after acquiring a runtime PM reference.
- [High] In `mma8452_runtime_suspend()`, if disabling the second regulator fails, the first regulator remains disabled but the PM state is left as RPM_ACTIVE.
- [High] Time-of-check to time-of-use (TOCTOU) race condition during read-modify-write of I2C event configuration registers.
--

--- Patch [8]: [PATCH v6 8/9] iio: accel: mma8452: Fix use-after-free bug in error error path ---
commit bad43bbf94c93f7ecf6436d8c780a1207e0288f4
Author: Esben Haabendal <esben@geanix.com>

iio: accel: mma8452: Fix use-after-free bug in error error path

This commit fixes a use-after-free in the mma8452_probe error path.
By adding a runtime_suspend label, it ensures the runtime PM disable step
happens before the IRQ is freed, avoiding callbacks with a freed device.

> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index 9ae2c3e605761..4a1eb196589ab 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
> @@ -1743,7 +1743,7 @@ static int mma8452_probe(struct i2c_client *client)
>  
>  	ret = iio_device_register(indio_dev);
>  	if (ret < 0)
> -		goto free_irq;
> +		goto runtime_suspend;
>  
>  	ret = mma8452_set_freefall_mode(data, false);

[Severity: High]
This isn't a bug introduced by this patch, but is there a data race in
mma8452_set_freefall_mode() called here?

It looks like mma8452_set_freefall_mode() reads the configuration register
locklessly before modifying it:

mma8452_set_freefall_mode() {
	...
	val = i2c_smbus_read_byte_data(data->client, MMA8452_FF_MT_CFG);
	...
	return mma8452_change_config(data, MMA8452_FF_MT_CFG, val);
}

Since mma8452_change_config() only acquires data->lock during the write,
could concurrent modifications via sysfs overwrite and discard each other's
changes? A similar pattern appears to exist in mma8452_write_event_config().

>  	if (ret < 0)

[ ... ]

> @@ -1754,6 +1754,10 @@ static int mma8452_probe(struct i2c_client *client)
>  unregister_device:
>  	iio_device_unregister(indio_dev);
>  
> +runtime_suspend:
> +	pm_runtime_disable(dev);
> +	pm_runtime_set_suspended(dev);
> +

[Severity: High]
This is a pre-existing issue, but does this error path skip placing the
hardware back into standby mode?

If mma8452_probe() fails and jumps to this runtime_suspend label, the
software resources are disabled, but the device might be left in ACTIVE
mode. Could this result in the device indefinitely asserting a shared
interrupt line if it was previously activated, since the IRQ handler is freed
shortly after?

Should there be a call to mma8452_standby() in this error path?

>  free_irq:
>  	if (client->irq)
>  		free_irq(client->irq, indio_dev);

[Severity: High]
This is a pre-existing issue, but does mma8452_read() leak a runtime PM
usage count on I2C errors?

If mma8452_set_runtime_pm_state(..., true) succeeds, but the subsequent I2C
block read fails:

mma8452_read() {
	...
	ret = i2c_smbus_read_i2c_block_data(data->client, MMA8452_OUT_X,
					    3 * sizeof(__be16), (u8 *)buf);
	if (ret < 0)
		return ret;
	...
}

The function returns immediately without rolling back the PM reference.
I noticed similar early returns in mma8452_write_event_config() and
mma8452_data_rdy_trigger_set_state() that might leave the device permanently
active.

[Severity: High]
This is also a pre-existing issue, but is the regulator state left unbalanced
if runtime suspend fails?

In mma8452_runtime_suspend():

mma8452_runtime_suspend() {
	...
	regulator_disable(data->vddio_reg);

	ret = regulator_disable(data->vdd_reg);
	if (ret) {
		dev_err(dev, "failed to disable VDD regulator\n");
		return ret;
	}
	...
}

If disabling vdd_reg fails, the function returns an error without
re-enabling vddio_reg. Since the PM core keeps the device in RPM_ACTIVE
on failure, won't subsequent suspend attempts call
regulator_disable(data->vddio_reg) again and underflow the enable count?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825-mma8452-open-drain-v6-0-9b252804ee80@geanix.com?part=8