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

[Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>]

[-- Attachment #1: Type: text/plain, Size: 5256 bytes --]

> 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?

I do think this issue can occur since the legacy cores never set rx_not_ls, so
the packet is always contained in a single buffer.

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

I think the main issue here is stmmac XDP implementation does not properly
support multi-buff. I am currently working on it.

> 
> [ ... ]
> 
> > @@ -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?

corret, I will fix it in v3.

Regards,
Lorenzo

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]