Patchwork 3c515: Write outside array bounds

login
register
mail settings
Submitter roel kluin
Date July 30, 2009, 10:26 a.m.
Message ID <4A717558.2050407@gmail.com>
Download mbox | patch
Permalink /patch/30379/
State Accepted
Delegated to: David Miller
Headers show

Comments

roel kluin - July 30, 2009, 10:26 a.m.
if dev_alloc_skb() fails on the first iteration, a write to
cp->rx_ring[-1] occurs.

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
>>>> can we error return like this?
>>>
>>> I doubt we can return here: there is a lot of cleaning missing.

>> I took drivers/net/3c59x.c as an example
>>
>> Is this going in the right direction?
> 
> The direction is right but a long and winding road... I guess it's for
> somebody with drivers knowhow. It seems most of the corkscrew_close()
> might be needed, including del_timer(). So, since this -1 case looks
> quite unlikely, it might be reasonable to only limit the most obvious
> damage with 'if (i != 0)' before [i - 1] write, like David advised
> in lmc case?
> 
> Cheers,
> Jarek P.

Thanks, here it is:

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - July 30, 2009, 8:28 p.m.
From: Roel Kluin <roel.kluin@gmail.com>
Date: Thu, 30 Jul 2009 12:26:32 +0200

> if dev_alloc_skb() fails on the first iteration, a write to
> cp->rx_ring[-1] occurs.
> 
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>

Applied.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/net/3c515.c b/drivers/net/3c515.c
index 3e00fa8..4a7c328 100644
--- a/drivers/net/3c515.c
+++ b/drivers/net/3c515.c
@@ -832,7 +832,9 @@  static int corkscrew_open(struct net_device *dev)
 			skb_reserve(skb, 2);	/* Align IP on 16 byte boundaries */
 			vp->rx_ring[i].addr = isa_virt_to_bus(skb->data);
 		}
-		vp->rx_ring[i - 1].next = isa_virt_to_bus(&vp->rx_ring[0]);	/* Wrap the ring. */
+		if (i != 0)
+			vp->rx_ring[i - 1].next =
+				isa_virt_to_bus(&vp->rx_ring[0]);	/* Wrap the ring. */
 		outl(isa_virt_to_bus(&vp->rx_ring[0]), ioaddr + UpListPtr);
 	}
 	if (vp->full_bus_master_tx) {	/* Boomerang bus master Tx. */