* [PATCH] 3c515: Write outside array bounds @ 2009-07-25 22:35 Roel Kluin 2009-07-26 21:16 ` Jarek Poplawski 0 siblings, 1 reply; 6+ messages in thread From: Roel Kluin @ 2009-07-25 22:35 UTC (permalink / raw) To: David S. Miller, netdev, Andrew Morton 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> --- Please review: can we error return like this? diff --git a/drivers/net/3c515.c b/drivers/net/3c515.c index 3e00fa8..c84815a 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! */ + return -ENOMEM; /* 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); ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] 3c515: Write outside array bounds 2009-07-25 22:35 [PATCH] 3c515: Write outside array bounds Roel Kluin @ 2009-07-26 21:16 ` Jarek Poplawski 2009-07-29 14:13 ` Roel Kluin 0 siblings, 1 reply; 6+ messages in thread From: Jarek Poplawski @ 2009-07-26 21:16 UTC (permalink / raw) To: Roel Kluin; +Cc: David S. Miller, netdev, Andrew Morton Roel Kluin wrote, On 07/26/2009 12:35 AM: > 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> > --- > Please review: can we error return like this? I doubt we can return here: there is a lot of cleaning missing. Jarek P. > > diff --git a/drivers/net/3c515.c b/drivers/net/3c515.c > index 3e00fa8..c84815a 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! */ > + return -ENOMEM; /* 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); > -- > 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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] 3c515: Write outside array bounds 2009-07-26 21:16 ` Jarek Poplawski @ 2009-07-29 14:13 ` Roel Kluin 2009-07-29 20:43 ` Jarek Poplawski 0 siblings, 1 reply; 6+ messages in thread From: Roel Kluin @ 2009-07-29 14:13 UTC (permalink / raw) To: Jarek Poplawski; +Cc: David S. Miller, netdev, Andrew Morton Op 26-07-09 23:16, Jarek Poplawski schreef: > Roel Kluin wrote, On 07/26/2009 12:35 AM: > >> 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> >> --- >> 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? 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) ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] 3c515: Write outside array bounds 2009-07-29 14:13 ` Roel Kluin @ 2009-07-29 20:43 ` Jarek Poplawski 2009-07-30 10:26 ` Roel Kluin 0 siblings, 1 reply; 6+ messages in thread From: Jarek Poplawski @ 2009-07-29 20:43 UTC (permalink / raw) To: Roel Kluin; +Cc: David S. Miller, netdev, Andrew Morton On Wed, Jul 29, 2009 at 04:13:24PM +0200, Roel Kluin wrote: > Op 26-07-09 23:16, Jarek Poplawski schreef: > > Roel Kluin wrote, On 07/26/2009 12:35 AM: > > > >> 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> > >> --- > >> 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. > > 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) ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] 3c515: Write outside array bounds 2009-07-29 20:43 ` Jarek Poplawski @ 2009-07-30 10:26 ` Roel Kluin 2009-07-30 20:28 ` David Miller 0 siblings, 1 reply; 6+ messages in thread From: Roel Kluin @ 2009-07-30 10:26 UTC (permalink / raw) To: Jarek Poplawski; +Cc: David S. Miller, netdev, Andrew Morton 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: 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. */ ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] 3c515: Write outside array bounds 2009-07-30 10:26 ` Roel Kluin @ 2009-07-30 20:28 ` David Miller 0 siblings, 0 replies; 6+ messages in thread From: David Miller @ 2009-07-30 20:28 UTC (permalink / raw) To: roel.kluin; +Cc: jarkao2, netdev, akpm 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-07-30 20:28 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-07-25 22:35 [PATCH] 3c515: Write outside array bounds Roel Kluin 2009-07-26 21:16 ` Jarek Poplawski 2009-07-29 14:13 ` Roel Kluin 2009-07-29 20:43 ` Jarek Poplawski 2009-07-30 10:26 ` Roel Kluin 2009-07-30 20:28 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).