Re: [PATCH] iio: adc: ad_sigma_delta: fix use-after-free on unbind

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 24 Sep 2026 09:45:07 +0100
Nuno Sá <nuno.sa@analog.com> wrote:

> On Wed, Sep 23, 2026 at 09:48:07AM +0000, Fan Wu wrote:
> > ad_sd_buffer_postenable() allocates sigma_delta->samples_buf with
> > devm_krealloc() at runtime, so its devres entry sits after all
> > probe-time entries of the driver. devm resources are released in
> > reverse allocation order, which means unbind frees samples_buf before
> > iio_device_unregister() disables the buffers and detaches the trigger
> > pollfunc. The data ready IRQ is still enabled at that point, so
> > ad_sd_trigger_handler() can still run and memcpy() incoming samples
> > into the freed samples_buf.
> > 
> > Fix this by preallocating the buffer in
> > devm_ad_sd_setup_buffer_and_trigger(), before the triggered buffer and
> > the IRQ are set up, so it is freed only after iio_device_unregister()
> > has drained the trigger handler via free_irq(). Size it for the worst
> > case of all sequencer slots being active; ad_sd_validate_scan_mask()
> > already caps the number of active channels at num_slots.
> > 
> > This issue was found by an in-house static analysis tool.
> > 
> > Fixes: 8bea9af887de ("iio: adc: ad_sigma_delta: Add sequencer support")
> > Cc: stable@vger.kernel.org
> > Co-developed-by: Song Li <songl@zju.edu.cn>
> > Signed-off-by: Song Li <songl@zju.edu.cn>
> > Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
> > ---  
> 
> Makes sense to me! One minor nit Jonathan might be ale to tweak when
> applying. With that:
Applied to the fixes-togreg branch of iio.git and tweaked as suggested.

Thanks,

Jonathan


> 
> Reviewed-by: Nuno Sá <nuno.sa@analog.com>
> 
> >  drivers/iio/adc/ad_sigma_delta.c | 32 +++++++++++++++++++-------------
> >  1 file changed, 19 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
> > index 1b41029..4f982c6 100644
> > --- a/drivers/iio/adc/ad_sigma_delta.c
> > +++ b/drivers/iio/adc/ad_sigma_delta.c
> > @@ -498,7 +498,6 @@ static int ad_sd_buffer_postenable(struct iio_dev *indio_dev)
> >  	const struct iio_scan_type *scan_type = &indio_dev->channels[0].scan_type;
> >  	struct spi_transfer *xfer = sigma_delta->sample_xfer;
> >  	unsigned int i, slot, channel;
> > -	u8 *samples_buf;
> >  	int ret;
> >  
> >  	if (sigma_delta->num_slots == 1) {
> > @@ -530,7 +529,7 @@ static int ad_sd_buffer_postenable(struct iio_dev *indio_dev)
> >  		xfer[1].bits_per_word = scan_type->realbits;
> >  		xfer[1].len = spi_bpw_to_bytes(scan_type->realbits);
> >  	} else {
> > -		unsigned int samples_buf_size, scan_size;
> > +		unsigned int scan_size;
> >  
> >  		if (sigma_delta->active_slots > 1) {
> >  			ret = ad_sigma_delta_append_status(sigma_delta, true);
> > @@ -538,17 +537,6 @@ static int ad_sd_buffer_postenable(struct iio_dev *indio_dev)
> >  				return ret;
> >  		}
> >  
> > -		samples_buf_size =
> > -			ALIGN(slot * BITS_TO_BYTES(scan_type->storagebits),
> > -			      sizeof(s64));
> > -		samples_buf_size += sizeof(s64);
> > -		samples_buf = devm_krealloc(&sigma_delta->spi->dev,
> > -					    sigma_delta->samples_buf,
> > -					    samples_buf_size, GFP_KERNEL);
> > -		if (!samples_buf)
> > -			return -ENOMEM;
> > -
> > -		sigma_delta->samples_buf = samples_buf;
> >  		scan_size = BITS_TO_BYTES(scan_type->realbits + scan_type->shift);
> >  		/* For 24-bit data, there is an extra byte of padding. */
> >  		xfer[1].rx_buf = &sigma_delta->rx_buf[scan_size == 3 ? 1 : 0];
> > @@ -855,6 +843,24 @@ int devm_ad_sd_setup_buffer_and_trigger(struct device *dev, struct iio_dev *indi
> >  
> >  		indio_dev->setup_ops = &ad_sd_buffer_setup_ops;
> >  	} else {
> > +		const struct iio_scan_type *scan_type =
> > +			&indio_dev->channels[0].scan_type;
> > +		unsigned int samples_buf_size;
> > +
> > +		/*
> > +		 * Worst-case size: all sequencer slots can be active, capped
> > +		 * at num_slots by ad_sd_validate_scan_mask().
> > +		 */
> > +		samples_buf_size =
> > +			ALIGN(sigma_delta->num_slots *
> > +			      BITS_TO_BYTES(scan_type->storagebits),
> > +			      sizeof(s64));
> > +		samples_buf_size += sizeof(s64);
> > +		sigma_delta->samples_buf =
> > +			devm_kzalloc(dev, samples_buf_size, GFP_KERNEL);  
> 
> Why the line break for devm_kzalloc(). Keep it in the same line please.
> Tbh same for for the ALIGN() call but at least that one is coherent with
> the original code.
> 
> - Nuno Sá
> 
> > +		if (!sigma_delta->samples_buf)
> > +			return -ENOMEM;
> > +
> >  		ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> >  						      &iio_pollfunc_store_time,
> >  						      &ad_sd_trigger_handler,
> >