"dma_pool_destroy(pool)" calls "kfree(pool)". The freed pointer "pool"
is again passed as an argument to the function "devres_destroy()".
This patch fixes the possible use after free.
Please let me know your comments.
Signed-off-by: Maxin B. John <[email protected]>
---
diff --git a/mm/dmapool.c b/mm/dmapool.c
index 03bf3bb..fbb58e3 100644
--- a/mm/dmapool.c
+++ b/mm/dmapool.c
@@ -500,7 +500,7 @@ void dmam_pool_destroy(struct dma_pool *pool)
{
struct device *dev = pool->dev;
- dma_pool_destroy(pool);
WARN_ON(devres_destroy(dev, dmam_pool_release, dmam_pool_match, pool));
+ dma_pool_destroy(pool);
}
EXPORT_SYMBOL(dmam_pool_destroy);
Maxin B John wrote:
> "dma_pool_destroy(pool)" calls "kfree(pool)". The freed pointer "pool"
> is again passed as an argument to the function "devres_destroy()".
> This patch fixes the possible use after free.
>
> Please let me know your comments.
The pool itself is not used there, only the address where the pool has been.
This will only lead to any trouble if something else is allocated to the same
place and inserted into the devres list of the same device between the
dma_pool_destroy() and devres_destroy().
But I agree that this is bad style. But if you are going to change this please
also have a look at devm_iounmap() in lib/devres.c. Maybe also the devm_*irq*
functions need the same changes.
Eike
On Thu, Jun 2, 2011 at 12:47 PM, Rolf Eike Beer <[email protected]> wrote:
> Maxin B John wrote:
>> "dma_pool_destroy(pool)" calls "kfree(pool)". The freed pointer
>> "pool" is again passed as an argument to the function "devres_destroy()".
>> This patch fixes the possible use after free.
>
> The pool itself is not used there, only the address where the pool
> has been.This will only lead to any trouble if something else is allocated to
> the same place and inserted into the devres list of the same device between
> the dma_pool_destroy() and devres_destroy().
Thank you very much for explaining it in detail.
> But I agree that this is bad style. But if you are going to change
> this please also have a look at devm_iounmap() in lib/devres.c. Maybe also the
> devm_*irq* functions need the same changes.
As per your suggestion, I have made similar modifications for lib/devres.c and
kernel/irq/devres.c
CCed the maintainers of the respective files.
Signed-off-by: Maxin B. John <[email protected]>
---
diff --git a/kernel/irq/devres.c b/kernel/irq/devres.c
index 1ef4ffc..bd8e788 100644
--- a/kernel/irq/devres.c
+++ b/kernel/irq/devres.c
@@ -87,8 +87,8 @@ void devm_free_irq(struct device *dev, unsigned int irq, void *dev_id)
{
struct irq_devres match_data = { irq, dev_id };
- free_irq(irq, dev_id);
WARN_ON(devres_destroy(dev, devm_irq_release, devm_irq_match,
&match_data));
+ free_irq(irq, dev_id);
}
EXPORT_SYMBOL(devm_free_irq);
diff --git a/lib/devres.c b/lib/devres.c
index 6efddf5..7c0e953 100644
--- a/lib/devres.c
+++ b/lib/devres.c
@@ -79,9 +79,9 @@ EXPORT_SYMBOL(devm_ioremap_nocache);
*/
void devm_iounmap(struct device *dev, void __iomem *addr)
{
- iounmap(addr);
WARN_ON(devres_destroy(dev, devm_ioremap_release, devm_ioremap_match,
(void *)addr));
+ iounmap(addr);
}
EXPORT_SYMBOL(devm_iounmap);
Hi,
On Thu, Jun 2, 2011 at 3:22 PM, Maxin B John <[email protected]> wrote:
> On Thu, Jun 2, 2011 at 12:47 PM, Rolf Eike Beer <[email protected]> wrote:
>> Maxin B John wrote:
>>> "dma_pool_destroy(pool)" calls "kfree(pool)". The freed pointer
>>> "pool" is again passed as an argument to the function "devres_destroy()".
>>> This patch fixes the possible use after free.
>>
>> The pool itself is not used there, only the address where the pool
>> has been.This will only lead to any trouble if something else is allocated to
>> the same place and inserted into the devres list of the same device between
>> the dma_pool_destroy() and devres_destroy().
>
> Thank you very much for explaining it in detail.
>
>> But I agree that this is bad style. But if you are going to change
>> this please also have a look at devm_iounmap() in lib/devres.c. Maybe also the
>> devm_*irq* functions need the same changes.
>
> As per your suggestion, I have made similar modifications for lib/devres.c and
> kernel/irq/devres.c
>
> CCed the maintainers of the respective files.
>
> Signed-off-by: Maxin B. John <[email protected]>
> ---
> diff --git a/kernel/irq/devres.c b/kernel/irq/devres.c
> index 1ef4ffc..bd8e788 100644
> --- a/kernel/irq/devres.c
> +++ b/kernel/irq/devres.c
> @@ -87,8 +87,8 @@ void devm_free_irq(struct device *dev, unsigned int irq, void *dev_id)
> ?{
> ? ? ? ?struct irq_devres match_data = { irq, dev_id };
>
> - ? ? ? free_irq(irq, dev_id);
> ? ? ? ?WARN_ON(devres_destroy(dev, devm_irq_release, devm_irq_match,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? &match_data));
> + ? ? ? free_irq(irq, dev_id);
> ?}
> ?EXPORT_SYMBOL(devm_free_irq);
> diff --git a/lib/devres.c b/lib/devres.c
> index 6efddf5..7c0e953 100644
> --- a/lib/devres.c
> +++ b/lib/devres.c
> @@ -79,9 +79,9 @@ EXPORT_SYMBOL(devm_ioremap_nocache);
> ?*/
> ?void devm_iounmap(struct device *dev, void __iomem *addr)
> ?{
> - ? ? ? iounmap(addr);
> ? ? ? ?WARN_ON(devres_destroy(dev, devm_ioremap_release, devm_ioremap_match,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? (void *)addr));
> + ? ? ? iounmap(addr);
> ?}
> ?EXPORT_SYMBOL(devm_iounmap);
>
Could you please let me know your thoughts on this patch ?
Best Regards,
Maxin
Maxin B John wrote:
> Could you please let me know your thoughts on this patch ?
Makes absolute sense to me.
Reviewed-by: Rolf Eike Beer <[email protected]>
Hi,
On Tue, Jun 7, 2011 at 7:05 PM, Rolf Eike Beer <[email protected]> wrote:
> Maxin B John wrote:
>
>> Could you please let me know your thoughts on this patch ?
>
> Makes absolute sense to me.
>
> Reviewed-by: Rolf Eike Beer <[email protected]>
Thanks a lot for reviewing the patch. Should I merge these two patches
and re-send it as a single one ?
Please let me know your opinion.
Warm Regards,
Maxin
> Hi,
>
> On Tue, Jun 7, 2011 at 7:05 PM, Rolf Eike Beer <[email protected]>
> wrote:
>> Maxin B John wrote:
>>
>>> Could you please let me know your thoughts on this patch ?
>>
>> Makes absolute sense to me.
>>
>> Reviewed-by: Rolf Eike Beer <[email protected]>
>
> Thanks a lot for reviewing the patch. Should I merge these two patches
> and re-send it as a single one ?
I would do so.
Eike
Hello,
On Thu, Jun 02, 2011 at 05:22:42PM +0300, Maxin B John wrote:
> > The pool itself is not used there, only the address where the pool
> > has been.This will only lead to any trouble if something else is allocated to
> > the same place and inserted into the devres list of the same device between
> > the dma_pool_destroy() and devres_destroy().
Which can't happen. devres release is bound to device driver model
and a device can't be re-attached before release is complete.
ie. those operations are serialized, so the failure mode is only
theoretical.
> Thank you very much for explaining it in detail.
>
> > But I agree that this is bad style. But if you are going to change
> > this please also have a look at devm_iounmap() in lib/devres.c. Maybe also the
> > devm_*irq* functions need the same changes.
>
> As per your suggestion, I have made similar modifications for lib/devres.c and
> kernel/irq/devres.c
>
> CCed the maintainers of the respective files.
>
> Signed-off-by: Maxin B. John <[email protected]>
But it shouldn't hurt and if it helps memleak.
Acked-by: Tejun Heo <[email protected]>
Thanks.
--
tejun