Skip to contentSkip to conversation

bridge: fix use-after-free in br_cleanup_bridges()

PATCH Applied Patrick McHardy 3 participants 3 messages Jul 2, 2008 – Jul 3, 2008

Subsystems (3)
Networking Network drivers Ethernet bridge
lore
Share & tools
lore

Patrick McHardy

author
#0 lore raw

commit 96f1dd78dad10d61bdd487edadea6adda5425e4c

Author: Patrick McHardy <[email protected]>
Date:   Wed Jul 2 15:02:23 2008 +0200

bridge: fix use-after-free in br_cleanup_bridges()

Unregistering a bridge device may cause virtual devices stacked on the bridge, like vlan or macvlan devices, to be unregistered as well. br_cleanup_bridges() uses for_each_netdev_safe() to iterate over all devices during cleanup. This is not enough however, if one of the additionally unregistered devices is next in the list to the bridge device, it will get freed as well and the iteration continues on the freed element.

Restart iteration after each bridge device removal from the beginning to fix this, similar to what rtnl_link_unregister() does.

Signed-off-by Patrick McHardy <[email protected]>
net/bridge/br_if.c +7 -3
index c2397f5..f38cc53 100644
@@ -442,12 +442,16 @@ int br_del_if(struct net_bridge *br, struct net_device *dev)
 
 void __exit br_cleanup_bridges(void)
 {
-	struct net_device *dev, *nxt;
+	struct net_device *dev;
 
 	rtnl_lock();
-	for_each_netdev_safe(&init_net, dev, nxt)
-		if (dev->priv_flags & IFF_EBRIDGE)
+restart:
+	for_each_netdev(&init_net, dev) {
+		if (dev->priv_flags & IFF_EBRIDGE) {
 			del_br(dev->priv);
+			goto restart;
+		}
+	}
 	rtnl_unlock();
 
 }
Acked-by
#1 lore raw
Quoted 14 lines · On Wed, 02 Jul 2008 15:04:14 +0200 Patrick McHardy <[email protected]> wrote:

commit 96f1dd78dad10d61bdd487edadea6adda5425e4c

Author: Patrick McHardy <[email protected]>
Date:   Wed Jul 2 15:02:23 2008 +0200

bridge: fix use-after-free in br_cleanup_bridges()

Unregistering a bridge device may cause virtual devices stacked on the bridge, like vlan or macvlan devices, to be unregistered as well. br_cleanup_bridges() uses for_each_netdev_safe() to iterate over all devices during cleanup. This is not enough however, if one of the additionally unregistered devices is next in the list to the bridge device, it will get freed as well and the iteration continues on the freed element.

Quoted 4 lines

Restart iteration after each bridge device removal from the beginning to fix this, similar to what rtnl_link_unregister() does.

Signed-off-by Patrick McHardy <[email protected]>
Acked-by Stephen Hemminger <[email protected]>
maintainer Applied
#2 lore raw

From: Stephen Hemminger <[email protected]>
Date: Wed, 2 Jul 2008 09:48:17 -0700

Quoted 17 lines
Quoted 14 lines · On Wed, 02 Jul 2008 15:04:14 +0200 Patrick McHardy <[email protected]> wrote:

commit 96f1dd78dad10d61bdd487edadea6adda5425e4c

Author: Patrick McHardy <[email protected]>
Date:   Wed Jul 2 15:02:23 2008 +0200

bridge: fix use-after-free in br_cleanup_bridges()

Unregistering a bridge device may cause virtual devices stacked on the bridge, like vlan or macvlan devices, to be unregistered as well. br_cleanup_bridges() uses for_each_netdev_safe() to iterate over all devices during cleanup. This is not enough however, if one of the additionally unregistered devices is next in the list to the bridge device, it will get freed as well and the iteration continues on the freed element.

Quoted 6 lines
Quoted 4 lines

Restart iteration after each bridge device removal from the beginning to fix this, similar to what rtnl_link_unregister() does.

Signed-off-by Patrick McHardy <[email protected]>
Acked-by Stephen Hemminger <[email protected]>

Applied, thanks everyone.