From: James Hilliard 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. Take the saved state at poll entry and clear the stored ownership immediately. Save it again only if the packet remains incomplete, including when the next descriptor is still DMA-owned. Drop the saved state both when the queue parameters are reset and when the ring is destroyed. stmmac_reset_rx_queue() is reached from __stmmac_open(), stmmac_xdp_open(), stmmac_resume() and stmmac_enable_rx_queue(); without draining it there, a partial frame saved before suspend (or before a queue is disabled and re-enabled) survives while cur_rx and dirty_rx are re-armed, and the first poll resumes that stale skb with descriptors from the freshly initialized ring. __free_dma_rx_desc_resources() releases it on teardown. Apply the same state handling to stmmac_rx_zc(), which also leaves state_saved set when a saved frame finishes at the budget boundary. That path saves only error and length bookkeeping, not an skb pointer. Track whether a frame remains incomplete independently of the status read from the next descriptor, so a DMA-owned descriptor does not erase the continuation state. Keep the running frame length when an XDP verdict ends a descriptor early. The old loop returned to read_again for STMMAC_XDP_CONSUMED while rx_not_ls was set, but the STMMAC_XDP_TX/STMMAC_XDP_REDIRECT path fell through to the next iteration. Resetting the frame state there makes the remaining descriptors of the same hardware frame get parsed as a new frame; on cores with a second RX buffer this makes stmmac_rx_buf2_len() report more bytes than the buffer holds and read past the page. Route both verdicts back to read_again while rx_not_ls is set, clearing skb first since it holds the XDP status rather than a packet. Fixes: ec222003bd94 ("net: stmmac: Prepare to add Split Header support") Fixes: bba2556efad6 ("net: stmmac: Enable RX via AF_XDP zero-copy") Signed-off-by: James Hilliard Signed-off-by: Lorenzo Bianconi --- drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 91 +++++++++++++---------- 1 file changed, 52 insertions(+), 39 deletions(-) diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index b8b4de4c5b1a..6128e6d9504c 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -2180,6 +2180,9 @@ static void __free_dma_rx_desc_resources(struct stmmac_priv *priv, else dma_free_rx_skbufs(priv, dma_conf, queue); + dev_kfree_skb_any(rx_q->state.skb); + rx_q->state.skb = NULL; + rx_q->state_saved = false; rx_q->buf_alloc_num = 0; rx_q->xsk_pool = NULL; @@ -5620,12 +5623,12 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue) unsigned int count = 0, error = 0, len = 0; int dirty = stmmac_rx_dirty(priv, queue); unsigned int next_entry = rx_q->cur_rx; + bool in_progress = rx_q->state_saved; u32 rx_errors = 0, rx_dropped = 0; unsigned int desc_size; struct bpf_prog *prog; bool failure = false; int xdp_status = 0; - int status = 0; if (netif_msg_rx_status(priv)) { void *rx_head = stmmac_get_rx_desc(priv, rx_q, 0); @@ -5636,23 +5639,25 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue) stmmac_display_ring(priv, rx_head, priv->dma_conf.dma_rx_size, true, rx_q->dma_rx_phy, desc_size); } + + if (rx_q->state_saved) { + error = rx_q->state.error; + len = rx_q->state.len; + rx_q->state_saved = false; + } + while (count < limit) { struct stmmac_rx_buffer *buf; struct stmmac_xdp_buff *ctx; unsigned int buf1_len = 0; struct dma_desc *np, *p; - int entry; + int entry, status; int res; - if (!count && rx_q->state_saved) { - error = rx_q->state.error; - len = rx_q->state.len; - } else { - rx_q->state_saved = false; + if (!in_progress) { error = 0; len = 0; } - read_again: if (count >= limit) break; @@ -5691,6 +5696,8 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue) if (!buf->xdp) break; + in_progress = status & rx_not_ls; + if (priv->extend_desc) stmmac_rx_extended_status(priv, &priv->xstats, rx_q->dma_erx + entry); @@ -5705,6 +5712,7 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue) if (unlikely(error && (status & rx_not_ls))) goto read_again; + if (unlikely(error)) { count++; continue; @@ -5763,7 +5771,7 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue) count++; } - if (status & rx_not_ls) { + if (in_progress) { rx_q->state_saved = true; rx_q->state.error = error; rx_q->state.len = len; @@ -5805,9 +5813,10 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue) struct stmmac_rx_queue *rx_q = &priv->dma_conf.rx_queue[queue]; struct stmmac_channel *ch = &priv->channel[queue]; unsigned int count = 0, error = 0, len = 0; - int status = 0, coe = priv->hw->rx_csum; unsigned int next_entry = rx_q->cur_rx; + bool in_progress = rx_q->state_saved; enum dma_data_direction dma_dir; + int coe = priv->hw->rx_csum; unsigned int desc_size; struct sk_buff *skb = NULL; struct stmmac_xdp_buff ctx; @@ -5827,25 +5836,28 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue) stmmac_display_ring(priv, rx_head, priv->dma_conf.dma_rx_size, true, rx_q->dma_rx_phy, desc_size); } + + if (rx_q->state_saved) { + skb = rx_q->state.skb; + error = rx_q->state.error; + len = rx_q->state.len; + rx_q->state.skb = NULL; + rx_q->state_saved = false; + } + while (count < limit) { unsigned int buf1_len = 0, buf2_len = 0; enum pkt_hash_types hash_type; struct stmmac_rx_buffer *buf; struct dma_desc *np, *p; - int entry; + int entry, status; u32 hash; - if (!count && rx_q->state_saved) { - skb = rx_q->state.skb; - error = rx_q->state.error; - len = rx_q->state.len; - } else { - rx_q->state_saved = false; + if (!in_progress) { skb = NULL; error = 0; len = 0; } - read_again: if (count >= limit) break; @@ -5874,6 +5886,8 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue) prefetch(np); + in_progress = status & rx_not_ls; + if (priv->extend_desc) stmmac_rx_extended_status(priv, &priv->xstats, rx_q->dma_erx + entry); if (unlikely(status == discard_frame)) { @@ -5886,11 +5900,10 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue) if (unlikely(error && (status & rx_not_ls))) goto read_again; + if (unlikely(error)) { dev_kfree_skb(skb); - skb = NULL; - count++; - continue; + goto next; } /* Buffer is good. Go on. */ @@ -5950,23 +5963,21 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue) buf->page = NULL; rx_dropped++; - /* Clear skb as it was set as - * status by XDP program. - */ - skb = NULL; - - if (unlikely((status & rx_not_ls))) + if (unlikely((status & rx_not_ls))) { + skb = NULL; goto read_again; - - count++; - continue; + } + goto next; } else if (xdp_res & (STMMAC_XDP_TX | STMMAC_XDP_REDIRECT)) { xdp_status |= xdp_res; buf->page = NULL; - skb = NULL; - count++; - continue; + + if (unlikely((status & rx_not_ls))) { + skb = NULL; + goto read_again; + } + goto next; } } } @@ -6023,10 +6034,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; } stmmac_get_rx_hwtstamp(priv, p, np, skb); @@ -6051,14 +6060,15 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue) skb_record_rx_queue(skb, queue); napi_gro_receive(&ch->rx_napi, skb); - skb = NULL; - rx_packets++; rx_bytes += len; +next: + in_progress = false; + skb = NULL; count++; } - if (status & rx_not_ls || skb) { + if (in_progress || skb) { rx_q->state_saved = true; rx_q->state.skb = skb; rx_q->state.error = error; @@ -8378,6 +8388,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; } -- 2.56.0