Re: [PATCH v6 2/9] iio: accel: mma8452: Optimize struct mma8452_data member orders

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Short I2C reads lead to an uninitialized kernel stack memory leak to userspace.
- [Medium] Runtime PM reference leak on I2C communication failure.
- [Medium] Unbalanced regulator disable and unpowered I2C access during module removal.
- [Medium] Data race during read-modify-write of event configuration registers.
--

--- Patch [2]: [PATCH v6 2/9] iio: accel: mma8452: Optimize struct mma8452_data member orders ---
commit 26f60bee6a5ce9626d25a99fe80c0216eb010682
Author: Esben Haabendal <esben@geanix.com>

iio: accel: mma8452: Optimize struct mma8452_data member orders
    
Reorder struct mma8452_data members to avoid holes.

> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index 7d683686dd9d6..f645a5c6fd1c2 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
> @@ -106,10 +106,7 @@ struct mma8452_data {
>  	struct i2c_client *client;
>  	struct mutex lock;
>  	struct iio_mount_matrix orientation;
> -	u8 ctrl_reg1;
> -	u8 data_cfg;
>  	const struct mma_chip_info *chip_info;
> -	int sleep_val;
>  	struct regulator *vdd_reg;
>  	struct regulator *vddio_reg;
>  
> @@ -118,6 +115,10 @@ struct mma8452_data {
>  		__be16 channels[3];
>  		aligned_s64 ts;
>  	} buffer;
> +
> +	int sleep_val;
> +	u8 ctrl_reg1;
> +	u8 data_cfg;
>  };

[Severity: Medium]
This is a pre-existing issue, but does this early return leak the runtime PM
reference count?

drivers/iio/accel/mma8452.c:mma8452_read() {
...
	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, the function returns directly,
skipping the mma8452_set_runtime_pm_state() call.

A similar issue appears in mma8452_data_rdy_trigger_set_state() where a
failure during i2c_smbus_read_byte_data() skips dropping the autosuspend
reference if state was true.

[Severity: Medium]
This isn't a bug introduced by this patch, but during module removal, does
mma8452_remove() execute operations on an already suspended device?

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 module is removed while the device is in a runtime-suspended state,
pm_runtime_disable() does not wake up the device. Subsequent calls to
mma8452_standby() and regulator_disable() are then executed on an unpowered
device.

[Severity: Medium]
This is a pre-existing issue, but can concurrent sysfs writes cause a data
race when updating event configuration registers?

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

The read-modify-write sequence is performed without acquiring the internal
driver lock.

If two threads concurrently write to different event attributes in sysfs,
could the initial read and bitwise modifications race, overwriting each
other's configuration bits?

[Severity: High]
This isn't a bug introduced by this patch, but can a short I2C read result
in uninitialized stack memory being leaked to userspace?

drivers/iio/accel/mma8452.c:mma8452_read() {
...
	ret = i2c_smbus_read_i2c_block_data(data->client, MMA8452_OUT_X,
					    3 * sizeof(__be16), (u8 *)buf);
	if (ret < 0)
		return ret;

	ret = mma8452_set_runtime_pm_state(data->client, false);

	return ret;
}

If i2c_smbus_read_i2c_block_data() encounters a short read, it returns a
positive number less than the requested size. The condition ret < 0 is false,
so the code proceeds and overwrites ret with the return value of
mma8452_set_runtime_pm_state(), which is 0.

When mma8452_read() returns 0 instead of propagating the short read:

drivers/iio/accel/mma8452.c:mma8452_read_raw() {
...
		ret = mma8452_read(data, buffer);
...
		*val = sign_extend32(be16_to_cpu(
			buffer[chan->scan_index]) >> chan->scan_type.shift,
			chan->scan_type.realbits - 1);
...
}

Does the caller process and return the uninitialized stack memory from buffer
via *val?

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