linux-kernel - Re: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: <6c72d3e3.4dde.186fd1aa9d7.Coremail.zyytlz.wz@163.com>
Date:   Mon, 20 Mar 2023 11:40:28 +0800 (CST)
From:   王征 <zyytlz.wz@....com>
To:     "Jonathan Cameron" <jic23@...nel.org>
Cc:     eugen.hristev@...labora.com, lars@...afoo.de,
        nicolas.ferre@...rochip.com, 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:Re: [PATCH] iio: at91-sama5d2_adc: Fix use after free bug in
 at91_adc_remove due to race condition

















At 2023-03-19 00:39:13, "Jonathan Cameron" <jic23@...nel.org> wrote:
>On Fri, 10 Mar 2023 17:12:39 +0800
>Zheng Wang <zyytlz.wz@....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@....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.

Hi,

Sorry for my late reply. I think we need to replace disable_irq_nosync with free_irq.

Thanks,
Zheng

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