Re: [PATCH v2 08/15] iio: adc: ad4134: Support buffered data read

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] iio: adc: ad4134: buffer compaction overwrites active channels due to skipped index increment
- [High] iio: adc: ad4134: undersized scan buffer causes -ENOSPC during triggered captures
- [High] iio: adc: ad4134: data corruption due to concurrent lockless access to shared rx_buf union
- [Medium] iio: adc: ad4134: uninitialized stack memory leak via out-of-bounds virtual register read
--

commit 67ed10376d89a2413bb4cf43e7a24f52e73e8a76
Author: Marcelo Schmitt <marcelo.schmitt@analog.com>

iio: adc: ad4134: Support buffered data read

Enable users to run buffered data captures triggered by IIO trigger device.
Add an IIO timestamp channel so each data scan is provided with measurement
time information. Require single-read operations to be in IIO device direct
access mode to prevent buffered and single-shot data captures to disrupt
each other.

> diff --git a/drivers/iio/adc/ad4134.c b/drivers/iio/adc/ad4134.c
> index 30c7b1d1bc67c..0b6843bf8a9e3 100644
> --- a/drivers/iio/adc/ad4134.c
> +++ b/drivers/iio/adc/ad4134.c

[ ... ]

> @@ -149,7 +161,10 @@ struct ad4134_state {
>  	 * DMA (thus cache coherency maintenance) requires the transfer buffers
>  	 * to live in their own cache lines.
>  	 */
> -	u8 rx_buf[AD4134_SPI_MAX_XFER_LEN] __aligned(IIO_DMA_MINALIGN);
> +	union {
> +		u8 reg[AD4134_SPI_MAX_XFER_LEN];
> +		u32 scan[ARRAY_SIZE(ad4134_chan_set)];

[Severity: High]
Does this array size fail to account for the timestamp's alignment padding?

The array is fixed at 20 bytes (5 elements). When all 4 channels and the 64-bit
timestamp are active, the IIO core computes scan_bytes as 24 bytes due to the
timestamp's 8-byte alignment requirement (16 bytes data + 8 bytes padding and
timestamp). Passing sizeof(st->rx_buf.scan) to iio_push_to_buffers_with_ts()
later in the trigger handler will cause the IIO core's safety check to fail,
aborting the buffer push with -ENOSPC on full-scan captures.

[Severity: High]
Can concurrent lockless accesses corrupt the rx_buf shared union?

The ad4134_trigger_handler() executes locklessly in a pollfunc context, while
regmap debugfs reads/writes (which do not acquire the IIO direct mode lock) can
concurrently use st->rx_buf.reg as a DMA buffer. This can result in corrupted
SPI scan data, corrupted regmap read/write data, and potentially invalid
hardware configurations or ADC samples if debugfs is accessed while a triggered
buffer capture is running.

> +	} rx_buf __aligned(IIO_DMA_MINALIGN);
>  	u8 tx_buf[AD4134_SPI_MAX_XFER_LEN];
>  };

[ ... ]

> @@ -269,8 +284,8 @@ static int ad4134_data_read(struct ad4134_state *st, unsigned int reg,
>  	 * interface. Now we read data from all channels but keep only the bits
>  	 * from the requested one.
>  	 */
> -	for (i = 0; i < ARRAY_SIZE(ad4134_chan_set); i++) {
> -		ret = spi_write_then_read(st->spi, NULL, 0, st->rx_buf,
> +	for (i = 0; i < AD4134_NUM_CHANNELS; i++) {
> +		ret = spi_write_then_read(st->spi, NULL, 0, st->rx_buf.reg,
>  					  BITS_TO_BYTES(AD4134_CHAN_PRECISION_BITS));
>  		if (ret)
>  			return ret;
> @@ -281,7 +296,7 @@ static int ad4134_data_read(struct ad4134_state *st, unsigned int reg,
>  		 * Clock out data from all channels to avoid that.
>  		 */
>  		if (i == AD4134_VREG_CH(reg))
> -			sample = get_unaligned_be24(st->rx_buf);
> +			sample = get_unaligned_be24(st->rx_buf.reg);
>  	}
>  	*val = sign_extend32(sample, AD4134_CHAN_PRECISION_BITS - 1);

[Severity: Medium]
Does this leak uninitialized stack memory if the soft timestamp channel is read
as a virtual register?

Because ad4134_regmap_config sets max_register to include the soft timestamp
channel (ARRAY_SIZE(ad4134_chan_set) includes the timestamp, making index 4
valid), userspace could read virtual register index 4 via regmap debugfs. In
that case, this loop iterates only up to AD4134_NUM_CHANNELS (indices 0 to 3),
meaning `i == AD4134_VREG_CH(reg)` (where AD4134_VREG_CH(reg) would be 4) is
never true. The sample variable is left uninitialized, and an uninitialized
32-bit stack chunk is processed and returned to userspace.

[ ... ]

> @@ -333,6 +348,37 @@ static const struct regmap_config ad4134_regmap_config = {
>  	.max_register = AD4134_CH_VREG(ARRAY_SIZE(ad4134_chan_set)),
>  };
>  
> +static irqreturn_t ad4134_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct ad4134_state *st = iio_priv(indio_dev);
> +	unsigned int i = 0;
> +	int ret;
> +
> +	gpiod_set_value_cansleep(st->odr_gpio, 1);
> +	fsleep(1);
> +	gpiod_set_value_cansleep(st->odr_gpio, 0);
> +
> +	for (unsigned int ch = 0; ch < AD4134_NUM_CHANNELS; ch++) {
> +		ret = spi_write_then_read(st->spi, NULL, 0, &st->rx_buf.scan[ch],
> +					  BITS_TO_BYTES(AD4134_CHAN_PRECISION_BITS));
> +		if (ret)
> +			goto err_out;
> +
> +		if (test_bit(ch, indio_dev->active_scan_mask) && ch != i)
> +			memcpy(&st->rx_buf.scan[i++], &st->rx_buf.scan[ch],

[Severity: High]
Does this conditional logic permanently corrupt the scan data by overwriting
active channels?

When the first active channel has the same index as the destination (for example,
both are 0), the condition `ch != i` evaluates to false, short-circuiting the
if statement and entirely skipping the post-increment `i++`. The next active
channel will then overwrite the previous data by being written to scan[0] again.
This triggers on any standard buffered capture involving multiple channels.

> +			       sizeof(st->rx_buf.scan[ch]));
> +	}
> +
> +	iio_push_to_buffers_with_ts(indio_dev, &st->rx_buf.scan,
> +				    sizeof(st->rx_buf.scan), pf->timestamp);
> +
> +err_out:
> +	iio_trigger_notify_done(indio_dev->trig);
> +	return IRQ_HANDLED;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1789494473.git.marcelo.schmitt@analog.com?part=8