linux-kernel - Re: [PATCH] iio: at91-sama5d2_adc: Fix use after free bug in at91_adc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <954acc8c-0df3-23a4-7237-ecbc31811a56@microchip.com>
Date:   Mon, 20 Mar 2023 10:35:24 +0100
From:   Nicolas Ferre <nicolas.ferre@...rochip.com>
To:     Zheng Wang <zyytlz.wz@....com>, <eugen.hristev@...labora.com>
CC:     <jic23@...nel.org>, <lars@...afoo.de>,
        <alexandre.belloni@...tlin.com>, <claudiu.beznea@...rochip.com>,
        <linux-iio@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] iio: at91-sama5d2_adc: Fix use after free bug in
 at91_adc_remove due to race condition

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@....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