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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Hardware FIFO stream corruption via inadvertent read of FIFO_DATA register
- [High] Uninitialized variable and inverted bounds check in FIFO parsing loops
--

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

    iio: imu: inv_icm42607: Add Buffer support for icm42607

This commit adds buffer support for the inv_icm42607 hardware by implementing
FIFO parsing and reading functions.

> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c
> new file mode 100644
> index 0000000000000..0919b4241241a
> --- /dev/null
> +++ 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;
> +	const s8 *temp;
> +	ssize_t size;
> +	int ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	/* 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 = min((max * inv_icm42607_get_packet_size(st->fifo.en)),
> +				sizeof(st->fifo.data));
> +
> +	/* read FIFO count value */
> +	ret = regmap_bulk_read(st->map, INV_ICM42607_REG_FIFO_COUNTH,
> +			       st->buffer, sizeof(u8) * 3);

Does reading 3 bytes here fetch the COUNTH, COUNTL, and FIFO_DATA registers?

If so, could this inadvertent read of the FIFO_DATA register automatically
dequeue 1 byte from the hardware FIFO?

This could cause the subsequent regmap_noinc_read() to fetch misaligned
packet headers, corrupting the decoded sensor stream. Should this be a
2-byte read instead?

> +	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 = min(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);
> +	if (ret)
> +		return ret;
> +
> +	/* compute number of samples for each sensor */
> +	for (i = 0; i < st->fifo.count && i + size > st->fifo.count; i += size) {

Is the size variable uninitialized on the first iteration of this loop?

Also, is the i + size > st->fifo.count condition logically inverted? This
appears to require the packet to overflow the buffer boundaries to continue,
which could cause the loop to terminate prematurely or not run at all.

Could this inverted logic allow a partial packet to bypass boundaries and
cause an out-of-bounds read inside inv_icm42607_fifo_is_data_valid() later
in the loop?

> +		size = inv_icm42607_fifo_decode_packet(&st->fifo.data[i],
> +				&accel, &gyro, &temp, &timestamp);
> +		if (size <= 0)
> +			break;
> +		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;
> +}

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