Re: [PATCH net 07/12] net: systemport: Fix potential packet length underflow in bcm_sysport_desc_rx()

[netdev-bot+sashiko]

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

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

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).

- [Medium] The commit message mischaracterizes the failure mode it claims
  to fix.
- [Medium] The new minimum-length test in bcm_sysport_desc_rx()
  (drivers/net/ethernet/broadcom/bcmsysport.c:784-785) only requires…

--

net: systemport: Fix potential packet length underflow in bcm_sysport_desc_rx()

The patch adds a lower bound to the RSB-reported packet length check in
bcm_sysport_desc_rx(), requiring len to be at least sizeof(*rsb) + 2,
plus ETH_FCS_LEN when CRC forwarding is enabled. Frames failing the new
check are counted as length errors and dropped.

> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
> index b69833d34bd5b..95cead1df1607 100644
> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
> @@ -781,8 +781,9 @@ static unsigned int bcm_sysport_desc_rx(struct bcm_sysport_priv *priv,
>  			  p_index, priv->rx_c_index, priv->rx_read_ptr,
>  			  len, status);
>  
> -		if (unlikely(len > RX_BUF_LENGTH)) {
> -			netif_err(priv, rx_status, ndev, "oversized packet\n");
> +		if (unlikely(len > RX_BUF_LENGTH ||
> +			     len < sizeof(*rsb) + 2 + (priv->crc_fwd ? ETH_FCS_LEN : 0))) {

[Severity: Medium]
Should this lower bound also reserve room for the Ethernet header that the
same function consumes a few lines later?

With crc_fwd clear, any len in the range 10..23 passes the new test. The
code that follows then does:

drivers/net/ethernet/broadcom/bcmsysport.c:bcm_sysport_desc_rx() {
	...
	skb_put(skb, len);
	...
	skb_pull(skb, sizeof(*rsb) + 2);
	len -= (sizeof(*rsb) + 2);
	processed_bytes += len;

	/* UniMAC may forward CRC */
	if (priv->crc_fwd) {
		skb_trim(skb, len - ETH_FCS_LEN);
		len -= ETH_FCS_LEN;
	}

	skb->protocol = eth_type_trans(skb, ndev);
	...
}

For len == 12 the skb reaching eth_type_trans() has skb->len == 2. With
crc_fwd set and len == 14, skb_trim(skb, 0) leaves a zero-length skb.

In that case eth_type_trans() reads the MAC header without checking that
ETH_HLEN bytes are present:

net/ethernet/eth.c:eth_type_trans() {
	...
	eth = eth_skb_pull_mac(skb);
	eth_skb_pkt_type(skb, dev);
	...
}

include/linux/etherdevice.h:eth_skb_pull_mac() {
	struct ethhdr *eth = (struct ethhdr *)skb->data;

	skb_pull_inline(skb, ETH_HLEN);
	return eth;
}

skb_pull_inline() returns NULL for skb->len < ETH_HLEN and leaves
skb->data untouched, but the return value is discarded, so eth->h_dest
and eth->h_proto are read past skb->len. Those bytes are inside the
2048-byte RX_BUF_LENGTH allocation from __netdev_alloc_skb() in
bcm_sysport_rx_refill(), which is not zeroed and which DMA only filled
with len bytes.

Can this make skb->pkt_type and skb->protocol derive from stale buffer
contents, and leak the 2-byte protocol value to userspace as sll_protocol
for AF_PACKET SOCK_RAW readers in packet_recvmsg()?

The MAC header is also never stripped in this case, yet the frame is
counted in ndev->stats.rx_packets/rx_bytes and stats64 as a good
reception and handed to napi_gro_receive() with mac/network offsets that
upper layers will parse at the wrong offset.

Would sizeof(*rsb) + 2 + ETH_HLEN (or an ETH_ZLEN based bound), plus
ETH_FCS_LEN when crc_fwd is set, be the bound that actually covers the
parsing done in this function? The same incomplete pattern exists in
bcmgenet (len < GENET_RSB_PAD), so this may be intentional copy-over, but
nothing here establishes that sub-ETH_HLEN lengths cannot be reported.

> +			netif_err(priv, rx_status, ndev, "invalid packet size: %d\n", len);
>  			ndev->stats.rx_length_errors++;
>  			ndev->stats.rx_errors++;
>  			dev_kfree_skb_any(skb);

[Severity: Medium]
Is the failure mode described in the commit message accurate? The message
says:

    calling skb_pull() will trigger a BUG() in __skb_pull() when pulling
    beyond skb->len

skb_pull() in net/core/skbuff.c is a wrapper around skb_pull_inline():

include/linux/skbuff.h:skb_pull_inline() {
	return unlikely(len > skb->len) ? NULL : __skb_pull(skb, len);
}

So an over-long pull returns NULL without touching the skb and never
reaches __skb_pull(). The BUG() inside __skb_pull() is conditional:

include/linux/skbuff.h:__skb_pull() {
	skb->len -= len;
	if (unlikely(skb->len < skb->data_len)) {
		...
		BUG();
	}
	...
}

These RX skbs come from __netdev_alloc_skb(RX_BUF_LENGTH) in
bcm_sysport_rx_refill() and are purely linear, so skb->data_len is 0 and
that condition is unreachable.

The message also mentions "potential out-of-bounds operations". The
reachable pre-patch effects look like the u16 wrap in

	len -= (sizeof(*rsb) + 2);

which corrupts ndev->stats.rx_bytes, stats64->rx_bytes and
priv->dim.bytes (the latter feeding the DIM coalescing logic), plus an
skb passed to eth_type_trans()/napi_gro_receive() with the Receive Status
Block and 2 pad bytes still present because the ignored skb_pull() return
meant nothing was stripped.

Since this carries a Fixes: tag and will be considered for stable, could
the changelog describe the stat corruption and unstripped RSB instead of
a BUG() and out-of-bounds accesses?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921231305.394773-1-florian.fainelli%40broadcom.com