Re: [PATCH] net: stmmac: fix integer underflow in chain mode jumbo_frm

[Simon Horman <horms@kernel.org>]

On Fri, Mar 20, 2026 at 11:10:58PM -0500, Tyllis Xu wrote:
> The jumbo_frm() chain-mode implementation unconditionally computes
> 
>     len = nopaged_len - bmax;
> 
> where nopaged_len = skb_headlen(skb) (linear bytes only) and bmax is
> BUF_SIZE_8KiB or BUF_SIZE_2KiB.  However, the caller stmmac_xmit()
> decides to invoke jumbo_frm() based on skb->len (total length including
> page fragments):
> 
>     is_jumbo = stmmac_is_jumbo_frm(priv, skb->len, enh_desc);
> 
> When a packet has a small linear portion (nopaged_len <= bmax) but a
> large total length due to page fragments (skb->len > bmax), the
> subtraction wraps as an unsigned integer, producing a huge len value
> (~0xFFFFxxxx).  This causes the while (len != 0) loop to execute
> hundreds of thousands of iterations, passing skb->data + bmax * i
> pointers far beyond the skb buffer to dma_map_single().  On IOMMU-less
> SoCs (the typical deployment for stmmac), this maps arbitrary kernel
> memory to the DMA engine, constituting a kernel memory disclosure and
> potential memory corruption from hardware.
> 
> The ring-mode counterpart already guards against this with:
> 
>     if (nopaged_len > BUF_SIZE_8KiB) { ... use len ... }
>     else { ... map nopaged_len directly ... }
> 
> Apply the same pattern to chain mode: guard the chunked-DMA path with
> if (nopaged_len > bmax), and add an else branch that maps the entire
> linear portion as a single descriptor when it fits within bmax.  The
> fragment loop in stmmac_xmit() handles page fragments afterward.
> 
> Fixes: 286a83721720 ("stmmac: add CHAINED descriptor mode support (V4)")
> Cc: stable@vger.kernel.org
> Signed-off-by: Tyllis Xu <LivelyCarpet87@gmail.com>

As a fix for code present in net this patch should be targeted at the net
tree like this:

Subject: [PATCH net] net: stmmac: fix integer underflow in chain mode

As is, our CI tries to apply this patch to the default tree, net-next.
Which fails due to a conflict with commit 6b4286e05508 ("net: stmmac:
rename STMMAC_GET_ENTRY() -> STMMAC_NEXT_ENTRY()"). So no CI tests were
run.

> ---
>  drivers/net/ethernet/stmicro/stmmac/chain_mode.c | 71 ++++++++++++++---------
>  1 file changed, 44 insertions(+), 27 deletions(-)

The bulk of this patch is whitespace change (indentation).
So seems useful to examine this patch with whitespace changes ignored.

git diff -w yeilds;

diff --git a/drivers/net/ethernet/stmicro/stmmac/chain_mode.c b/drivers/net/ethernet/stmicro/stmmac/chain_mode.c
index 120a009c9992..c8980482dea2 100644
--- a/drivers/net/ethernet/stmicro/stmmac/chain_mode.c
+++ b/drivers/net/ethernet/stmicro/stmmac/chain_mode.c
@@ -31,6 +31,7 @@ static int jumbo_frm(struct stmmac_tx_queue *tx_q, struct sk_buff *skb,
 	else
 		bmax = BUF_SIZE_2KiB;
 
+	if (nopaged_len > bmax) {
 		len = nopaged_len - bmax;
 
 		des2 = dma_map_single(priv->device, skb->data,
@@ -77,6 +78,18 @@ static int jumbo_frm(struct stmmac_tx_queue *tx_q, struct sk_buff *skb,
 				len = 0;
 			}
 		}
+	} else {
+		des2 = dma_map_single(priv->device, skb->data,
+				      nopaged_len, DMA_TO_DEVICE);
+		desc->des2 = cpu_to_le32(des2);
+		if (dma_mapping_error(priv->device, des2))
+			return -1;
+		tx_q->tx_skbuff_dma[entry].buf = des2;
+		tx_q->tx_skbuff_dma[entry].len = nopaged_len;
+		stmmac_prepare_tx_desc(priv, desc, 1, nopaged_len, csum,
+				STMMAC_CHAIN_MODE, 0, !skb_is_nonlinear(skb),
+				skb->len);
+	}
 
 	tx_q->cur_tx = entry;

The code in the else arm of the new condition is quite similar to
the (not visible in the diff above) code at the top of the non-else
arm of the condition.

I do see this is consistent with the ring-mode code.  So perhaps it is
appropriate as a fix. But I do wonder if this could be consolidated - e.g.
by setting up some local variables rather than moving the mapping logic
into a condition.