Re: [PATCH net-next v2 7/7] net: bcmgenet: reassemble jumbo frames from status block fragments

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 2 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] Use-after-free read of the last status block in
  bcmgenet_desc_rx() when CRC forwarding is on.
- [Medium] TX descriptor length can overflow the 12-bit length field once
  the MTU goes past about 4 KB.
- [Medium] Short SOP descriptor followed by continuation fragments can
  reach BUG() in eth_type_trans().
- [Low] The comment on bcmgenet_pkt_rdy_thld() is now stale.

--

net: bcmgenet: reassemble jumbo frames from status block fragments

The hardware splits a frame longer than the packet ready threshold across
several descriptors, each with its own status block. This patch reassembles
those descriptors into one skb by attaching the later ones as page frags.
It also raises max_mtu on v2+ to ENET_MAX_JUMBO_MTU, which is derived from
the 14-bit UMAC_MAX_FRAME_LEN.

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index e8f86374c7cd8..faa13f12ce7e6 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -84,11 +84,8 @@
>  				      ENET_THLD_MAX * ENET_THLD_UNIT, \
>  				      ENET_THLD_PAGE_LEN)
>  
> -/* Largest MTU that fits one descriptor, with room for a VLAN tag so a VLAN
> - * interface can use the parent MTU.
> - */
> -#define ENET_MAX_MTU		(ENET_THLD_MAX_LEN - GENET_RBUF_ALIGN - \
> -				 ETH_HLEN - VLAN_HLEN)
> +/* UMAC_MAX_FRAME_LEN is 14 bits wide and counts the FCS */
> +#define ENET_MAX_JUMBO_MTU	(GENMASK(13, 0) - ENET_FRAME_OVERHEAD)

[Severity: Medium]
This limit comes from the UMAC frame length only. Once bcmgenet_probe()
sets max_mtu to it, the TX path can see MTUs up to 16347. Can the TX
descriptor length field hold buffers that large?

bcmgenet_xmit() still maps the linear head and each page frag as a single
descriptor. It writes the size into len_stat with no mask, cap or split:

bcmgenet_xmit() {
    ...
		len_stat = (size << DMA_BUFLENGTH_SHIFT) |
			   (priv->hw_params->qtag_mask << DMA_TX_QTAG_SHIFT);
    ...
}

The only in-tree description of that field says it is 12 bits wide:

bcmgenet.h:
/* Tx/Rx Dma Descriptor common bits*/
#define DMA_BUFLENGTH_MASK		0x0fff
#define DMA_BUFLENGTH_SHIFT		16

At jumbo MTU, skb_headlen() can approach 16 KB, plus the 64-byte TSB that
bcmgenet_add_tsb() adds. TCP frags of around 9 KB are also common.

If the field really is 12 bits, would the length wrap modulo 4096, with the
upper bits spilling into bits 31:28? Would that truncate or corrupt the
frame on the wire? Before this patch the largest TX buffer was about 3.9 KB.

Separately, bcmgenet_init_tx_ring() still sets DMA_RING_BUF_SIZE from
priv->rx_buf_len, which is at most about 3.9 KB:

bcmgenet_init_tx_ring() {
    ...
	bcmgenet_tdma_ring_writel(priv, index,
				  ((size << DMA_RING_SIZE_SHIFT) |
				   priv->rx_buf_len), DMA_RING_BUF_SIZE);
    ...
}

Is that setting still valid now that one TX descriptor can be several times
larger? Could the commit message say whether the TX descriptor length and
the TX buffer size limits allow more than 4095 bytes?

[ ... ]

> @@ -2333,6 +2330,54 @@ static int bcmgenet_rx_refill(struct bcmgenet_rx_ring *ring,
>  	return 0;
>  }
>  

[ ... ]

> +/* A frame longer than the threshold arrives in several descriptors, each with
> + * its own status block. Only the first one carries a header, so hand the page
> + * of every later one to the frame already being collected. Returns the frame
> + * once EOP is in, NULL while more descriptors are expected or once the frame
> + * had to be dropped.
> + */

[Severity: Low]
This isn't a bug, but the comment above bcmgenet_pkt_rdy_thld() now looks
stale:

/* Receive threshold in register units. Covers the alignment bytes and the
 * frame, but not the status block, which the hardware adds on top.
 */

With max_mtu raised to ENET_MAX_JUMBO_MTU, any MTU above about 3.5-3.8 KB
is clamped to ENET_THLD_MAX_LEN / ENET_THLD_UNIT. The threshold then no
longer covers the frame, which contradicts the comment here. Should that
comment be updated?

[ ... ]

> @@ -2490,8 +2556,18 @@ static unsigned int bcmgenet_desc_rx(struct bcmgenet_rx_ring *ring,
>  		skb_reserve(skb, GENET_RSB_PAD);
>  		__skb_put(skb, len - GENET_RSB_PAD);
>  
> -		if (priv->crc_fwd_en) {
> -			skb_trim(skb, skb->len - ETH_FCS_LEN);
> +		if (unlikely(!(dma_flag & DMA_EOP))) {
> +			ring->frag_head = skb;
> +			goto next;
> +		}

[Severity: Medium]
Could a short SOP head lead to BUG() in eth_type_trans()?

For an SOP descriptor the only length check is len >= GENET_RSB_PAD (66). A
descriptor with SOP and no EOP that carries fewer than ETH_HLEN frame bytes
would still become ring->frag_head. Later descriptors are then added as
frags.

Nothing calls pskb_may_pull() before this line after deliver:

		skb->protocol = eth_type_trans(skb, priv->dev);

If the linear part has fewer than 14 bytes and frags are present,
__skb_pull() would see skb->len < skb->data_len and call BUG().

bcmgenet_pkt_rdy_thld() keeps the threshold at ENET_THLD_DEFAULT (2048
bytes) or more. Correct hardware should therefore always fill an SOP
descriptor without EOP to around 2 KB. Reaching this would take a
malformed status block from the MAC, so this is hardening rather than a
path reachable from the network.

Would it be worth checking that an SOP descriptor without EOP holds at
least ETH_HLEN frame bytes?

> +
> +deliver:
> +
> +		if (priv->crc_fwd_en &&
> +		    unlikely(pskb_trim(skb, skb->len - ETH_FCS_LEN))) {
> +			BCMGENET_STATS64_INC(stats, dropped);
> +			dev_kfree_skb_any(skb);
> +			goto next;
>  		}
>  
>  		/* Set up checksum offload */

[Severity: High]
Can this read the status block from a page that has already been freed?

For a reassembled frame, status still points to page_address(rx_page) +
rx_offset of the EOP descriptor. bcmgenet_add_frag() has already given that
page to the skb as its last frag.

When crc_fwd_en is set, pskb_trim() on this non-linear skb goes through
___pskb_trim(). That can release the page in two ways.

First, if the EOP descriptor carries 4 or fewer frame bytes, the last frag
is released with skb_frag_unref(). A non-SOP len of 64-68 passes the
min_len check.

Second, skb->sk is NULL, so skb_condense() runs. It may pull the remaining
frag data into the head and free every frag.

The page then goes back through napi_pp_put_page(). If it cannot be
recycled (pfmemalloc, remote NUMA node, or full cache and ring), it can be
returned to the page allocator via page_pool_return_netmem().

The very next statements read the status block:

		/* Set up checksum offload */
		if (dev->features & NETIF_F_RXCSUM) {
			rx_csum = (__force __be16)(status->rx_csum & 0xffff);

The value read there is then used as CHECKSUM_COMPLETE. Before this patch
the skb was always linear and status pointed into the head buffer, which
skb_trim() never frees.

Should rx_csum be read before pskb_trim() is called?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261006-nb-genet-mtu-nn-v2-v2-0-c4be21151912%40tipi-net.de