Re: [PATCH net v2] net: bgmac: fix oversized RX frame check

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

Critical: 0 · High: 2 · 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] The fix only tightens the upper bound of the RX length check in
  bgmac_dma_rx_read() (drivers/net/ethernet/broadcom/bgmac.c).
- [High] bgmac_dma_rx_read() loses track of the RX descriptor ring when a
  frame spans more than one descriptor, and a remote sender can then…
- [Medium] The commit message rests on the premise that 'a valid frame is
  at most BGMAC_RX_MAX_FRAME_SIZE bytes long' (1536 including FCS).

--

net: bgmac: fix oversized RX frame check

This patch changes the RX length sanity check in bgmac_dma_rx_read(). The
length from the RX header is now compared against BGMAC_RX_MAX_FRAME_SIZE
instead of BGMAC_RX_ALLOC_SIZE. The intent is to drop frames with a
corrupted length instead of hitting skb_over_panic() in the following
skb_put().

[Severity: High]

Should this check also have a lower bound?

len is a u16 read directly from the RX header. A value of 0..3 passes both
the 0xdead/0xbeef poison check and this upper-bound check. Further down in
bgmac_dma_rx_read():

	/* Omit CRC. */
	len -= ETH_FCS_LEN;

	skb = build_skb(buf, BGMAC_RX_ALLOC_SIZE);
	...
	skb_put(skb, BGMAC_RX_FRAME_OFFSET +
		BGMAC_RX_BUF_OFFSET + len);

The subtraction wraps len to 65532..65535. skb_put() then asks for about
64KiB, but build_skb() only provided
SKB_DATA_ALIGN(BGMAC_RX_BUF_SIZE + BGMAC_RX_BUF_OFFSET) bytes of linear
space. skb->tail ends up past skb->end, and skb_over_panic() calls BUG().

Isn't this the same "corrupted length leads to skb_over_panic()" case that
the commit message describes?

Lengths 4..17 also get through. They produce an skb shorter than an
Ethernet header, and eth_type_trans() then reads the header from bytes past
the end of the received data.

Would it work to also reject len < ETH_HLEN + ETH_FCS_LEN (or at least
len < ETH_FCS_LEN) in the same rx_length_errors branch?

[Severity: Medium]

The commit message says:

    so a valid frame is at most BGMAC_RX_MAX_FRAME_SIZE bytes long.

Does this match the limits the driver sets elsewhere?

bgmac_chip_init() programs the MAC with:

	bgmac_umac_write(bgmac, UMAC_MAX_FRAME_LEN, 32 + ETHER_MAX_LEN);

That allows 1550 bytes. bgmac_change_mtu() writes 32 + mtu, which goes up
to 1564. bgmac_enet_probe() also sets:

	/* Omit FCS from max MTU size */
	net_dev->max_mtu = BGMAC_RX_MAX_FRAME_SIZE - ETH_FCS_LEN;

This leaves out ETH_HLEN and any VLAN/DSA tag, so a frame at MTU 1532 is at
least 1550 bytes long.

With this patch, frames of 1537..1564 bytes that the MAC accepts are
dropped here. They are logged as "Found oversized packet ... DMA issue!" and
counted in rx_length_errors. Frames of 1537..1540 bytes, where only FCS bytes
went past the buffer, used to be delivered intact.

bgmac_change_mtu() also never updates net_dev->mtu. The MTU the stack sees
and the hardware limit can therefore drift apart.

Should UMAC_MAX_FRAME_LEN and max_mtu be derived from
BGMAC_RX_MAX_FRAME_SIZE, so the MAC never accepts a frame larger than one RX
buffer can hold?

[Severity: High]

Can a frame longer than one RX buffer put the ring out of sync here?

bgmac_dma_rx_setup_desc() sets up each descriptor with:

	ctl1 |= BGMAC_RX_BUF_SIZE & BGMAC_DESC_CTL1_LEN;

That leaves room for 1536 frame bytes after the RX header. The MAC is
programmed to accept up to 1550 bytes, or 1564 after bgmac_change_mtu().

b43 uses the same Broadcom DMA engine, and its dma_rx() handles this case
explicitly:

	if (unlikely(len + ring->frameoffset > ring->rx_buffersize)) {
		/* The data did not fit into one descriptor buffer
		 * and is split over multiple buffers.

b43 then recycles every slot the frame used, and brcmsmac chains
continuation buffers in a similar way. Only the first descriptor carries the
status header, and continuation data starts at offset 0 of the next buffer.
This split-descriptor behavior is inferred from those sibling drivers, not
from bgmac-specific documentation.

On the oversized path, bgmac_dma_rx_read() frees only the first buffer and
advances ring->start by one. The next iteration reads the continuation
descriptor as a new frame. Its rx->len and rx->flags are then frame bytes
1536..1539, which is payload the sender chooses.

If those bytes decode to a len of 0..3, len -= ETH_FCS_LEN wraps and
skb_put() reaches skb_over_panic(). Other values produce frames built from
stale buffer contents.

With the default UMAC_MAX_FRAME_LEN, can a single 1544..1550 byte frame
from a LAN peer reach this panic?

For the fix to cover this, would it need to skip the continuation
descriptors (as b43 does), keep UMAC_MAX_FRAME_LEN within the buffer
capacity, and reject len < ETH_FCS_LEN?

 				if (net_ratelimit())
 					netdev_err(bgmac->net_dev, "Found oversized packet at slot %d, DMA issue!\n",
 						   ring->start);

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