| Message ID | 20230717143443.163732-1-carlos.bilbao@amd.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | tg3: fix array subscript out of bounds compilation error | expand |
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
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
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
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
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
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.
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++) { struct tg3_napi *tnapi = &tp->napi[i]; tnapi->tp = tp;
Fix encountered compilation error in tg3.c where an array subscript was 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(-)