Message-ID: <954acc8c-0df3-23a4-7237-ecbc31811a56@microchip.com>
Date:   Mon, 20 Mar 2023 10:35:24 +0100
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:     Zheng Wang <zyytlz.wz@163.com>, <eugen.hristev@collabora.com>
CC:     <jic23@kernel.org>, <lars@metafoo.de>,
        <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>
From:   Nicolas Ferre <nicolas.ferre@microchip.com>
Organization: microchip
In-Reply-To: <20230310091239.1440279-1-zyytlz.wz@163.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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.


> 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);

Regards,
   Nicolas

>          iio_device_unregister(indio_dev);
> 
>          at91_adc_dma_disable(st);
> --
> 2.25.1
> 

-- 
Nicolas Ferre