Skip to contentSkip to conversation

rapidio: use after free in unregister function

PATCH Superseded Dan Carpenter 3 participants 7 messages Jul 5, 2013 – Jul 8, 2013

Subsystems (2)
Kernel janitors Rapidio
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 rapidio: use after free in unregister function

We need to use the _safe version of list_for_each_entry() because we are freeing the iterator.

Signed-off-by Dan Carpenter <[email protected]>
drivers/rapidio/rio.c +2 -2
index f4f30af..84ac64a 100644
@@ -1701,7 +1701,7 @@ EXPORT_SYMBOL_GPL(rio_register_scan);
 int rio_unregister_scan(int mport_id, struct rio_scan *scan_ops)
 {
 	struct rio_mport *port;
-	struct rio_scan_node *scan;
+	struct rio_scan_node *scan, *tmp;
 
 	pr_debug("RIO: %s for mport_id=%d\n", __func__, mport_id);
 

@@ -1715,7 +1715,7 @@ int rio_unregister_scan(int mport_id, struct rio_scan *scan_ops)
 		    (mport_id = RIO_MPORT_ANY && port->nscan = scan_ops))
 			port->nscan = NULL;
 
-	list_for_each_entry(scan, &rio_scans, node)
+	list_for_each_entry_safe(scan, tmp, &rio_scans, node)
 		if (scan->mport_id = mport_id) {
 			list_del(&scan->node);
 			kfree(scan);
#1 lore raw
Quoted 19 lines · Dan Carpenter

We need to use the _safe version of list_for_each_entry() because we are freeing the iterator.

Signed-off-by Dan Carpenter <[email protected]>
drivers/rapidio/rio.c +1 -1
index f4f30af..84ac64a 100644
@@ -1701,7 +1701,7 @@ EXPORT_SYMBOL_GPL(rio_register_scan);
 int rio_unregister_scan(int mport_id, struct rio_scan *scan_ops)
 {
 	struct rio_mport *port;
-	struct rio_scan_node *scan;
+	struct rio_scan_node *scan, *tmp;
 
 	pr_debug("RIO: %s for mport_id=%d\n", __func__, mport_id);

drivers/rapidio/rio.c int rio_unregister_scan(int mport_id, struct rio_scan *scan_ops) · line 1721

Quoted 8 lines
drivers/rapidio/rio.c +1 -1
@@ -1715,7 +1715,7 @@ int rio_unregister_scan(int mport_id, struct rio_scan *scan_ops)
 		    (mport_id = RIO_MPORT_ANY && port->nscan = scan_ops))
 			port->nscan = NULL;
 
-	list_for_each_entry(scan, &rio_scans, node)
+	list_for_each_entry_safe(scan, tmp, &rio_scans, node)
 		if (scan->mport_id = mport_id) {
 			list_del(&scan->node);
 			kfree(scan);

It looks like an mport_id can only be assigned to one scan entry (see rio_register_scan), so you can use list_for_each_entry and break; after the kfree(scan); instead.

~Ryan

author
#2 lore raw
Quoted 4 lines · Ryan Mallon

On 05/07/13 16:02, Dan Carpenter wrote:
It looks like an mport_id can only be assigned to one scan entry (see rio_register_scan), so you can use list_for_each_entry and break; after the kfree(scan); instead.

Yeah. You're right. I'll resend.

regards,
dan carpenter

#3 lore raw
Quoted 11 lines

From: Dan Carpenter [mailto:[email protected]]
Sent: Friday, July 05, 2013 3:19 AM
To: Ryan Mallon
Cc: Matt Porter; Bounine, Alexandre; [email protected]; [email protected]
Subject: Re: [patch] rapidio: use after free in unregister function

Quoted 3 lines · Ryan Mallon

On 05/07/13 16:02, Dan Carpenter wrote:
It looks like an mport_id can only be assigned to one scan entry (see rio_register_scan), so you can use list_for_each_entry and break;

Quoted 7 lines

after

Quoted 1 line

the kfree(scan); instead.

Yeah. You're right. I'll resend.

regards,
dan carpenter

Thank you for catching it. Missed it because we have only one enumerator so far.

Alex.

author
#4 lore raw

PATCH v2 1/1 rapidio: use after free in unregister function

In reply to Ryan Mallon

We're freeing the list iterator so we can't move to the next entry. Since there is only one matching mport_id, we can just break after finding it.

Signed-off-by Dan Carpenter <[email protected]>

---
v2: cleaner fix than v1

drivers/rapidio/rio.c +3 -1
index f4f30af..2e8a20c 100644
@@ -1715,11 +1715,13 @@ int rio_unregister_scan(int mport_id, struct rio_scan *scan_ops)
 		    (mport_id = RIO_MPORT_ANY && port->nscan = scan_ops))
 			port->nscan = NULL;
 
-	list_for_each_entry(scan, &rio_scans, node)
+	list_for_each_entry(scan, &rio_scans, node) {
 		if (scan->mport_id = mport_id) {
 			list_del(&scan->node);
 			kfree(scan);
+			break;
 		}
+	}
 
 	mutex_unlock(&rio_mport_list_lock);
Reviewed-by on v2 1/1
#5 lore raw
Quoted 17 lines · Dan Carpenter

We're freeing the list iterator so we can't move to the next entry. Since there is only one matching mport_id, we can just break after finding it.

Signed-off-by Dan Carpenter <[email protected]>

---
v2: cleaner fix than v1

drivers/rapidio/rio.c +0 -1
index f4f30af..2e8a20c 100644
@@ -1715,11 +1715,13 @@ int rio_unregister_scan(int mport_id, struct rio_scan *scan_ops)
 		    (mport_id = RIO_MPORT_ANY && port->nscan = scan_ops))
 			port->nscan = NULL;
 
-	list_for_each_entry(scan, &rio_scans, node)

drivers/rapidio/rio.c int rio_unregister_scan(int mport_id, struct rio_scan *scan_ops) · line 1727

Quoted 10 lines
drivers/rapidio/rio.c +3 -0
+	list_for_each_entry(scan, &rio_scans, node) {
 		if (scan->mport_id = mport_id) {
 			list_del(&scan->node);
 			kfree(scan);
+			break;
 		}
+	}
 
 	mutex_unlock(&rio_mport_list_lock);
Reviewed-by Ryan Mallon <[email protected]>
Acked-by on v2 1/1
#6 lore raw
In reply to Dan Carpenter
Quoted 18 lines · Dan Carpenter

We're freeing the list iterator so we can't move to the next entry. Since there is only one matching mport_id, we can just break after finding it.

Signed-off-by Dan Carpenter <[email protected]>

---
v2: cleaner fix than v1

drivers/rapidio/rio.c +0 -0
index f4f30af..2e8a20c 100644
@@ -1715,11 +1715,13 @@ int rio_unregister_scan(int mport_id, struct
 rio_scan *scan_ops)
 		    (mport_id = RIO_MPORT_ANY && port->nscan = scan_ops))
 			port->nscan = NULL;

drivers/rapidio/rio.c int rio_unregister_scan(int mport_id, struct · line 1727

Quoted 10 lines
drivers/rapidio/rio.c +3 -1
-	list_for_each_entry(scan, &rio_scans, node)
+	list_for_each_entry(scan, &rio_scans, node) {
 		if (scan->mport_id = mport_id) {
 			list_del(&scan->node);
 			kfree(scan);
+			break;
 		}
+	}
 
 	mutex_unlock(&rio_mport_list_lock);
Acked-by Alexandre Bounine <[email protected]>