Re: [PATCH] 3c515: Write outside array bounds
From: Jarek Poplawski <hidden>
Date: 2009-07-29 20:43:41
On Wed, Jul 29, 2009 at 04:13:24PM +0200, Roel Kluin wrote:
Op 26-07-09 23:16, Jarek Poplawski schreef:quoted
Roel Kluin wrote, On 07/26/2009 12:35 AM:quoted
if dev_alloc_skb() fails on the first iteration, a write to cp->rx_ring[-1] occurs. Signed-off-by: Roel Kluin <redacted> --- Please review: can we error return like this?I doubt we can return here: there is a lot of cleaning missing. Jarek P.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.
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/3c515.c b/drivers/net/3c515.c index 3e00fa8..e94867d 100644 --- a/drivers/net/3c515.c +++ b/drivers/net/3c515.c@@ -827,7 +827,7 @@ static int corkscrew_open(struct net_device *dev) skb = dev_alloc_skb(PKT_BUF_SZ); vp->rx_skbuff[i] = skb; if (skb == NULL) - break; /* Bad news! */ + goto error; /* Bad news! */ skb->dev = dev; /* Mark as being used by this device. */ skb_reserve(skb, 2); /* Align IP on 16 byte boundaries */ vp->rx_ring[i].addr = isa_virt_to_bus(skb->data);@@ -864,6 +864,17 @@ static int corkscrew_open(struct net_device *dev) ioaddr + EL3_CMD); return 0; +error: + pr_emerg("%s: no memory for rx ring\n", dev->name); + int j; + for (j = 0; j < i; j++) { + if (vp->rx_skbuff[j]) { + dev_kfree_skb(vp->rx_skbuff[j]); + vp->rx_skbuff[j] = NULL; + } + } + free_irq(dev->irq, dev); + return -ENOMEM; } static void corkscrew_timer(unsigned long data)