Re: [PATCH net v2 06/10] net: systemport: Fix potential packet length underflow in bcm_sysport_desc_rx()

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · 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).

- [Medium] The new minimum length in bcm_sysport_desc_rx() does not
  include ETH_HLEN.
- [Low] The commit message for "net: systemport: Fix potential packet
  length underflow in bcm_sysport_desc_rx()" has an unfinished sentence,…

--

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

This patch adds a lower bound check to bcm_sysport_desc_rx() for the
length reported in the RSB. The length must now cover the Receive Status
Block, the 2 byte pad and, when CRC forwarding is enabled, the FCS.
Shorter frames are dropped and counted as rx_length_errors.

> In bcm_sysport_desc_rx(), the packet length 'len' extracted from the RSB
> is only validated against RX_BUF_LENGTH. If a malformed or corrupted
> frame is received with 'len' smaller than the prepended Receive Status
> Block (sizeof(*rsb)) plus 2 padding bytes (and optional FCS).

[Severity: Low]
This isn't a bug, but the second sentence here is unfinished. It doesn't
say what happens in that case.

Is "a malformed or corrupted frame is received" accurate? rbuf_init()
always enables the RSB and 4 byte alignment:

rbuf_init() {
    ...
	reg |= RBUF_4B_ALGN | RBUF_RSB_EN;
    ...
}

So every length the MAC reports already includes the 8 byte RSB and the
2 pad bytes. The driver also never sets CMD_RUNT_FILTER_DIS, so the
UniMAC runt filter stays on.

It looks like this can only happen if the hardware misreports the length,
not because of a frame on the wire.

> Furthermore, subtracting (sizeof(*rsb) + 2) from 'len' (u16) will
> underflow, resulting in corrupted packet stats and potential
> out-of-bounds operations.

Does "potential out-of-bounds operations" overstate the impact before this
patch? When len < 10, skb_pull() goes through skb_pull_inline():

	return unlikely(len > skb->len) ? NULL : __skb_pull(skb, len);

so skb->data doesn't move.

The u16 len then wraps to 65526 or more. That inflates processed_bytes,
priv->dim.bytes, ndev->stats.rx_bytes and stats64->rx_bytes.

skb_trim() does nothing. eth_type_trans() reads the RSB as the Ethernet
header, past skb->tail but still inside the 2048 byte RX buffer. Nothing
seems to write out of bounds.

Could the message describe this as a guard against the hardware
misreporting the length? It could also list the actual consequences:
corrupted byte counters and a garbage runt skb passed up the stack.

> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
> index 9c1b515dc8cc9..ed4337af58da1 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 include ETH_HLEN? As written, a reported len
of 10 to 23 (or 14 to 27 with crc_fwd) passes the check. The rest of
bcm_sysport_desc_rx() then does this:

		skb_put(skb, len);
		...
		skb_pull(skb, sizeof(*rsb) + 2);
		len -= (sizeof(*rsb) + 2);
		...
		if (priv->crc_fwd) {
			skb_trim(skb, len - ETH_FCS_LEN);
		...
		skb->protocol = eth_type_trans(skb, ndev);

After that, skb->len can be anywhere from 0 to 13.

In the call chain eth_type_trans()->eth_skb_pull_mac(), the result of the
pull is ignored:

	struct ethhdr *eth = (struct ethhdr *)skb->data;

	skb_pull_inline(skb, ETH_HLEN);
	return eth;

skb_pull_inline() refuses to pull 14 bytes from the shorter skb, but the
header pointer is still returned. eth_skb_pkt_type() then reads
eth->h_dest, and eth_type_trans() reads eth->h_proto. Both reads are past
skb->tail, in bytes of the RX_BUF_LENGTH buffer that the device didn't
write for this transfer.

Can this set skb->protocol and skb->pkt_type from stale memory? AF_PACKET
listeners see those values as sll_protocol and sll_pkttype.

The runt skb is also counted in rx_packets and rx_bytes and handed to
napi_gro_receive(). Its skb->len is below ETH_HLEN, and skb->data was
never moved past the MAC header.

This is the same kind of hardware misreport the patch guards against. The
eth_type_trans() read past the data is also the only out-of-bounds
operation on this path that the commit message could be referring to.
Would a bound like this close the gap?

	len < sizeof(*rsb) + 2 + ETH_HLEN + (priv->crc_fwd ? ETH_FCS_LEN : 0)

[ ... ]

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