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

[Jonathan Cameron]

> 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.
> 
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@xxxxxxxxxx>

Hi Marcelo

Sashiko had a bunch of feedback on this one.  Note that you can save
time if you check out what it comes up with and reply to the thread
to say what is valid and not.

I haven't yet asked for those emails to go directly to linux-iio
as the quality is mixed and I am reluctant to confuse newcomers, but
most like I will ask them to turn those emails on in a month or so
if not before. In meantime more experienced folk can look at the
website directly.

> diff --git a/drivers/iio/adc/ad4134.c b/drivers/iio/adc/ad4134.c
> index 30c7b1d1bc67..0b6843bf8a9e 100644

>  

>  
>  struct ad4134_state {
> @@ -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)];


Sashiko:
[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.
-

That does indeed look wrong. I'd expect to see one of the DECLARE
buffer macros used here to ensure a large enough buffer.  This
should have shown up in testing so I'm curious why it didn't.

[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.

-
Also sounds valid though I haven't confirmed everything stated.
A buffer like this should be protected by a local lock, not rely on
serialization via the mode claim mechanism.

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


> @@ -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.
-
Please check this one.
>  



> @@ -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.
-

Check this one as well. Looks plausible to me.

-- 
Jonathan Cameron <jonathan.cameron@xxxxxxxxxxxxxxxx>