net: stmmac: guard FCS stripping against runt frames

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

Posted by Aldo Ariel Panzardo 20 hours ago

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

Posted by Lorenzo Bianconi 16 hours ago

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

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

Posted by Andrew Lunn 10 hours ago

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

Posted by Lorenzo Bianconi 5 hours ago

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

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

Posted by Andrew Lunn 8 minutes ago

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

Posted by Andrew Lunn 17 hours ago

> 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

Posted by Aldo Ariel Panzardo 17 hours ago

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

Posted by Andrew Lunn 16 hours ago

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