3c515: Write outside array bounds
Subsystems (2)
PATCH 1/1 3c515: Write outside array bounds
if dev_alloc_skb() fails on the first iteration, a write to cp->rx_ring[-1] occurs.
---
Please review: can we error return like this?
drivers/net/3c515.c +1 -1
@@ -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);
Roel Kluin wrote, On 07/26/2009 12:35 AM:
Quoted 6 lines
if
dev_alloc_skb()fails on the first iteration, a write to cp->rx_ring[-1] occurs.Signed-off-by Roel Kluin <[email protected]>---
Please review: can we error return like this?
I doubt we can return here: there is a lot of cleaning missing.
Jarek P.
Quoted 19 lines
drivers/net/3c515.c +1
-2@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Op 26-07-09 23:16, Jarek Poplawski schreef:
Quoted 5 lines
Roel Kluin wrote, On 07/26/2009 12:35 AM:
Quoted 3 lines
if
dev_alloc_skb()fails on the first iteration, a write to cp->rx_ring[-1] occurs.
Quoted 8 lines
Quoted 3 lines
Signed-off-by Roel Kluin <[email protected]>---
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?
drivers/net/3c515.c +12 -1
@@ -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)
Quoted 11 lines · Roel Kluin
Op 26-07-09 23:16, Jarek Poplawski schreef:
Quoted 10 lines
Roel Kluin wrote, On 07/26/2009 12:35 AM:
Quoted 6 lines
if
dev_alloc_skb()fails on the first iteration, a write to cp->rx_ring[-1] occurs.Signed-off-by Roel Kluin <[email protected]>---
Please review: can we error return like this?
Quoted 6 lines
Quoted 3 lines
I doubt we can return here: there is a lot of cleaning missing.
Jarek P.
I took
drivers/net/3c59x.cas an exampleIs 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 32 lines
drivers/net/3c515.c +12
-1@@ -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)
if dev_alloc_skb() fails on the first iteration, a write to cp->rx_ring[-1] occurs.
---
Quoted 15 lines
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
can we error return like this?
I doubt we can return here: there is a lot of cleaning missing.
I took
drivers/net/3c59x.cas an exampleIs 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, includingdel_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?
Quoted 2 lines
Cheers,
Jarek P.
Thanks, here it is:
drivers/net/3c515.c +3 -1
@@ -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. */
From: Roel Kluin <[email protected]>
Date: Thu, 30 Jul 2009 12:26:32 +0200
Quoted 4 lines
if
dev_alloc_skb()fails on the first iteration, a write to cp->rx_ring[-1] occurs.Signed-off-by Roel Kluin <[email protected]>
Applied.