[PATCH] net: stmmac: guard FCS stripping against runt frames

[PATCH] net: stmmac: guard FCS stripping against runt frames

[Aldo Ariel Panzardo]

stmmac_rx_zc() and stmmac_rx() strip the 4-byte FCS from the last
buffer when ACS (Automatic Checksum Stripping) is disabled:

    buf1_len -= ETH_FCS_LEN;
    len -= ETH_FCS_LEN;

Neither path checks that the buffer actually contains at least
ETH_FCS_LEN bytes. A runt frame delivered by the hardware with a
buf1_len or buf2_len smaller than 4 underflows the unsigned
subtraction, producing a very large value. In the XDP zero-copy path
this wraps data_end backwards:

    buf->xdp->data_end = buf->xdp->data + buf1_len;

giving the XDP/BPF program an enormous data region that extends into
adjacent kernel memory.

Add the missing lower-bound checks so that undersized frames are
silently passed through without stripping.

Fixes: bba2556efad6 ("net: stmmac: Enable RX via AF_XDP zero-copy")
Cc: stable@vger.kernel.org
Reported-by: Sashiko <sashiko-bot@kernel.org>
Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@gmail.com>
---
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 1fb5f80..4526669 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -5636,7 +5636,8 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue)
 		len += buf1_len;
 
 		/* ACS is disabled; strip manually. */
-		if (likely(!(status & rx_not_ls))) {
+		if (likely(!(status & rx_not_ls)) &&
+		    likely(buf1_len >= ETH_FCS_LEN)) {
 			buf1_len -= ETH_FCS_LEN;
 			len -= ETH_FCS_LEN;
 		}
@@ -5808,10 +5809,10 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 
 		/* ACS is disabled; strip manually. */
 		if (likely(!(status & rx_not_ls))) {
-			if (buf2_len) {
+			if (buf2_len >= ETH_FCS_LEN) {
 				buf2_len -= ETH_FCS_LEN;
 				len -= ETH_FCS_LEN;
-			} else if (buf1_len) {
+			} else if (buf1_len >= ETH_FCS_LEN) {
 				buf1_len -= ETH_FCS_LEN;
 				len -= ETH_FCS_LEN;
 			}
-- 
2.43.0

Re: [PATCH] net: stmmac: guard FCS stripping against runt frames

[Andrew Lunn]

> Neither path checks that the buffer actually contains at least
> ETH_FCS_LEN bytes.

Please could you include a link to the freescale documentation that
says such runt frames are actually delivered.

Or maybe turn this around, add a comment that the freescale
documentation says packets less than 64 bytes are dropped by the
hardware, and remove all these unneeded length checks.

    Andrew

---
pw-bot: cr

Re: [PATCH] net: stmmac: guard FCS stripping against runt frames

[Aldo Ariel Panzardo]

Hi Andrew,

You're right that the relevant question is not merely whether the MAC can
forward frames shorter than 64 bytes, but whether a descriptor with a
reported length smaller than ETH_FCS_LEN can reach this path.

I found a closer NXP/Freescale reference: the i.MX RT1170 ENET_QOS
documentation (IMXRT1170RM, Chapter 61), which describes the Synopsys
DWC EQoS receive descriptors used by this driver:

  https://www.nxp.com/webapp/Download?colCode=IMXRT1170RM

The MTL receive configuration explicitly supports forwarding error packets
and undersized good packets (FEP/FUP), and RDES3.PL is documented as
the number of bytes transferred to system memory, including CRC. I don't
see a documented lower bound on PL.

However, I also noticed that dwmac4_wrback_get_rx_status() returns
discard_frame when RDES3_ERROR_SUMMARY is set, including CRC and
receive errors. So an ordinary CRC-error runt would be discarded before
reaching the FCS subtraction.

I therefore don't claim the documentation alone proves that a good-status
descriptor with PL < 4 can occur. I'll verify whether such a descriptor
can actually be produced by the hardware before claiming that as the
trigger.

The length check may still be useful as defensive validation of the
hardware-supplied descriptor length, but that is a different justification
from the runt-frame scenario in the current changelog. Happy to respin
with that framing if you prefer.

Thanks,
Aldo

Re: [PATCH] net: stmmac: guard FCS stripping against runt frames

[Lorenzo Bianconi]

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

> stmmac_rx_zc() and stmmac_rx() strip the 4-byte FCS from the last
> buffer when ACS (Automatic Checksum Stripping) is disabled:
> 
>     buf1_len -= ETH_FCS_LEN;
>     len -= ETH_FCS_LEN;
> 
> Neither path checks that the buffer actually contains at least
> ETH_FCS_LEN bytes. A runt frame delivered by the hardware with a
> buf1_len or buf2_len smaller than 4 underflows the unsigned
> subtraction, producing a very large value. In the XDP zero-copy path
> this wraps data_end backwards:
> 
>     buf->xdp->data_end = buf->xdp->data + buf1_len;
> 
> giving the XDP/BPF program an enormous data region that extends into
> adjacent kernel memory.
> 
> Add the missing lower-bound checks so that undersized frames are
> silently passed through without stripping.
> 
> Fixes: bba2556efad6 ("net: stmmac: Enable RX via AF_XDP zero-copy")
> Cc: stable@vger.kernel.org
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@gmail.com>
> ---
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 1fb5f80..4526669 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -5636,7 +5636,8 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue)
>  		len += buf1_len;
>  
>  		/* ACS is disabled; strip manually. */
> -		if (likely(!(status & rx_not_ls))) {
> +		if (likely(!(status & rx_not_ls)) &&
> +		    likely(buf1_len >= ETH_FCS_LEN)) {
>  			buf1_len -= ETH_FCS_LEN;
>  			len -= ETH_FCS_LEN;
>  		}
> @@ -5808,10 +5809,10 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
>  
>  		/* ACS is disabled; strip manually. */
>  		if (likely(!(status & rx_not_ls))) {
> -			if (buf2_len) {
> +			if (buf2_len >= ETH_FCS_LEN) {

I do not think this approach is correct since, at least theoretically, the FCS can be
splitted between buf1 and buf2.

Please note the non-XDP case is already fixed in the following patch:
https://lore.kernel.org/netdev/20260923-stmmac-rx-sg-fix-v3-1-ed26fea7180d@oss.qualcomm.com/

The XDP-case will be properly handled introducing XDP multi-buff support (I
have already posted v1 and I need to respin the v2).

Regards,
Lorenzo

>  				buf2_len -= ETH_FCS_LEN;
>  				len -= ETH_FCS_LEN;
> -			} else if (buf1_len) {
> +			} else if (buf1_len >= ETH_FCS_LEN) {
>  				buf1_len -= ETH_FCS_LEN;
>  				len -= ETH_FCS_LEN;
>  			}
> -- 
> 2.43.0
> 

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

Re: [PATCH] net: stmmac: guard FCS stripping against runt frames

[Andrew Lunn]

On Wed, Sep 23, 2026 at 04:33:54PM -0300, Aldo Ariel Panzardo wrote:
> Hi Andrew,
> 
> You're right that the relevant question is not merely whether the MAC can
> forward frames shorter than 64 bytes, but whether a descriptor with a
> reported length smaller than ETH_FCS_LEN can reach this path.
> 
> I found a closer NXP/Freescale reference: the i.MX RT1170 ENET_QOS
> documentation (IMXRT1170RM, Chapter 61), which describes the Synopsys
> DWC EQoS receive descriptors used by this driver:
> 
>   https://www.nxp.com/webapp/Download?colCode=IMXRT1170RM
> 
> The MTL receive configuration explicitly supports forwarding error packets
> and undersized good packets (FEP/FUP), and RDES3.PL is documented as
> the number of bytes transferred to system memory, including CRC. I don't
> see a documented lower bound on PL.

Will, a frame which is smaller than the FCS cannot pass the FCS
check. So you don't need to worry about undersized good packets
hitting this condition.

Given that an ethernet header is 14 octets the FCS is 4 octets, any
frame smaller than 18 octets should be dropped by the network
stack. So why not do one test at the beginning for ETH_HLEN +
ETH_FCS_LEN?

	Andrew

Re: [PATCH] net: stmmac: guard FCS stripping against runt frames

[Andrew Lunn]

> > @@ -5808,10 +5809,10 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
> >  
> >  		/* ACS is disabled; strip manually. */
> >  		if (likely(!(status & rx_not_ls))) {
> > -			if (buf2_len) {
> > +			if (buf2_len >= ETH_FCS_LEN) {
> 
> I do not think this approach is correct since, at least theoretically, the FCS can be
> splitted between buf1 and buf2.

We are talking about runt frames here, so less than 64 bytes in
size. Can such a frame be split over two buffers? What is the minimum
size of the first buffer?

	Andrew

Re: [PATCH] net: stmmac: guard FCS stripping against runt frames

[Lorenzo Bianconi]

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

> > > @@ -5808,10 +5809,10 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
> > >  
> > >  		/* ACS is disabled; strip manually. */
> > >  		if (likely(!(status & rx_not_ls))) {
> > > -			if (buf2_len) {
> > > +			if (buf2_len >= ETH_FCS_LEN) {
> > 
> > I do not think this approach is correct since, at least theoretically, the FCS can be
> > splitted between buf1 and buf2.
> 
> We are talking about runt frames here, so less than 64 bytes in
> size. Can such a frame be split over two buffers? What is the minimum
> size of the first buffer?

Why are talking just about runt frames?  According to my understanding,
this codebase (at least the one in stmmac_rx()) is executed on all
'last fragments'. Am I missing something?

Regards,
Lorenzo

> 
> 	Andrew

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

Re: [PATCH] net: stmmac: guard FCS stripping against runt frames

[Andrew Lunn]

On Thu, Sep 24, 2026 at 09:57:14AM +0200, Lorenzo Bianconi wrote:
> > > > @@ -5808,10 +5809,10 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
> > > >  
> > > >  		/* ACS is disabled; strip manually. */
> > > >  		if (likely(!(status & rx_not_ls))) {
> > > > -			if (buf2_len) {
> > > > +			if (buf2_len >= ETH_FCS_LEN) {
> > > 
> > > I do not think this approach is correct since, at least theoretically, the FCS can be
> > > splitted between buf1 and buf2.
> > 
> > We are talking about runt frames here, so less than 64 bytes in
> > size. Can such a frame be split over two buffers? What is the minimum
> > size of the first buffer?
> 
> Why are talking just about runt frames?  According to my understanding,
> this codebase (at least the one in stmmac_rx()) is executed on all
> 'last fragments'. Am I missing something?

That the patch subject is wrong?

[PATCH] net: stmmac: guard FCS stripping against runt frames

I suspect this is an AI generated bug report, a minimal fix has been
proposed, but no actual thought applied to the situation, such as does
the hardware even allow it to happen, does it apply to more complex
situations, such as fragmentation etc. The usual AI problems....

	Andrew

Re: [PATCH] net: stmmac: guard FCS stripping against runt frames

[Lorenzo Bianconi]

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

> On Thu, Sep 24, 2026 at 09:57:14AM +0200, Lorenzo Bianconi wrote:
> > > > > @@ -5808,10 +5809,10 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
> > > > >  
> > > > >  		/* ACS is disabled; strip manually. */
> > > > >  		if (likely(!(status & rx_not_ls))) {
> > > > > -			if (buf2_len) {
> > > > > +			if (buf2_len >= ETH_FCS_LEN) {
> > > > 
> > > > I do not think this approach is correct since, at least theoretically, the FCS can be
> > > > splitted between buf1 and buf2.
> > > 
> > > We are talking about runt frames here, so less than 64 bytes in
> > > size. Can such a frame be split over two buffers? What is the minimum
> > > size of the first buffer?
> > 
> > Why are talking just about runt frames?  According to my understanding,
> > this codebase (at least the one in stmmac_rx()) is executed on all
> > 'last fragments'. Am I missing something?
> 
> That the patch subject is wrong?
> 
> [PATCH] net: stmmac: guard FCS stripping against runt frames

ack, I missed the subject, but I think it is wrong, the issue is not just on
runt frames.

Regards,
Lorenzo

> 
> I suspect this is an AI generated bug report, a minimal fix has been
> proposed, but no actual thought applied to the situation, such as does
> the hardware even allow it to happen, does it apply to more complex
> situations, such as fragmentation etc. The usual AI problems....
> 
> 	Andrew

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

Re: [PATCH] net: stmmac: guard FCS stripping against runt frames

[Andrew Lunn]

On Thu, Sep 24, 2026 at 10:51:11AM -0300, Aldo Ariel wrote:
> Hi Andrew, Lorenzo,
> 
> Fair point on the subject — the trigger is not limited to runt frames.
>  
> The FCS stripping in stmmac_rx() at lines 5810-5817 runs on every
> last-segment descriptor regardless of total frame size:
> 
>     if (likely(!(status & rx_not_ls))) {
>         if (buf2_len) {
>             buf2_len -= ETH_FCS_LEN;
>         } else if (buf1_len) {
>             buf1_len -= ETH_FCS_LEN;
>         }
>     }      
>        
> For a multi-descriptor frame, the last descriptor's buf2_len or
> buf1_len carries only the tail of the frame. With the default
> dma_buf_sz = BUF_SIZE_2KiB for MTU 1500, a 4097-byte frame splits as:

Is the MTU programmed into the hardware? Will it even try to transfer
an 4097 octet frame when the MTU is configured to 1500?

   Andrew

Re: [PATCH] net: stmmac: guard FCS stripping against runt frames

[netdev-bot+sashiko]

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

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

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] stmmac_rx() still handles the FCS wrongly when the last buffer
  holds fewer than ETH_FCS_LEN bytes, which is exactly the case the…
- [Medium] The commit message gets the trigger, the scope and the stable
  targeting wrong.

Pre-existing issues:
- [Medium] Under the new guards, a complete frame shorter than
  ETH_FCS_LEN (0..3 bytes, no secondary buffer) is passed on rather than…

--

net: stmmac: guard FCS stripping against runt frames

This commit adds lower bound checks before the 4-byte FCS is stripped in
stmmac_rx_zc() and stmmac_rx(). A final buffer shorter than ETH_FCS_LEN
no longer underflows buf1_len or buf2_len. Such frames are passed
through without stripping.

> Neither path checks that the buffer actually contains at least
> ETH_FCS_LEN bytes. A runt frame delivered by the hardware with a
> buf1_len or buf2_len smaller than 4 underflows the unsigned
> subtraction, producing a very large value. In the XDP zero-copy path
> this wraps data_end backwards:
>
>     buf->xdp->data_end = buf->xdp->data + buf1_len;
>
> giving the XDP/BPF program an enormous data region that extends into
> adjacent kernel memory.

[Severity: Medium]

Is "runt frame" the right description of the trigger for the
stmmac_rx() hunk?

With Split Header active, stmmac_rx_buf2_len() returns the remaining
bytes of the frame:

    plen = stmmac_get_rx_frame_len(priv, p, coe);

    return plen - len;

On the last descriptor this is 1..3 whenever a normal-sized frame
leaves only part of the FCS in the last buffer. Two examples:

  - a frame of dma_buf_sz + 1..3 bytes
  - a frame of hlen + dma_buf_sz + 1..3 bytes that spills into a
    second descriptor

GMAC_CORE_INIT sets GMAC_CONFIG_JE, so the MAC accepts these lengths,
and any sender on the LAN can choose them.

Before this patch, buf2_len wrapped to about 4 GiB in the default
non-XDP receive path and was then passed to:

    if (buf2_len) {
        dma_sync_single_for_cpu(priv->device, buf->sec_addr,
                                buf2_len, dma_dir);
        skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags,
                        buf->sec_page, 0, buf2_len,
                        priv->dma_conf.dma_buf_sz);

Could the commit message describe this case? It looks like the one that
is reachable in practice.

In the zero-copy path, stmmac_xdp_set_prog() disables SPH while XDP is
active:

    priv->sph_active = priv->sph_capable && !stmmac_xdp_is_enabled(priv);

So buf1_len < 4 there needs a complete, error-free frame shorter than
4 bytes. stmmac never enables MTL FUP/FEP, and CRC errors set
RDES3_ERROR_SUMMARY, which gives discard_frame. That seems to need a
malfunctioning device.

Also, buf1_len is unsigned int. On 64-bit, data + buf1_len moves
data_end about 4 GiB forward, not "backwards". On 32-bit it does wrap
backwards, but then data_end < data and the region is empty, not
"enormous". Could that sentence be reworded?

> Fixes: bba2556efad6 ("net: stmmac: Enable RX via AF_XDP zero-copy")

This tag covers only the stmmac_rx_zc() hunk. The FCS stripping in
stmmac_rx() came from 88ebe2cf7f3f ("net: stmmac: Rework stmmac_rx()").
It was last changed by 0f296e782f21 ("stmmac_pci: Fix underflow size in
stmmac_rx").

Should there be another Fixes: tag, so that stable trees without
zero-copy support also pick up the stmmac_rx() hunk?

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index d5a984ad864f2..d3e16a523abde 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -5638,7 +5638,8 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue)
>  		len += buf1_len;
>  
>  		/* ACS is disabled; strip manually. */
> -		if (likely(!(status & rx_not_ls))) {
> +		if (likely(!(status & rx_not_ls)) &&
> +		    likely(buf1_len >= ETH_FCS_LEN)) {
>  			buf1_len -= ETH_FCS_LEN;
>  			len -= ETH_FCS_LEN;
>  		}

[Severity: Medium]

This isn't a bug introduced by this patch, but with this guard a
complete frame of 0..3 bytes is passed through rather than dropped. It
goes to the XDP program and, on XDP_PASS, to stmmac_dispatch_skb_zc().
The new check in stmmac_rx() does the same for a single-buffer frame
under 4 bytes.

The short skb then reaches two helpers that don't check its length:

  - stmmac_rx_vlan() reads h_vlan_proto and the TCI without checking
    skb->len.
  - In eth_type_trans(), eth_skb_pull_mac() ignores a failed ETH_HLEN
    pull, so h_proto is read from beyond the received data.

Later, eth_header_parse() can copy h_source bytes that were never
received into the AF_PACKET sll_addr.

To reach this, the device has to report a good, complete frame under
4 bytes. That should not happen, since FUP/FEP are not enabled and CRC
errors end up as discard_frame. Frames of 4..17 bytes already produced
the same kind of short skb before this patch.

Would it make sense to drop frames shorter than ETH_HLEN + ETH_FCS_LEN
instead of passing them through, perhaps as a separate hardening
change?

> @@ -5810,10 +5811,10 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
>  
>  		/* ACS is disabled; strip manually. */
>  		if (likely(!(status & rx_not_ls))) {
> -			if (buf2_len) {
> +			if (buf2_len >= ETH_FCS_LEN) {
>  				buf2_len -= ETH_FCS_LEN;
>  				len -= ETH_FCS_LEN;
> -			} else if (buf1_len) {
> +			} else if (buf1_len >= ETH_FCS_LEN) {
>  				buf1_len -= ETH_FCS_LEN;
>  				len -= ETH_FCS_LEN;
>  			}

[Severity: High]

When buf2_len is 1..3, doesn't the FCS straddle two buffers? The FCS is
always the last 4 bytes of the frame. In that case buf2 holds only the
tail of the FCS, and the other 4 - buf2_len FCS bytes sit at the end of
the previous buffer.

Take a single-descriptor frame with SPH active that is not header
split (hlen == 0), for example a non-IP frame of dma_buf_sz + k bytes
with k in 1..3:

  - stmmac_rx_buf1_len() returns min(dma_buf_sz, plen), which is
    dma_buf_sz.
  - stmmac_rx_buf2_len() returns plen - len, which is k.

The new else branch then trims 4 bytes from buf1, which removes k real
payload bytes. Later, the k FCS bytes in buf2 are still appended:

    if (buf2_len) {
        ...
        skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags,
                        buf->sec_page, 0, buf2_len,
                        priv->dma_conf.dma_buf_sz);

Does the skb end up with FCS bytes in place of its last k payload
bytes?

Now take a multi-descriptor SPH frame that spills k = 1..3 bytes into
its last descriptor. stmmac_rx_buf1_len() returns 0 there:

    /* Not first descriptor, buffer is always zero */
    if (priv->sph_active && len)
        return 0;

So neither branch runs and nothing is stripped. All 4 FCS bytes stay at
the end of the skb and are counted in rx_bytes. Is that intended?

Any LAN host can send these lengths, since GMAC_CORE_INIT sets JE, SPH
is on by default on capable cores, and no XDP is involved. Later, IP
frames may still be marked CHECKSUM_UNNECESSARY.

Would it be more correct to trim ETH_FCS_LEN - buf2_len bytes from the
preceding buffer or frag and set buf2_len to 0, or else drop the frame?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260923162339.1375698-1-qwe.aldo%40gmail.com