Re: [PATCH] tg3: fix array subscript out of bounds compilation error

[Carlos Bilbao <carlos.bilbao@amd.com>]

On 7/17/23 14:24, Kuniyuki Iwashima wrote:
> From: Carlos Bilbao <carlos.bilbao@amd.com>
> Date: Mon, 17 Jul 2023 09:34:43 -0500
>> Fix encountered compilation error in tg3.c where an array subscript was
> 
> What is the error ?

drivers/net/ethernet/broadcom/tg3.c: In function ‘tg3_init_one’:
drivers/net/ethernet/broadcom/tg3.c:17795:51: error: array subscript 5 is 
above array bounds of ‘struct tg3_napi[5]’ [-Werror=array-bounds=]
17795 |                 struct tg3_napi *tnapi = &tp->napi[i];
       |                                           ~~~~~~~~^~~
In file included from drivers/net/ethernet/broadcom/tg3.c:72:
drivers/net/ethernet/broadcom/tg3.h:3203:41: note: while referencing ‘napi’
  3203 |         struct tg3_napi                 napi[TG3_IRQ_MAX_VECS];
       |                                         ^~~~
drivers/net/ethernet/broadcom/tg3.c:17795:51: error: array subscript 5 is 
above array bounds of ‘struct tg3_napi[5]’ [-Werror=array-bounds=]
17795 |                 struct tg3_napi *tnapi = &tp->napi[i];
       |                                           ~~~~~~~~^~~
drivers/net/ethernet/broadcom/tg3.h:3203:41: note: while referencing ‘napi’
  3203 |         struct tg3_napi                 napi[TG3_IRQ_MAX_VECS];

> 
> 
>> above the array bounds of 'struct tg3_napi[5]'. Add an additional check in
>> the for loop to ensure that it does not exceed the bounds of
>> 'struct tg3_napi' (defined by TG3_IRQ_MAX_VECS).
>>
>> Reviewed-By: Carlos Bilbao <carlos.bilbao@amd.com>
>> ---
>>   drivers/net/ethernet/broadcom/tg3.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
>> index 4179a12fc881..33ad75b7ed91 100644
>> --- a/drivers/net/ethernet/broadcom/tg3.c
>> +++ b/drivers/net/ethernet/broadcom/tg3.c
>> @@ -17791,7 +17791,7 @@ static int tg3_init_one(struct pci_dev *pdev,
>>   	intmbx = MAILBOX_INTERRUPT_0 + TG3_64BIT_REG_LOW;
>>   	rcvmbx = MAILBOX_RCVRET_CON_IDX_0 + TG3_64BIT_REG_LOW;
>>   	sndmbx = MAILBOX_SNDHOST_PROD_IDX_0 + TG3_64BIT_REG_LOW;
>> -	for (i = 0; i < tp->irq_max; i++) {
>> +	for (i = 0; i < tp->irq_max && i < TG3_IRQ_MAX_VECS; i++) {
> 
> I'm not familiar with this driver, but it seem tg3_init_one() calls
> tg3_get_invariants() before the loop and initialises irq_max as 1
> or TG3_IRQ_MAX_VECS.

I'm also not familiar with this driver, but what you mention seems to be
the case, and also tp->irq_max is used for loop boundaries elsewhere just
fine. I don't know why was the compiler complaining, maybe a false positive.

> 
> Where does tp->irq_max go over TG3_IRQ_MAX_VECS ?
> 
> 
>>   		struct tg3_napi *tnapi = &tp->napi[i];
>>   
>>   		tnapi->tp = tp;
>> -- 
>> 2.41.0

Thanks,
Carlos