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

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

[Kuniyuki Iwashima]

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 ?


> 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.

Where does tp->irq_max go over TG3_IRQ_MAX_VECS ?


>  		struct tg3_napi *tnapi = &tp->napi[i];
>  
>  		tnapi->tp = tp;
> -- 
> 2.41.0

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

[Carlos Bilbao]

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

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

[Jakub Kicinski]

On Tue, 18 Jul 2023 10:52:39 -0500 Carlos Bilbao wrote:
> >> 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’:

What compiler are you using? Any extra flags?

I remember seeing this warning too, but I can't repro it now (gcc 13.1;
clang 16).

> >> 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>

We need a sign-off tag
-- 
pw-bot: cr

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

[Eric Dumazet]

On Wed, Jul 19, 2023 at 4:54 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 18 Jul 2023 10:52:39 -0500 Carlos Bilbao wrote:
> > >> 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’:
>
> What compiler are you using? Any extra flags?
>
> I remember seeing this warning too, but I can't repro it now (gcc 13.1;
> clang 16).

Same here, I think I was seeing this 4 or 5 years ago.
I ignored the warning at that time because we were using an old compiler.

>
> > >> 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>
>
> We need a sign-off tag
> --
> pw-bot: cr

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

[Carlos Bilbao]

On 7/18/23 9:54 PM, Jakub Kicinski wrote:
> On Tue, 18 Jul 2023 10:52:39 -0500 Carlos Bilbao wrote:
>>>> 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’:
> 
> What compiler are you using? Any extra flags?

GCC version 13.1.1. No special flags.

> 
> I remember seeing this warning too, but I can't repro it now (gcc 13.1;
> clang 16).
> 
>>>> 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>
> 
> We need a sign-off tag

Apologies, Signed-off-By: Carlos Bilbao <carlos.bilbao@amd.com>

Thanks,
Carlos

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

[Michael Chan]

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

On Fri, Jul 21, 2023 at 3:45 PM Carlos Bilbao <carlos.bilbao@amd.com> wrote:
>
> On 7/18/23 9:54 PM, Jakub Kicinski wrote:
> > On Tue, 18 Jul 2023 10:52:39 -0500 Carlos Bilbao wrote:
> >>>> 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’:
> >
> > What compiler are you using? Any extra flags?
>
> GCC version 13.1.1. No special flags.
>

I've reviewed the driver code.  tp->irq_max is always properly
initialized to 1 or TG3_IRQ_MAX_VECS.  So this gcc warning seems
bogus.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]