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

[Lorenzo Bianconi]

> 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. Release a
> saved partial skb when the RX ring is destroyed.
> 
> Fixes: ec222003bd94 ("net: stmmac: Prepare to add Split Header support")
> Signed-off-by: James Hilliard <james.hilliard1@xxxxxxxxx>

Hi James,

I guess we have a similar issue for stmmac_rx_zc() path as well, can you please
fix it as well?

> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 25 ++++++++++++++++-------
>  1 file changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index ec62fa7418f4..1a4d03aaaf78 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -2149,6 +2149,11 @@ static void __free_dma_rx_desc_resources(struct stmmac_priv *priv,
>  	else
>  		dma_free_rx_skbufs(priv, dma_conf, queue);
>  
> +	if (rx_q->state_saved)
> +		dev_kfree_skb_any(rx_q->state.skb);

nit: you can drop if (rx_q->state_saved) and just run dev_kfree_skb_any().

> +	rx_q->state.skb = NULL;
> +	rx_q->state_saved = 0;

nit: rx_q->state_saved = false;

> +
>  	rx_q->buf_alloc_num = 0;
>  	rx_q->xsk_pool = NULL;
>  
> @@ -5726,6 +5731,7 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
>  	struct stmmac_xdp_buff ctx;
>  	bool fcs_stripped = false;
>  	int xdp_status = 0;
> +	bool in_progress = rx_q->state_saved;

can you please respect RCT here?

Regards,
Lorenzo

>  	int bufsz;
>  
>  	dma_dir = page_pool_get_dma_dir(rx_q->page_pool);
> @@ -5740,6 +5746,14 @@ 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 (in_progress) {
> +		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;
> @@ -5748,12 +5762,7 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
>  		int entry;
>  		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;
> @@ -5787,6 +5796,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)) {
> @@ -5971,7 +5982,7 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
>  		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;
> 
> ---
> base-commit: 7375d38364a9aa66fb31716bcefef38aecad75d8
> change-id: 20260930-stmmac-rx-state-041371e43e8c
> 
> Best regards,
> --  
> James Hilliard <james.hilliard1@xxxxxxxxx>
> 
>

Attachment: signature.asc
Description: PGP signature