Re: [PATCH] mm: dmapool: fix possible use after free in dmam_pool_destroy()

Re: [PATCH] mm: dmapool: fix possible use after free in dmam_pool_destroy()

[Maxin B John]

On Thu, Jun 2, 2011 at 12:47 PM, Rolf Eike Beer <eike-kernel@sf-tec.de> 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 <maxin.john@gmail.com>
---
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);
 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: dmapool: fix possible use after free in dmam_pool_destroy()

[Maxin B John]

Hi,

On Thu, Jun 2, 2011 at 3:22 PM, Maxin B John <maxin.john@gmail.com> wrote:
> On Thu, Jun 2, 2011 at 12:47 PM, Rolf Eike Beer <eike-kernel@sf-tec.de> 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 <maxin.john@gmail.com>
> ---
> 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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: dmapool: fix possible use after free in dmam_pool_destroy()

[Rolf Eike Beer]

[-- Attachment #1: Type: text/plain, Size: 163 bytes --]

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 <eike-kernel@sf-tec.de>

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] mm: dmapool: fix possible use after free in dmam_pool_destroy()

[Maxin B John]

Hi,

On Tue, Jun 7, 2011 at 7:05 PM, Rolf Eike Beer <eike-kernel@sf-tec.de> 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 <eike-kernel@sf-tec.de>

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: dmapool: fix possible use after free in dmam_pool_destroy()

[Rolf Eike Beer]

> Hi,
>
> On Tue, Jun 7, 2011 at 7:05 PM, Rolf Eike Beer <eike-kernel@sf-tec.de>
> 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 <eike-kernel@sf-tec.de>
>
> 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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: dmapool: fix possible use after free in dmam_pool_destroy()

[Tejun Heo]

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 <maxin.john@gmail.com>

But it shouldn't hurt and if it helps memleak.

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH] mm: dmapool: fix possible use after free in dmam_pool_destroy()

[Maxin B John]

"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 <maxin.john@gmail.com>
---
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);

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: dmapool: fix possible use after free in dmam_pool_destroy()

[Rolf Eike Beer]

[-- Attachment #1: Type: text/plain, Size: 718 bytes --]

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]