Re: [PATCH net v2] net: stmmac: fix rx Scatter-Gather support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] stmmac: FCS stripping check corrupts payload on legacy cores
- [High] net: stmmac: skb_trim() on non-linear SKB triggers WARN_ON and fails
--

commit a400f52631710a5add56c0a39b6ab0128eae6a53
Author: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>

net: stmmac: fix rx Scatter-Gather support

This commit addresses SMMU translation faults on oversized incoming frames by
allocating and enabling a secondary RX buffer (buffer2) for non-SPH
scatter-gather frames on supported cores. Legacy cores keep buffer2 disabled,
and the FCS stripping logic is adjusted to handle both linear and assembled
frames correctly.

Link: https://lore.kernel.org/r/20260916-stmmac-rx-sg-fix-v1-1-b49b7b8f725f@oss.qualcomm.com

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

[ ... ]

> @@ -5807,24 +5812,31 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
>  		len += buf2_len;
>  
>  		/* ACS is disabled; strip manually. */
> -		if (likely(!(status & rx_not_ls))) {
> -			if (buf2_len) {
> -				buf2_len -= ETH_FCS_LEN;
> -				len -= ETH_FCS_LEN;
> -			} else if (buf1_len) {
> -				buf1_len -= ETH_FCS_LEN;
> -				len -= ETH_FCS_LEN;
> -			}
> -		}
> +		if (likely(!(status & rx_not_ls)))
> +			len -= ETH_FCS_LEN;
>  
>  		if (!skb) {
>  			unsigned int pre_len, sync_len;
>  
> +			/* Each frame starts here: reset the FCS handling */
> +			fcs_stripped = false;
> +
>  			dma_sync_single_for_cpu(priv->device, buf->addr,
>  						buf1_len, dma_dir);
>  			net_prefetch(page_address(buf->page) +
>  				     buf->page_offset);
>  
> +			/* The XDP program must not see the FCS. This only
> +			 * applies to a single-buffer frame (buf2_len == 0),
> +			 * where the whole frame and its FCS sit in buffer1;
> +			 * otherwise the FCS is stripped from the assembled
> +			 * frame with skb_trim().
> +			 */
> +			if (stmmac_xdp_is_enabled(priv) && !buf2_len) {
> +				buf1_len -= ETH_FCS_LEN;
> +				fcs_stripped = true;
> +			}

[Severity: High]
Does this logic incorrectly strip the payload on legacy cores for
scatter-gather frames?

On legacy hardware without set_sec_addr support, stmmac_rx_check_buf2_cap()
returns false, meaning buf2_len is always 0. When a multi-descriptor (jumbo)
frame arrives on a legacy core with XDP enabled, the first descriptor enters
this if (!skb) block. Since buf2_len is 0, this check evaluates to true,
improperly subtracting ETH_FCS_LEN (4 bytes) from buf1_len and truncating the
payload. It also sets fcs_stripped = true, which means the actual FCS at the
end of the frame won't be trimmed later, leaving a corrupted frame.

[Severity: Critical]
This is a pre-existing issue, but can this underflow buf1_len and cause an
out-of-bounds access in the BPF program?

If a multi-descriptor frame is received and the XDP program drops the first
fragment, the driver frees the buffer, sets skb = NULL, and loops to read
the next descriptor via goto read_again. On the final descriptor,
stmmac_rx_buf2_len() returns 0. Since skb is NULL, the code re-enters this
if (!skb) block.

This check evaluates to true, executing buf1_len -= ETH_FCS_LEN. If the final
fragment only contains 1 to 3 bytes of data, this unsigned subtraction
underflows to a massive integer. This corrupted length is then passed to
xdp_prepare_buff(), setting a huge data_end and allowing the BPF program to
read and write out of bounds.

[ ... ]

> @@ -5924,6 +5936,10 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
>  
>  		/* Got entire packet into SKB. Finish it. */
>  
> +		/* Remove FCS if needed */
> +		if (!fcs_stripped)
> +			skb_trim(skb, len);
> +

[Severity: High]
Can this trigger a WARN_ON and fail to strip the FCS for scatter-gather
frames?

For multi-buffer frames, data is appended using skb_add_rx_frag(), which
creates a non-linear SKB (skb->data_len > 0). However, skb_trim() requires a
linear SKB. Calling it on a non-linear SKB evaluates
WARN_ON(skb_is_nonlinear(skb)) and returns immediately without modifying the
SKB, leaving the FCS attached.

Should this use pskb_trim(skb, len) instead to correctly handle paged SKBs?

>  		stmmac_get_rx_hwtstamp(priv, p, np, skb);
>  
>  		if (priv->hw->hw_vlan_en)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260921-stmmac-rx-sg-fix-v2-1-b6d88c5ac2d7@oss.qualcomm.com?part=1