Re: [PATCH V8 09/10] iio: imu: inv_icm42607: Add Gyroscope to icm42607

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Probe race condition leads to NULL pointer dereference on `st->indio_gyro`.
- [High] Data race and TOCTOU vulnerability in `hwfifo_flush` causes data duplication and timestamp corruption.
- [High] Incorrect devres registration order causes Use-After-Free during teardown.
- [Medium] The available sampling frequencies sysfs attribute exposes uninitialized memory (zeros).
- [Medium] Software timestamp state becomes permanently inconsistent if hardware configuration fails.
- [Medium] Gyroscope is left powered on indefinitely after a single raw read.
--

commit db77d2b5f802169a721fb840e81fd3f9add01065
Author: Chris Morgan <macromorgan@hotmail.com>

iio: imu: inv_icm42607: Add Gyroscope to icm42607

Add gyroscope functions to the icm42607 driver.

> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c
> index 0e723a7dd5623..355cd38601b9c 100644
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c
[ ... ]
> @@ -506,12 +519,14 @@ int inv_icm42607_buffer_fifo_parse(struct inv_icm42607_state *st)
>  int inv_icm42607_buffer_hwfifo_flush(struct inv_icm42607_state *st,
>  				     unsigned int count)
>  {
> +	struct inv_icm42607_sensor_state *gyro_st = iio_priv(st->indio_gyro);
>  	struct inv_icm42607_sensor_state *accel_st = iio_priv(st->indio_accel);
>  	struct inv_sensors_timestamp *ts;
> -	s64 accel_ts;
> +	s64 gyro_ts, accel_ts;
>  	int ret;
>  
>  	accel_ts = iio_get_time_ns(st->indio_accel);
> +	gyro_ts = iio_get_time_ns(st->indio_gyro);
>  
>  	ret = inv_icm42607_buffer_fifo_read(st, count);
>  	if (ret)
> @@ -520,6 +535,14 @@ int inv_icm42607_buffer_hwfifo_flush(struct inv_icm42607_state *st,
>  	if (st->fifo.nb.total == 0)
>  		return 0;
>  
> +	if (st->fifo.nb.gyro > 0) {
> +		ts = &gyro_st->ts;
> +		inv_sensors_timestamp_interrupt(ts, st->fifo.nb.gyro, gyro_ts);
> +		ret = inv_icm42607_gyro_parse_fifo(st->indio_gyro);
> +		if (ret)
> +			return ret;
> +	}

Since inv_icm42607_buffer_fifo_read() drops st->lock before returning, could
this result in a data race?

Concurrent calls from sysfs could overwrite the shared st->fifo.data and
state between the lock release and the subsequent lock acquisition inside
inv_icm42607_gyro_parse_fifo(), potentially causing duplicated data and
corrupted timestamp state.

[ ... ]
> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> index 4bbcbbe8905f4..a6b668e82c1a7 100644
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
[ ... ]
> @@ -440,6 +483,11 @@ int inv_icm42607_core_probe(struct regmap *regmap, const struct inv_icm42607_hw
>  	if (IS_ERR(st->indio_accel))
>  		return PTR_ERR(st->indio_accel);
>  
> +	/* Initialize IIO device for Gyro */
> +	st->indio_gyro = inv_icm42607_gyro_init(st);
> +	if (IS_ERR(st->indio_gyro))
> +		return PTR_ERR(st->indio_gyro);

If a user or udev interacts with the accelerometer sysfs interface (like
triggering hwfifo_flush) immediately after it is registered but before
st->indio_gyro is initialized, will this cause a NULL pointer dereference
when inv_icm42607_buffer_hwfifo_flush() unconditionally accesses
st->indio_gyro?

Additionally, does allocating the IIO devices after inv_icm42607_buffer_init()
introduce a use-after-free risk during teardown?

Since devres uses LIFO ordering, the IIO devices will be freed before the
hardware interrupt handler is removed. If an interrupt fires during removal,
the handler might access the freed st->indio_gyro and st->indio_accel.

[ ... ]
> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c
> new file mode 100644
> index 0000000000000..16c505ce70ae8
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c
[ ... ]
> +static int inv_icm42607_gyro_read_sensor(struct iio_dev *indio_dev,
> +					 struct iio_chan_spec const *chan,
> +					 s16 *val)
> +{
[ ... ]
> +	/* enable gyro sensor */
> +	conf.mode = gyro_st->power_mode;
> +	ret = inv_icm42607_set_gyro_conf(st, &conf, NULL);
> +	if (ret)
> +		return ret;
> +
> +	/* read gyro register data */
> +	data = &st->buffer[0];
> +	ret = regmap_bulk_read(st->map, reg, data, sizeof(*data));
> +	if (ret)
> +		return ret;
> +
> +	*val = be16_to_cpup(data);
> +	if (*val == INV_ICM42607_DATA_INVALID)
> +		return -EINVAL;
> +
> +	return 0;
> +}

Does this leave the gyroscope powered on indefinitely after a raw read?

It doesn't appear to be turned off again before returning, meaning the driver
will maintain the ON state even after resuming from runtime PM.

[ ... ]
> +static const int inv_icm42607_gyro_odr[][2] = {
> +	[INV_ICM42607_ODR_12_5HZ] = { 12, 500000 },
> +	[INV_ICM42607_ODR_25HZ] = { 25, 0 },
> +	[INV_ICM42607_ODR_50HZ] = { 50, 0 },
> +	[INV_ICM42607_ODR_100HZ] = { 100, 0 },
> +	[INV_ICM42607_ODR_200HZ] = { 200, 0 },
> +	[INV_ICM42607_ODR_400HZ] = { 400, 0 },
> +	[INV_ICM42607_ODR_800HZ] = { 800, 0 },
> +	[INV_ICM42607_ODR_1600HZ] = { 1600, 0 },
> +};

Could this initialization expose zeros to userspace?

Since the first index (INV_ICM42607_ODR_12_5HZ) evaluates to a non-zero value,
the leading elements in this array will be implicitly zero-initialized. When
returned via inv_icm42607_gyro_read_avail(), those zeros might break the IIO
sysfs API contract.

[ ... ]
> +static int inv_icm42607_gyro_write_odr(struct iio_dev *indio_dev,
> +				       int val, int val2)
> +{
[ ... ]
> +	ret = inv_sensors_timestamp_update_odr(ts, inv_icm42607_odr_to_period(conf.odr),
> +					       iio_buffer_enabled(indio_dev));
> +	if (ret)
> +		return ret;
> +
> +	ret = inv_icm42607_set_gyro_conf(st, &conf, NULL);
> +	if (ret)
> +		return ret;

If inv_icm42607_set_gyro_conf() fails here, does the software timestamp state
become permanently inconsistent?

It seems ts is updated before the I2C/SPI write, and there is no rollback
of the timestamp configuration on failure.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518200526.458421-1-macroalpha82@gmail.com?part=9