drivers/net/chelsio/sge.c: two array overflows

drivers/net/chelsio/sge.c: two array overflows

[Adrian Bunk]

The Coverity checker spotted the following two array overflows in 
drivers/net/chelsio/sge.c (in both cases, the arrays contain 3 elements):

<--  snip  -->

...
static void restart_tx_queues(struct sge *sge)
{
...
                                sge->stats.cmdQ_restarted[3]++;
...
static int t1_sge_tx(struct sk_buff *skb, struct adapter *adapter,
                     unsigned int qid, struct net_device *dev)
{
...
                        sge->stats.cmdQ_full[3]++;
...

<--  snip  -->


cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: drivers/net/chelsio/sge.c: two array overflows

[Scott Bardone]

[-- Attachment #1: Type: text/plain, Size: 731 bytes --]

Adrian,

This is a bug. The array should contain 2 elements.

Attached is a patch which fixes it.
Thanks.

Signed-off-by: Scott Bardone <sbardone@chelsio.com>


Adrian Bunk wrote:
> The Coverity checker spotted the following two array overflows in 
> drivers/net/chelsio/sge.c (in both cases, the arrays contain 3 elements):
> 
> <--  snip  -->
> 
> ...
> static void restart_tx_queues(struct sge *sge)
> {
> ...
>                                 sge->stats.cmdQ_restarted[3]++;
> ...
> static int t1_sge_tx(struct sk_buff *skb, struct adapter *adapter,
>                      unsigned int qid, struct net_device *dev)
> {
> ...
>                         sge->stats.cmdQ_full[3]++;
> ...
> 
> <--  snip  -->
> 
> 
> cu
> Adrian
> 

[-- Attachment #2: sge.patch --]
[-- Type: text/x-patch, Size: 907 bytes --]

--- sge.c	2006-02-17 14:23:45.000000000 -0800
+++ sge.fix.c	2006-03-13 10:51:24.000000000 -0800
@@ -1021,7 +1021,7 @@
 			if (test_and_clear_bit(nd->if_port,
 					       &sge->stopped_tx_queues) &&
 			    netif_running(nd)) {
-				sge->stats.cmdQ_restarted[3]++;
+				sge->stats.cmdQ_restarted[2]++;
 				netif_wake_queue(nd);
 			}
 		}
@@ -1350,7 +1350,7 @@
 	 	if (unlikely(credits < count)) {
 			netif_stop_queue(dev);
 			set_bit(dev->if_port, &sge->stopped_tx_queues);
-			sge->stats.cmdQ_full[3]++;
+			sge->stats.cmdQ_full[2]++;
 			spin_unlock(&q->lock);
 			if (!netif_queue_stopped(dev))
 				CH_ERR("%s: Tx ring full while queue awake!\n",
@@ -1358,7 +1358,7 @@
 			return NETDEV_TX_BUSY;
 		}
 		if (unlikely(credits - count < q->stop_thres)) {
-			sge->stats.cmdQ_full[3]++;
+			sge->stats.cmdQ_full[2]++;
 			netif_stop_queue(dev);
 			set_bit(dev->if_port, &sge->stopped_tx_queues);
 		}

Re: drivers/net/chelsio/sge.c: two array overflows

[Jeff Garzik]

Scott Bardone wrote:
> Adrian,
> 
> This is a bug. The array should contain 2 elements.
> 
> Attached is a patch which fixes it.
> Thanks.
> 
> Signed-off-by: Scott Bardone <sbardone@chelsio.com>

applied.  please avoid attachments and use a proper patch description in 
the future.  I had to hand-edit and hand-apply your patch.

See http://linux.yyz.us/patch-format.html for more info, or use git.

	Jeff

Re: drivers/net/chelsio/sge.c: two array overflows

[Hans-Peter Jansen]

[from the nitpick department..]

Hi Jeff, hi Scott,

Adrian wrote:
>The Coverity checker spotted the following two array overflows in 
>drivers/net/chelsio/sge.c (in both cases, the arrays contain 3 
>elements):

Am Freitag, 17. März 2006 01:21 schrieb Jeff Garzik:
> Scott Bardone wrote:
> > Adrian,
> >
> > This is a bug. The array should contain 2 elements.
> >
> > Attached is a patch which fixes it.
> > Thanks.
> >
> > Signed-off-by: Scott Bardone <sbardone@chelsio.com>
>
> applied.  please avoid attachments and use a proper patch description
> in the future.  I had to hand-edit and hand-apply your patch.

where you wrote in kernel tree commit 
347a444e687b5f8cf0f6485704db1c6024d3:
This is a bug. The array should contain 2 elements.  Here is the fix.

If I'm not completely off the track, you both committed a description 
off by one error: since the patch doesn't change the array size, it's 
presumely¹ still 3 elements, where index 2 references the last one.

Here's hopefully a better patch description:
Fixed off by one thinko in stats accounting, spotted by Coverity 
checker, notified by Adrian "The Cleanman" Bunk.

SCR,
Pete

¹) otherwise, it's still off by one..

Re: drivers/net/chelsio/sge.c: two array overflows

[Scott Bardone]

Thanks Pete,

This is correct, the array should contain 3 elements. The bug was we were 
accessing a 4th element ([3]) which did not exist. We should be modifying the 
last element ([2]) instead.

-Scott

Hans-Peter Jansen wrote:
> [from the nitpick department..]
> 
> Hi Jeff, hi Scott,
> 
> Adrian wrote:
> 
>>The Coverity checker spotted the following two array overflows in 
>>drivers/net/chelsio/sge.c (in both cases, the arrays contain 3 
>>elements):
> 
> 
> Am Freitag, 17. März 2006 01:21 schrieb Jeff Garzik:
> 
>>Scott Bardone wrote:
>>
>>>Adrian,
>>>
>>>This is a bug. The array should contain 2 elements.
>>>
>>>Attached is a patch which fixes it.
>>>Thanks.
>>>
>>>Signed-off-by: Scott Bardone <sbardone@chelsio.com>
>>
>>applied.  please avoid attachments and use a proper patch description
>>in the future.  I had to hand-edit and hand-apply your patch.
> 
> 
> where you wrote in kernel tree commit 
> 347a444e687b5f8cf0f6485704db1c6024d3:
> This is a bug. The array should contain 2 elements.  Here is the fix.
> 
> If I'm not completely off the track, you both committed a description 
> off by one error: since the patch doesn't change the array size, it's 
> presumely¹ still 3 elements, where index 2 references the last one.
> 
> Here's hopefully a better patch description:
> Fixed off by one thinko in stats accounting, spotted by Coverity 
> checker, notified by Adrian "The Cleanman" Bunk.
> 
> SCR,
> Pete
> 
> ¹) otherwise, it's still off by one..