* [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).