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

[Esben Haabendal <esben@geanix.com>]

<sashiko-bot@kernel.org> writes:

> 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.

Adressed in follow-up patch series.

> [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?

Adressed in follow-up patch series.

> [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?

I will be converting the i2c register access to regmap in the follow-up
patch series I am working on, and the regmap API does not have this
issue, so should resolve this issue also.

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

/Esben