Re:Re: [PATCH] iio: at91-sama5d2_adc: Fix use after free bug in at91_adc_remove due to race condition

[王征 <zyytlz.wz@163.com>]

At 2023-03-20 16:35:24, "Nicolas Ferre" <nicolas.ferre@microchip.com> wrote:
>On 10/03/2023 at 10:12, Zheng Wang 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
>
>There is no such thing as a SMP platform using this driver (yet?), so we 
>agree that this fix is purely theoretical, cannot be reproduced nor its 
>fix validated.
>
>That being said, I'm happy that enhancements are provided to this 
>driver, no doubt about that.
>

Hi Nicolas,

Thanks for your reply. I'm not familiar with the module and I think you're right.

>
>> 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);
>
>About stopping the source of interrupt, I would recommend using a 
>sequence already exposed in at91_adc_hw_init (and possibly make it 
>common), like:
>
>    if (st->soc_info.platform->layout->EOC_IDR)
>            at91_adc_writel(st, EOC_IDR, 0xffffffff);
>    at91_adc_writel(st, IDR, 0xffffffff);
>

Thanks fou your advice. I'll apply it in the next version.

Best regards,
Zheng

>Regards,
>   Nicolas
>
>>          iio_device_unregister(indio_dev);
>> 
>>          at91_adc_dma_disable(st);
>> --
>> 2.25.1
>> 
>
>-- 
>Nicolas Ferre