Re: [PATCH 2/2] iio: imu: Add driver for the ADI ADIS16607

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] iio: imu: adis16607: out-of-bounds read when timestamp channel is enabled
- [Medium] iio: imu: adis16607: missing implementation for ADIS16607-1 variant
- [Medium] iio: imu: adis16607: incorrect sign for temperature channel
- [Medium] iio: imu: adis16607: interrupt storm in FIFO mode due to DR_GPIO configuration
--

commit 3e574b431c0ea32c1a98ab8b0aa4b4a2110aea1a
Author: Radu Sabau <radu.sabau@analog.com>

iio: imu: Add driver for the ADI ADIS16607

The ADIS16607 is a Precision MEMS IMU from Analog Devices featuring
triaxial gyroscope, triaxial accelerometer, delta angle, delta velocity,
and temperature channels. The device communicates over SPI Half-Duplex,
requiring custom read and write ops that differ from the standard ADIS
library protocol.

> diff --git a/drivers/iio/imu/adis16607.c b/drivers/iio/imu/adis16607.c
> new file mode 100644
> index 0000000000000..da9665aff8188
> --- /dev/null
> +++ b/drivers/iio/imu/adis16607.c
> @@ -0,0 +1,1513 @@
[ ... ]
> +static const u8 adis16607_burst_offsets[] = {
> +	[ADIS16607_SCAN_ACCEL_X] = 0,
> +	[ADIS16607_SCAN_ACCEL_Y] = 4,
> +	[ADIS16607_SCAN_ACCEL_Z] = 8,
> +	[ADIS16607_SCAN_GYRO_X] = 12,
> +	[ADIS16607_SCAN_GYRO_Y] = 16,
> +	[ADIS16607_SCAN_GYRO_Z] = 20,
> +	[ADIS16607_SCAN_DELTVEL_X] = 24,
> +	[ADIS16607_SCAN_DELTVEL_Y] = 28,
> +	[ADIS16607_SCAN_DELTVEL_Z] = 32,
> +	[ADIS16607_SCAN_DELTANG_X] = 36,
> +	[ADIS16607_SCAN_DELTANG_Y] = 40,
> +	[ADIS16607_SCAN_DELTANG_Z] = 44,
> +	[ADIS16607_SCAN_TEMP] = 48,
> +};

[ ... ]

> +static irqreturn_t adis16607_trigger_handler(int irq, void *p)
> +{
[ ... ]
> +	for_each_set_bit(bit, indio_dev->active_scan_mask, indio_dev->masklength) {
> +		u8 burst_off = adis16607_burst_offsets[bit];

[Severity: High]
Does this code perform an out-of-bounds read when the IIO timestamp channel
(scan_index 13) is enabled?

The adis16607_burst_offsets array only has 13 elements (indices 0 through 12).

[ ... ]

> +static void adis16607_fifo_push_sample(struct iio_dev *indio_dev,
> +				       u8 *fifo_data, s64 timestamp)
> +{
[ ... ]
> +	for_each_set_bit(bit, indio_dev->active_scan_mask, indio_dev->masklength) {
> +		u8 burst_off = adis16607_burst_offsets[bit];
> +
> +		if (bit == ADIS16607_SCAN_TEMP) {
> +			/* TEMP is 16-bit */
> +			put_unaligned_be16(get_unaligned_be16(&fifo_data[burst_off]),
> +					   &st->data[offset]);
> +			offset += 2;
> +		} else {
> +			/* All other channels are 24-bit stored as 32-bit */
> +			msw = get_unaligned_be16(&fifo_data[burst_off]);
> +			lsw = get_unaligned_be16(&fifo_data[burst_off + 2]);

[Severity: High]
Is it possible for this to leak kernel stack memory or cause a crash?

If the timestamp channel is enabled, the garbage burst_off value is used to
read out-of-bounds from the fifo_data array on the kernel stack.

[ ... ]

> +static irqreturn_t adis16607_trigger_handler_with_fifo(int irq, void *p)
> +{
[ ... ]
> +	/* Check if FIFO threshold is met */
> +	ret = __adis_read_reg_16(&st->adis, ADIS16607_REG_DIAG_STAT, &diag_stat);
> +	if (ret)
> +		goto unlock;
> +
> +	if (!(diag_stat & ADIS16607_FIFO_THR_MET_MASK))
> +		goto unlock;

[Severity: Medium]
Will the CPU be forced to process and discard an interrupt storm here?

[ ... ]

> +static int adis16607_initial_startup(struct adis16607 *st)
> +{
[ ... ]
> +	ret = __adis_write_reg_16(adis, ADIS16607_REG_USER_GPIO_CFG,
> +				  FIELD_PREP(ADIS16607_DR_GPIO_MASK, 1) |
> +				  (gpio ? FIELD_PREP(ADIS16607_RESET_GPIO_MASK, 1) : 0));

[Severity: Medium]
The interrupt pin appears hardcoded to fire on Data Ready (8000 times per
second), even when FIFO mode is enabled.

Does this defeat the performance benefits of the hardware FIFO?

[ ... ]

> +#define ADIS16607_TEMP_CHANNEL() { \
> +	.type = IIO_TEMP, \
> +	.indexed = 1, \
> +	.channel = 0, \
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> +			      BIT(IIO_CHAN_INFO_SCALE) | \
> +			      BIT(IIO_CHAN_INFO_OFFSET), \
> +	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> +	.address = ADIS16607_REG_TEMP, \
> +	.scan_index = ADIS16607_SCAN_TEMP, \
> +	.scan_type = { \
> +		.sign = 'u', \
> +		.realbits = 16, \
> +		.storagebits = 16, \
> +		.endianness = IIO_BE, \
> +	}, \
> +}

[Severity: Medium]
Can this sign type corrupt temperature calculations when the temperature is
below 25°C?

Since the sensor uses an offset of 5000 where 0 raw counts equals 25°C,
temperatures below 25°C will produce negative raw values. Using an unsigned
type will cause these negative values to underflow into large positive
numbers.

[ ... ]

> +static const struct spi_device_id adis16607_id[] = {
> +	{ "adis16607-2", (kernel_ulong_t)&adis16607_2_chip_info },
> +	{ "adis16607-3", (kernel_ulong_t)&adis16607_3_chip_info },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(spi, adis16607_id);
> +
> +static const struct of_device_id adis16607_of_match[] = {
> +	{ .compatible = "adi,adis16607-2", .data = &adis16607_2_chip_info },
> +	{ .compatible = "adi,adis16607-3", .data = &adis16607_3_chip_info },
> +	{ }
> +};

[Severity: Medium]
Is the ADIS16607-1 variant missing from the device ID tables?

The commit message indicates support for three variants including ADIS16607-1,
but it cannot be probed since it is missing here.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001-adis16607-v1-0-9c89cee35d56@analog.com?part=2