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

Tyllis Xu
2026-03-24T06:08:01Z

I will try to change the code to consolidate with the ring-mode
code, avoid whitespace diff, and resubmit the patch with a new
correct subject line. Thank you for your feedback!

On Mon, Mar 23, 2026 at 9:18 AM Simon Horman <horms@kernel.org> wrote:
>
> 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.