Date:   Sat, 18 Mar 2023 17:39:13 +0000
From:   Jonathan Cameron <jic23@kernel.org>
To:     Zheng Wang <zyytlz.wz@163.com>
Cc:     eugen.hristev@collabora.com, lars@metafoo.de,
        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
Subject: Re: [PATCH] iio: at91-sama5d2_adc: Fix use after free bug in
 at91_adc_remove due to race condition
Message-ID: <20230318173913.19e8a1b1@jic23-huawei>
In-Reply-To: <20230310091239.1440279-1-zyytlz.wz@163.com>
References: <20230310091239.1440279-1-zyytlz.wz@163.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

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.

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