Fatal bug in Boomerang driver found

[Rob Riggs]

This is a multi-part message in MIME format.
--------------1A4FCDFAC8B99586E5EBD4DF
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit

The boomerang driver (3c59x.c) has two fatal bugs in
boomerang_rx() that will cause a kernel panic.

Bug #1 will affect *all* machines once the 'signed int'
counter vp->cur_rx goes greater than MAX_INT. At this
point vp->cur_rx becomes a negative number and the for()
loop used to fill the ring buffer always fails. The skbs
in the ring buffer will never be allocated (all will be
set to NULL). The driver does not test for a NULL skb
and calls skb_put with a NULL value, generating a kernel
panic. The solution is to test for the wrap and adjust
both vp->cur_rx and vp->dirty_rx accordingly.

Bug #2 can affect any machine, but machines that have
plenty of RAM are less likely to be affected. If the
driver fails to replenish one of the skbs in the ring
buffers (because DEV_ALLOC_SKB returned NULL) then the
buffer will point to NULL (or 0x00). The driver does
not test for a NULL skb and calls skb_put with a NULL
value, generating a kernel panic. I beleive the correct
solution here is to break from the read loop and try to
replenish the ring buffer. There may be other issues
involved here that I am not aware of, but Donald will
need to check this out.

There is one other issue that I have with the driver.
In boomerang_rx(), rx_work_limit is set and is not
checked until *after* it has tried to put at least
one packet into an skb. However, it is possible that
the we do not have any skbs ready in the ring buffer
(as described above). It would be better to check this
variable *before* trying to fill the NULL skb. In fact,
now that I think about it, that is probably the best
way to deal with Bug #2.

The enclosed patch is against .99E and should fix both
#1 and #2 (with the caveat noted above), but does not
deal with the last issue. Please test and report results.

(Please note that I follow the above lists through the
various archives, rather that subscribing directly.
Should you have comments for me, please cc: me directly.)

Thanks.

-Rob
-- 
Rob Riggs                        Devil's Thumb Entertainment
Network Administrator            Boulder, CO - (303) 938-1200
rob@DevilsThumb.COM              http://www.DevilsThumb.COM/~rob
"The notion of errors is ill-defined." - IRIX 'netstat' man page
--------------1A4FCDFAC8B99586E5EBD4DF
Content-Type: text/plain; charset=us-ascii; name="3c59x.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline; filename="3c59x.patch"

--- 3c59x.c.orig	Mon Jul  6 12:16:25 1998
+++ 3c59x.c	Mon Jul  6 17:31:17 1998
@@ -1801,6 +1801,11 @@
 				void *temp;
 				/* Pass up the skbuff already on the Rx ring. */
 				skb = vp->rx_skbuff[entry];
+				if (skb == NULL) {
+					printk(KERN_ERR "boomerang: Tried to fill a NULL skb!\n");
+					break;
+				}
+
 				vp->rx_skbuff[entry] = NULL;
 #if LINUX_VERSION_CODE >= 0x10300
 				temp = skb_put(skb, pkt_len);
@@ -1833,6 +1838,11 @@
 			vp->stats.rx_packets++;
 		}
 		entry = (++vp->cur_rx) % RX_RING_SIZE;
+		/* Wrap our counters -- otherwise the next for() loop fails */
+		if (vp->cur_rx < 0) {
+			vp->cur_rx = 0;
+			vp->dirty_rx -= INT_MAX;
+		}
 		if (--rx_work_limit < 0)
 			break;
 	}
@@ -1842,8 +1852,10 @@
 		entry = vp->dirty_rx % RX_RING_SIZE;
 		if (vp->rx_skbuff[entry] == NULL) {
 			skb = DEV_ALLOC_SKB(PKT_BUF_SZ);
-			if (skb == NULL)
+			if (skb == NULL) {
+				printk(KERN_ERR "boomerang: Could not allocate skb on ring buffer!\n");
 				break;			/* Bad news!  */
+			}
 			skb->dev = dev;			/* Mark as being used by this device. */
 #if LINUX_VERSION_CODE > 0x10300
 			skb_reserve(skb, 2);	/* Align IP on 16 byte boundaries */

--------------1A4FCDFAC8B99586E5EBD4DF--