linux-kernel - Re: [PATCH] drivers/iio/adc/at91_adc.c: use devm

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]

Date:	Tue, 31 Jul 2012 16:23:46 +0200
From:	Lars-Peter Clausen <lars@...afoo.de>
To:	Julia Lawall <julia.lawall@...6.fr>
CC:	Jonathan Cameron <jic23@....ac.uk>,
	kernel-janitors@...r.kernel.org, linux-iio@...r.kernel.org,
	linux-kernel@...r.kernel.org, rob.herring@...xeda.com
Subject: Re: [PATCH] drivers/iio/adc/at91_adc.c: use devm_ functions

On 07/31/2012 03:54 PM, Julia Lawall wrote:
> 
> 
> On Tue, 31 Jul 2012, Lars-Peter Clausen wrote:
> 
>> Hi,
>>
>> On 07/31/2012 12:09 PM, Julia Lawall wrote:
>>> From: Julia Lawall <Julia.Lawall@...6.fr>
>>> @@ -720,20 +698,14 @@ error_ret:
>>>  static int __devexit at91_adc_remove(struct platform_device *pdev)
>>>  {
>>>  	struct iio_dev *idev = platform_get_drvdata(pdev);
>>> -	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>  	struct at91_adc_state *st = iio_priv(idev);
>>>
>>>  	iio_device_unregister(idev);
>>> [...]
>>> -	free_irq(st->irq, idev);
>>> [...]
>>>  	iio_device_free(idev);
>>
>> I think we have to be careful here. The interrupted is now freed after the
>> device has been freed, which means that it could trigger after the device
>> has been freed. And since we use the device in the interrupt handler we'll
>> get a use after free.
> 
> Perhaps the same would be true in the following code, from the file
> drivers/edac/highbank_l2_edac.c:
> 
>         res = devm_request_irq(&pdev->dev, drvdata->sb_irq,
>                                highbank_l2_err_handler,
>                                0, dev_name(&pdev->dev), dci);
>         if (res < 0)
>                 goto err;
> 
>         dci->mod_name = dev_name(&pdev->dev);
>         dci->dev_name = dev_name(&pdev->dev);
> 
>         if (edac_device_add_device(dci))
>                 goto err;
> 
>         devres_close_group(&pdev->dev, NULL);
>         return 0;
> err:
>     	devres_release_group(&pdev->dev, NULL);
>         edac_device_free_ctl_info(dci);

Yes looks like this has the same issue.

> 
> Is devm_request_irq perhaps not a very good idea?
> 

devm_request_irq has to be used carefully. It is ok to use it if the objects
which are accessed in the interrupt handler are also devres managed. devres
will free objects in the reverse order of which they are allocated.

E.g. if you do

obj = dev_kzalloc(...);
...
devm_request_irq(..., obj);

it is save to use, because 'obj' will be freed after the IRQ has been freed.

- Lars
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/