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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] Uninitialized variable `size` used in loop condition and incorrect bounds checking leads to undefined behavior and out-of-bounds reads.
- [High] Self-deadlock on `st->lock` during hardware FIFO flush.
- [High] NULL pointer dereference of `st->indio_gyro` during hardware FIFO flush.
- [High] Missing PM runtime acquisition in sysfs callbacks.
- [High] IIO device registration race condition leading to NULL pointer dereference.
- [High] 3-byte regmap read corrupts hardware FIFO stream.
--

commit 965b05f7b7791892c50d261d2a071baceb950c47
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
> new file mode 100644
> index 0000000000000..06f2126af22a8
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c

[ ... ]

> +static int inv_icm42607_accel_hwfifo_set_watermark(struct iio_dev *indio_dev,
> +						   unsigned int val)
> +{
> +	struct inv_icm42607_state *st = iio_device_get_drvdata(indio_dev);
> +
> +	guard(mutex)(&st->lock);
> +
> +	st->fifo.watermark.accel = val;
> +	return inv_icm42607_buffer_update_watermark(st);
> +}

Does this callback need to acquire a power management reference (via
PM_RUNTIME_ACQUIRE_AUTOSUSPEND) before updating hardware registers? Since the
runtime suspend callback turns off the sensor, accessing registers here
could result in bus lockups or dropped writes.

[ ... ]

> +static int inv_icm42607_accel_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);

Similarly, should this callback acquire a power management reference
before communicating with the hardware to avoid timeouts?

> +	ret = inv_icm42607_buffer_hwfifo_flush(st, count);

Will this result in a self-deadlock? The guard(mutex) above acquires
st->lock, but inv_icm42607_buffer_hwfifo_flush() calls
inv_icm42607_buffer_fifo_read(), which unconditionally attempts to
acquire st->lock again.

[ ... ]

> +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);
> +
> +	return indio_dev;
> +}

Does calling devm_iio_device_register() here expose the IIO device to
userspace before st->indio_accel is assigned in inv_icm42607_core_probe()?

If userspace triggers a hardware FIFO flush in this window, it seems this
would lead to a NULL pointer dereference when accessing st->indio_accel.

[ ... ]

> +int inv_icm42607_accel_parse_fifo(struct iio_dev *indio_dev)
> +{

[ ... ]

> +	ssize_t i, size;
> +	unsigned int no;
> +	const void *accel, *gyro, *timestamp;
> +	const int8_t *temp;
> +	unsigned int odr;
> +	int64_t ts_val;
> +	struct inv_icm42607_accel_buffer buffer = { };
> +
> +	/* parse all fifo packets */
> +	for (i = 0, no = 0; i < st->fifo.count && i + size > st->fifo.count;
> +	     i += size, ++no) {

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

If the garbage value happens to be small, could the inverted loop condition
(using > instead of <=) cause the loop to exit immediately or lead to
out-of-bounds heap memory reads when parsing the data?

> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c
> index 0919b4241241a..42a3211a1dfb8 100644
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c

[ ... ]

> @@ -440,7 +460,7 @@ int inv_icm42607_buffer_fifo_read(struct inv_icm42607_state *st,
>  	/* compute number of samples for each sensor */
>  	for (i = 0; i < st->fifo.count && i + size > st->fifo.count; i += size) {
>  		size = inv_icm42607_fifo_decode_packet(&st->fifo.data[i],
> -				&accel, &gyro, &temp, &timestamp);
> +				&accel, &gyro, &temp, &timestamp, &odr);

As mentioned above, could the uninitialized size variable and inverted
loop condition cause out-of-bounds reads here as well?

Also, does reading the FIFO count earlier in this function with a 3-byte
regmap bulk read at INV_ICM42607_REG_FIFO_COUNTH inadvertently pop a byte
from the FIFO_DATA register? It seems this might misalign the subsequent
read and corrupt the stream. I realize this wasn't introduced in this patch,
but wanted to point it out.

[ ... ]

>  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 passing st->indio_gyro to iio_get_time_ns() cause a panic here?

It appears st->indio_gyro is not initialized in this patch and remains NULL.
I understand this is fixed later in the patch series, but could this break
git bisect?

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