Skip to contentSkip to conversation

devres: Fix possible use after free

PATCH RESEND In review Maxin B John 3 participants 7 messages Jun 14, 2011 – Jun 15, 2011

Subsystems (2)
Library code IRQ
lore
Share & tools
lore

Maxin B John

author
#0 lore raw

PATCH RESEND 1/1 devres: Fix possible use after free

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 <[email protected]>
Reviewed-by Rolf Eike Beer <[email protected]>
Acked-by Tejun Heo <[email protected]>

---

kernel/irq/devres.c +1 -1
index 1ef4ffc..bd8e788 100644
@@ -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);
lib/devres.c +1 -1
index 6efddf5..7c0e953 100644
@@ -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);
#1 lore raw

Hello,

Quoted 5 lines · 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.

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.

Signature · 1 line
tejun
author
#2 lore raw

Hi,

Quoted 5 lines · Tejun Heo
Quoted 4 lines · 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.

Quoted 7 lines
Quoted 1 line

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

#3 lore raw

Hello, Maxin.

Quoted 8 lines · Maxin B John

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.

Quoted 7 lines

"
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.

Signature · 1 line
tejun
author
#4 lore raw

Hi Tejun,

Quoted 6 lines · Tejun Heo

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

#5 lore raw
In reply to Tejun Heo
Quoted 24 lines

Hello, Maxin.

Quoted 10 lines · Maxin B John

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.

Quoted 10 lines

"
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.
"

Quoted 4 lines

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

author
#6 lore raw

Hi Eike,

Quoted 3 lines · Rolf Eike Beer

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