Messages in this thread | | | Date | Sat, 18 Mar 2023 10:36:04 -0700 | Subject | Re: [PATCH] iio: at91-sama5d2_adc: Fix use after free bug in at91_adc_remove due to race condition | From | Lars-Peter Clausen <> |
| |
On 3/18/23 10:39, Jonathan Cameron wrote: > On Fri, 10 Mar 2023 17:12:39 +0800 > Zheng Wang <zyytlz.wz@163.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@163.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().
| |