Re: [PATCH net-next v5 10/14] net: ethernet: cortina: Validate RX fragment lengths

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit "net: ethernet: cortina: Validate RX fragment lengths"
  fixes real RX descriptor defects that have been in gmac_rx() since the…
- [Low] The commit message says the patch will "Derive the final fragment
  length from the total frame byte count rather than treating the…

--

net: ethernet: cortina: Validate RX fragment lengths

This patch adds bounds checks on RX descriptor lengths in gmac_rx(). It
rejects a short initial fragment before NET_IP_ALIGN is applied, frame
length underflow on EOF, zero-length non-final descriptors, and fragments
that run past the DMA fragment or the page. An empty EOF descriptor now
completes the skb that was already assembled, and the buffer_size comment
in gemini.h is corrected.

> Assisted-by: LLM
> Signed-off-by: Linus Walleij <linusw@kernel.org>

[Severity: Low]
Should this patch have a Fixes: tag? The defects it closes in gmac_rx()
look like they go back to when the driver was added:

  - frag_len -= NET_IP_ALIGN with no lower bound check
  - frag_len = frame_len - skb->len on EOF with no underflow check, so a
    wrapped length can reach skb_fill_page_desc()
  - a zero-length fragment that is logged but still appended

Other patches in this series that fix bugs of the same age carry this tag.
Examples are "Keep shared free queue parent-owned", "Drain free queue IRQ
before resize" and "Preserve in-flight free queue pages":

Fixes: 4d5ae32f5e1e ("net: ethernet: Add a driver for Gemini gigabit ethernet")

Without the tag, stable backporters may not pick up the underflow fix.

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index e42eeefd4d195..70aa418d70370 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c

[ ... ]

> @@ -1827,15 +1831,33 @@ static unsigned int gmac_rx(struct net_device *netdev, unsigned int budget,
>  			goto err_drop;
>  		}
>  
> -		if (word3.bits32 & EOF_BIT)
> +		if (word3.bits32 & EOF_BIT) {
> +			if (frame_len < ETH_ZLEN ||
> +			    frame_len < skb->len)
> +				goto err_drop;
>  			frag_len = frame_len - skb->len;
> +		}

[Severity: Low]
The commit message says:

  Derive the final fragment length from the total frame byte count
  rather than treating the descriptor buffer size as an EOF payload length.

Does that match the code? Before this patch, gmac_rx() already derived
the EOF fragment length from byte_count:

	if (word3.bits32 & EOF_BIT)
		frag_len = frame_len - skb->len;

In this hunk the assignment is an unchanged context line. The only new
code here is the frame_len < ETH_ZLEN || frame_len < skb->len guard.

Could this sentence be reworded so it doesn't suggest that the old code
used buffer_size as the EOF payload length?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015-gemini-ethernet-fixes-3-v5-0-b5c9e2f7b8a3%40kernel.org