Re: [PATCH 2/2] iio: dac: add AD5413 driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] iio: dac: ad5413: missing range check in ad5413_write_raw()
- [Low] iio: dac: ad5413: unused supplies array in struct ad5413_state
--

commit bcfddef4b6cda1b4c239a6094a38aa835c2860bd
Author: Bruce Tsao <bruce.tsao@analog.com>

iio: dac: add AD5413 driver

This commit adds an IIO driver for the Analog Devices AD5413 single-channel
14-bit voltage and current output DAC over SPI.

> diff --git a/drivers/iio/dac/ad5413.c b/drivers/iio/dac/ad5413.c
> new file mode 100644
> index 0000000000000..96cce32fc640e
> --- /dev/null
> +++ b/drivers/iio/dac/ad5413.c
> @@ -0,0 +1,666 @@

[ ... ]

> +struct ad5413_state {
> +	struct spi_device *spi;
> +	struct regulator_bulk_data supplies[3];

[Severity: Low]
Is supplies[3] in struct ad5413_state needed? ad5413_probe() enables the
regulators using devm_regulator_bulk_get_enable() with ad5413_supply_names, so
this array appears to be unused.

> +	struct mutex lock; /* Protects all register access via SPI */
> +	const struct ad5413_range *out_range;
> +	unsigned int slew_time;
> +	bool pwr_down;
> +	u8 crc8_tbl[CRC8_TABLE_SIZE];
> +	__be32 d32[3] __aligned(IIO_DMA_MINALIGN);
> +};

[ ... ]

> +static int ad5413_spi_reg_write(struct ad5413_state *st, unsigned int addr,
> +				unsigned int val)
> +{
> +	u8 buf[3] = { };
> +
> +	buf[0] = AD5413_WR_FLAG_MSK(addr);
> +	buf[1] = val >> 8;
> +	buf[2] = val;
> +
> +	st->d32[0] = cpu_to_be32((buf[0] << 24) | (buf[1] << 16) |
> +				 (buf[2] << 8) |
> +				 crc8(st->crc8_tbl, buf, sizeof(buf), 0));
> +
> +	return spi_write(st->spi, &st->d32[0], sizeof(st->d32[0]));
> +}

[ ... ]

> +static int ad5413_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan, int val, int val2,
> +			    long info)
> +{
> +	struct ad5413_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_RAW: {
> +		guard(mutex)(&st->lock);
> +		/* 14-bit DAC data is in bits [15:2] of the register. */
> +		ret = ad5413_spi_reg_write(st, AD5413_REG_DAC_INPUT, val << 2);
> +		return ret;
> +	}

[Severity: Medium]
Should ad5413_write_raw() validate that val is within the 14-bit range
(0 <= val < BIT(14)) before shifting and writing to AD5413_REG_DAC_INPUT?

When userspace writes a negative value or a value >= BIT(14) to the raw sysfs
attribute, evaluating val << 2 on a negative signed int or a large positive
int triggers signed left-shift undefined behavior, and ad5413_spi_reg_write()
silently truncates the shifted value into the lower 16 bits of buf[1] and
buf[2], programming an unintended DAC output instead of returning -EINVAL.

> +	default:
> +		return -EINVAL;
> +	}
> +}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261017-ad5413-upstream-v1-0-61f40c8fef25@analog.com?part=2