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

[Paolo Abeni]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
bnxt_en: Bound SW TPA IDs to prevent crashes

The patch allocates rxr->rx_tpa with bp->max_tpa rounded up to the next
power of two, stored in a new bp->max_tpa_roundup_size field, and masks the
firmware TPA ID with that size in bnxt_alloc_agg_idx() instead of using
MAX_TPA_P5_MASK.  All of the TPA info allocation and free loops now use the
new field as their bound.

> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 9c2cc50276a59..ba710704b1921 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -1517,14 +1517,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);

[Severity: Medium]

This isn't a bug introduced by this patch, but the value stored is now
bounded while the index it is stored at is still the raw firmware ID, just
below the last quoted line:

	map->agg_id_tbl[agg_id] = idx;

agg_id comes from TPA_START_AGG_ID_P5(), whose descriptor field is 12 bits
wide:

	#define RX_TPA_START_CMP_AGG_ID_P5	(0x0fff << 16)

and the table it indexes is 1024 entries, immediately followed by the
in-use bitmap:

struct bnxt_tpa_idx_map {
	u16		agg_id_tbl[1024];
	DECLARE_BITMAP(agg_idx_bmap, MAX_TPA_P5);
};

Can an ID of 1024 or above write past agg_id_tbl[] into agg_idx_bmap?

The same raw ID is used for the read side in bnxt_lookup_agg_idx(), whose
result is used unchecked by bnxt_tpa_end() and bnxt_tpa_agg():

	agg_id = bnxt_lookup_agg_idx(rxr, agg_id);
	tpa_info = &rxr->rx_tpa[agg_id];

The commit message says "TPA IDs are generated by FW and can be up to
1024", which matches the table size, and I could not find a capability
field that would allow firmware to emit an ID above 1023, so this may not
be reachable in practice.

Would an agg_id &= ARRAY_SIZE(map->agg_id_tbl) - 1 in both helpers be worth
adding while this function is being touched?

[ ... ]

> @@ -3784,7 +3786,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;
>  		}
> @@ -3810,13 +3812,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;

[Severity: High]

This is a pre-existing issue, but this loop now runs up to
bp->max_tpa_roundup_size times (for example 33 becoming 64), so there are
more chances for the 272 byte kzalloc_objs(*agg, MAX_SKB_FRAGS) to fail.

On failure, bnxt_alloc_one_tpa_info() returns before rx_tpa_idx_map is
replaced, and the caller for a queue restart goes to the cleanup path:

	if (bp->flags & BNXT_FLAG_TPA) {
		rc = bnxt_alloc_one_tpa_info(bp, clone);
		if (rc)
			goto err_free_tpa_info;
	...
err_free_tpa_info:
	bnxt_free_one_tpa_info(bp, clone);

clone is a shallow copy of the live ring made in bnxt_queue_mem_alloc():

	memcpy(clone, rxr, sizeof(*rxr));
	bnxt_init_rx_ring_struct(bp, clone);
	bnxt_reset_rx_ring_struct(bp, clone);
	...
	clone->rx_agg_bmap = NULL;

Only rx_agg_bmap is cleared there, and bnxt_reset_rx_ring_struct() does not
touch rx_tpa or rx_tpa_idx_map.

So does bnxt_free_one_tpa_info(bp, clone) free the running ring's map here?

	kfree(rxr->rx_tpa_idx_map);
	rxr->rx_tpa_idx_map = NULL;

The live ring's own rx_tpa_idx_map field is not cleared by that, and the
NAPI path writes through it on every TPA completion, in
bnxt_alloc_agg_idx():

	__set_bit(idx, map->agg_idx_bmap);
	map->agg_id_tbl[agg_id] = idx;

with a second kfree() of the same pointer when the source ring is torn down.
Should clone->rx_tpa and clone->rx_tpa_idx_map be set to NULL alongside
clone->rx_agg_bmap before allocating?

> @@ -3843,6 +3846,8 @@ static int bnxt_alloc_tpa_info(struct bnxt *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++) {
>  		struct bnxt_rx_ring_info *rxr = &bp->rx_ring[i];
>  

[Severity: Medium]

Can the early return earlier in bnxt_alloc_tpa_info() skip this assignment
while bp->max_tpa is already set?

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

On a P5+ device whose firmware does not report max_aggs_supported,
bp->max_tpa_v2 is 0, so bp->max_tpa_roundup_size keeps its zero value while
bp->max_tpa is MAX_TPA.

bnxt_alloc_one_tpa_info() is also reachable from bnxt_queue_mem_alloc(),
which is gated only on the TPA flag and not on max_tpa_v2:

	if (bp->flags & BNXT_FLAG_TPA) {
		rc = bnxt_alloc_one_tpa_info(bp, clone);

With a count of 0, does kzalloc_objs(struct bnxt_tpa_info, 0) return
ZERO_SIZE_PTR and pass the following check?

	if (!rxr->rx_tpa)
		return -ENOMEM;

And in bnxt_alloc_agg_idx(), does the new mask become a no-op when the field
is 0?

	u16 idx = agg_id & (bp->max_tpa_roundup_size - 1);

That would leave idx as large as 4095 for the test_bit()/__set_bit() calls
on the 256 bit agg_idx_bmap and for &rxr->rx_tpa[idx] in bnxt_tpa_start().

BNXT_FLAG_TPA can be set in that state because bnxt_set_rx_skb_mode(), which
bnxt_xdp_set() calls on XDP detach, adds NETIF_F_LRO to hw_features without
a BNXT_SUPPORTS_TPA() check:

		if (rx > 1) {
			bp->flags &= ~BNXT_FLAG_NO_AGG_RINGS;
			bp->dev->hw_features |= NETIF_F_LRO;
		}

Would setting bp->max_tpa_roundup_size next to every bp->max_tpa assignment
keep the two in sync?

> @@ -4554,7 +4559,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)

[Severity: High]

This is a pre-existing issue, but this loop now performs up to twice as many
GFP_KERNEL allocations, and the queue restart caller ignores the -ENOMEM
that follows the quoted check, in bnxt_queue_mem_alloc():

	if (bp->flags & BNXT_FLAG_TPA)
		bnxt_alloc_one_tpa_info_data(bp, clone);

	return 0;

The open path checks the same call, so is the missing check here
intentional?

If the first __bnxt_alloc_rx_frag() fails, the remaining rxr->rx_tpa[i]
entries keep data == NULL and mapping == 0, and bnxt_queue_start() installs
the array into the live ring:

	rxr->rx_tpa = clone->rx_tpa;
	rxr->rx_tpa_idx_map = clone->rx_tpa_idx_map;

Does bnxt_tpa_start() then post a zero DMA address to the device and a NULL
buffer into the software ring for such an entry?

	prod_rx_buf->data = tpa_info->data;
	prod_rx_buf->data_ptr = tpa_info->data_ptr;

	mapping = tpa_info->mapping;
	prod_rx_buf->mapping = mapping;
	...
	prod_bd->rx_bd_haddr = cpu_to_le64(mapping);

When that descriptor completes, bnxt_rx_pkt() uses rx_buf->data without a
NULL check.
-- 
This is an AI-generated review.