Fix infinite loop on dev_mc_unsync()

PATCH In review Luis R. Rodriguez 5 participants 8 messages Nov 9, 2007 – Nov 11, 2007 2 messages not in the archive

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Luis R. Rodriguez

author

2007-11-09 15:11 UTC #0 lore raw

PATCH 1/1 Fix infinite loop on dev_mc_unsync()

While reviewing net/core/dev_mcast.c I found what I think is an infinite loop on dev_mc_unsync(). This fixes it. We make use of this guy on mac80211 in ieee80211_stop(). This is untested.

Signed-off-by Luis R. Rodriguez <mcgrof-NvJAj8134tRxEa0u/P/[email protected]>

net/core/dev_mcast.c +3 -1

index 15241cf..5373c03 100644
@@ -168,8 +168,10 @@ void dev_mc_unsync(struct net_device *to, struct net_device *from)
 	da = from->mc_list;
 	while (da != NULL) {
 		next = da->next;
-		if (!da->da_synced)
+		if (!da->da_synced) {
+			da = next;
 			continue;
+		}
 		__dev_addr_delete(&to->mc_list, &to->mc_count,
 				  da->da_addr, da->da_addrlen, 0);
 		da->da_synced = 0;

Luis R. Rodriguez

author

2007-11-09 18:51 UTC #1 lore raw

Sorry, forgot to CC David.

On Fri, Nov 09, 2007 at 10:11:35AM -0500, Luis R. Rodriguez wrote:

While reviewing net/core/dev_mcast.c I found what I think is an infinite loop on dev_mc_unsync(). This fixes it. We make use of this guy on mac80211 in ieee80211_stop(). This is untested.

Signed-off-by Luis R. Rodriguez <mcgrof-NvJAj8134tRxEa0u/P/[email protected]>

net/core/dev_mcast.c +3 -1

index 15241cf..5373c03 100644
@@ -168,8 +168,10 @@ void dev_mc_unsync(struct net_device *to, struct net_device *from)
 	da = from->mc_list;
 	while (da != NULL) {
 		next = da->next;
-		if (!da->da_synced)
+		if (!da->da_synced) {
+			da = next;
 			continue;
+		}
 		__dev_addr_delete(&to->mc_list, &to->mc_count,
 				  da->da_addr, da->da_addrlen, 0);
 		da->da_synced = 0;

Joe Perches

2007-11-09 19:07 UTC #2 lore raw

Quoted 23 lines · Luis R. Rodriguez

While reviewing net/core/dev_mcast.c I found what I think is an infinite loop on dev_mc_unsync(). This fixes it. We make use of this guy on mac80211 in ieee80211_stop(). This is untested.
Signed-off-by Luis R. Rodriguez <mcgrof-NvJAj8134tRxEa0u/P/[email protected]>
net/core/dev_mcast.c +3 -2
index 15241cf..5373c03 100644
@@ -168,8 +168,10 @@ void dev_mc_unsync(struct net_device *to, struct net_device *from)
 	da = from->mc_list;
 	while (da != NULL) {
 		next = da->next;
-		if (!da->da_synced)
+		if (!da->da_synced) {
+			da = next;
 			continue;
+		}
 		__dev_addr_delete(&to->mc_list, &to->mc_count,
 				  da->da_addr, da->da_addrlen, 0);
 		da->da_synced = 0;
-

Quoted 3 lines

To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Perhaps this is clearer as:

void dev_mc_unsync(struct net_device *to, struct net_device *from)

{
	struct dev_addr_list *da;

	netif_tx_lock_bh(from);
	netif_tx_lock_bh(to);

	da = from->mc_list;
	while (da) {
		if (da->da_synced) {
			__dev_addr_delete(&to->mc_list, &to->mc_count,
					  da->da_addr, da->da_addrlen, 0);
			__dev_addr_delete(&from->mc_list, &from->mc_count,
					  da->da_addr, da->da_addrlen, 0);
			da->da_synced = 0;
		}
		da = da->next;
	}

	__dev_set_rx_mode(to);

	netif_tx_unlock_bh(to);
	netif_tx_unlock_bh(from);
}

EXPORT_SYMBOL(dev_mc_unsync);

Luis R. Rodriguez

author

2007-11-09 19:21 UTC #3 lore raw

Quoted 54 lines · Joe Perches

Quoted 23 lines · Luis R. Rodriguez
While reviewing net/core/dev_mcast.c I found what I think is an infinite loop on dev_mc_unsync(). This fixes it. We make use of this guy on mac80211 in ieee80211_stop(). This is untested.
Signed-off-by Luis R. Rodriguez <mcgrof-NvJAj8134tRxEa0u/P/[email protected]>
net/core/dev_mcast.c +3 -2
index 15241cf..5373c03 100644
@@ -168,8 +168,10 @@ void dev_mc_unsync(struct net_device *to, struct net_device *from)
 	da = from->mc_list;
 	while (da != NULL) {
 		next = da->next;
-		if (!da->da_synced)
+		if (!da->da_synced) {
+			da = next;
 			continue;
+		}
 		__dev_addr_delete(&to->mc_list, &to->mc_count,
 				  da->da_addr, da->da_addrlen, 0);
 		da->da_synced = 0;
-
Quoted 3 lines
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Perhaps this is clearer as:
void dev_mc_unsync(struct net_device *to, struct net_device *from)
{
	struct dev_addr_list *da;

	netif_tx_lock_bh(from);
	netif_tx_lock_bh(to);

	da = from->mc_list;
	while (da) {
		if (da->da_synced) {
			__dev_addr_delete(&to->mc_list, &to->mc_count,
					  da->da_addr, da->da_addrlen, 0);
			__dev_addr_delete(&from->mc_list, &from->mc_count,
					  da->da_addr, da->da_addrlen, 0);
			da->da_synced = 0;
		}
		da = da->next;
	}

	__dev_set_rx_mode(to);

	netif_tx_unlock_bh(to);
	netif_tx_unlock_bh(from);
}

Quoted 1 line

EXPORT_SYMBOL(dev_mc_unsync);

Sure, or better with a for loop and do away with next pointer then:

void dev_mc_unsync(struct net_device *to, struct net_device *from)

{
        struct dev_addr_list *da;

        netif_tx_lock_bh(from);
        netif_tx_lock_bh(to);

        for (da = from->mc_list; da; da = da->next) {
                if (!da->da_synced)
                        continue;
                __dev_addr_delete(&to->mc_list, &to->mc_count,
                                  da->da_addr, da->da_addrlen, 0);
                da->da_synced = 0;
                __dev_addr_delete(&from->mc_list, &from->mc_count,
                                  da->da_addr, da->da_addrlen, 0);
        }
        __dev_set_rx_mode(to);

        netif_tx_unlock_bh(to);
        netif_tx_unlock_bh(from);
}

EXPORT_SYMBOL(dev_mc_unsync);

Patch below.

Signed-off-by Luis R. Rodriguez <mcgrof-NvJAj8134tRxEa0u/P/[email protected]>

net/core/dev_mcast.c +2 -5

index 15241cf..2aea8e1 100644
@@ -160,14 +160,12 @@ EXPORT_SYMBOL(dev_mc_sync);
  */
 void dev_mc_unsync(struct net_device *to, struct net_device *from)
 {
-	struct dev_addr_list *da, *next;
+	struct dev_addr_list *da;
 
 	netif_tx_lock_bh(from);
 	netif_tx_lock_bh(to);
 
-	da = from->mc_list;
-	while (da != NULL) {
-		next = da->next;
+	for (da = from->mc_list; da; da = da->next) {
 		if (!da->da_synced)
 			continue;
 		__dev_addr_delete(&to->mc_list, &to->mc_count,

@@ -175,7 +173,6 @@ void dev_mc_unsync(struct net_device *to, struct net_device *from)
 		da->da_synced = 0;
 		__dev_addr_delete(&from->mc_list, &from->mc_count,
 				  da->da_addr, da->da_addrlen, 0);
-		da = next;
 	}
 	__dev_set_rx_mode(to);

Patrick McHardy

2007-11-09 23:12 UTC #4 lore raw

Quoted 9 lines · Luis R. Rodriguez

net/core/dev_mcast.c +0 -1

index 15241cf..2aea8e1 100644
@@ -160,14 +160,12 @@ EXPORT_SYMBOL(dev_mc_sync);
  */
 void dev_mc_unsync(struct net_device *to, struct net_device *from)
 {
-	struct dev_addr_list *da, *next;

net/core/dev_mcast.c EXPORT_SYMBOL(dev_mc_sync); · line 169

Quoted 10 lines

net/core/dev_mcast.c +2 -3

+	struct dev_addr_list *da;
 
 	netif_tx_lock_bh(from);
 	netif_tx_lock_bh(to);
 
-	da = from->mc_list;
-	while (da != NULL) {
-		next = da->next;
+	for (da = from->mc_list; da; da = da->next) {

This may cause a use-after-free since __dev_addr_delete frees the address when all references are gone.

Quoted 10 lines

net/core/dev_mcast.c +0 -0

 		if (!da->da_synced)
 			continue;

net/core/dev_mcast.c +0 -1

 		__dev_addr_delete(&to->mc_list, &to->mc_count,

@@ -175,7 +173,6 @@ void dev_mc_unsync(struct net_device *to, struct net_device *from)
 		da->da_synced = 0;
 		__dev_addr_delete(&from->mc_list, &from->mc_count,
 				  da->da_addr, da->da_addrlen, 0);
-		da = next;
 	}

A message that is not in the archive (sent off-list or not yet received): [email protected]

Joe Perches

2007-11-10 00:08 UTC #6 lore raw

Quoted 2 lines · Patrick McHardy

This may cause a use-after-free since __dev_addr_delete frees the address when all references are gone.

How about a comment then? Perhaps:

net/core/dev_mcast.c +14 -7

index ae35405..63576aa 100644
@@ -165,16 +165,23 @@ void dev_mc_unsync(struct net_device *to, struct net_device *from)
 	netif_tx_lock_bh(from);
 	netif_tx_lock_bh(to);
 
+	/*
+	  This while loop can't be written as
+		for (da = from->mc_list; da; da = da->next)
+	  da = from->mc_list and __dev_addr_delete can kfree(from->mc_list)
+	  which could cause a use-after-free of da->next
+	*/
+
 	da = from->mc_list;
 	while (da != NULL) {
 		next = da->next;
-		if (!da->da_synced)
-			continue;
-		__dev_addr_delete(&to->mc_list, &to->mc_count,
-				  da->da_addr, da->da_addrlen, 0);
-		da->da_synced = 0;
-		__dev_addr_delete(&from->mc_list, &from->mc_count,
-				  da->da_addr, da->da_addrlen, 0);
+		if (da->da_synced) {
+			__dev_addr_delete(&to->mc_list, &to->mc_count,
+					  da->da_addr, da->da_addrlen, 0);
+			da->da_synced = 0;
+			__dev_addr_delete(&from->mc_list, &from->mc_count,
+					  da->da_addr, da->da_addrlen, 0);
+		}
 		da = next;
 	}
 	__dev_set_rx_mode(to);

Patrick McHardy

2007-11-10 00:13 UTC #7 lore raw

Quoted 13 lines · Joe Perches

Quoted 3 lines · Patrick McHardy
This may cause a use-after-free since __dev_addr_delete frees the address when all references are gone.
How about a comment then? Perhaps:
net/core/dev_mcast.c +0 -0
index ae35405..63576aa 100644

net/core/dev_mcast.c void dev_mc_unsync(struct net_device *to, struct net_device *from) · line 174

Quoted 10 lines

net/core/dev_mcast.c +6 -0

@@ -165,16 +165,23 @@ void dev_mc_unsync(struct net_device *to, struct net_device *from)
 	netif_tx_lock_bh(from);
 	netif_tx_lock_bh(to);
 
+	/*
+	  This while loop can't be written as
+		for (da = from->mc_list; da; da = da->next)
+	  da = from->mc_list and __dev_addr_delete can kfree(from->mc_list)
+	  which could cause a use-after-free of da->next
+	*/

Seems unnecessary to me, we also don't comment each list_for_each_entry_safe iteration. I consider the use of a seperate next variable self-explanatory.

A message that is not in the archive (sent off-list or not yet received): [email protected]

David Miller

2007-11-11 05:34 UTC #9 lore raw

From: Patrick McHardy <[email protected]>
Date: Sat, 10 Nov 2007 01:13:42 +0100

Quoted 17 lines

Quoted 16 lines · Joe Perches
Quoted 3 lines · Patrick McHardy
This may cause a use-after-free since __dev_addr_delete frees the address when all references are gone.
How about a comment then? Perhaps:
net/core/dev_mcast.c +0 -0
index ae35405..63576aa 100644
@@ -165,16 +165,23 @@ void dev_mc_unsync(struct net_device *to, struct net_device *from)
 	netif_tx_lock_bh(from);
 	netif_tx_lock_bh(to);
 

Quoted 10 lines

net/core/dev_mcast.c void dev_mc_unsync(struct net_device *to, struct net_device *from)
Quoted 7 lines
net/core/dev_mcast.c +6 -0
+	/*
+	  This while loop can't be written as
+		for (da = from->mc_list; da; da = da->next)
+	  da = from->mc_list and __dev_addr_delete can kfree(from->mc_list)
+	  which could cause a use-after-free of da->next
+	*/
 
Seems unnecessary to me, we also don't comment each list_for_each_entry_safe iteration. I consider the use of a seperate next variable self-explanatory.

Agreed, this comment is pointless.

I'll apply Joe's patch without the comment.