Re: [PATCH v3] iio: adc: at91-sama5d2_adc: Fix potential use-after-free in sama5d2_adc driver

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 6 Nov 2025 16:24:07 +0200
Eugen Hristev <eugen.hristev@linaro.org> wrote:

> On 11/2/25 13:54, Jonathan Cameron wrote:
> > On Wed, 29 Oct 2025 10:40:16 +0800
> > Pei Xiao <xiaopei01@kylinos.cn> wrote:
> >   
> >> at91_adc_interrupt can call at91_adc_touch_data_handler function
> >> to start the work by schedule_work(&st->touch_st.workq).
> >>
> >> If we remove the module which will call at91_adc_remove to
> >> make cleanup, it will free indio_dev through iio_device_unregister but
> >> quite a bit later. While the work mentioned above will be used. The
> >> sequence of operations that may lead to a UAF bug is as follows:
> >>
> >> CPU0                                      CPU1
> >>
> >>                                      | at91_adc_workq_handler
> >> at91_adc_remove                      |
> >> iio_device_unregister(indio_dev)     |
> >> //free indio_dev a bit later         |
> >>                                      | iio_push_to_buffers(indio_dev)
> >>                                      | //use indio_dev
> >>
> >> Fix it by ensuring that the work is canceled before proceeding with
> >> the cleanup in at91_adc_remove.
> >>
> >> Fixes: 3ec2774f1cc ("iio: adc: at91-sama5d2_adc: add support for position and pressure channels")  
> > This ID doesn't exist in my history  it should be
> > 23ec2774f1cc
> > 
> > I'll fix that up whilst applying.  Ideally I'd like Eugen to take a look
> > but I'm fairly confident so I'll queue this up on the fixes-togreg branch
> > of iio.git and mark it for stable.
> > 
> > Thanks,
> > 
> > Jonathan
> > 
> > 
> >   
> >> Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
> >> ---
> >> changlog in v3: move cancel_work_sync after iio_device_unregister
> >> changlog in v2: use correct Fix id
> >> ---
> >>  drivers/iio/adc/at91-sama5d2_adc.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> >> index b4c36e6a7490..aa4ba3f5a506 100644
> >> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> >> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> >> @@ -2481,6 +2481,7 @@ static void at91_adc_remove(struct platform_device *pdev)
> >>  	struct at91_adc_state *st = iio_priv(indio_dev);
> >>  
> >>  	iio_device_unregister(indio_dev);
> >> +	cancel_work_sync(&st->touch_st.workq);  
> 
> Hi Jonathan,
> 
> Can we push to buffers *after* device was unregistered with
> iio_device_unregister() ? Is that right ? Both Pei and I considered it's
> not.
I started answering this confidently with 'sure that's fine' then got
less confident as I tried to show that :(

iio_buffer_deactivate_all() should have removed all the buffers from
the list that iio_push_to_buffers() walks to find out what buffers are
registered.  So it should end up as a noop after iio_device_unregister()

That call is made from iio_disable_all_buffers() in iio_device_unregister()

It's worth checking there are no races if we end up with work whilst
the unregister is ongoing. That I'm rather doubtful about having looked
at this code for first time for a long while. The removal is under the
info_exist_lock but the walk of the buffers in iio_push_to_buffers isn't.

My first instinct is we should have a specific lock to protect the buffer
list against concurrent accesses but that is messy.  We might be able to
use the existence lock for that. (info_exist_lock) + a check on
indio_dev->info (for consistency with the meaning of that lock rather than
anything else as the buffers are actually removed just before that is set null).

Before posing a patch though I'd like a few more eyes on this.

Completely untested potential fix:
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index f1448ae1b843..ab76fc25c3b5 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -2382,6 +2382,10 @@ int iio_push_to_buffers(struct iio_dev *indio_dev, const void *data)
        int ret;
        struct iio_buffer *buf;
 
+       guard(mutex)(&iio_dev_opaque->info_exist_lock);
+       if (!indio_dev->info)
+               return -ENODEV;
+
        list_for_each_entry(buf, &iio_dev_opaque->buffer_list, buffer_list) {
                ret = iio_push_to_buffer(buf, data);
                if (ret < 0)



Thanks,

Jonathan



> 
> Eugen
> 
> 
> 
> >>  
> >>  	at91_adc_dma_disable(st);
> >>    
> >   
> 
>