Re: [PATCH net-next v9 3/7] net: bcmgenet: add basic XDP support (PASS/DROP)

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unconditionally applying CHECKSUM_COMPLETE using the original hardware checksum after an XDP program has executed.
- [High] Calculation of pkt_len can underflow when receiving short frames with crc_fwd_en enabled, leading to massive out-of-bounds SKB lengths.
--

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

net: bcmgenet: add basic XDP support (PASS/DROP)

This commit adds basic XDP program attachment via ndo_bpf and executes
XDP programs in the RX path. It handles building SKBs from the xdp_buff
on XDP_PASS and returning pages to the page_pool on XDP_DROP.

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index 5bedc18685b09..ee1d4ecc2b874 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c

[ ... ]

> @@ -2413,30 +2471,34 @@ static unsigned int bcmgenet_desc_rx(struct bcmgenet_rx_ring *ring,
>  			goto next;
>  		} /* error packet */
>  
> -		/* Build SKB from the page - data starts at hard_start,
> -		 * frame begins after RSB(64) + pad(2) = 66 bytes.
> -		 */
> -		skb = napi_build_skb(hard_start, PAGE_SIZE - XDP_PACKET_HEADROOM);
> -		if (unlikely(!skb)) {
> -			BCMGENET_STATS64_INC(stats, dropped);
> -			page_pool_put_full_page(ring->page_pool, rx_page,
> -						true);
> -			goto next;
> -		}
> +		pkt_len = len - GENET_RSB_PAD;
> +		if (priv->crc_fwd_en)
> +			pkt_len -= ETH_FCS_LEN;

Can pkt_len underflow here if we receive a short frame?

The RX validation earlier in bcmgenet_desc_rx() only ensures that
len >= GENET_RSB_PAD. If len is less than GENET_RSB_PAD + 4 (ETH_FCS_LEN),
subtracting ETH_FCS_LEN will result in a negative pkt_len.

When this negative pkt_len is passed to xdp_prepare_buff(), xdp->data_end
will be set before xdp->data. Later, bcmgenet_xdp_build_skb() calls
__skb_put(skb, xdp->data_end - xdp->data), which lacks bounds checking and
converts the negative difference into a very large unsigned integer, causing
an out-of-bounds SKB length.

> -		skb_mark_for_recycle(skb);
> +		/* Save rx_csum before XDP runs - an XDP program
> +		 * could overwrite the RSB via bpf_xdp_adjust_head.
> +		 */
> +		if (dev->features & NETIF_F_RXCSUM)
> +			rx_csum = (__force __be16)(status->rx_csum & 0xffff);
>  
> -		/* Reserve the RSB + pad, then set the data length */
> -		skb_reserve(skb, GENET_RSB_PAD);
> -		__skb_put(skb, len - GENET_RSB_PAD);
> +		xdp_init_buff(&xdp, PAGE_SIZE, &ring->xdp_rxq);
> +		xdp_prepare_buff(&xdp, page_address(rx_page),
> +				 GENET_RX_HEADROOM, pkt_len, true);
>  
> -		if (priv->crc_fwd_en) {
> -			skb_trim(skb, skb->len - ETH_FCS_LEN);
> +		xdp_act = bcmgenet_run_xdp(ring, xdp_prog, &xdp, rx_page);
> +		if (xdp_act != XDP_PASS)
> +			goto next;
>  
> -		}
>  
> -		/* Set up checksum offload */
> -		if (dev->features & NETIF_F_RXCSUM) {
> -			rx_csum = (__force __be16)(status->rx_csum & 0xffff);
> +		skb = bcmgenet_xdp_build_skb(ring, &xdp);
> +		if (unlikely(!skb)) {
> +			BCMGENET_STATS64_INC(stats, dropped);
> +			page_pool_put_full_page(ring->page_pool,
> +						rx_page, true);
> +			goto next;
> +		}
> +
> +		/* Set up checksum offload */
> +		if (dev->features & NETIF_F_RXCSUM) {
>  			if (rx_csum) {
>  				skb->csum = (__force __wsum)ntohs(rx_csum);
>  				skb->ip_summed = CHECKSUM_COMPLETE;

Is it safe to unconditionally apply CHECKSUM_COMPLETE using the original
hardware checksum after an XDP program has executed?

Since XDP programs are allowed to modify the packet headers and payload,
if the program modifies the packet and returns XDP_PASS, the hardware-computed
1's complement sum saved in rx_csum will no longer match the packet contents.

This could lead to the network stack detecting a checksum mismatch and
incorrectly dropping a valid packet. Should the checksum status be cleared
or set to CHECKSUM_NONE if an XDP program is present?

>  			}
>  		}

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