Message-ID: <d62ece80-4d88-36a6-9561-fa0f5afc40c1@metafoo.de>
Date:   Sat, 18 Mar 2023 10:36:04 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [PATCH] iio: at91-sama5d2_adc: Fix use after free bug in
 at91_adc_remove due to race condition
Content-Language: en-US
To:     Jonathan Cameron <jic23@kernel.org>, Zheng Wang <zyytlz.wz@163.com>
Cc:     eugen.hristev@collabora.com, nicolas.ferre@microchip.com,
        alexandre.belloni@bootlin.com, claudiu.beznea@microchip.com,
        linux-iio@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org
References: <20230310091239.1440279-1-zyytlz.wz@163.com>
 <20230318173913.19e8a1b1@jic23-huawei>
From:   Lars-Peter Clausen <lars@metafoo.de>
In-Reply-To: <20230318173913.19e8a1b1@jic23-huawei>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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().