Re: [PATCH v7 7/8] 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] Missing runtime PM reference during probe() allows concurrent suspend and unbalanced regulator disables.
- [High] Unpowered I2C access and unbalanced regulator disables in mma8452_remove().
- [High] Runtime PM reference leak in mma8452_read() on I2C error path.
- [High] Runtime PM reference leak in mma8452_write_event_config() and mma8452_data_rdy_trigger_set_state() on I2C error paths.
--

commit f144db47a6c84b2bd25c6e5cc7cf38650461861a
Author: Esben Haabendal <esben@geanix.com>

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

If mma8452_probe() fails in iio_device_register() or later, we could end up
with runtime suspend callback being called with a now freed device pointer.

Cc: stable@vger.kernel.org

> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index bf56ebd6e091a..9619b200fa5a9 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
> @@ -1745,7 +1745,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);
>  	if (ret < 0)
> @@ -1756,6 +1756,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);
> +
>  free_irq:
>  	if (client->irq)
>  		free_irq(client->irq, indio_dev);

[Severity: High]
This isn't a bug introduced by this patch, but looking at the error paths here
in mma8452_probe(), is it possible to hit an unbalanced regulator disable if
the device suspends concurrently?

Because the shared IRQ is requested earlier in mma8452_probe(), if the
interrupt fires, mma8452_interrupt() increments and then drops the usage count
via pm_runtime_put_autosuspend():

drivers/iio/accel/mma8452.c:mma8452_interrupt() {
    ...
    if (pm_status > 0)
        pm_runtime_put_autosuspend(&data->client->dev);
    ...
}

Since pm_runtime_use_autosuspend() hasn't been called yet during this early
phase in mma8452_probe():

drivers/iio/accel/mma8452.c:mma8452_probe() {
    ...
    pm_runtime_enable(dev);
    pm_runtime_set_autosuspend_delay(dev, MMA8452_AUTO_SUSPEND_DELAY_MS);
    pm_runtime_use_autosuspend(dev);
    ...
}

Dropping the count schedules an immediate asynchronous suspend via the
workqueue. The device could suspend mid-probe, and mma8452_runtime_suspend()
would disable the hardware regulators. Subsequent I2C accesses in the
remainder of probe would hit unpowered hardware, leading to bus errors or
hangs.

If mma8452_probe() subsequently fails, the error path unconditionally calls
regulator_disable():

drivers/iio/accel/mma8452.c:mma8452_probe() {
    ...
disable_regulators:
    regulator_disable(data->vddio_reg);
    ...
}

Could this cause an unbalanced disable WARN? Should probe hold a PM usage
reference via pm_runtime_get_noresume() before enabling runtime PM?

[Severity: High]
This is a pre-existing issue, but does mma8452_remove() safely access I2C
hardware and disable regulators during driver unbind?

drivers/iio/accel/mma8452.c:mma8452_remove() {
    ...
    pm_runtime_disable(&client->dev);
    pm_runtime_set_suspended(&client->dev);

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

    iio_triggered_buffer_cleanup(indio_dev);
    mma8452_trigger_cleanup(indio_dev);
    mma8452_standby(iio_priv(indio_dev));

    regulator_disable(data->vddio_reg);
    regulator_disable(data->vdd_reg);
    ...
}

If the device was runtime suspended (which happens automatically after 2
seconds of inactivity), the hardware is unpowered and regulators are already
disabled. mma8452_standby() performs I2C accesses on unpowered hardware,
which will fail or hang the bus. The subsequent regulator_disable() calls
drop the enable count below zero, triggering a kernel WARN. Should
mma8452_remove() wake the device via pm_runtime_get_sync() before performing
hardware cleanup?

[Severity: High]
This isn't a bug introduced by this patch, but does mma8452_read() leak a
runtime PM reference on the I2C error path?

drivers/iio/accel/mma8452.c:mma8452_read() {
    ...
    ret = mma8452_set_runtime_pm_state(data->client, true);
    if (ret)
        return ret;

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

If i2c_smbus_read_i2c_block_data() fails, it returns the error immediately
without calling mma8452_set_runtime_pm_state(..., false). This leaves the PM
usage count unbalanced, preventing the device from entering runtime suspend
and causing a permanent power drain.

[Severity: High]
This isn't a bug introduced by this patch, but there appear to be similar PM
reference leaks in mma8452_write_event_config() and
mma8452_data_rdy_trigger_set_state() on I2C error paths:

drivers/iio/accel/mma8452.c:mma8452_write_event_config() {
    ...
        val = i2c_smbus_read_byte_data(data->client, ev_regs->ev_cfg);
        if (val < 0)
            return val;
    ...
}

drivers/iio/accel/mma8452.c:mma8452_data_rdy_trigger_set_state() {
    ...
    reg = i2c_smbus_read_byte_data(data->client, MMA8452_CTRL_REG4);
    if (reg < 0)
        return reg;
    ...
}

Both functions acquire a PM reference. If the subsequent hardware
configuration via I2C read fails, they return the error immediately without
rolling back the PM state, permanently breaking power management for the
device.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831-mma8452-open-drain-v7-0-22946812c928@geanix.com?part=7