Re: [PATCH net-next v8 4/7] net: bcmgenet: add XDP_TX support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Out-of-bounds array access in `bcmgenet_tx_reclaim` during interface teardown
- [High] Incorrect DMA address calculation for local XDP_TX causes information leak and packet corruption
- [High] Prepending `status_64` block overwrites XDP metadata and causes memory corruption
- [Medium] XDP TX ring size is smaller than standard NAPI budget
--

commit 9add891fa4e7cf62ec26683116338b5877c21662
Author: Nicolai Buchwitz <nb@tipi-net.de>

net: bcmgenet: add XDP_TX support

Implement XDP_TX using ring 16 (DESC_INDEX), the hardware default
descriptor ring, dedicated to XDP TX for isolation from SKB TX queues.

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index 1b60571446e1..3c3b0c44ea8a 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -48,8 +48,10 @@
>  
>  #define GENET_Q0_RX_BD_CNT	\
>  	(TOTAL_DESC - priv->hw_params->rx_queues * priv->hw_params->rx_bds_per_q)
> +#define GENET_Q16_TX_BD_CNT	32

Is it possible to drop packets here during bursts since this TX ring is smaller
than the standard NAPI budget?

The standard RX NAPI budget is 64, and TX completions are reclaimed only once
before the RX loop. If the RX loop processes a full budget of 64 packets and
they all result in an XDP_TX action, the first 32 will fill the TX ring.

The remaining 32 packets would fail in bcmgenet_xdp_xmit_frame() because
ring->free_bds < 1, and be dropped.

[ ... ]
> @@ -2043,6 +2069,9 @@ static void bcmgenet_tx_reclaim_all(struct net_device *dev)
>  	do {
>  		bcmgenet_tx_reclaim(dev, &priv->tx_rings[i++], true);
>  	} while (i <= priv->hw_params->tx_queues && netif_is_multiqueue(dev));
> +
> +	/* Also reclaim XDP TX ring */
> +	bcmgenet_tx_reclaim(dev, &priv->xdp_tx_ring, true);
>  }

Can this cause an out-of-bounds array access during interface teardown?

When bcmgenet_tx_reclaim_all() calls bcmgenet_tx_reclaim() with all == true,
it unconditionally calls netdev_tx_reset_queue(netdev_get_tx_queue(dev, ring->index)).

The XDP TX ring has an index of 16 (DESC_INDEX), but the network device is
allocated with only GENET_MAX_MQ_CNT + 1 TX queues (typically 5). Accessing
index 16 of dev->_tx is out-of-bounds, and netdev_tx_reset_queue() will write
to this invalid memory.

[ ... ]
> @@ -2299,11 +2328,96 @@ static struct sk_buff *bcmgenet_xdp_build_skb(struct bcmgenet_rx_ring *ring,
>  	return skb;
>  }
>  
> +static bool bcmgenet_xdp_xmit_frame(struct bcmgenet_priv *priv,
> +				     struct xdp_frame *xdpf, bool dma_map)
> +{
[ ... ]
> +	if (dma_map) {
[ ... ]
> +	} else {
> +		struct page *page = virt_to_page(xdpf->data);
> +
> +		/* For local XDP_TX the caller already prepended the TSB
> +		 * into xdpf->data/len, so dma_len == xdpf->len.
> +		 */
> +		dma_len = xdpf->len;
> +		mapping = page_pool_get_dma_addr(page) +
> +			  sizeof(*xdpf) + xdpf->headroom;

Does this calculation result in an incorrect DMA address?

The calculation implicitly assumes that xdpf resides exactly at the beginning
of the page. However, the driver configures the RX page pool with
offset = XDP_PACKET_HEADROOM (typically 256 bytes). 

By omitting the distance from the start of the page to xdpf, the calculated
DMA address points XDP_PACKET_HEADROOM bytes before the actual packet data,
which could lead to transmitting uninitialized memory.

> +		dma_sync_single_for_device(kdev, mapping, dma_len,
> +					   DMA_BIDIRECTIONAL);
> +	}
[ ... ]
> +	case XDP_TX:
> +		/* Prepend a zeroed TSB (Transmit Status Block).  The GENET
> +		 * MAC has TBUF_64B_EN set globally, so hardware expects every
> +		 * TX buffer to begin with a 64-byte struct status_64.  Back
> +		 * up xdp->data into the RSB area (which is no longer needed
> +		 * after the BPF program ran) and zero it.
> +		 */
> +		if (xdp->data - xdp->data_hard_start <
> +		    sizeof(struct status_64) + sizeof(struct xdp_frame)) {
> +			page_pool_put_full_page(ring->page_pool, rx_page,
> +						true);
> +			return XDP_DROP;
> +		}
> +		xdp->data -= sizeof(struct status_64);
> +		xdp->data_meta -= sizeof(struct status_64);
> +		memset(xdp->data, 0, sizeof(struct status_64));

Could this overwrite XDP metadata and cause memory corruption?

The space check does not account for metasize (xdp->data - xdp->data_meta).
If the BPF program appended metadata, the memset() here completely overwrites
it.

Furthermore, if metasize is > 0, the shifted xdp->data_meta pointer could
underflow and point exactly to or before xdp->data_hard_start. When
xdp_convert_buff_to_frame() is subsequently called, it writes the xdp_frame
structure at data_hard_start, which would overlap with the shifted data_meta.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260428205846.2625550-1-nb@tipi-net.de?part=4