Re: [PATCH v4 5/8] iio: imu: Add driver for the ADI ADIS16607

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] iio: imu: adis16607: negative frequency wraparound in adis16607_write_raw()
--

commit 686b8ffae8bb8850532fb9fd2ba72d9f11184ab1
Author: Radu Sabau <radu.sabau@analog.com>

iio: imu: Add driver for the ADI ADIS16607

This commit adds the base IIO driver for the Analog Devices ADIS16607-2
and ADIS16607-3 precision MEMS IMUs over a custom SPI Half-Duplex
protocol. It provides support for raw channel reads, sampling frequency
control, startup/self-test via adislib, and debugfs identity entries.

> diff --git a/drivers/iio/imu/adis16607.c b/drivers/iio/imu/adis16607.c
> new file mode 100644
> index 0000000000000..2dda306d7b547
> --- /dev/null
> +++ b/drivers/iio/imu/adis16607.c
[ ... ]
> +static int adis16607_set_freq(struct adis16607 *st, u32 freq)
> +{
> +	u32 dec;
> +	int ret;
> +
> +	if (!freq)
> +		return -EINVAL;
> +
> +	adis_dev_auto_lock(&st->adis);
> +
> +	dec = DIV_ROUND_CLOSEST(st->clk_freq_mhz, freq);
> +	if (dec)
> +		dec--;
> +
> +	dec = min_t(u32, dec, st->info->max_dec);
> +
> +	ret = __adis_write_reg_16(&st->adis, ADIS16607_REG_DEC_RATE, dec);
> +	if (ret)
> +		return ret;
> +
> +	st->sample_period_ns = DIV_ROUND_CLOSEST_ULL((u64)NSEC_PER_SEC * 1000ULL * (dec + 1),
> +						     st->clk_freq_mhz);
> +
> +	return 0;
> +}
[ ... ]
> +static int adis16607_write_raw(struct iio_dev *indio_dev,
> +			       const struct iio_chan_spec *chan,
> +			       int val, int val2, long info)
> +{
> +	struct adis16607 *st = iio_priv(indio_dev);
> +	u32 tmp;
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		tmp = val * 1000 + val2 / 1000;
> +		return adis16607_set_freq(st, tmp);

[Severity: Medium]
What happens in adis16607_write_raw() if userspace writes a negative
sampling frequency via sysfs?

In drivers/iio/industrialio-core.c, iio_write_channel_info() parses the
input string through __iio_str_to_fixpoint(), which accepts a leading '-'
sign and passes signed negative val or val2 values into write_raw().

Without checking if (val < 0 || val2 < 0) return -EINVAL; (like
adis16480_set_freq() does in drivers/iio/imu/adis16480.c), computing
val * 1000 + val2 / 1000 and assigning a negative result to u32 tmp wraps
around to a large unsigned value.

When that wrapped value is passed to adis16607_set_freq(), it bypasses the
if (!freq) check, DIV_ROUND_CLOSEST(st->clk_freq_mhz, freq) evaluates to 0,
and __adis_write_reg_16() programs ADIS16607_REG_DEC_RATE to 0 (the maximum
8 kHz rate) instead of rejecting the invalid input. Also, if val >
INT_MAX / 1000, val * 1000 overflows signed 32-bit arithmetic.

Should adis16607_write_raw() validate that val and val2 are non-negative
and within bounds before computing tmp?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023-adis16607-v4-0-a4d541ebfd13@analog.com?part=5