[PATCH net v2 1/6] net: stmmac: take ownership of saved RX state at poll entry
Lorenzo Bianconi
lorenzo.bianconi at oss.qualcomm.com
Fri Oct 30 01:38:48 PDT 2026
> Thank you for your contribution! Sashiko AI review found more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
>
> Low priority / nit picks for your consideration:
>
> - [Low] The commit message gives the trigger as a saved partial packet
> completing "with a poll budget of one".
>
> Pre-existing issues:
> - [High] In stmmac_rx(), if napi_build_skb() fails on a non-last
> descriptor, the code recycles the page, does rx_dropped++ and count++,…
> - [High] On resume, the RX ring is handed back to the DMA without its
> buffers being refilled or their addresses rewritten, and this patch…
>
> --
>
> net: stmmac: take ownership of saved RX state at poll entry
>
> This patch makes stmmac_rx() and stmmac_rx_zc() take the saved
> partial-frame state at poll entry, clear it right away, and save it again
> only if the frame is still incomplete when the loop exits. The in-progress
> flag is now tracked separately from the status of the last descriptor
> read. The saved skb is also dropped in stmmac_reset_rx_queue() and
> __free_dma_rx_desc_resources().
>
> > When a saved partial packet completes with a poll budget of one, the
> > old loop can leave state_saved and state.skb pointing at an skb that
> > has already been delivered or freed. The next poll then reuses that
> > pointer, causing a use-after-free or double free.
>
> [Severity: Low]
> Could the commit message say how a budget of one happens on a running
> system?
I guess this comment is minor, I can amend it if I need to rebase.
>
> With limit > 1, the old else branch cleared state_saved on the second loop
> iteration. So the stale pointer only survives when limit == 1. Normal
> net_rx_action() polling passes the NAPI weight.
>
> stmmac_napi_poll_rx() passes its budget straight through:
>
> work_done = stmmac_rx(priv, budget, chan);
>
> Busy polling can supply a budget of 1. In ep_eventpoll_ioctl(), EPIOCSPARAMS
> only asks for CAP_NET_ADMIN above NAPI_POLL_WEIGHT:
>
> if (epoll_params.busy_poll_budget > NAPI_POLL_WEIGHT &&
> !capable(CAP_NET_ADMIN))
> return -EPERM;
>
> SO_BUSY_POLL_BUDGET in sk_setsockopt() can also be lowered without
> privilege.
>
> So an unprivileged local user who receives multi-descriptor frames can
> reach the use-after-free. Would it be worth saying so in the changelog, so
> stable reviewers can see this is a user-triggerable memory-safety fix?
>
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 9ded31150fbe6..fa7108919e90d 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>
> [ ... ]
>
> > @@ -6041,10 +6051,8 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
> > /* Remove FCS if needed */
> > if (!fcs_stripped && pskb_trim(skb, len)) {
> > dev_kfree_skb_any(skb);
> > - skb = NULL;
> > rx_dropped++;
> > - count++;
> > - continue;
> > + goto next;
> > }
>
> [Severity: High]
> This is a pre-existing issue, but what happens in stmmac_rx() when
> napi_build_skb() fails on a descriptor that is not the last one in the
> frame?
This issue has not been introduced by this patch and it needs to be fixed in a
dedicated patch.
>
> skb = napi_build_skb(page_address(buf->page),
> rx_q->napi_skb_frag_size);
> if (!skb) {
> page_pool_recycle_direct(rx_q->page_pool,
> buf->page);
> buf->page = NULL;
> rx_dropped++;
> count++;
> goto drain_data;
> }
>
> error stays 0 and skb stays NULL, and drain_data jumps back to read_again
> because rx_not_ls is set. The next descriptor of the same hardware frame
> passes the error checks and takes the if (!skb) frame-start path. A new
> skb is then built from only the tail of the frame.
>
> len still counts the dropped prefix, so the pskb_trim(skb, len) above does
> nothing and the FCS is not removed. The trailing payload bytes are then
> delivered as a standalone Ethernet frame.
>
> With Split Header active, stmmac_rx_buf1_len() returns 0 for any
> descriptor after the first:
>
> if (priv->sph_active && len)
> return 0;
>
> That leaves the rebuilt skb with no linear data, only the buf2 frag.
> Wouldn't eth_type_trans() then hit the BUG() in __skb_pull()?
>
> eth_type_trans()
> eth_skb_pull_mac()
> skb_pull_inline(skb, ETH_HLEN)
> __skb_pull()
> skb->len < skb->data_len -> BUG()
>
> If the stale page bytes look like a VLAN header, stmmac_rx_vlan() can hit
> the same BUG() through skb_pull(skb, VLAN_HLEN).
>
> An on-link peer sending a frame that spans several descriptors, plus a
> GFP_ATOMIC allocation failure, is enough to trigger this. With this patch,
> the in_progress tracking also carries this skb == NULL, error == 0 state
> across a poll boundary. Nothing else in the series seems to address it.
>
> Should the napi_build_skb() failure path set error = 1, so the rest of the
> frame is thrown away?
>
> [ ... ]
>
> > @@ -8421,6 +8430,9 @@ static void stmmac_reset_rx_queue(struct stmmac_priv *priv, u32 queue)
> > {
> > struct stmmac_rx_queue *rx_q = &priv->dma_conf.rx_queue[queue];
> >
> > + dev_kfree_skb_any(rx_q->state.skb);
> > + rx_q->state.skb = NULL;
> > + rx_q->state_saved = false;
> > rx_q->cur_rx = 0;
> > rx_q->dirty_rx = 0;
> > }
>
> [Severity: High]
> This is a pre-existing issue, but this new free happens just before
> stmmac_resume() re-arms the RX ring in place:
This issue has not been introduced by this patch and it needs to be fixed in a
dedicated patch.
Regards,
Lorenzo
>
> stmmac_resume()
> stmmac_reset_queues_param()
> stmmac_reset_rx_queue() <- frees state.skb, rewinds cur/dirty
> stmmac_free_tx_skbufs()
> stmmac_clear_descriptors()
> stmmac_clear_rx_descriptors()
> stmmac_init_rx_desc() <- sets OWN only
> stmmac_hw_setup()
> stmmac_init_dma_engine() <- restores the tail pointer
>
> No refill and no stmmac_set_desc_addr() runs before the DMA restarts. That
> means two kinds of descriptors get re-armed with a bad buffer address:
>
> - slots stmmac_rx() consumed whose refill failed. stmmac_rx_refill()
> uses GFP_ATOMIC and stops at the first page_pool_alloc_pages()
> failure, so buf->page is left NULL.
>
> - descriptors the DMA wrote back after napi_disable() in
> stmmac_suspend() and before stmmac_stop_all_dma().
>
> On dwmac4/xgmac, write-back overwrites des0/des1, which hold the buffer
> address in read format (dwmac4_wrback_get_rx_vlan_tci() reads des0 as the
> VLAN tag). dwmac4_rd_init_rx_desc() only sets OWN/BUF1V/IOC in RDES3. The
> DMA would then write to whatever status data is left in those words.
>
> On norm/enh descriptors, the old address stays in des2. The slots most
> likely to have failed refill are the ones whose pages went into the saved
> partial skb. The dev_kfree_skb_any() above now returns those pages, still
> DMA-mapped, to the page_pool just before the hardware is re-armed to write
> into them.
>
> Can this lead to DMA into pages the pool has already handed to another
> descriptor or released?
>
> Also, if DMA completes into a slot with buf->page == NULL, stmmac_rx()
> calls page_address(NULL) and napi_build_skb() on it, which would oops.
>
> The commit message notes that "the RX buffer addresses are not
> reprogrammed" on this path. Would a full fix need stmmac_resume() to
> refill and reprogram every RX descriptor before setting OWN?
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261027-stmmac-xdp-fixes-v2-0-10eae8a093a8%40oss.qualcomm.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20261030/30ee0cc6/attachment-0001.sig>
More information about the linux-arm-kernel
mailing list