Bug in bridge interface removal?

Bug in bridge interface removal?

[Erich Schubert]

Hi, on an embedded system i had a crash when doing the following
(invalid) sequence of commands:
Note that this is an outdated kernel, 2.4.19-uc1 with some
modifications; i'm not sure if this is maybe already fixed.

brctl addbr br0
brctl addbr br1
brctl addif br0 eth0
brctl delif br1 eth0
(causing a kernel crash after a second)

yes, i'm deleting the interface from the wrong bridge. Since this
requires root privileges, this isn't much of an "exploit" or so. But it
takes down the system reliably here (but it is an MMUless ARM, you can
take that one down really easy...)

Having a short look at the source i didn't see any safety measure in
http://lxr.linux.no/source/net/bridge/br_if.c#L254
(Neither in 2.4.x, nor in 2.6.x)

I'd suggest adding the following line to the beginning of br_del_if:

if (dev->br_port->br != br) return -EINVAL;

After adding this line i get
  device eth1 is not a slave of br0
instead of the reboot.

Greetings,
Erich

Re: Bug in bridge interface removal?

[Erich Schubert]

Oops, this is fixed in 2.6, and the fix is neater than mine.
I only looked at "del_nbp".

Backporting that from 2.6.x is probably nicer than my fix.

Greetings,
Erich

Bug in interface removal from bridges.

[Erich Schubert]

Hi,
this is the straight backport from 2.6.5 of the fix.
Can someone with authority review this and forward it to marcelo for
inclusion in the next 2.4.x release?

To verify your system is vulnerable (need bridge support):
$ brctl addbr br0
$ brctl addbr br1
$ brctl addif br0 eth0
$ brctl delif br1 eth0
(note br1 in last line, not br0!)

Here's the fix as taken from 2.6:
(fixed sometime in 2.5.x it seems; it might be worth looking at when
this was fixed - it might contain other fixes, too.)

--- net/bridge/br_if.c.2.4.21	2004-05-20 14:34:50.000000000 +0200
+++ net/bridge/br_if.c	2004-05-20 14:37:22.000000000 +0200
@@ -254,6 +254,10 @@
 int br_del_if(struct net_bridge *br, struct net_device *dev)
 {
 	int retval;
+	struct net_bridge_port *p;
+
+	if ((p = dev->br_port) == NULL || p->br != br)
+		return -EINVAL;
 
 	br_write_lock_bh(BR_NETPROTO_LOCK);
 	write_lock(&br->lock);

Greetings,
Erich