Re: [PATCH V5 06/11] iio: imu: inv_icm42607: Add Buffer support for icm42607

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [Critical] Uninitialized variable usage and kernel heap buffer overflow during FIFO read.
- [Critical] Uninitialized pointer dereference and infinite loop vulnerability due to integer type mismatch.
- [High] Hardware FIFO stream corruption due to oversized register read.
- [High] Logic inversion completely breaks watermark truncation.
- [High] Integer underflow on internal state variable permanently disables the device on error rollback.
- [High] Race condition due to missing lock in `hwfifo_flush` callback.
- [High] Data corruption due to missing bounds check on partial hardware FIFO packets.
- [Medium] Missing hardware delays during buffer postdisable.
- [Medium] Watermark interrupt is leaked/permanently disabled on regmap error path.
- [Medium] Ignored return value leads to silent failure during system resume.
--

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

iio: imu: inv_icm42607: Add Buffer support for icm42607

This commit adds FIFO parsing and reading functions to support buffer
transfers on the inv_icm42607 hardware.

diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c
--- /dev/null
+++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c
@@ -148,15 +148,12 @@
 
 [ ... ]
 
> +static unsigned int inv_icm42607_wm_truncate(unsigned int watermark,
> +					     size_t packet_size)
> +{
> +	size_t wm_size;
> +
> +	wm_size = watermark * packet_size;
> +	wm_size = max(wm_size, INV_ICM42607_FIFO_WATERMARK_MAX);

Is this max() intentional? It forces the watermark to always be greater
than or equal to the maximum hardware limit. Should this be min() instead?

> +
> +	return wm_size / packet_size;
> +}
> +
> +/**
> + * inv_icm42607_buffer_update_watermark - update watermark FIFO threshold

[ ... ]

> +	/* changing FIFO watermark requires to turn off watermark interrupt */
> +	ret = regmap_update_bits_check(st->map, INV_ICM42607_REG_INT_SOURCE0,
> +				       INV_ICM42607_INT_SOURCE0_FIFO_THS_INT1_EN,
> +				       0, &restore);
> +	if (ret)
> +		return ret;
> +
> +	/* compute watermark value in bytes */
> +	wm_size = watermark * packet_size;
> +	raw_wm = INV_ICM42607_FIFO_WATERMARK_VAL(wm_size);
> +	memcpy(st->buffer, &raw_wm, sizeof(raw_wm));
> +	ret = regmap_bulk_write(st->map, INV_ICM42607_REG_FIFO_CONFIG2,
> +				st->buffer, sizeof(raw_wm));
> +	if (ret)
> +		return ret;

If this regmap_bulk_write() fails, the function returns immediately without
restoring the interrupt. Could this leave the watermark interrupt permanently
disabled?

> +
> +	/* restore watermark interrupt */
> +	if (restore) {
> +		ret = regmap_update_bits(st->map, INV_ICM42607_REG_INT_SOURCE0,
> +					 INV_ICM42607_INT_SOURCE0_FIFO_THS_INT1_EN,
> +					 INV_ICM42607_INT_SOURCE0_FIFO_THS_INT1_EN);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}

[ ... ]

> +static int inv_icm42607_buffer_predisable(struct iio_dev *indio_dev)
> +{
> +	struct inv_icm42607_state *st = iio_device_get_drvdata(indio_dev);
> +	int ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	if (st->fifo.on > 1) {
> +		st->fifo.on--;
> +		return 0;
> +	}
> +
> +	/* set FIFO in bypass mode */
> +	ret = regmap_write(st->map, INV_ICM42607_REG_FIFO_CONFIG1,
> +			   INV_ICM42607_FIFO_CONFIG1_BYPASS);
> +	if (ret)
> +		return ret;
> +
> +	/* flush FIFO data */
> +	ret = regmap_write(st->map, INV_ICM42607_REG_SIGNAL_PATH_RESET,
> +			   INV_ICM42607_SIGNAL_PATH_RESET_FIFO_FLUSH);
> +	if (ret)
> +		return ret;
> +
> +	/* disable FIFO threshold interrupt */
> +	ret = regmap_update_bits(st->map, INV_ICM42607_REG_INT_SOURCE0,
> +				 INV_ICM42607_INT_SOURCE0_FIFO_THS_INT1_EN, 0);
> +	if (ret)
> +		return ret;
> +
> +	st->fifo.on--;

If inv_icm42607_buffer_postenable() fails early before incrementing
st->fifo.on, and the IIO core rolls back by calling this predisable
function, st->fifo.on would be 0 here. Since it is unsigned, could this
decrement cause it to underflow to UINT_MAX, leaving the device permanently
disabled for future calls?

> +
> +	return 0;
> +}
> +
> +static int inv_icm42607_buffer_postdisable(struct iio_dev *indio_dev)
> +{
> +	struct inv_icm42607_state *st = iio_device_get_drvdata(indio_dev);
> +	struct device *dev = regmap_get_device(st->map);
> +	unsigned int sensor;
> +	unsigned int *watermark;
> +	unsigned int sleep_temp = 0;
> +	unsigned int sleep_sensor = 0;
> +	unsigned int sleep;
> +	int ret;

[ ... ]

> +	*watermark = 0;
> +	ret = inv_icm42607_buffer_update_watermark(st);
> +	if (ret)
> +		goto out_unlock;
> +
> +out_unlock:
> +	mutex_unlock(&st->lock);
> +
> +	/* sleep maximum required time */
> +	sleep = max(sleep_sensor, sleep_temp);
> +	if (sleep)
> +		msleep(sleep);

Both sleep_temp and sleep_sensor are initialized to 0 and never modified
before this check. Does this mean the required hardware shutdown
power-transition delays are skipped completely?

[ ... ]

> +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;
> +	const s8 *temp;
> +	int ret;
> +
> +	/* reset all samples counters */
> +	st->fifo.count = 0;
> +	st->fifo.nb.gyro = 0;
> +	st->fifo.nb.accel = 0;
> +	st->fifo.nb.total = 0;
> +
> +	/* 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));

If max != 0, max_count is passed uninitialized to clamp(). Could this lead
to undefined behavior?

> +
> +	/* read FIFO count value */
> +	ret = regmap_bulk_read(st->map, INV_ICM42607_REG_FIFO_COUNTH,
> +			       st->buffer, sizeof(st->buffer));

Is it safe to use sizeof(st->buffer) here? st->buffer is declared as an
array of 3 __be16 elements (6 bytes). The hardware FIFO count register is 2
bytes, so a 6-byte read would spill over into the adjacent FIFO_DATA
registers, unintentionally popping 4 bytes of data and breaking alignment.

> +	if (ret)
> +		return ret;
> +	st->fifo.count = be16_to_cpup(st->buffer);
> +
> +	/* check and clamp FIFO count value */
> +	if (st->fifo.count == 0)
> +		return 0;
> +
> +	st->fifo.count = max(st->fifo.count, max_count);

Should this be min() instead of max()? Using max() forces the count to be
at least the potentially uninitialized max_count value. This could cause
regmap_noinc_read() to read past the hardware count and overflow the
st->fifo.data buffer.

> +
> +	/* read all FIFO data in internal buffer */
> +	ret = regmap_noinc_read(st->map, INV_ICM42607_REG_FIFO_DATA,
> +				st->fifo.data, st->fifo.count);
> +	if (ret)
> +		return ret;
> +
> +	/* compute number of samples for each sensor */
> +	for (i = 0; i < st->fifo.count; i += size) {
> +		size = inv_icm42607_fifo_decode_packet(&st->fifo.data[i],
> +				&accel, &gyro, &temp, &timestamp);
> +		if (size <= 0)
> +			break;

inv_icm42607_fifo_decode_packet() returns a ssize_t which can be -EINVAL.
Since size is of type size_t, the negative error code becomes a huge
unsigned integer. The size <= 0 check will evaluate to false, skipping
error handling.

Since decode_packet() does not initialize accel and gyro on error, could
this result in dereferencing uninitialized pointers below?

Also, if the hardware returns a byte count that ends with a partial packet,
could this loop access memory beyond what was populated from the hardware
since there is no check for i + size > st->fifo.count?

> +		if (gyro != NULL && inv_icm42607_fifo_is_data_valid(gyro))
> +			st->fifo.nb.gyro++;
> +		if (accel != NULL && inv_icm42607_fifo_is_data_valid(accel))
> +			st->fifo.nb.accel++;
> +		st->fifo.nb.total++;
> +	}
> +
> +	return 0;
> +}
> +
> +int inv_icm42607_buffer_hwfifo_flush(struct inv_icm42607_state *st,
> +				     unsigned int count)
> +{
> +	s64 gyro_ts, accel_ts;
> +	int ret;
> +
> +	gyro_ts = iio_get_time_ns(st->indio_gyro);
> +	accel_ts = iio_get_time_ns(st->indio_accel);

These timestamps appear to be unused. Should they be passed to the buffer
logic or are they just dead code?

> +
> +	ret = inv_icm42607_buffer_fifo_read(st, count);

Does this need to be protected by mutex_lock(&st->lock)? Since this
callback can be triggered via sysfs concurrently with the normal interrupt
handler, it seems possible to have interleaved regmap reads or corrupted
internal FIFO sample counters.

> +
> +	return ret;
> +}

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
@@ -404,8 +404,11 @@
 	if (ret)
 		return ret;
 
-	/* Nothing else to restore at this time. */
+	if (st->fifo.on) {
+		ret = regmap_write(st->map, INV_ICM42607_REG_FIFO_CONFIG1,
+				   INV_ICM42607_FIFO_CONFIG1_MODE);
+	}
 
 	return 0;
 }

Is the return value from regmap_write() meant to be checked? Currently, ret
is assigned but not used, so the function unconditionally returns 0 even if
the write fails.

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