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

[Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>]

[-- Attachment #1: Type: text/plain, Size: 8110 bytes --]

> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]