Re: [PATCH V5 08/11] iio: imu: inv_icm42607: Add Accelerometer for icm42607

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [High] Uninitialized `st->indio_gyro` dereference in `hwfifo_flush`.
- [High] Uninitialized variable and buffer overflow in `inv_icm42607_buffer_fifo_read`.
- [High] Out-of-bounds read in `inv_icm42607_accel_parse_fifo` due to missing boundary check.
- [High] Torn read and data race on `st->timestamp.accel`.
- [Medium] Ignored error code in `inv_icm42607_accel_read_sensor`.
- [Medium] Missing rollback of temperature sensor configuration on error.
- [Medium] Software timestamp state is not rolled back if hardware ODR configuration fails.
- [Medium] IIO device registration race with wakeup configuration.
- [Medium] Invalid Array Hole / Reserved State Exposure in `inv_icm42607_accel_odr`.
--

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

iio: imu: inv_icm42607: Add Accelerometer for icm42607

Add icm42607 accelerometer sensor for icm42607.

> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c
> index 0000000000000..f90ba46bd7e48
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c
[ ... ]
> +static int inv_icm42607_accel_update_scan_mode(struct iio_dev *indio_dev,
> +					       const unsigned long *scan_mask)
> +{
[ ... ]
> +	if (*scan_mask & INV_ICM42607_SCAN_MASK_TEMP) {
> +		/* enable temp sensor */
> +		ret = inv_icm42607_set_temp_conf(st, true, &sleep_temp);
> +		if (ret)
> +			goto out_unlock;
> +		fifo_en |= INV_ICM42607_SENSOR_TEMP;
> +	}
> +
> +	if (*scan_mask & INV_ICM42607_SCAN_MASK_ACCEL_3AXIS) {
> +		/* enable accel sensor */
> +		conf.mode = accel_st->power_mode;
> +		conf.filter = accel_st->filter;
> +		ret = inv_icm42607_set_accel_conf(st, &conf, &sleep_accel);
> +		if (ret)
> +			goto out_unlock;

If inv_icm42607_set_accel_conf() fails here, the function jumps to out_unlock
and returns an error.

Does this leave the temperature sensor actively consuming power indefinitely,
since it is not disabled on the error path?

[ ... ]
> +static int inv_icm42607_accel_read_sensor(struct iio_dev *indio_dev,
> +					  struct iio_chan_spec const *chan,
> +					  s16 *val)
> +{
[ ... ]
> +	*val = be16_to_cpup(data);
> +	if (*val == INV_ICM42607_DATA_INVALID)
> +		ret = -EINVAL;
> +
> +	return 0;
> +}

Should this return ret instead of 0?

If *val == INV_ICM42607_DATA_INVALID, ret is set to -EINVAL but then discarded.
This regression would silently leak the invalid data value (-32768) to
user-space.

[ ... ]
> +static const int inv_icm42607_accel_odr[][2] = {
> +	[INV_ICM42607_ODR_1_5625HZ_LP] = { 1, 562500 },
> +	[INV_ICM42607_ODR_3_125HZ_LP] = { 3, 125000 },
> +	[INV_ICM42607_ODR_6_25HZ_LP] = { 6, 250000 },
> +	[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 }
> +};

Does this array leave indices 0 through 4 zero-initialized?

Since inv_icm42607_accel_read_avail() exposes ARRAY_SIZE elements, this
regression could allow user-space to configure the hardware with a
zeroed/reserved ODR state.

[ ... ]
> +static int inv_icm42607_accel_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_accel_conf(st, &conf, NULL);
> +	if (ret)
> +		return ret;

If inv_icm42607_set_accel_conf() fails here, the software timestamping state
has already been updated to the new ODR.

Will this regression break software timestamp tracking for all future samples
because the hardware continues running at the old ODR?

[ ... ]
> +struct iio_dev *inv_icm42607_accel_init(struct inv_icm42607_state *st)
> +{
[ ... ]
> +	ret = devm_iio_device_register(dev, indio_dev);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	/* accel events are wakeup capable */
> +	ret = devm_device_init_wakeup(&indio_dev->dev);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	return indio_dev;
> +}

Could registering the IIO device before initializing its wakeup capabilities
cause a race condition?

Once devm_iio_device_register() succeeds, the device is visible to user-space,
so udev rules might attempt to access the power/wakeup sysfs attributes before
they are configured.

[ ... ]
> +int inv_icm42607_accel_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;

Is it possible for this to read past the end of st->fifo.data if a packet
header is near the end of the buffer?

If the remaining bytes are fewer than size, there is no boundary check against
st->fifo.count before the data is parsed and copied. Could this leak adjacent
kernel memory?

> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c
> index d8784ccf26728..eb629b86446b0 100644
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c
[ ... ]
> +int inv_icm42607_buffer_fifo_read(struct inv_icm42607_state *st,
> +				  unsigned int max)
> +{
> +	const void *accel, *gyro, *timestamp;
> +	size_t i, max_count, size;
[ ... ]
> +	/* compute maximum FIFO read size */
> +	if (max == 0)
> +		max_count = sizeof(st->fifo.data);
> +	else
> +		max_count = clamp(max_count,
> +				  (max * inv_icm42607_get_packet_size(st->fifo.en)),
> +				  sizeof(st->fifo.data));

Does this use an uninitialized value for max_count?

The clamp() macro evaluates its first parameter, which hasn't been initialized
here.

[ ... ]
> +	/* check and clamp FIFO count value */
> +	if (st->fifo.count == 0)
> +		return 0;
> +
> +	st->fifo.count = max(st->fifo.count, max_count);
> +
> +	/* read all FIFO data in internal buffer */
> +	ret = regmap_noinc_read(st->map, INV_ICM42607_REG_FIFO_DATA,
> +				st->fifo.data, st->fifo.count);

Should this use min() instead of max()?

If the hardware returns an erroneously large count, using max() could cause
st->fifo.count to exceed the size of st->fifo.data, leading to a buffer
overflow during regmap_noinc_read().

[ ... ]
> +int inv_icm42607_buffer_fifo_parse(struct inv_icm42607_state *st)
> +{
[ ... ]
> +	/* handle accelerometer timestamp and FIFO data parsing */
> +	if (st->fifo.nb.accel > 0) {
> +		ts = &accel_st->ts;
> +		inv_sensors_timestamp_interrupt(ts, st->fifo.watermark.eff_accel,
> +						st->timestamp.accel);

Can st->timestamp.accel be overwritten by a new hardirq while it is being read
here?

Since st->timestamp.accel is a 64-bit value updated in the hardirq handler
without locks, reading it in the threaded irq handler could cause a torn read
on 32-bit architectures, or associate the parsed FIFO data with the timestamp
of a newer interrupt.

[ ... ]
> +int inv_icm42607_buffer_hwfifo_flush(struct inv_icm42607_state *st,
> +				     unsigned int count)
> +{
> +	struct inv_icm42607_sensor_state *accel_st = iio_priv(st->indio_accel);
> +	struct inv_sensors_timestamp *ts;
> +	s64 gyro_ts, accel_ts;
> +	int ret;
> +
> +	gyro_ts = iio_get_time_ns(st->indio_gyro);

Will this crash if st->indio_gyro is NULL?

Since gyroscope support is not yet implemented in this patch, st->indio_gyro
remains uninitialized. Calling iio_get_time_ns() evaluates to_iio_dev_opaque(),
which could compute a negative offset from the NULL pointer and cause a kernel
oops.

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