Re: [PATCH v4 07/13] iio: adc: ad4134: Support buffered data read

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] iio: data race on rx_buf between trigger handler and regmap debugfs
- [Medium] iio: stack memory leak via regmap due to ARRAY_SIZE(ad4134_chan_set) increase
- [Medium] iio: adc: ad4134: DMA buffers rx_buf and tx_buf share the same cacheline
--

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

iio: adc: ad4134: Support buffered data read

This commit enables buffered data captures triggered by the IIO trigger
device and adds an IIO timestamp channel. It also requires single-read
operations to be in IIO device direct access mode to prevent buffered and
single-shot captures from disrupting each other.

> diff --git a/drivers/iio/adc/ad4134.c b/drivers/iio/adc/ad4134.c
> index 3048e61a3cb13..b5207f16054e4 100644
> --- a/drivers/iio/adc/ad4134.c
> +++ b/drivers/iio/adc/ad4134.c
> [ ... ]
> @@ -131,6 +141,7 @@ static const struct iio_chan_spec ad4134_chan_set[] = {
>  	AD4134_CHANNEL(1),
>  	AD4134_CHANNEL(2),
>  	AD4134_CHANNEL(3),
> +	IIO_CHAN_SOFT_TIMESTAMP(4),
>  };
>  
>  struct ad4134_state {
> @@ -149,7 +160,7 @@ 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);
> +	u8 rx_buf[AD4134_SPI_MAX_XFER_LEN * AD4134_NUM_CHANNELS] __aligned(IIO_DMA_MINALIGN);
>  	u8 tx_buf[AD4134_SPI_MAX_XFER_LEN];
>  };

[Severity: Medium]
Do tx_buf and rx_buf end up sharing the same cacheline here?

Because rx_buf is only 12 bytes long (assuming AD4134_SPI_MAX_XFER_LEN is 3
and AD4134_NUM_CHANNELS is 4), tx_buf will be placed immediately after it at
offset 12. Since tx_buf lacks __aligned(IIO_DMA_MINALIGN), they will occupy
the same cacheline.

If the CPU writes to tx_buf during a debugfs operation while the SPI core is
concurrently mapping rx_buf for a triggered read, could the DMA FROM_DEVICE
cache invalidation discard the CPU's writes to tx_buf and corrupt memory?

> [ ... ]
> @@ -269,7 +280,7 @@ 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++) {
> +	for (i = 0; i < AD4134_NUM_CHANNELS; i++) {
>  		ret = spi_write_then_read(st->spi, NULL, 0, st->rx_buf,
>  					  BITS_TO_BYTES(AD4134_CHAN_PRECISION_BITS));

[Severity: Medium]
Could this leak uninitialized stack memory to userspace if accessed via the
regmap debugfs interface?

The patch adds IIO_CHAN_SOFT_TIMESTAMP(4) to ad4134_chan_set, which increases
its ARRAY_SIZE from 4 to 5. The max_register calculation for the regmap
configuration uses ARRAY_SIZE(ad4134_chan_set) - 1.

If userspace reads this new max register (0x54) via the regmap debugfs
interface, it will call ad4134_data_read(). Inside this loop, it only
iterates up to AD4134_NUM_CHANNELS (which is 4). 

Since AD4134_VREG_CH(reg) would evaluate to 4, the condition
if (i == AD4134_VREG_CH(reg)) never matches, leaving the stack variable
'sample' uninitialized before it is sign-extended and returned.

> [ ... ]
> @@ -333,6 +344,44 @@ static const struct regmap_config ad4134_regmap_config = {
>  	.max_register = AD4134_CH_VREG(ARRAY_SIZE(ad4134_chan_set) - 1),
>  };
>  
> +static irqreturn_t ad4134_trigger_handler(int irq, void *p)
> +{
> +	IIO_DECLARE_BUFFER_WITH_TS(u32, channels, AD4134_NUM_CHANNELS) = { };
> +	unsigned int chan_bytes = BITS_TO_BYTES(AD4134_CHAN_PRECISION_BITS);
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct ad4134_state *st = iio_priv(indio_dev);
> +	struct spi_transfer xfer = {
> +		.rx_buf = st->rx_buf,
> +		.cs_off = true,
> +		.len = chan_bytes * AD4134_NUM_CHANNELS,
> +	};
> +	int ret;
> +
> +	gpiod_set_value_cansleep(st->odr_gpio, 1);
> +	fsleep(1);
> +	gpiod_set_value_cansleep(st->odr_gpio, 0);
> +
> +	/* Run enough DCLK cycles to match the data frame length. */
> +	ret = spi_sync_transfer(st->spi, &xfer, 1);

[Severity: High]
Does this introduce a data race on st->rx_buf?

The trigger handler runs asynchronously and uses st->rx_buf directly in
spi_sync_transfer() without taking any locks.

Meanwhile, if userspace reads from the regmap debugfs node, the regmap core
takes its internal lock and calls ad4134_data_read(), which invokes
spi_write_then_read() using the exact same st->rx_buf.

Because regmap debugfs access does not claim IIO direct mode, and the trigger
handler bypasses the regmap lock, could both paths execute concurrently
and cause concurrent DMA mapping of the same buffer?

> [ ... ]

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