lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Sun, 19 Mar 2023 15:22:22 +0000
From:   Jonathan Cameron <jic23@...nel.org>
To:     Lars-Peter Clausen <lars@...afoo.de>
Cc:     Zheng Wang <zyytlz.wz@....com>, eugen.hristev@...labora.com,
        nicolas.ferre@...rochip.com, alexandre.belloni@...tlin.com,
        claudiu.beznea@...rochip.com, linux-iio@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iio: at91-sama5d2_adc: Fix use after free bug in
 at91_adc_remove due to race condition

On Sat, 18 Mar 2023 10:36:04 -0700
Lars-Peter Clausen <lars@...afoo.de> wrote:

> On 3/18/23 10:39, Jonathan Cameron wrote:
> > On Fri, 10 Mar 2023 17:12:39 +0800
> > Zheng Wang <zyytlz.wz@....com> wrote:
> >  
> >> In at91_adc_probe, &st->touch_st.workq is bound with
> >> at91_adc_workq_handler. Then it will be started by irq
> >> handler at91_adc_touch_data_handler
> >>
> >> If we remove the driver which will call at91_adc_remove
> >>    to make cleanup, there may be a unfinished work.
> >>
> >> The possible sequence is as follows:
> >>
> >> Fix it by finishing the work before cleanup in the at91_adc_remove
> >>
> >> CPU0                  CPU1
> >>
> >>                      |at91_adc_workq_handler
> >> at91_adc_remove     |
> >> iio_device_unregister|
> >> iio_dev_release     |
> >> kfree(iio_dev_opaque);|
> >>                      |
> >>                      |iio_push_to_buffers
> >>                      |&iio_dev_opaque->buffer_list
> >>                      |//use
> >> Fixes: 23ec2774f1cc ("iio: adc: at91-sama5d2_adc: add support for position and pressure channels")
> >> Signed-off-by: Zheng Wang <zyytlz.wz@....com>
> >> ---
> >>   drivers/iio/adc/at91-sama5d2_adc.c | 2 ++
> >>   1 file changed, 2 insertions(+)
> >>
> >> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> >> index 50d02e5fc6fc..1b95d18d9e0b 100644
> >> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> >> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> >> @@ -2495,6 +2495,8 @@ static int at91_adc_remove(struct platform_device *pdev)
> >>   	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> >>   	struct at91_adc_state *st = iio_priv(indio_dev);
> >>   
> >> +	disable_irq_nosync(st->irq);
> >> +	cancel_work_sync(&st->touch_st.workq);  
> > I'd like some input form someone more familiar with this driver than I am.
> >
> > In particular, whilst it fixes the bug seen I'm not sure what the most
> > logical ordering for the disable is or the best way to do it.
> >
> > I'd prefer to see the irq cut off at source by disabling it at the device
> > feature that is generating the irq followed by cancelling or waiting for
> > completion of any in flight work.  
> The usually way you'd do this by calling free_irq() before the 
> cancel_work_sync().

I'd go a little further than that and disable the interrupt source at the
device (if possible) then call free_irq() then cancel_work_sync()

Otherwise the device is merrily monitoring something and generating interrupts
that we don't care about.  Might well be wasting power doing that, though I haven't
checked the flow in this particular case.

Jonathan

Powered by blists - more mailing lists