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

[Tyllis Xu]

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@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx
> > Signed-off-by: Tyllis Xu <LivelyCarpet87@xxxxxxxxx>
>
> 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.