[PATCH net v3] bnxt_en: Bound SW TPA IDs to prevent crashes

[PATCH net v3] bnxt_en: Bound SW TPA IDs to prevent crashes

[Joe Damato]

TPA IDs are generated by FW and can be up to 1024. bnxt_alloc_agg_idx is
intended to wrap the FW ID down to a software ID which is used to index
rxr->rx_tpa, and to generate a mapping between FW IDs and the wrapped
software ID.

On a 57608 with firmware version 233, the firmware advertises 32
concurrent TPAs. As of the commit under fixes, bp->max_tpa on this NIC
is set to 32.

If the software ID from bnxt_alloc_agg_idx is above 31, this results in
an invalid address being loaded on this line:

  tpa_info = &rxr->rx_tpa[agg_id];

because rx_tpa is allocated with only bp->max_tpa (32) entries. Writes
to tpa_info later in the code are out of bounds.

This bug results in a crash at boot:

Oops: general protection fault, kernel NULL pointer dereference 0x8: 0000 [#1] SMP NOPTI
RIP: 0010:bnxt_rx_pkt+0xc0/0x1560
RSP: 0018:ffffc900009b8c78 EFLAGS: 00010246
RAX: 0000000000000000 RBX: 0000000000000048 RCX: 0000000206682516
RDX: ffffc900009b8db4 RSI: 0000000000000000 RDI: 01ffffff038fe1c0
RBP: ffffc9006e687480 R08: ffffc9006e687000 R09: 0000000000003048
R10: 0000000000000480 R11: ffff8881c6083900 R12: 0000000006682516
R13: ffff8881c6095400 R14: 0000000000000016 R15: ffff8881c6b66680
FS:  0000000000000000(0000) GS:ffff88fef3c77000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007fc8bda40584 CR3: 000000807c812001 CR4: 0000000008772ef0
PKRU: 55555554
Call Trace:
 <IRQ>
 ? __netif_receive_skb_list_core+0x1ca/0x250
 __bnxt_poll_work+0x152/0x280
 bnxt_poll_p5+0x1cd/0x480
 __napi_poll+0x30/0x180
 net_rx_action+0x20b/0x3b0
 ? note_gp_changes+0x53/0xe0
 ? tick_setup_sched_timer+0x180/0x180
 ? __napi_schedule+0x9a/0xb0
 ? bnxt_msix+0x24/0x30
 handle_softirqs+0xdd/0x2c0
 __irq_exit_rcu.llvm.3171231171502365008+0x47/0xf0
 common_interrupt+0x85/0x90
 </IRQ>
 <TASK>
 asm_common_interrupt+0x22/0x40

This stack trace is from a crash triggered when an out of bounds rx_tpa
is dereferenced. The invalid write mentioned above is silent in this
particular crash.

Fix this by allocating rx_tpa with bp->max_tpa rounded up to the next
power of 2 (bp->max_tpa_roundup_size) entries and masking the FW TPA ID
with that size, so the wrapped ID can never index past the end of the
array.

Fixes: 54c28fab2fa5 ("bnxt_en: Set bp->max_tpa according to what the FW supports")
Reported-by: Raphael Cardoso Fernandes <raphaelcf@meta.com>
Suggested-by: Michael Chan <michael.chan@broadcom.com>
Cc: stable@vger.kernel.org
Signed-off-by: Joe Damato <joe@dama.to>
---

v3:
  - Addressed an issue Sashiko pointed out, where max_tpa_roundup_size may be
    left unset if bnxt_alloc_tpa_info returns early, which would lead to out
    of bounds access.
  - The other pre-existing issues Sashiko pointed out are unrelated to this
    patch and would need different Fixes tags, so they are better served with
    separate patches in the future.

v2: https://lore.kernel.org/netdev/20260825001842.2501798-1-joe@dama.to/
  - Followed Michael's suggestion on the v1 to increase the size of the tpa
    array so that wrapping indexes into the array is a simple mask.
  - Add Suggested-by because the approach was suggested by Michael.
  - Add a Reported-by so that Raphael gets credit for reporting this bug.
  - Boot tested on a machine with a 57608 and the crash did not reproduce.

v1: https://lore.kernel.org/netdev/20260821233549.3134699-1-joe@dama.to/

 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 25 ++++++++++++++---------
 drivers/net/ethernet/broadcom/bnxt/bnxt.h |  2 +-
 2 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index d59bcca73a2b..8f4791fb468d 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -1514,14 +1514,16 @@ static int bnxt_discard_rx(struct bnxt *bp, struct bnxt_cp_ring_info *cpr,
 	return 0;
 }
 
-static u16 bnxt_alloc_agg_idx(struct bnxt_rx_ring_info *rxr, u16 agg_id)
+static u16 bnxt_alloc_agg_idx(struct bnxt *bp, struct bnxt_rx_ring_info *rxr,
+			      u16 agg_id)
 {
 	struct bnxt_tpa_idx_map *map = rxr->rx_tpa_idx_map;
-	u16 idx = agg_id & MAX_TPA_P5_MASK;
+	u16 idx = agg_id & (bp->max_tpa_roundup_size - 1);
 
 	if (test_bit(idx, map->agg_idx_bmap)) {
-		idx = find_first_zero_bit(map->agg_idx_bmap, MAX_TPA_P5);
-		if (idx >= MAX_TPA_P5)
+		idx = find_first_zero_bit(map->agg_idx_bmap,
+					  bp->max_tpa_roundup_size);
+		if (idx >= bp->max_tpa_roundup_size)
 			return INVALID_HW_RING_ID;
 	}
 	__set_bit(idx, map->agg_idx_bmap);
@@ -1586,7 +1588,7 @@ static void bnxt_tpa_start(struct bnxt *bp, struct bnxt_rx_ring_info *rxr,
 
 	if (bp->flags & BNXT_FLAG_CHIP_P5_PLUS) {
 		agg_id = TPA_START_AGG_ID_P5(tpa_start);
-		agg_id = bnxt_alloc_agg_idx(rxr, agg_id);
+		agg_id = bnxt_alloc_agg_idx(bp, rxr, agg_id);
 		if (unlikely(agg_id == INVALID_HW_RING_ID)) {
 			netdev_warn(bp->dev, "Unable to allocate agg ID for ring %d, agg 0x%x\n",
 				    rxr->bnapi->index,
@@ -3584,7 +3586,7 @@ static void bnxt_free_one_tpa_info_data(struct bnxt *bp,
 {
 	int i;
 
-	for (i = 0; i < bp->max_tpa; i++) {
+	for (i = 0; i < bp->max_tpa_roundup_size; i++) {
 		struct bnxt_tpa_info *tpa_info = &rxr->rx_tpa[i];
 		u8 *data = tpa_info->data;
 
@@ -3781,7 +3783,7 @@ static void bnxt_free_one_tpa_info(struct bnxt *bp,
 	kfree(rxr->rx_tpa_idx_map);
 	rxr->rx_tpa_idx_map = NULL;
 	if (rxr->rx_tpa) {
-		for (i = 0; i < bp->max_tpa; i++) {
+		for (i = 0; i < bp->max_tpa_roundup_size; i++) {
 			kfree(rxr->rx_tpa[i].agg_arr);
 			rxr->rx_tpa[i].agg_arr = NULL;
 		}
@@ -3807,13 +3809,14 @@ static int bnxt_alloc_one_tpa_info(struct bnxt *bp,
 	struct rx_agg_cmp *agg;
 	int i;
 
-	rxr->rx_tpa = kzalloc_objs(struct bnxt_tpa_info, bp->max_tpa);
+	rxr->rx_tpa = kzalloc_objs(struct bnxt_tpa_info,
+				   bp->max_tpa_roundup_size);
 	if (!rxr->rx_tpa)
 		return -ENOMEM;
 
 	if (!(bp->flags & BNXT_FLAG_CHIP_P5_PLUS))
 		return 0;
-	for (i = 0; i < bp->max_tpa; i++) {
+	for (i = 0; i < bp->max_tpa_roundup_size; i++) {
 		agg = kzalloc_objs(*agg, MAX_SKB_FRAGS);
 		if (!agg)
 			return -ENOMEM;
@@ -3831,6 +3834,7 @@ static int bnxt_alloc_tpa_info(struct bnxt *bp)
 	int i, rc;
 
 	bp->max_tpa = MAX_TPA;
+	bp->max_tpa_roundup_size = MAX_TPA;
 	if (bp->flags & BNXT_FLAG_CHIP_P5_PLUS) {
 		if (!bp->max_tpa_v2)
 			return 0;
@@ -3838,6 +3842,7 @@ static int bnxt_alloc_tpa_info(struct bnxt *bp)
 		/* Older P5 FW sets max_tpa_v2 low by mistake except NPAR */
 		if (bp->max_tpa <= 32 && BNXT_CHIP_P5(bp) && !BNXT_NPAR(bp))
 			bp->max_tpa = MAX_TPA_P5;
+		bp->max_tpa_roundup_size = roundup_pow_of_two(bp->max_tpa);
 	}
 
 	for (i = 0; i < bp->rx_nr_rings; i++) {
@@ -4551,7 +4556,7 @@ static int bnxt_alloc_one_tpa_info_data(struct bnxt *bp,
 	u8 *data;
 	int i;
 
-	for (i = 0; i < bp->max_tpa; i++) {
+	for (i = 0; i < bp->max_tpa_roundup_size; i++) {
 		data = __bnxt_alloc_rx_frag(bp, &mapping, rxr,
 					    GFP_KERNEL);
 		if (!data)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
index ab894f8addef..de46b42d7c98 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
@@ -789,7 +789,6 @@ struct nqe_cn {
 
 #define MAX_TPA		64
 #define MAX_TPA_P5	256
-#define MAX_TPA_P5_MASK	(MAX_TPA_P5 - 1)
 #define MAX_TPA_SEGS_P5	0x3f
 
 #if (BNXT_PAGE_SHIFT == 16)
@@ -2380,6 +2379,7 @@ struct bnxt {
 
 	u16			max_tpa_v2;
 	u16			max_tpa;
+	u16			max_tpa_roundup_size;
 	u32			rx_buf_size;
 	u32			rx_buf_use_size;	/* useable size */
 	u16			rx_offset;

base-commit: e2a6641e3bfde58f2284f9859c2b0fdcc6d1c0da
-- 
2.53.0-Meta

Re: [PATCH net v3] bnxt_en: Bound SW TPA IDs to prevent crashes

[Michael Chan]

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

On Thu, Aug 27, 2026 at 11:57 AM Joe Damato <joe@dama.to> wrote:

> @@ -3831,6 +3834,7 @@ static int bnxt_alloc_tpa_info(struct bnxt *bp)
>         int i, rc;
>
>         bp->max_tpa = MAX_TPA;
> +       bp->max_tpa_roundup_size = MAX_TPA;

This is strictly not needed.  If we return early below, it means the
chip does not support TPA at all.  We skip allocating the TPA array
for every ring, so it really makes no difference.

BNXT_SUPPORTS_TPA() will be false if we return early.  LRO and HW_GRO
will not be supported.  So I would say it's a false positive from
Sashiko.  Thanks.

>         if (bp->flags & BNXT_FLAG_CHIP_P5_PLUS) {
>                 if (!bp->max_tpa_v2)
>                         return 0;

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

Re: [PATCH net v3] bnxt_en: Bound SW TPA IDs to prevent crashes

[Joe Damato]

On Thu, Aug 27, 2026 at 04:07:16PM -0700, Michael Chan wrote:
> On Thu, Aug 27, 2026 at 11:57 AM Joe Damato <joe@dama.to> wrote:
> 
> > @@ -3831,6 +3834,7 @@ static int bnxt_alloc_tpa_info(struct bnxt *bp)
> >         int i, rc;
> >
> >         bp->max_tpa = MAX_TPA;
> > +       bp->max_tpa_roundup_size = MAX_TPA;
> 
> This is strictly not needed.  If we return early below, it means the
> chip does not support TPA at all.  We skip allocating the TPA array
> for every ring, so it really makes no difference.
> 
> BNXT_SUPPORTS_TPA() will be false if we return early.  LRO and HW_GRO
> will not be supported.  So I would say it's a false positive from
> Sashiko.  Thanks.

I see, OK.

I suppose that means I'll have to resubmit the v2 over again in 24hr?

Re: [PATCH net v3] bnxt_en: Bound SW TPA IDs to prevent crashes

[Joe Damato]

On Thu, Aug 27, 2026 at 04:07:16PM -0700, Michael Chan wrote:
> On Thu, Aug 27, 2026 at 11:57 AM Joe Damato <joe@dama.to> wrote:
> 
> > @@ -3831,6 +3834,7 @@ static int bnxt_alloc_tpa_info(struct bnxt *bp)
> >         int i, rc;
> >
> >         bp->max_tpa = MAX_TPA;
> > +       bp->max_tpa_roundup_size = MAX_TPA;
> 
> This is strictly not needed.  If we return early below, it means the
> chip does not support TPA at all.  We skip allocating the TPA array
> for every ring, so it really makes no difference.
> 
> BNXT_SUPPORTS_TPA() will be false if we return early.  LRO and HW_GRO
> will not be supported.  So I would say it's a false positive from
> Sashiko.  Thanks.

Sorry, I should have been more explicit in my last message: is it worth a
respin to remove this? Unless I am misunderstanding something, it's harmless
and while not necessary, might be more clear for a human reader?

LMK what you think.

Re: [PATCH net v3] bnxt_en: Bound SW TPA IDs to prevent crashes

[Michael Chan]

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

On Thu, Aug 27, 2026 at 4:41 PM Joe Damato <joe@dama.to> wrote:
>
> On Thu, Aug 27, 2026 at 04:07:16PM -0700, Michael Chan wrote:
> > On Thu, Aug 27, 2026 at 11:57 AM Joe Damato <joe@dama.to> wrote:
> >
> > > @@ -3831,6 +3834,7 @@ static int bnxt_alloc_tpa_info(struct bnxt *bp)
> > >         int i, rc;
> > >
> > >         bp->max_tpa = MAX_TPA;
> > > +       bp->max_tpa_roundup_size = MAX_TPA;
> >
> > This is strictly not needed.  If we return early below, it means the
> > chip does not support TPA at all.  We skip allocating the TPA array
> > for every ring, so it really makes no difference.
> >
> > BNXT_SUPPORTS_TPA() will be false if we return early.  LRO and HW_GRO
> > will not be supported.  So I would say it's a false positive from
> > Sashiko.  Thanks.
>
> Sorry, I should have been more explicit in my last message: is it worth a
> respin to remove this? Unless I am misunderstanding something, it's harmless
> and while not necessary, might be more clear for a human reader?
>

Yes, the extra line is harmless.  But I think it will further confuse
the reader (or AI) into thinking that TPA is supported when we return
early.  I slightly prefer not adding this line.  If you end up
re-spinning, maybe add an extra comment explaining that TPA is not
supported when we return early?  Thanks.

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

Re: [PATCH net v3] bnxt_en: Bound SW TPA IDs to prevent crashes

[Joe Damato]

On Thu, Aug 27, 2026 at 05:00:29PM -0700, Michael Chan wrote:
> On Thu, Aug 27, 2026 at 4:41 PM Joe Damato <joe@dama.to> wrote:
> >
> > On Thu, Aug 27, 2026 at 04:07:16PM -0700, Michael Chan wrote:
> > > On Thu, Aug 27, 2026 at 11:57 AM Joe Damato <joe@dama.to> wrote:
> > >
> > > > @@ -3831,6 +3834,7 @@ static int bnxt_alloc_tpa_info(struct bnxt *bp)
> > > >         int i, rc;
> > > >
> > > >         bp->max_tpa = MAX_TPA;
> > > > +       bp->max_tpa_roundup_size = MAX_TPA;
> > >
> > > This is strictly not needed.  If we return early below, it means the
> > > chip does not support TPA at all.  We skip allocating the TPA array
> > > for every ring, so it really makes no difference.
> > >
> > > BNXT_SUPPORTS_TPA() will be false if we return early.  LRO and HW_GRO
> > > will not be supported.  So I would say it's a false positive from
> > > Sashiko.  Thanks.
> >
> > Sorry, I should have been more explicit in my last message: is it worth a
> > respin to remove this? Unless I am misunderstanding something, it's harmless
> > and while not necessary, might be more clear for a human reader?
> >
> 
> Yes, the extra line is harmless.  But I think it will further confuse
> the reader (or AI) into thinking that TPA is supported when we return
> early.  I slightly prefer not adding this line.  If you end up
> re-spinning, maybe add an extra comment explaining that TPA is not
> supported when we return early?  Thanks.

I think I'll propose something like this for the next version:

@@ -3832,6 +3835,10 @@ static int bnxt_alloc_tpa_info(struct bnxt *bp)

        bp->max_tpa = MAX_TPA;
        if (bp->flags & BNXT_FLAG_CHIP_P5_PLUS) {
+               /* TPA is not supported at all, so there is nothing to
+                * allocate. BNXT_SUPPORTS_TPA() is false in this case and
+                * neither LRO nor HW GRO can be enabled.
+                */
                if (!bp->max_tpa_v2)
                        return 0;
                bp->max_tpa = min_t(u16, bp->max_tpa_v2, MAX_TPA_P5);
@@ -3839,6 +3846,7 @@ static int bnxt_alloc_tpa_info(struct bnxt *bp)
                if (bp->max_tpa <= 32 && BNXT_CHIP_P5(bp) && !BNXT_NPAR(bp))
                        bp->max_tpa = MAX_TPA_P5;
        }
+       bp->max_tpa_roundup_size = roundup_pow_of_two(bp->max_tpa);

Because otherwise max_tpa_roundup_size would be 0 and bnxt_alloc_one_tpa_info
(and other tpa functions) would use an alloc size of 0 and return
ZERO_SIZE_PTR (non-NULL) which would then break checks like:

  if (!rxr->rx_tpa)

because it would be ZERO_SIZE_PTR instead of NULL.

Am I getting that right?

Re: [PATCH net v3] bnxt_en: Bound SW TPA IDs to prevent crashes

[Michael Chan]

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

On Thu, Aug 27, 2026 at 8:03 PM Joe Damato <joe@dama.to> wrote:

> @@ -3832,6 +3835,10 @@ static int bnxt_alloc_tpa_info(struct bnxt *bp)
>
>         bp->max_tpa = MAX_TPA;
>         if (bp->flags & BNXT_FLAG_CHIP_P5_PLUS) {
> +               /* TPA is not supported at all, so there is nothing to
> +                * allocate. BNXT_SUPPORTS_TPA() is false in this case and
> +                * neither LRO nor HW GRO can be enabled.
> +                */
>                 if (!bp->max_tpa_v2)
>                         return 0;
>                 bp->max_tpa = min_t(u16, bp->max_tpa_v2, MAX_TPA_P5);
> @@ -3839,6 +3846,7 @@ static int bnxt_alloc_tpa_info(struct bnxt *bp)
>                 if (bp->max_tpa <= 32 && BNXT_CHIP_P5(bp) && !BNXT_NPAR(bp))
>                         bp->max_tpa = MAX_TPA_P5;
>         }
> +       bp->max_tpa_roundup_size = roundup_pow_of_two(bp->max_tpa);
>
This looks good to me.  Thanks.

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