[Bridge] [patch] rstpd crashes with GARP/GMRP packets

[Bridge] [patch] rstpd crashes with GARP/GMRP packets

[Solomon Peachy]

[-- Attachment #1.1: Type: text/plain, Size: 725 bytes --]

The attached patch, against Shrinivas's May 7, 2008 snapshot, fixes a 
null pointer dereference that occurs when we receive a packet from the 
brige interface that bears the STP MACADDR, but is *not* a STP packet.

Specifically, I was receiving GMRP packets (see 802.1D-2004 10.1) from a 
3Com switch.

I don't know what we should do with these -- but crashing isn't it.  I 
can send over a packet dump and more debugging info if desired.

 - Solomon
-- 
Solomon Peachy                        solomon@linux-wlan.com
AbsoluteValue Systems                 http://www.linux-wlan.com
721-D North Drive                     +1 (321) 259-0737  (office)
Melbourne, FL 32934                   +1 (321) 259-0286  (fax)

[-- Attachment #1.2: rstp_fixes.diff --]
[-- Type: text/plain, Size: 2731 bytes --]

diff --git a/packages/foss/rstp/brmon.c b/packages/foss/rstp/brmon.c
index d29e7f5..db0d3bb 100644
--- a/packages/foss/rstp/brmon.c
+++ b/packages/foss/rstp/brmon.c
@@ -153,7 +153,7 @@ static int dump_msg(const struct sockaddr_nl *who, struct nlmsghdr *n,
           int newlink = (n->nlmsg_type == RTM_NEWLINK);
           int up = 0;
           if (newlink && tb[IFLA_OPERSTATE]) {
-            int state = *(int*)RTA_DATA(tb[IFLA_OPERSTATE]);
+            int state = *(uint8_t*)RTA_DATA(tb[IFLA_OPERSTATE]);
             up = (state == IF_OPER_UP) || (state == IF_OPER_UNKNOWN);
           }
 
diff --git a/packages/foss/rstp/brstate.c b/packages/foss/rstp/brstate.c
index 1fe792e..c31a647 100644
--- a/packages/foss/rstp/brstate.c
+++ b/packages/foss/rstp/brstate.c
@@ -42,7 +42,7 @@ static int br_set_state(struct rtnl_handle *rth, unsigned ifindex, __u8 state)
 	req.ifi.ifi_family = AF_BRIDGE;
 	req.ifi.ifi_index = ifindex;
 
-	addattr32(&req.n, sizeof(req.buf), IFLA_PROTINFO, state);
+	addattr8(&req.n, sizeof(req.buf), IFLA_PROTINFO, state);
 
 	return rtnl_talk(rth, &req.n, 0, 0, NULL, NULL, NULL);
 }
diff --git a/packages/foss/rstp/include/libnetlink.h b/packages/foss/rstp/include/libnetlink.h
index 63cc3c8..35d76f0 100644
--- a/packages/foss/rstp/include/libnetlink.h
+++ b/packages/foss/rstp/include/libnetlink.h
@@ -33,6 +33,7 @@ extern int rtnl_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n, pid_t peer,
 extern int rtnl_send(struct rtnl_handle *rth, const char *buf, int);
 
 
+extern int addattr8(struct nlmsghdr *n, int maxlen, int type, __u8 data);
 extern int addattr32(struct nlmsghdr *n, int maxlen, int type, __u32 data);
 extern int addattr_l(struct nlmsghdr *n, int maxlen, int type, const void *data, int alen);
 extern int addraw_l(struct nlmsghdr *n, int maxlen, const void *data, int len);
diff --git a/packages/foss/rstp/libnetlink.c b/packages/foss/rstp/libnetlink.c
index 7752236..aaae102 100644
--- a/packages/foss/rstp/libnetlink.c
+++ b/packages/foss/rstp/libnetlink.c
@@ -508,6 +508,24 @@ int addattr32(struct nlmsghdr *n, int maxlen, int type, __u32 data)
 	return 0;
 }
 
+int addattr8(struct nlmsghdr *n, int maxlen, int type, __u8 data)
+{
+	int len = RTA_LENGTH(1);
+	struct rtattr *rta;
+	if (NLMSG_ALIGN(n->nlmsg_len) + len > maxlen) {
+		fprintf(stderr,
+			"addattr32: Error! max allowed bound %d exceeded\n",
+			maxlen);
+		return -1;
+	}
+	rta = NLMSG_TAIL(n);
+	rta->rta_type = type;
+	rta->rta_len = len;
+	memcpy(RTA_DATA(rta), &data, 1);
+	n->nlmsg_len = NLMSG_ALIGN(n->nlmsg_len) + len;
+	return 0;
+}
+
 int addattr_l(struct nlmsghdr *n, int maxlen, int type, const void *data,
 	      int alen)
 {

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [Bridge] [patch] rstpd crashes with GARP/GMRP packets

[Srinivas M.A.]

This looks like the earlier patch, for NETLINK size fixes. Could you
resend the intended patch. Thanks.

On Thu, Jul 3, 2008 at 4:39 AM, Solomon Peachy <solomon@linux-wlan.com> wrote:
> The attached patch, against Shrinivas's May 7, 2008 snapshot, fixes a
> null pointer dereference that occurs when we receive a packet from the
> brige interface that bears the STP MACADDR, but is *not* a STP packet.
>
> Specifically, I was receiving GMRP packets (see 802.1D-2004 10.1) from a
> 3Com switch.
>
> I don't know what we should do with these -- but crashing isn't it.  I
> can send over a packet dump and more debugging info if desired.

Looks like we aren't validating the BPDU as we should be. Please send
any debugging information you have.

Re: [Bridge] [patch] rstpd crashes with GARP/GMRP packets

[Solomon Peachy]

[-- Attachment #1.1: Type: text/plain, Size: 1160 bytes --]

Whoops, the correct patch is attached.

On Thu, Jul 03, 2008 at 11:53:06AM +0530, Srinivas M.A. wrote:
> This looks like the earlier patch, for NETLINK size fixes. Could you
> resend the intended patch. Thanks.
>
> On Thu, Jul 3, 2008 at 4:39 AM, Solomon Peachy <solomon@linux-wlan.com> wrote:
> > The attached patch, against Shrinivas's May 7, 2008 snapshot, fixes a
> > null pointer dereference that occurs when we receive a packet from the
> > brige interface that bears the STP MACADDR, but is *not* a STP packet.
> >
> > Specifically, I was receiving GMRP packets (see 802.1D-2004 10.1) from a
> > 3Com switch.
> >
> > I don't know what we should do with these -- but crashing isn't it.  I
> > can send over a packet dump and more debugging info if desired.
> 
> Looks like we aren't validating the BPDU as we should be. Please send
> any debugging information you have.

-- 
Solomon Peachy                        solomon@linux-wlan.com
AbsoluteValue Systems                 http://www.linux-wlan.com
721-D North Drive                     +1 (321) 259-0737  (office)
Melbourne, FL 32934                   +1 (321) 259-0286  (fax)

[-- Attachment #1.2: rstpd_crash_fix.diff --]
[-- Type: text/plain, Size: 503 bytes --]

diff --git a/packages/foss/rstp/bridge_track.c b/packages/foss/rstp/bridge_track.c
index e93c014..becfff6 100644
--- a/packages/foss/rstp/bridge_track.c
+++ b/packages/foss/rstp/bridge_track.c
@@ -448,6 +448,12 @@ void bridge_bpdu_rcv(int if_index, const unsigned char *data, int len)
 		return;
 
 	TST(ifc->up,);
+
+	/* Note, we can receive packets via the bridge interface 
+	   that are not STP, eg GMRP */
+	if (!ifc->master) 
+		return;
+
 	if (!ifc->master->stp_up)
 		return;
 

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [Bridge] [patch] rstpd crashes with GARP/GMRP packets

[Srinivas M.A.]

Thanks. I put in a modified patch, checking ifc->is_bridge, so that
the bridge and port specific fields of struct ifdata can be put into a
variant record at some point.

commit 8172012df1d981ef1934ede430c4936eae50e5f7
Author: Srinivas Aji <Aji_Srinivas@emc.com>
Date:   Thu Jul 3 22:45:03 2008 +0530

    Fix crash when BPDU received on bridge if.

    We could receive non STP BPDU's on bridge interface, or even STP BPDUs
    if STP is off for the bridge. The crash is avoided by checking for bridge
    interface and not processing further in that case.

    Patch based on one from Solomon Peachy <solomon@linux-wlan.com>

diff --git a/bridge_track.c b/bridge_track.c
index e93c014..802db7a 100644
--- a/bridge_track.c
+++ b/bridge_track.c
@@ -447,6 +447,12 @@ void bridge_bpdu_rcv(int if_index, const unsigned char *dat
        if (!ifc)
                return;

+       /* We could get a STP BPDU from a bridge interface if STP is off,
+          or a non STP packet as well, e.g. GMRP, even when STP is on.
+          Don't process it, else we will crash at ifc->master->stp_up */
+       if (ifc->is_bridge)
+               return;
+
        TST(ifc->up,);
        if (!ifc->master->stp_up)
                return;