Re: [PATCH] [RESEND] devres: Fix possible use after free

Re: [PATCH] [RESEND] devres: Fix possible use after free

[Maxin B John]

devres uses the pointer value as key after it's freed, which is safe but 
triggers spurious use-after-free warnings on some static analysis tools. 
Rearrange code to avoid such warnings.

Signed-off-by: Maxin B. John <maxin.john@gmail.com>
Reviewed-by: Rolf Eike Beer <eike-kernel@sf-tec.de>
Acked-by: Tejun Heo <tj@kernel.org>
---
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);
 
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);

[PATCH] [RESEND] devres: Fix possible use after free

[Maxin B John]

A freed pointer is passed as an argument to the function "devres_destroy()" in
"kernel/irq/devres.c" and "lib/devres.c". This patch fixes the possible use 
after free.

It's notabug at this time, but the code is dangerous.

Signed-off-by: Maxin B. John <maxin.john@gmail.com>
Reviewed-by: Rolf Eike Beer <eike-kernel@sf-tec.de>
Acked-by: Tejun Heo <tj@kernel.org>
---
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);
 

Re: [PATCH] [RESEND] devres: Fix possible use after free

[Tejun Heo]

Hello,

On Tue, Jun 14, 2011 at 10:07:32AM +0300, Maxin B John wrote:
> A freed pointer is passed as an argument to the function "devres_destroy()" in
> "kernel/irq/devres.c" and "lib/devres.c". This patch fixes the possible use 
> after free.
> 
> It's notabug at this time, but the code is dangerous.

The code is not dangerous.  The pointer value is used strictly as key
and the code path is always properly serialized.  The change is to
appease access-after-free detection logic, similar to the way we
change the code to better accomodate sparse or other code analysys
tools.  Can you please update the patch description to reflect that?

Thank you.

-- 
tejun

Re: [PATCH] [RESEND] devres: Fix possible use after free

[Maxin B John]

Hi,

On Tue, Jun 14, 2011 at 8:15 AM, Tejun Heo <tj@kernel.org> wrote:
> On Tue, Jun 14, 2011 at 10:07:32AM +0300, Maxin B John wrote:
>> A freed pointer is passed as an argument to the function "devres_destroy()" in
>> "kernel/irq/devres.c" and "lib/devres.c". This patch fixes the possible use
>> after free.
>>
>> It's notabug at this time, but the code is dangerous.
>
> The code is not dangerous.  The pointer value is used strictly as key
> and the code path is always properly serialized.  The change is to
> appease access-after-free detection logic, similar to the way we
> change the code to better accomodate sparse or other code analysys
> tools.  Can you please update the patch description to reflect that?

You are right. I shouldn't have mentioned it as dangerous. I was trying to
make this patch description similar to the description present in the
previous patch:
http://www.spinics.net/lists/mm-commits/msg84313.html

This patch silences the Coverity Prevent's complains about this as
use-after-free bug. Please let me know if the updated patch description is OK.

"
A freed pointer is passed as an argument to the function "devres_destroy()" in
"kernel/irq/devres.c" and "lib/devres.c". This patch fixes the possible use
after free.
The change silences the static analysis tool (Coverity Prevent) complains
about this as use-after-free bug.
"

Best Regards,
Maxin

Re: [PATCH] [RESEND] devres: Fix possible use after free

[Tejun Heo]

Hello, Maxin.

On Tue, Jun 14, 2011 at 03:09:34PM +0100, Maxin B John wrote:
> You are right. I shouldn't have mentioned it as dangerous. I was trying to
> make this patch description similar to the description present in the
> previous patch:
> http://www.spinics.net/lists/mm-commits/msg84313.html
> 
> This patch silences the Coverity Prevent's complains about this as
> use-after-free bug. Please let me know if the updated patch description is OK.
> 
> "
> A freed pointer is passed as an argument to the function "devres_destroy()" in
> "kernel/irq/devres.c" and "lib/devres.c". This patch fixes the possible use
> after free.
> The change silences the static analysis tool (Coverity Prevent) complains
> about this as use-after-free bug.
> "

Yeap, sounds mostly okay but there's no 'possible use after free'.
Maybe something like, "devres uses the pointer value as key after it's
freed, which is safe but triggers spurious use-after-free warnings on
some static analysis tools.  Rearrange code to avoid such warnings".

Thanks.

-- 
tejun

Re: [PATCH] [RESEND] devres: Fix possible use after free

[Maxin B John]

Hi Tejun,

On Wed, Jun 15, 2011 at 9:13 AM, Tejun Heo <tj@kernel.org> wrote:
>
> Yeap, sounds mostly okay but there's no 'possible use after free'.
> Maybe something like, "devres uses the pointer value as key after it's
> freed, which is safe but triggers spurious use-after-free warnings on
> some static analysis tools.  Rearrange code to avoid such warnings".
>
Thanks a lot for this suggestion. I think, this describes the patch in
a much better way.

Warm Regards,
Maxin

Re: [PATCH] [RESEND] devres: Fix possible use after free

[Rolf Eike Beer]

> Hello, Maxin.
>
> On Tue, Jun 14, 2011 at 03:09:34PM +0100, Maxin B John wrote:
>> You are right. I shouldn't have mentioned it as dangerous. I was trying
>> to
>> make this patch description similar to the description present in the
>> previous patch:
>> http://www.spinics.net/lists/mm-commits/msg84313.html
>>
>> This patch silences the Coverity Prevent's complains about this as
>> use-after-free bug. Please let me know if the updated patch description
>> is OK.
>>
>> "
>> A freed pointer is passed as an argument to the function
>> "devres_destroy()" in
>> "kernel/irq/devres.c" and "lib/devres.c". This patch fixes the possible
>> use
>> after free.
>> The change silences the static analysis tool (Coverity Prevent)
>> complains
>> about this as use-after-free bug.
>> "
>
> Yeap, sounds mostly okay but there's no 'possible use after free'.
> Maybe something like, "devres uses the pointer value as key after it's
> freed, which is safe but triggers spurious use-after-free warnings on
> some static analysis tools.  Rearrange code to avoid such warnings".

Sounds good. And please merge both patches together.

Eike

Re: [PATCH] [RESEND] devres: Fix possible use after free

[Maxin B John]

Hi Eike,

On Wed, Jun 15, 2011 at 11:00 AM, Rolf Eike Beer <eike-kernel@sf-tec.de> wrote:
>
> Sounds good. And please merge both patches together.
>
I'll merge both patches into a single patch and repost it today itself.

Warm Regards,
Maxin