ng_bridge(4) has an easily exploitable memory leak

[Ruslan Ermilov]

On Wed, Apr 07, 2004 at 12:28:38PM -0700, Julian Elischer wrote:
> On Wed, 7 Apr 2004, Ruslan Ermilov wrote:
> 
> > On RELENG_4, ng_bridge(4) has an easily exploitable memory leak,
> > and may quickly run system out of mbufs.  It's enough to just
> > have only one link connected to the bridge, e.g., the "upper"
> > hook of the ng_ether(4) with IP address assigned, and pinging
> > the broadcast IP address on the interface.  The bug is more
> > real when constructing a bridge, or, like we experienced it,
> > by shutting down all except one bridge's link.  The following
> > patch fixes it:
> > 
[snipped]

> > An alternate solution is to MFC most of ng_bridge.c,v 1.8.  Julian?
> 
> what does an MFC diff look like?
> (bridge is one of archies's nodes)
> 
But it was _you_ who masked this bugfix (which would equally apply
to RELENG_4 as well) under the "SMP preparation" emblem.  ;)

Anyway, the patch for RELENG_4 is attached.  I've only compile tested
it.


Cheers,
-- 
Ruslan Ermilov
ru at FreeBSD.org
FreeBSD committer
-------------- next part --------------
Index: ng_bridge.c
===================================================================
RCS file: /home/ncvs/src/sys/netgraph/ng_bridge.c,v
retrieving revision 1.1.2.6
diff -u -p -r1.1.2.6 ng_bridge.c
--- ng_bridge.c	9 Jan 2004 08:58:06 -0000	1.1.2.6
+++ ng_bridge.c	8 Apr 2004 09:53:43 -0000
@@ -518,7 +518,8 @@ ng_bridge_rcvdata(hook_p hook, struct mb
 	struct ng_bridge_link *link;
 	struct ether_header *eh;
 	int error = 0, linkNum;
-	int i, manycast;
+	int manycast;
+	struct ng_bridge_link *firstLink;
 
 	/* Get link number */
 	linkNum = LINK_NUM(hook);
@@ -657,20 +658,43 @@ ng_bridge_rcvdata(hook_p hook, struct mb
 	}
 
 	/* Distribute unknown, multicast, broadcast pkts to all other links */
-	for (linkNum = i = 0; i < priv->numLinks - 1; linkNum++) {
-		struct ng_bridge_link *const destLink = priv->links[linkNum];
+	firstLink = NULL;
+	for (linkNum = 0; linkNum <= priv->numLinks; linkNum++) {
+		struct ng_bridge_link *destLink;
 		meta_p meta2 = NULL;
-		struct mbuf *m2;
+		struct mbuf *m2 = NULL;
 
-		/* Skip incoming link and disconnected links */
-		if (destLink == NULL || destLink == link)
-			continue;
-
-		/* Copy mbuf and meta info */
-		if (++i == priv->numLinks - 1) {		/* last link */
-			m2 = m;
-			meta2 = meta;
-		}  else {
+		/*
+		 * If we have checked all the links then now
+		 * send the original on its reserved link
+		 */
+		if (linkNum == priv->numLinks) {
+			/* If we never saw a good link, leave. */
+			if (firstLink == NULL) {
+				NG_FREE_DATA(m, meta);
+				return (0);
+			}	
+			destLink = firstLink;
+		} else {
+			destLink = priv->links[linkNum];
+			/* Skip incoming link and disconnected links */
+			if (destLink == NULL || destLink == link) {
+				continue;
+			}
+			if (firstLink == NULL) {
+				/*
+				 * This is the first usable link we have found.
+				 * Reserve it for the originals.
+				 * If we never find another we save a copy.
+				 */
+				firstLink = destLink;
+				continue;
+			}
+
+			/*
+			 * It's usable link but not the reserved (first) one.
+			 * Copy mbuf and meta info for sending.
+			 */
 			m2 = m_dup(m, M_NOWAIT);	/* XXX m_copypacket() */
 			if (m2 == NULL) {
 				link->stats.memoryFailures++;
@@ -701,7 +725,16 @@ ng_bridge_rcvdata(hook_p hook, struct mb
 		}
 
 		/* Send packet */
-		NG_SEND_DATA(error, destLink->hook, m2, meta2);
+		if (destLink == firstLink) { 
+			/*
+			 * If we've sent all the others, send the original
+			 * on the first link we found.
+			 */
+			NG_SEND_DATA(error, destLink->hook, m, meta);
+			break; /* always done last - not really needed. */
+		} else {
+			NG_SEND_DATA(error, destLink->hook, m2, meta2);
+		}
 	}
 	return (error);
 }
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 187 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-stable/attachments/20040408/a803a040/attachment.bin