Re: [PATCH net-next v2 08/11] net: ethernet: cortina: Validate RX fragment lengths

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 1 (1)

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).

- [Medium] The new err_length label in gmac_rx() counts a frame as bad
  (rx_errors++, rx_length_errors++) after gmac_skb_if_good_frame() has…
- [Low] The commit "net: ethernet: cortina: Validate RX fragment lengths"
  fixes real defects in gmac_rx()…

Pre-existing issues:
- [Low] This problem wasn't introduced by this patch, but in
  drivers/net/ethernet/cortina/gemini.h the comment on `union…

--

net: ethernet: cortina: Validate RX fragment lengths

This patch adds bounds checks on RX descriptor lengths in gmac_rx(). It
rejects a short first fragment, an EOF frame length underflow, fragments
that run past the DMA fragment or the page, and zero-length fragments.
These drops are counted as rx_errors and rx_length_errors.

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index d677d7431ab2..258bb44d5570 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c

[ ... ]

> @@ -1667,6 +1668,9 @@ static unsigned int gmac_rx(struct net_device *netdev, unsigned int budget,
>  			if (!skb)
>  				goto err_drop;
>  
> +			if (frag_len < NET_IP_ALIGN)
> +				goto err_length;
> +
>  			page_offs += NET_IP_ALIGN;
>  			frag_len -= NET_IP_ALIGN;

[Severity: Low]
This isn't a bug introduced by this patch, but the comment on
buffer_size in union gmac_rxdesc_0 in gemini.h looks like it was copied
from the field next to it:

drivers/net/ethernet/cortina/gemini.h:
		/* bit 15:0 number of descriptors used for the current frame */
		unsigned int buffer_size:16;
		/* bit 21:16 number of descriptors used for the current frame */
		unsigned int desc_count:6;

gmac_rx() treats word0.bits.buffer_size as a byte length. It subtracts
NET_IP_ALIGN from it and, with this patch, compares it against the
freeq fragment size and PAGE_SIZE.

Could the comment be changed to describe a per-buffer byte count? This
doesn't affect the new bounds arithmetic.

[ ... ]

> -		if (word3.bits32 & EOF_BIT)
> +		if (word3.bits32 & EOF_BIT) {
> +			if (frame_len < skb->len)
> +				goto err_length;
>  			frag_len = frame_len - skb->len;
> +		}

[Severity: Low]
Should this commit have a Fixes: tag?

Without this check, frame_len < skb->len at EOF wraps frag_len to a huge
unsigned value. That value then goes into skb_fill_page_desc(),
skb->len and skb->data_len.

The EOF computation and the unchecked skb_fill_page_desc() and skb->len
updates date back to the original driver:

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

Earlier RX fixes to gmac_rx() carry that tag, for example 6520198c430c
("net: ethernet: cortina: Count RX drops once per frame").

The commit uses the Assisted-by workflow, and step 6 of
Documentation/process/coding-assistants.rst also asks for a Fixes tag on
such fixes. The trailers here only have Assisted-by and Signed-off-by.

[ ... ]

> @@ -1698,6 +1713,11 @@ static unsigned int gmac_rx(struct net_device *netdev, unsigned int budget,
>  		}
>  		goto next_desc;
>  
> +err_length:
> +		if (!dropping) {
> +			port->stats.rx_errors++;
> +			port->stats.rx_length_errors++;
> +		}

[Severity: Medium]
Does this count the same frame as both a good packet and a bad packet?

Each new goto err_length runs after gmac_skb_if_good_frame() has already
returned an skb for the SOF descriptor. By then that function has done:

drivers/net/ethernet/cortina/gemini.c:gmac_skb_if_good_frame() {
    ...
update_exit:
	port->stats.rx_bytes += frame_len;
	port->stats.rx_packets++;
	return skb;
}

So when a frame hits any of the new checks, it gets rx_packets and
rx_bytes from the SOF path, then rx_errors and rx_length_errors here,
then rx_dropped in err_drop. gmac_get_stats64() then reports it in both
groups.

In include/uapi/linux/if_link.h, rx_packets counts good packets and
rx_errors counts bad ones. Before this patch the two never overlapped,
because gmac_skb_if_good_frame() rejects error frames before it touches
rx_packets.

The same header defines rx_length_errors for 802.3 in terms of in-range
length, out-of-range length field and frame-too-long errors. An
inconsistent DMA descriptor length doesn't seem to fit any of those.

Would it be better either to undo the good-packet accounting here, or
to count these drops only in rx_dropped?

>  err_drop:
>  		if (skb) {
>  			napi_free_frags(&port->napi);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928-gemini-ethernet-fixes-3-v2-0-758a795d7a78%40kernel.org