[PATCH] net/bridge/br_if.c: fix possible use-after-free in port_carrier_check()

If del_nbp()->cancel_delayed_work(carrier_check) fails, port_carrier_check()
may run later and access an already freed container (struct net_bridge_port).

With this patch, carrier_check owns a reference to "struct net_bridge_port",
not net_device, so it is always safe to acces the container.

port_carrier_check() uses p->dev->br_port == NULL as indication that net_bridge_port
is under destruction. Otherwise it assumes that p->dev->br_port == p.

Signed-off-by: Oleg Nesterov <redacted>
Acked-By: David Howells <dhowells@redhat.com>

--- WQ/net/bridge/br_if.c~5_bridge_uaf	2007-02-18 23:06:15.000000000 +0300
+++ WQ/net/bridge/br_if.c	2007-02-20 00:59:54.000000000 +0300

@@ -83,14 +83,14 @@ static void port_carrier_check(struct wo
 	struct net_device *dev;
 	struct net_bridge *br;
 
-	dev = container_of(work, struct net_bridge_port,
-			   carrier_check.work)->dev;
+	p = container_of(work, struct net_bridge_port, carrier_check.work);
 
 	rtnl_lock();
-	p = dev->br_port;
-	if (!p)
-		goto done;
 	br = p->br;
+	dev = p->dev;
+
+	if (!dev->br_port)
+		goto done;
 
 	if (netif_carrier_ok(dev))
 		p->path_cost = port_cost(dev);

@@ -107,14 +107,16 @@ static void port_carrier_check(struct wo
 		spin_unlock_bh(&br->lock);
 	}
 done:
-	dev_put(dev);
 	rtnl_unlock();
+	kobject_put(&p->kobj);
 }
 
 static void release_nbp(struct kobject *kobj)
 {
 	struct net_bridge_port *p
 		= container_of(kobj, struct net_bridge_port, kobj);
+
+	dev_put(p->dev);
 	kfree(p);
 }
 

@@ -127,12 +129,6 @@ static struct kobj_type brport_ktype = {
 
 static void destroy_nbp(struct net_bridge_port *p)
 {
-	struct net_device *dev = p->dev;
-
-	p->br = NULL;
-	p->dev = NULL;
-	dev_put(dev);
-
 	kobject_put(&p->kobj);
 }
 

@@ -162,7 +158,7 @@ static void del_nbp(struct net_bridge_po
 	dev_set_promiscuity(dev, -1);
 
 	if (cancel_delayed_work(&p->carrier_check))
-		dev_put(dev);
+		kobject_put(&p->kobj);
 
 	spin_lock_bh(&br->lock);
 	br_stp_disable_port(p);

@@ -446,7 +442,7 @@ int br_add_if(struct net_bridge *br, str
 	br_stp_recalculate_bridge_id(br);
 	br_features_recompute(br);
 	if (schedule_delayed_work(&p->carrier_check, BR_PORT_DEBOUNCE))
-		dev_hold(dev);
+		kobject_get(&p->kobj);
 
 	spin_unlock_bh(&br->lock);
 

On Wed, 21 Feb 2007 01:19:41 +0300
Oleg Nesterov [off-list ref] wrote:

Moving this around is problematic.
The ordering here was chosen to be RCU friendly so that
p->dev indicates the port is in process of being deleted but traffic
may still be using old reference, but new traffic should not use it.

Probably the best thing to do is pull the whole delayed work queue
and auto port speed stuff. When STP is moved to user space then
it can do the ethtool op there.




-- 
Stephen Hemminger [off-list ref]

On Tue, Feb 20, 2007 at 04:24:34PM -0800, Stephen Hemminger wrote:

On Wed, 21 Feb 2007 01:19:41 +0300
Oleg Nesterov [off-list ref] wrote:

quoted

If del_nbp()->cancel_delayed_work(carrier_check) fails, port_carrier_check()
may run later and access an already freed container (struct net_bridge_port).

With this patch, carrier_check owns a reference to "struct net_bridge_port",
not net_device, so it is always safe to acces the container.

port_carrier_check() uses p->dev->br_port == NULL as indication that net_bridge_port
is under destruction. Otherwise it assumes that p->dev->br_port == p.

Signed-off-by: Oleg Nesterov <redacted>
Acked-By: David Howells <dhowells@redhat.com>

--- WQ/net/bridge/br_if.c~5_bridge_uaf	2007-02-18 23:06:15.000000000 +0300
+++ WQ/net/bridge/br_if.c	2007-02-20 00:59:54.000000000 +0300

@@ -83,14 +83,14 @@ static void port_carrier_check(struct wo
 	struct net_device *dev;
 	struct net_bridge *br;
 
-	dev = container_of(work, struct net_bridge_port,
-			   carrier_check.work)->dev;
+	p = container_of(work, struct net_bridge_port, carrier_check.work);
 
 	rtnl_lock();
-	p = dev->br_port;
-	if (!p)
-		goto done;
 	br = p->br;

It doesn't matter very much but I think this would look
better after the first if check.

quoted

+	dev = p->dev;
+
+	if (!dev->br_port)
+		goto done;
 
 	if (netif_carrier_ok(dev))
 		p->path_cost = port_cost(dev);

@@ -107,14 +107,16 @@ static void port_carrier_check(struct wo
 		spin_unlock_bh(&br->lock);
 	}
 done:
-	dev_put(dev);
 	rtnl_unlock();
+	kobject_put(&p->kobj);
 }
 
 static void release_nbp(struct kobject *kobj)
 {
 	struct net_bridge_port *p
 		= container_of(kobj, struct net_bridge_port, kobj);
+
+	dev_put(p->dev);
 	kfree(p);
 }
 

@@ -127,12 +129,6 @@ static struct kobj_type brport_ktype = {
 
 static void destroy_nbp(struct net_bridge_port *p)
 {
-	struct net_device *dev = p->dev;
-
-	p->br = NULL;
-	p->dev = NULL;
-	dev_put(dev);
-
 	kobject_put(&p->kobj);
 }

Moving this around is problematic.
The ordering here was chosen to be RCU friendly so that
p->dev indicates the port is in process of being deleted but traffic
may still be using old reference, but new traffic should not use it.

I have known issues with RCU, but dare to disagree here.
It's done during call_rcu, so anything RCU friendly shouldn't
see this at the moment at all. It could be needed for those
with refcounting - than it should be checked, if there is
anything more than port_carrier_check.

I don't have enough time to check this deep enough, but at
the moment I think this patch is right (there is really a
very short race time between calling this function and
container_of).

Regards,
Jarek P.

On Wed, Feb 21, 2007 at 09:23:45AM +0100, Jarek Poplawski wrote:
...

I have known issues with RCU, but dare to disagree here.
It's done during call_rcu, so anything RCU friendly shouldn't
see this at the moment at all. It could be needed for those
with refcounting - than it should be checked, if there is
anything more than port_carrier_check.

Sorry for this than-ing!
(It's my next favorite issue after RCU.)

Jarek P.

On 02/20, Stephen Hemminger wrote:

On Wed, 21 Feb 2007 01:19:41 +0300
Oleg Nesterov [off-list ref] wrote:

quoted

 static void release_nbp(struct kobject *kobj)
 {
 	struct net_bridge_port *p
 		= container_of(kobj, struct net_bridge_port, kobj);
+
+	dev_put(p->dev);
 	kfree(p);
 }
 

@@ -127,12 +129,6 @@ static struct kobj_type brport_ktype = {
 
 static void destroy_nbp(struct net_bridge_port *p)
 {
-	struct net_device *dev = p->dev;
-
-	p->br = NULL;
-	p->dev = NULL;
-	dev_put(dev);
-
 	kobject_put(&p->kobj);
 }

Moving this around is problematic.
The ordering here was chosen to be RCU friendly so that
p->dev indicates the port is in process of being deleted but traffic
may still be using old reference, but new traffic should not use it.

But it is still RCU friendly? destroy_nbp() is rcu-callback which
calls release_nbp() if we have a last reference to ->kobj. This
means that dev_put() may be done a bit later, but not earlier.
And RCU can only garantee "not before", any rcu-callback could be
delayed unpredictably.

Stephen, I know nothing about net/, and

Probably the best thing to do is pull the whole delayed work queue
and auto port speed stuff. When STP is moved to user space then
it can do the ethtool op there.

I can't understand any single word in the paragraph above :)

But the bug (the stable tree has it too) is real. If this patch is
really wrong, could you please take care of it?

Oleg.

On 02/21, Jarek Poplawski wrote:

quoted

On Wed, 21 Feb 2007 01:19:41 +0300
Oleg Nesterov [off-list ref] wrote:

quoted

+	p = container_of(work, struct net_bridge_port, carrier_check.work);
 
 	rtnl_lock();
-	p = dev->br_port;
-	if (!p)
-		goto done;
 	br = p->br;

It doesn't matter very much but I think this would look
better after the first if check.

OK. I can re-send if this patch is otherwise correct.

Oleg.

This is what I was suggesting by getting rid of the work queue completely.

---
 net/bridge/br_if.c      |   34 ++++++++--------------------------
 net/bridge/br_notify.c  |   25 +++++++++++--------------
 net/bridge/br_private.h |    5 ++---
 3 files changed, 21 insertions(+), 43 deletions(-)

--- bridge.orig/net/bridge/br_if.c	2007-02-21 10:22:46.000000000 -0800
+++ bridge/net/bridge/br_if.c	2007-02-21 10:53:25.000000000 -0800

@@ -77,26 +77,15 @@
  * Called from work queue to allow for calling functions that
  * might sleep (such as speed check), and to debounce.
  */
-static void port_carrier_check(struct work_struct *work)
+void br_port_carrier_check(struct net_bridge_port *p)
 {
-	struct net_bridge_port *p;
-	struct net_device *dev;
-	struct net_bridge *br;
-
-	dev = container_of(work, struct net_bridge_port,
-			   carrier_check.work)->dev;
-	work_release(work);
-
-	rtnl_lock();
-	p = dev->br_port;
-	if (!p)
-		goto done;
-	br = p->br;
+	struct net_device *dev = p->dev;
+	struct net_bridge *br = p->br;
 
 	if (netif_carrier_ok(dev))
 		p->path_cost = port_cost(dev);
 
-	if (br->dev->flags & IFF_UP) {
+	if (netif_running(br->dev)) {
 		spin_lock_bh(&br->lock);
 		if (netif_carrier_ok(dev)) {
 			if (p->state == BR_STATE_DISABLED)

@@ -107,9 +96,6 @@
 		}
 		spin_unlock_bh(&br->lock);
 	}
-done:
-	dev_put(dev);
-	rtnl_unlock();
 }
 
 static void release_nbp(struct kobject *kobj)

@@ -162,9 +148,6 @@
 
 	dev_set_promiscuity(dev, -1);
 
-	if (cancel_delayed_work(&p->carrier_check))
-		dev_put(dev);
-
 	spin_lock_bh(&br->lock);
 	br_stp_disable_port(p);
 	spin_unlock_bh(&br->lock);

@@ -282,7 +265,6 @@
 	p->port_no = index;
 	br_init_port(p);
 	p->state = BR_STATE_DISABLED;
-	INIT_DELAYED_WORK_NAR(&p->carrier_check, port_carrier_check);
 	br_stp_port_timer_init(p);
 
 	kobject_init(&p->kobj);

@@ -442,16 +424,16 @@
 	dev_set_promiscuity(dev, 1);
 
 	list_add_rcu(&p->list, &br->port_list);
-
+
 	spin_lock_bh(&br->lock);
 	br_stp_recalculate_bridge_id(br);
 	br_features_recompute(br);
-	if (schedule_delayed_work(&p->carrier_check, BR_PORT_DEBOUNCE))
-		dev_hold(dev);
-
 	spin_unlock_bh(&br->lock);
 
 	dev_set_mtu(br->dev, br_min_mtu(br));
+
+	br_port_carrier_check(p);
+
 	kobject_uevent(&p->kobj, KOBJ_ADD);
 
 	return 0;
--- bridge.orig/net/bridge/br_notify.c	2007-02-21 10:26:26.000000000 -0800
+++ bridge/net/bridge/br_notify.c	2007-02-21 10:34:12.000000000 -0800

@@ -42,51 +42,48 @@
 
 	br = p->br;
 
-	spin_lock_bh(&br->lock);
 	switch (event) {
 	case NETDEV_CHANGEMTU:
 		dev_set_mtu(br->dev, br_min_mtu(br));
 		break;
 
 	case NETDEV_CHANGEADDR:
+		spin_lock_bh(&br->lock);
 		br_fdb_changeaddr(p, dev->dev_addr);
 		br_ifinfo_notify(RTM_NEWLINK, p);
 		br_stp_recalculate_bridge_id(br);
+		spin_unlock_bh(&br->lock);
 		break;
 
 	case NETDEV_CHANGE:
-		if (br->dev->flags & IFF_UP)
-			if (schedule_delayed_work(&p->carrier_check,
-						BR_PORT_DEBOUNCE))
-				dev_hold(dev);
+		br_port_carrier_check(p);
 		break;
 
 	case NETDEV_FEAT_CHANGE:
-		if (br->dev->flags & IFF_UP)
+		spin_lock_bh(&br->lock);
+		if (netif_running(br->dev))
 			br_features_recompute(br);
-
-		/* could do recursive feature change notification
-		 * but who would care??
-		 */
+		spin_unlock_bh(&br->lock);
 		break;
 
 	case NETDEV_DOWN:
+		spin_lock_bh(&br->lock);
 		if (br->dev->flags & IFF_UP)
 			br_stp_disable_port(p);
+		spin_unlock_bh(&br->lock);
 		break;
 
 	case NETDEV_UP:
+		spin_lock_bh(&br->lock);
 		if (netif_carrier_ok(dev) && (br->dev->flags & IFF_UP))
 			br_stp_enable_port(p);
+		spin_unlock_bh(&br->lock);
 		break;
 
 	case NETDEV_UNREGISTER:
-		spin_unlock_bh(&br->lock);
 		br_del_if(br, dev);
-		goto done;
+		break;
 	}
-	spin_unlock_bh(&br->lock);
 
- done:
 	return NOTIFY_DONE;
 }
--- bridge.orig/net/bridge/br_private.h	2007-02-21 10:22:43.000000000 -0800
+++ bridge/net/bridge/br_private.h	2007-02-21 10:53:49.000000000 -0800

@@ -16,7 +16,6 @@
 #define _BR_PRIVATE_H
 
 #include <linux/netdevice.h>
-#include <linux/miscdevice.h>
 #include <linux/if_bridge.h>
 
 #define BR_HASH_BITS 8

@@ -29,7 +28,7 @@
 
 #define BR_PORT_DEBOUNCE (HZ/10)
 
-#define BR_VERSION	"2.2"
+#define BR_VERSION	"2.3"
 
 typedef struct bridge_id bridge_id;
 typedef struct mac_addr mac_addr;

@@ -82,7 +81,6 @@
 	struct timer_list		hold_timer;
 	struct timer_list		message_age_timer;
 	struct kobject			kobj;
-	struct delayed_work		carrier_check;
 	struct rcu_head			rcu;
 };
 

@@ -173,6 +171,7 @@
 		      int clone);
 
 /* br_if.c */
+extern void br_port_carrier_check(struct net_bridge_port *p);
 extern int br_add_bridge(const char *name);
 extern int br_del_bridge(const char *name);
 extern void br_cleanup_bridges(void);

On 02/21, Stephen Hemminger wrote:

This is what I was suggesting by getting rid of the work queue completely.

Can't comment this patch, but if we can get rid of the work_struct - good!

-static void port_carrier_check(struct work_struct *work)
+void br_port_carrier_check(struct net_bridge_port *p)
 {
-	struct net_bridge_port *p;
-	struct net_device *dev;
-	struct net_bridge *br;
-
-	dev = container_of(work, struct net_bridge_port,
-			   carrier_check.work)->dev;
-	work_release(work);

May I ask you to redo this patch on top of

	[PATCH 1/3] net/bridge/br_if.c: don't use _WORK_NAR
	http://marc.theaimsgroup.com/?l=linux-kernel&m=117183517612775

?

We are removing the _NAR stuff, it would be nice to do this in a separate
patch.

Thanks!

Oleg.

On Wed, 21 Feb 2007 23:09:16 +0300
Oleg Nesterov [off-list ref] wrote:

On 02/21, Stephen Hemminger wrote:

quoted

This is what I was suggesting by getting rid of the work queue completely.

Can't comment this patch, but if we can get rid of the work_struct - good!

quoted

-static void port_carrier_check(struct work_struct *work)
+void br_port_carrier_check(struct net_bridge_port *p)
 {
-	struct net_bridge_port *p;
-	struct net_device *dev;
-	struct net_bridge *br;
-
-	dev = container_of(work, struct net_bridge_port,
-			   carrier_check.work)->dev;
-	work_release(work);

May I ask you to redo this patch on top of

	[PATCH 1/3] net/bridge/br_if.c: don't use _WORK_NAR
	http://marc.theaimsgroup.com/?l=linux-kernel&m=117183517612775

?

We are removing the _NAR stuff, it would be nice to do this in a separate
patch.

Thanks!

Oleg.

I would rather put it in a bugfix patchset for 2.6.21 and 2.6.20-stable

-- 
Stephen Hemminger [off-list ref]

On 02/21, Stephen Hemminger wrote:

I would rather put it in a bugfix patchset for 2.6.21 and 2.6.20-stable

OK. Even better. Could you also remove br_private.h:BR_PORT_DEBOUNCE then?

Oleg.

On Wed, Feb 21, 2007 at 10:55:55AM -0800, Stephen Hemminger wrote:

This is what I was suggesting by getting rid of the work queue completely.

...

What about this comment?

-static void port_carrier_check(struct work_struct *work)
+void br_port_carrier_check(struct net_bridge_port *p)

Of course my opinion shouldn't matter here, but it looks
like withdrawing (or giving up) to the older way. So I'm
not excited, but I trust there is a reason for this. 

Cheers,
Jarek P.