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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Reversed bounds check in FIFO parser causes valid packets to be dropped and out-of-bounds reads
- [High] Unprotected lockless access to shared FIFO state leads to data races and corruption
- [High] Missing packet bounds check in `inv_icm42607_buffer_fifo_read` allows out-of-bounds memory access
- [High] Unsupported TEMP-only scan mask causes silent data drops and userspace deadlock
- [Medium] Reserved array elements exposed as zeroed sampling frequencies in userspace
--

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

iio: imu: inv_icm42607: Add Accelerometer for icm42607
    
This commit adds accelerometer sensor support to the icm42607 driver. It
introduces device configuration, power management, timestamp handling, and data
parsing from the hardware FIFO. It also exposes available sampling frequencies
and scales to userspace.

> 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..b354a84fd2ed4
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c
[ ... ]
> +static const unsigned long inv_icm42607_accel_scan_masks[] = {
> +	INV_ICM42607_SCAN_MASK_ACCEL_3AXIS,
> +	INV_ICM42607_SCAN_MASK_TEMP,
> +	INV_ICM42607_SCAN_MASK_ACCEL_3AXIS | INV_ICM42607_SCAN_MASK_TEMP,
> +	0
> +};

Does offering INV_ICM42607_SCAN_MASK_TEMP by itself cause issues? If userspace 
requests only the temperature channel, inv_icm42607_accel_update_scan_mode() 
enables only the TEMP sensor. If the hardware generates packets without ACCEL 
or GYRO headers, inv_icm42607_fifo_decode_packet() returns -EINVAL. 

Even if it generates an ACCEL packet, inv_icm42607_accel_parse_fifo() explicitly
discards packets lacking accelerometer data. Would this cause the TEMP data to be
silently dropped, potentially leaving userspace applications deadlocked waiting
for data?

[ ... ]
> +static int inv_icm42607_accel_read_avail(struct iio_dev *indio_dev,
> +					 struct iio_chan_spec const *chan,
> +					 const int **vals,
> +					 int *type, int *length, long mask)
> +{
> +	if (chan->type != IIO_ACCEL)
> +		return -EINVAL;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		*vals = (const int *)inv_icm42607_accel_scale_nano;
> +		*type = IIO_VAL_INT_PLUS_NANO;
> +		*length = ARRAY_SIZE(inv_icm42607_accel_scale_nano) * 2;
> +		return IIO_AVAIL_LIST;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		*vals = (const int *)inv_icm42607_accel_odr;
> +		*type = IIO_VAL_INT_PLUS_MICRO;
> +		*length = ARRAY_SIZE(inv_icm42607_accel_odr) * 2;
> +		return IIO_AVAIL_LIST;

Can this expose uninitialized array elements to userspace? Because 
inv_icm42607_accel_odr has indices 0-5 reserved and implicitly initialized to 
zero, passing the full ARRAY_SIZE() could expose those 0.000000 sampling 
frequencies as available to userspace.

[ ... ]
> +	/* 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;
> +
> +		/* If the packet size could cause us to overflow, return. */
> +		if (i + size <= st->fifo.count)
> +			return -EIO;

Is this condition reversed? If i + size <= st->fifo.count, the packet fits 
within the valid buffer limit, but the parser returns -EIO and valid packets 
are dropped. Conversely, if the packet exceeds the buffer boundaries 
(i + size > st->fifo.count), it evaluates to false, allowing the parser to 
read out-of-bounds kernel memory.

> +
> +		/* skip packet if no accel data or data is invalid */
> +		if (accel == NULL || !inv_icm42607_fifo_is_data_valid(accel))
> +			continue;
[ ... ]
> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c
> index 74e5213d92671..4371f428cc3f8 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) {
>  		size = inv_icm42607_fifo_decode_packet(&st->fifo.data[i],
> -				&accel, &gyro, &temp, &timestamp);
> +				&accel, &gyro, &temp, &timestamp, &odr);
>  		/* Make sure the size is at least 1 valid packet. */
>  		if (size < INV_ICM42607_FIFO_1SENSOR_PACKET_SIZE)
>  			break;

Could this code access memory out-of-bounds? If the hardware returns a 
partial packet at the very end of the st->fifo.data array, the loop does not 
verify that i + size <= st->fifo.count before passing the decoded pointers. 
inv_icm42607_fifo_decode_packet() could then yield pointers that point 
outside the populated bounds, which inv_icm42607_fifo_is_data_valid() would 
subsequently dereference.

[ ... ]
>  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 accel_ts;
>  	int ret;
>  
> +	accel_ts = iio_get_time_ns(st->indio_accel);
> +
>  	ret = inv_icm42607_buffer_fifo_read(st, count);
> +	if (ret)
> +		return ret;
>  
> -	return ret;
> +	if (st->fifo.nb.total == 0)
> +		return 0;
> +
> +	if (st->fifo.nb.accel > 0) {
> +		ts = &accel_st->ts;
> +		inv_sensors_timestamp_interrupt(ts, st->fifo.nb.accel, accel_ts);
> +		ret = inv_icm42607_accel_parse_fifo(st->indio_accel);

Can a data race happen here? inv_icm42607_buffer_fifo_read() properly 
acquires and releases st->lock, but then inv_icm42607_accel_parse_fifo() is 
called without holding the lock. A concurrent flush operation could acquire 
the lock, overwrite st->fifo.data, and reset st->fifo.count while the 
original thread is mid-parse, leading to memory corruption and out-of-bounds 
reads.

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