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

[Simon Horman]

On Thu, Oct 29, 2026 at 09:19:53AM -0700, Florian Fainelli wrote:
> On 10/29/26 08:22, 'Simon Horman' via BCM-KERNEL-FEEDBACK-LIST,PDL wrote:
> > On Thu, Oct 29, 2026 at 01:12:29AM +0000, netdev-bot+sashiko@xxxxxxxxxx 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.
> 
> Yes it is legitimate, the absolute minimal size should be ETH_FCS_LEN, but
> that's basically a zero-sized Ethernet frame after stripping the FCS, not
> very useful. I suppose ETH_HLEN + ETH_FCS_LEN would be more reasonable here.

Yes, agreed.

...