Re: [PATCH net v2 1/6] net: stmmac: take ownership of saved RX state at poll entry

[netdev-bot+sashiko@kernel.org]

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?

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?

    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:

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