Re: [PATCH net v3] net: bgmac: fix RX frame length check

[netdev-bot+sashiko@kernel.org]

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

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

- [High] When bgmac_dma_rx_read() rejects an oversized frame, it only
  drops the current slot.
- [Medium] The new upper bound in bgmac_dma_rx_read()
  (drivers/net/ethernet/broadcom/bgmac.c:463-464, `len >…

--

net: bgmac: fix RX frame length check

This patch tightens the RX length check in bgmac_dma_rx_read(). It now
compares against BGMAC_RX_MAX_FRAME_SIZE instead of BGMAC_RX_ALLOC_SIZE,
adds a lower bound of ETH_HLEN + ETH_FCS_LEN, and prints the bad length.

> diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
> index 669eb7ce7d40b..b5e1d3c0003d6 100644
> --- a/drivers/net/ethernet/broadcom/bgmac.c
> +++ b/drivers/net/ethernet/broadcom/bgmac.c
> @@ -460,10 +460,11 @@ static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring,
>  				break;
>  			}
>  
> -			if (len > BGMAC_RX_ALLOC_SIZE) {
> +			if (len < ETH_HLEN + ETH_FCS_LEN ||
> +			    len > BGMAC_RX_MAX_FRAME_SIZE) {

[Severity: Medium]
This caps the whole L2 frame at 1536 bytes, Ethernet header and FCS
included. Do the other limits in the driver still agree with that?

bgmac_enet_probe() still advertises:

	net_dev->max_mtu = BGMAC_RX_MAX_FRAME_SIZE - ETH_FCS_LEN;

That allows an MTU of 1532 and does not account for ETH_HLEN or VLAN/DSA
tags.

bgmac_chip_init() programs the MAC with this, which is 1550:

	bgmac_umac_write(bgmac, UMAC_MAX_FRAME_LEN, 32 + ETHER_MAX_LEN);

bgmac_change_mtu() programs 32 + mtu, which can be up to 1564.

So the MAC can accept and DMA frames of 1537 to 1564 bytes, or 1537 to
1550 bytes with the default settings. Won't the new check always drop
these frames? Each drop bumps rx_length_errors and rx_errors and logs
"DMA issue", even though the MAC was configured to accept the frame.

With an MTU between 1519 and 1522, frames of 1537 to 1540 bytes used to
be delivered intact. Only FCS bytes spilled past frame byte 1536, and the
FCS is stripped. These frames are now always dropped, and the commit
message doesn't mention this change in behaviour.

The commit message also says:

    A corrupted length between those two limits passes the check, and the
    following skb_put() runs past the skb tailroom and calls
    skb_over_panic() instead of dropping the frame.

Is that accurate for the whole range? Lengths up to about 1570 (32 byte
cache lines) or about 1602 (64 byte cache lines) seem to fit in the
SKB_DATA_ALIGN tailroom. Those frames were delivered, not panicked on.

Should max_mtu and UMAC_MAX_FRAME_LEN be brought in line with how many
frame bytes an RX buffer can hold, as part of this fix?

>  				if (net_ratelimit())
> -					netdev_err(bgmac->net_dev, "Found oversized packet at slot %d, DMA issue!\n",
> -						   ring->start);
> +					netdev_err(bgmac->net_dev, "Found packet with invalid length %u at slot %d, DMA issue!\n",
> +						   len, ring->start);
>  				put_page(virt_to_head_page(buf));
>  				bgmac->net_dev->stats.rx_length_errors++;
>  				bgmac->net_dev->stats.rx_errors++;

[Severity: High]
When an oversized frame is rejected here, only the current slot is
dropped. What happens to the continuation descriptors that hold the rest
of that frame?

bgmac_dma_rx_enable() sets:

	ctl |= BGMAC_DMA_RX_OVERFLOW_CONT;

As noted above, the MAC accepts frames of up to 1550 bytes by default.
Each buffer only holds 1536 frame bytes after BGMAC_RX_FRAME_OFFSET.

Other drivers for the same Broadcom DMA engine show that such frames
spill into the next descriptor, with the rest written from offset 0.
brcmsmac's dma_rx() computes resid = len - (rxbufsize - rxoffset) and
walks the extra buffers. b43's dma_rx() recycles the continuation
descriptors:

	/* The data did not fit into one descriptor buffer
	 * and is split over multiple buffers.
	 ...

After the break, ring->start moves to the next slot. The loop then reads
rx->len and rx->flags from offset 0 of that buffer. Those would be bytes
1536 to 1539 of the oversized frame, which the remote sender controls.

Any len from 18 to 1536 passes the new check, and flags won't be 0xbeef.
The code then calls build_skb(), skb_put(), eth_type_trans() and
napi_gro_receive() with up to 1532 bytes.

For a 1550 byte frame the hardware writes only 14 bytes into that second
buffer. All of them land before the skb data start at offset 30, and
bgmac_dma_rx_skb_for_slot() poisons only the 4 byte rx header.

Wouldn't this pass stale netdev_alloc_frag() memory, such as old packet
data or other freed kernel memory, up the stack as a received packet?

The commit message says the change stops eth_type_trans() from parsing
"bytes the hardware never wrote". This path still seems reachable.

Before this patch, the same continuation slot could also reach the
skb_over_panic() the commit message describes. The new bounds remove the
panic, but the stale memory is still passed up.

Could the continuation descriptors be consumed the way b43 does, or
SOF/EOF be checked? Another option is to cap UMAC_MAX_FRAME_LEN and
max_mtu so that no frame can exceed BGMAC_RX_MAX_FRAME_SIZE.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261029173122.29636-1-rosenp%40gmail.com