Re: [PATCH V5 10/11] iio: imu: inv_icm42607: Add Gyroscope to icm42607

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [Critical] Missing PM runtime wake in `inv_icm42607_gyro_hwfifo_flush` triggers a heap buffer overflow.
- [High] Missing hardware wait, ignored error code, and power state leak in `inv_icm42607_gyro_read_sensor`.
- [High] Software state desynchronization causes unintended hardware shutdown.
- [High] Out-of-bounds kernel memory read in `inv_icm42607_gyro_parse_fifo`.
- [High] Standalone temperature scanning is completely broken due to parser logic.
- [Medium] Type mismatch and invalid boundary units in `inv_icm42607_gyro_hwfifo_flush`.
- [Medium] Uninitialized padding in `inv_icm42607_gyro_odr` array bypasses validation and writes reserved hardware state.
- [Medium] Gyroscope timestamp initialized using the accelerometer's ODR.
- [Medium] Premature IIO device registration causes race condition during probe.
--

commit c1f5aefdb4a5a29bea5b6cae45743d275de06e35
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_core.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
[ ... ]
> @@ -215,6 +215,49 @@ int inv_icm42607_set_accel_conf(struct inv_icm42607_state *st,
>  					  st->conf.temp_en, sleep_ms);
>  }
>  
> +int inv_icm42607_set_gyro_conf(struct inv_icm42607_state *st,
> +			       struct inv_icm42607_sensor_conf *conf,
> +			       unsigned int *sleep_ms)
> +{
> +	struct inv_icm42607_sensor_conf *oldconf = &st->conf.gyro;
> +	unsigned int val;
> +	int ret;
> +
> +	if (conf->mode < 0)
> +		conf->mode = oldconf->mode;
[ ... ]
> +	if (conf->filter != oldconf->filter) {
> +		val = FIELD_PREP(INV_ICM42607_GYRO_CONFIG1_FILTER_MASK,
> +				 conf->filter);
> +		ret = regmap_update_bits(st->map, INV_ICM42607_REG_GYRO_CONFIG1,
> +					 INV_ICM42607_GYRO_CONFIG1_FILTER_MASK, val);
> +		if (ret)
> +			return ret;
> +		oldconf->filter = conf->filter;
> +	}
> +
> +	return inv_icm42607_set_pwr_mgmt0(st, conf->mode, st->conf.accel.mode,
> +					  st->conf.temp_en, sleep_ms);
> +}

Does this code need to assign conf->mode to oldconf->mode? By not updating the
cached state, is it possible that subsequent operations will save an incorrect
power state during suspend, causing an unintended hardware shutdown on resume?

[ ... ]
> @@ -575,6 +619,11 @@ int inv_icm42607_core_probe(struct regmap *regmap, const struct inv_icm42607_hw
>  	pm_runtime_set_autosuspend_delay(dev, INV_ICM42607_SUSPEND_DELAY_MS);
>  	pm_runtime_use_autosuspend(dev);
>  
> +	/* 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);
> +
>  	/* Initialize IIO device for Accel */
>  	st->indio_accel = inv_icm42607_accel_init(st);

Since inv_icm42607_irq_init() is called later in inv_icm42607_core_probe(),
could this expose the IIO device to userspace before the interrupt handler is
ready, allowing a race condition if the buffer is enabled immediately?

> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c
[ ... ]
> +static int inv_icm42607_gyro_read_sensor(struct inv_icm42607_state *st,
> +					 struct iio_chan_spec const *chan,
> +					 s16 *val)
> +{
[ ... ]
> +	/* enable gyro sensor */
> +	conf.mode = INV_ICM42607_SENSOR_MODE_LOW_NOISE;
> +	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)
> +		ret = -EINVAL;
> +
> +	return 0;
> +}

Are there a few potential issues in this function?

First, by passing NULL for sleep_ms to inv_icm42607_set_gyro_conf(), does
this bypass the required sleep for the hardware to settle, resulting in a read
of stale registers?

Second, when the data is invalid and ret is set to -EINVAL, the function
unconditionally returns 0. Will this expose the invalid value to userspace
instead of returning the error?

Finally, does the function need to restore the sensor power mode to an off
state before returning? As written, does it leave the gyroscope permanently
powered on after a single sysfs read?

[ ... ]
> +static const int inv_icm42607_gyro_odr[][2] = {
> +	[INV_ICM42607_ODR_12_5HZ] = { 12, 500000 },
> +	[INV_ICM42607_ODR_25HZ] = { 25, 0 },
[ ... ]
> +static int inv_icm42607_gyro_write_odr(struct iio_dev *indio_dev,
> +				       int val, int val2)
> +{
[ ... ]
> +	for (idx = 0; idx < ARRAY_SIZE(inv_icm42607_gyro_odr); ++idx) {
> +		if (val == inv_icm42607_gyro_odr[idx][0] &&
> +		    val2 == inv_icm42607_gyro_odr[idx][1])
> +			break;
> +	}
> +	if (idx >= ARRAY_SIZE(inv_icm42607_gyro_odr))
> +		return -EINVAL;
> +
> +	conf.odr = idx;

Since the inv_icm42607_gyro_odr array uses designated initializers starting at
index 5 (INV_ICM42607_ODR_1600HZ), are indices 0-4 implicitly initialized to
{0, 0}? If userspace writes a frequency of 0, will this bypass validation
and write a reserved value of 0 into the hardware register?

[ ... ]
> +static int inv_icm42607_gyro_hwfifo_flush(struct iio_dev *indio_dev,
> +					  unsigned int count)
> +{
> +	struct inv_icm42607_state *st = iio_device_get_drvdata(indio_dev);
> +	int ret;
> +
> +	if (count == 0)
> +		return 0;
> +
> +	/* Make sure we don't try to count more than the buffer can hold. */
> +	count = min(count, sizeof(st->fifo.data));
> +
> +	guard(mutex)(&st->lock);
> +
> +	ret = inv_icm42607_buffer_hwfifo_flush(st, count);

Should this function call PM_RUNTIME_ACQUIRE_AUTOSUSPEND() before accessing
device registers via inv_icm42607_buffer_hwfifo_flush()? If called while
suspended, could it read 0xFFFF from the hardware, which might lead to a
buffer overflow downstream when max() is used with this value?

Additionally, does the min() check mix unsigned int and size_t, which might
cause a compiler warning? Also, since count is a number of samples, does
checking it against the byte capacity of the buffer (sizeof) permit requesting
far more samples than the buffer can hold?

[ ... ]
> +struct iio_dev *inv_icm42607_gyro_init(struct inv_icm42607_state *st)
> +{
[ ... ]
> +	/*
> +	 * clock period is 32kHz (31250ns)
> +	 * jitter is +/- 2% (20 per mille)
> +	 */
> +	ts_chip.clock_period = 31250;
> +	ts_chip.jitter = 20;
> +	ts_chip.init_period = inv_icm42607_odr_to_period(st->conf.accel.odr);
> +	inv_sensors_timestamp_init(&gyro_st->ts, &ts_chip);

Should this use st->conf.gyro.odr instead of st->conf.accel.odr? If the
default output data rates differ, could this cause the software IIO timestamps
for the gyroscope to start out skewed?

[ ... ]
> +int inv_icm42607_gyro_parse_fifo(struct iio_dev *indio_dev)
> +{
[ ... ]
> +	/* parse all fifo packets */
> +	for (i = 0, no = 0; i < st->fifo.count; i += size, ++no) {
> +		size = inv_icm42607_fifo_decode_packet(&st->fifo.data[i],
> +				&accel, &gyro, &temp, &timestamp, &odr);
> +		/* quit if error or FIFO is empty */
> +		if (size <= 0)
> +			return size;
> +
> +		/* skip packet if no gyro data or data is invalid */
> +		if (gyro == NULL || !inv_icm42607_fifo_is_data_valid(gyro))
> +			continue;
> +
> +		/* update odr */
> +		if (odr & INV_ICM42607_SENSOR_GYRO)
> +			inv_sensors_timestamp_apply_odr(ts, st->fifo.period,
> +							st->fifo.nb.total, no);
> +
> +		memcpy(&buffer.gyro, gyro, sizeof(buffer.gyro));

Does the loop need a bounds check to ensure i + size <= st->fifo.count?
Without it, if a partial packet is present, could this copy out-of-bounds
kernel memory into the IIO buffer?

Also, if only the temperature channel is enabled, will gyro always be NULL?
Since this loop continues when gyro is NULL, will temperature packets be
silently discarded, leaving userspace readers without data?

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