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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 0 · Low: 2

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] net: gmac_rx() drops entire valid packet on zero-length EOF
  fragment
- [Low] The commit describes itself as a bug fix and has no Fixes: tag.
- [Low] The patch makes RX word0.bits.buffer_size a strict byte-length
  bound on every EOF descriptor.

--

net: ethernet: cortina: Validate RX fragment lengths

This patch adds bounds checks on RX descriptor lengths in gmac_rx(). It
rejects short initial fragments, frame length underflow and fragments that
extend past the DMA fragment or the page. Zero-length fragments are now
dropped instead of being appended to the skb.

> Reject a short initial fragment before applying NET_IP_ALIGN, reject frame
> length underflow and ranges extending beyond either the DMA fragment or the
> page, and drop zero-length fragments instead of adding them to the skb.

[Severity: Low]
Should this patch carry a Fixes: tag?

Three of the issues addressed here appear to date back to the original
driver:

  - the unchecked frag_len -= NET_IP_ALIGN
  - the unsigned underflow in frag_len = frame_len - skb->len
  - zero-length fragments that are logged but still appended

That would make the tag:

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

Other patches in the same series, such as "Keep shared free queue
parent-owned" and "Correct free queue DMA mappings", already carry this
tag.

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index 943c31c026743..d8bafcdf4d5f1 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c

[ ... ]

> @@ -1811,6 +1812,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_drop;
> +
>  			page_offs += NET_IP_ALIGN;
>  			frag_len -= NET_IP_ALIGN;
>  			frag_nr = 0;
> @@ -1819,15 +1823,28 @@ 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 ||
> +			    frame_len - skb->len > frag_len)
> +				goto err_drop;
>  			frag_len = frame_len - skb->len;
> +		}

[Severity: High]
Does this drop more than the commit message says it does?

The message says zero-length fragments are dropped instead of being
added to the skb. The goto err_drop does not skip just that fragment.
It calls napi_free_frags() on the whole skb assembled so far, puts the
page and increments rx_dropped.

Consider a multi-descriptor frame where the earlier descriptors
already carry all of the data. One example is a frame that exactly
fills the first free queue fragment after the NET_IP_ALIGN offset,
which is 2046 bytes with the default 2 KiB fragment.

If the hardware then closes the chain with an EOF descriptor that
carries no new data, frame_len equals skb->len. The new EOF checks
all pass and frag_len becomes frame_len - skb->len == 0. gmac_rx()
then lands in the !frag_len branch.

Before this patch that frame was handed to napi_gro_frags() with an
empty trailing fragment. After it, a complete and valid frame is
discarded and counted as rx_dropped. Nothing later in the series
seems to change this.

Can the Gemini DMA engine emit such a trailing descriptor at the end
of a chain?

If it can, the EOF case with nothing left to append could be handled
separately: put_page() the unused fragment and still pass the skb to
napi_gro_frags(). err_drop would then be kept for zero-length
descriptors in the middle of a chain. Note that the
frag_nr == MAX_SKB_FRAGS check runs first, so an empty EOF arriving
after a full frags array would also discard the frame unless that
case is handled before it.

[Severity: Low]
Before this patch, gmac_rx() never used word0.bits.buffer_size on an EOF
descriptor, because frag_len was always overwritten with
frame_len - skb->len. Now buffer_size on every EOF descriptor is a strict
byte bound on whether the frame gets delivered.

For a single-descriptor (SOF|EOF) frame, the NET_IP_ALIGN subtraction plus
this check now require:

    buffer_size >= frame_len + NET_IP_ALIGN

What guarantees that the hardware reports buffer_size this way? The driver
never writes buffer_size into free queue entries. geth_freeq_post_page()
and geth_setup_freeq() only set word2.buf_adr, so the hardware supplies the
value.

The only in-tree definition, in gemini.h, describes the field as a
descriptor count:

union gmac_rxdesc_0 {
	...
		/* 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;

That comment looks copied from desc_count. It doesn't say whether the value
includes the bytes inserted via rd_insert_bytes = NET_IP_ALIGN in
gmac_start_dma(). It also doesn't say how the field is filled on an EOF
descriptor that is not the SOF.

If buffer_size excluded the inserted bytes, wouldn't every
single-descriptor frame be dropped?

There is a second case with an MTU above 1530, where frames span multiple
descriptors. If buffer_size is filled differently on the last descriptor,
could good jumbo frames be silently counted as rx_dropped?

The DIR-685 testing at standard MTU suggests the SOF|EOF case works. The
commit message also says:

  "validated against the Gemini receive descriptor definitions"

The in-tree definitions don't seem to support that. Could the buffer_size
comment in gemini.h be corrected to describe the byte-length semantics this
patch now relies on, including whether the NET_IP_ALIGN bytes are counted?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261011-gemini-ethernet-fixes-3-v4-0-4b1631b08f5d%40kernel.org