acpi/ec.c: fix use-after-free

PATCH Applied Adrian Bunk 4 participants 5 messages Oct 24, 2007 – Oct 25, 2007 ACPI

lore

Share & tools

lore

Adrian Bunk

author

2007-10-24 16:25 UTC #0 lore raw

PATCH 1/1 acpi/ec.c: fix use-after-free

This patch fixes a use-after-free introduced by
commit 30c08574da0ead1a47797ce028218ce5b2de61c7.

Spotted by the Coverity checker.

Signed-off-by Adrian Bunk <[email protected]>

---

drivers/acpi/ec.c renamed from drivers/acpi/ec.c.old +2 -2

@@ -434,11 +442,11 @@
 EXPORT_SYMBOL_GPL(acpi_ec_add_query_handler);
 
 void acpi_ec_remove_query_handler(struct acpi_ec *ec, u8 query_bit)
 {
-	struct acpi_ec_query_handler *handler;
+	struct acpi_ec_query_handler *handler, *tmp;
 	mutex_lock(&ec->lock);
-	list_for_each_entry(handler, &ec->list, node) {
+	list_for_each_entry_safe(handler, tmp, &ec->list, node) {
 		if (query_bit == handler->query_bit) {
 			list_del(&handler->node);
 			kfree(handler);
 		}

Alexey Starikovskiy

2007-10-24 17:15 UTC #1 lore raw

Adrian,

commit 30c08574da0ead1a47797ce028218ce5b2de61c7 can not introduce use-after-free.

Please check...

Regards,
Alex.

Quoted 30 lines · Adrian Bunk

This patch fixes a use-after-free introduced by
commit 30c08574da0ead1a47797ce028218ce5b2de61c7.
Spotted by the Coverity checker.
Signed-off-by Adrian Bunk <[email protected]>
---
drivers/acpi/ec.c renamed from drivers/acpi/ec.c.old +2 -3
@@ -434,11 +442,11 @@
 EXPORT_SYMBOL_GPL(acpi_ec_add_query_handler);
 
 void acpi_ec_remove_query_handler(struct acpi_ec *ec, u8 query_bit)
 {
-	struct acpi_ec_query_handler *handler;
+	struct acpi_ec_query_handler *handler, *tmp;
 	mutex_lock(&ec->lock);
-	list_for_each_entry(handler, &ec->list, node) {
+	list_for_each_entry_safe(handler, tmp, &ec->list, node) {
 		if (query_bit == handler->query_bit) {
 			list_del(&handler->node);
 			kfree(handler);
 		}
 
-
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Adrian Bunk

author

2007-10-24 17:25 UTC #2 lore raw

Quoted 5 lines · Alexey Starikovskiy

Adrian,
commit 30c08574da0ead1a47797ce028218ce5b2de61c7 can not introduce use-after-free.
Please check...

Commit 30c08574da0ead1a47797ce028218ce5b2de61c7 did:

<-- snip -->

 	list_for_each_entry(handler, &ec->list, node) {
                if (query_bit == handler->query_bit) {
                        list_del(&handler->node);
                        kfree(handler);
-                       break;
                }
	}

<-- snip -->

If you look at the definition of list_for_each_entry() in include/linux/list.h:

<-- snip -->

#define list_for_each_entry(pos, head, member)                          \
        for (pos = list_entry((head)->next, typeof(*pos), member);      \
             prefetch(pos->member.next), &pos->member != (head);        \
             pos = list_entry(pos->member.next, typeof(*pos), member))
                              ^^^^^^^^^^^^^^^^

<-- snip -->

Without the "break", "handler" is being dereferenced after it was freed.

Quoted 16 lines

Regards,
Alex.
Quoted 13 lines · Adrian Bunk
This patch fixes a use-after-free introduced by
commit 30c08574da0ead1a47797ce028218ce5b2de61c7.
Spotted by the Coverity checker.
Signed-off-by Adrian Bunk <[email protected]>
---
drivers/acpi/ec.c renamed from drivers/acpi/ec.c.old +0 -0
@@ -434,11 +442,11 @@
 EXPORT_SYMBOL_GPL(acpi_ec_add_query_handler);
 

Quoted 12 lines

drivers/acpi/ec.c line 452

Quoted 12 lines

drivers/acpi/ec.c renamed from drivers/acpi/ec.c.old +2 -2

 void acpi_ec_remove_query_handler(struct acpi_ec *ec, u8 query_bit)
 {
-	struct acpi_ec_query_handler *handler;
+	struct acpi_ec_query_handler *handler, *tmp;
 	mutex_lock(&ec->lock);
-	list_for_each_entry(handler, &ec->list, node) {
+	list_for_each_entry_safe(handler, tmp, &ec->list, node) {
 		if (query_bit == handler->query_bit) {
 			list_del(&handler->node);
 			kfree(handler);
 		}

cu
Adrian

Signature · 4 lines

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Alexey Starikovskiy

2007-10-24 17:30 UTC #3 lore raw

Quoted 34 lines · Adrian Bunk

Quoted 5 lines · Alexey Starikovskiy

Adrian,
commit 30c08574da0ead1a47797ce028218ce5b2de61c7 can not introduce use-after-free.
Please check...

Commit 30c08574da0ead1a47797ce028218ce5b2de61c7 did:

<-- snip -->

drivers/acpi/ec.c renamed from drivers/acpi/ec.c.old +0 -1

 	list_for_each_entry(handler, &ec->list, node) {
                if (query_bit == handler->query_bit) {
                        list_del(&handler->node);
                        kfree(handler);
-                       break;
                }

<-- snip -->

If you look at the definition of list_for_each_entry() in include/linux/list.h:

<-- snip -->

#define list_for_each_entry(pos, head, member)                          \
        for (pos = list_entry((head)->next, typeof(*pos), member);      \
             prefetch(pos->member.next), &pos->member != (head);        \
             pos = list_entry(pos->member.next, typeof(*pos), member))
                              ^^^^^^^^^^^^^^^^

Quoted 4 lines

<-- snip -->
Without the "break", "handler" is being dereferenced after it was freed.

Yes, found it minute before :(
Acked, thanks.

Quoted 35 lines

Quoted 18 lines
Regards,
Alex.
Quoted 13 lines · Adrian Bunk
This patch fixes a use-after-free introduced by
commit 30c08574da0ead1a47797ce028218ce5b2de61c7.
Spotted by the Coverity checker.
Signed-off-by Adrian Bunk <[email protected]>
---
drivers/acpi/ec.c renamed from drivers/acpi/ec.c.old +0 -0
@@ -434,11 +442,11 @@
 EXPORT_SYMBOL_GPL(acpi_ec_add_query_handler);
 
Quoted 12 lines
drivers/acpi/ec.c line 452
Quoted 12 lines
drivers/acpi/ec.c renamed from drivers/acpi/ec.c.old +2 -2
 void acpi_ec_remove_query_handler(struct acpi_ec *ec, u8 query_bit)
 {
-	struct acpi_ec_query_handler *handler;
+	struct acpi_ec_query_handler *handler, *tmp;
 	mutex_lock(&ec->lock);
-	list_for_each_entry(handler, &ec->list, node) {
+	list_for_each_entry_safe(handler, tmp, &ec->list, node) {
 		if (query_bit == handler->query_bit) {
 			list_del(&handler->node);
 			kfree(handler);
 		}
cu
Adrian

Len Brown

reviewer Applied

2007-10-25 20:38 UTC #4 lore raw

Applied.

thanks,
-Len

Quoted 83 lines · Alexey Starikovskiy

Quoted 34 lines · Adrian Bunk
Quoted 5 lines · Alexey Starikovskiy
Adrian,
commit 30c08574da0ead1a47797ce028218ce5b2de61c7 can not introduce use-after-free.
Please check...
Commit 30c08574da0ead1a47797ce028218ce5b2de61c7 did:
<-- snip -->
drivers/acpi/ec.c renamed from drivers/acpi/ec.c.old +0 -1
 	list_for_each_entry(handler, &ec->list, node) {
                if (query_bit == handler->query_bit) {
                        list_del(&handler->node);
                        kfree(handler);
-                       break;
                }
	}
<-- snip -->
If you look at the definition of list_for_each_entry() in include/linux/list.h:
<-- snip -->
#define list_for_each_entry(pos, head, member)                          \
        for (pos = list_entry((head)->next, typeof(*pos), member);      \
             prefetch(pos->member.next), &pos->member != (head);        \
             pos = list_entry(pos->member.next, typeof(*pos), member))
                              ^^^^^^^^^^^^^^^^
Quoted 4 lines
<-- snip -->
Without the "break", "handler" is being dereferenced after it was freed.
Yes, found it minute before :(
Acked, thanks.
Quoted 18 lines
Quoted 18 lines
Regards,
Alex.
Quoted 13 lines · Adrian Bunk
This patch fixes a use-after-free introduced by
commit 30c08574da0ead1a47797ce028218ce5b2de61c7.
Spotted by the Coverity checker.
Signed-off-by Adrian Bunk <[email protected]>
---
drivers/acpi/ec.c renamed from drivers/acpi/ec.c.old +0 -0
@@ -434,11 +442,11 @@
 EXPORT_SYMBOL_GPL(acpi_ec_add_query_handler);
 
Quoted 17 lines
Quoted 4 lines
Quoted 4 lines
drivers/acpi/ec.c renamed from drivers/acpi/ec.c.old +1 -1
 void acpi_ec_remove_query_handler(struct acpi_ec *ec, u8 query_bit)
 {
-	struct acpi_ec_query_handler *handler;
+	struct acpi_ec_query_handler *handler, *tmp;
Quoted 8 lines
drivers/acpi/ec.c line 452
Quoted 8 lines
drivers/acpi/ec.c renamed from drivers/acpi/ec.c.old +1 -1
 	mutex_lock(&ec->lock);
-	list_for_each_entry(handler, &ec->list, node) {
+	list_for_each_entry_safe(handler, tmp, &ec->list, node) {
 		if (query_bit == handler->query_bit) {
 			list_del(&handler->node);
 			kfree(handler);
 		}
cu
Adrian
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/