Memory corruption in 8390.c ? (was Re: Possible leaks in network drivers)

[Alan Cox <alan@lxorguk.ukuu.org.uk>]

Ar Mer, 2006-06-21 am 18:28 +0200, ysgrifennodd Eric Sesterhenn:
> of the driver. Where we call skb=skb_padto(skb, ETH_ZLEN),
> and dont free the skb later when something goes wrong.

skb_padto() returns either a new buffer or the existing one depending
upon the space situation. If it returns a new buffer then it frees the
old one.

The sequence is

dev_queue_xmit(skb)
	->hard_start_xmit(dev, skb)
	if (0)
		free skb
	return

Which I think means that the error path for a short packet would double
free the skb buffer and leak nskb.


So these drivers should indeed be checking their status before they
clone the buffer. At the point they do an skb_padto they must not fail
if the skb_padto succeeds.

In the case of 8390.c this was broken in 2.6.9 when the efficient 8390
padding code was replaced by something slower and it turns out broken -
although nobody realised that last bit until the checker came along.

See: http://linux.derkeiler.com/Mailing-Lists/Kernel/2005-02/4480.html

Although reverting the change was proposed it was not actually done.


The following should do the trick:

Signed-off-by: Alan Cox <alan@redhat.com>

--- drivers/net/8390.c~	2006-06-21 17:41:12.006145536 +0100
+++ drivers/net/8390.c	2006-06-21 17:41:12.007145384 +0100
@@ -275,12 +275,14 @@
 	struct ei_device *ei_local = (struct ei_device *) netdev_priv(dev);
 	int send_length = skb->len, output_page;
 	unsigned long flags;
+	char buf[64];
+	char *data = skb->data;
 
 	if (skb->len < ETH_ZLEN) {
-		skb = skb_padto(skb, ETH_ZLEN);
-		if (skb == NULL)
-			return 0;
+		memset(buf, 0, ETH_ZLEN);	/* more efficient than doing just the needed bits */
+		memcpy(buf, data, ETH_ZLEN);
 		send_length = ETH_ZLEN;
+		data = buf;
 	}
 
 	/* Mask interrupts from the ethercard. 
@@ -347,7 +349,7 @@
 	 * trigger the send later, upon receiving a Tx done interrupt.
 	 */
 	 
-	ei_block_output(dev, send_length, skb->data, output_page);
+	ei_block_output(dev, send_length, data, output_page);
 		
 	if (! ei_local->txing) 
 	{