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

[Simon Horman <horms@kernel.org>]

On Thu, Oct 29, 2026 at 01:12:29AM +0000, netdev-bot+sashiko@kernel.org wrote:
> 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().
> 
> > diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
> > index b3a92ae95de30..6873c7bb7a141 100644
> > --- a/drivers/net/ethernet/broadcom/bgmac.c
> > +++ b/drivers/net/ethernet/broadcom/bgmac.c
> > @@ -455,7 +455,7 @@ static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring,
> >  				break;
> >  			}
> >  
> > -			if (len > BGMAC_RX_ALLOC_SIZE) {
> > +			if (len > BGMAC_RX_MAX_FRAME_SIZE) {
> 
> [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?

The problem above does seem legit to me.

> 
> [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?

But the one above seem speculative.

-- 
pw-bot: changes-requested