__stmmac_open() copies the freshly allocated dma_conf into priv->dma_conf
via memcpy() before it can fail, e.g. in stmmac_request_irq().  When it
does fail, the callers free the dma_conf descriptor resources and the
struct itself, but priv->dma_conf still holds pointers to those freed
resources, a use-after-free that triggers when a subsequent
stmmac_release() or another MTU change releases the resources again.

Undo the memcpy in the error path by zeroing priv->dma_conf, leaving the
state consistent regardless of the caller.

Fixes: 30134b7c47bd2 ("net: ethernet: stmicro: stmmac: fix possible memory leak in __stmmac_open")
Signed-off-by: ZhaoJinming <zhaojinming@uniontech.com>
---
Changes in v2:
- Move the clearing of priv->dma_conf from stmmac_change_mtu() into
  __stmmac_open() error path, undoing the memcpy() at the point where it
  was made and covering both callers.
- Link to v1: https://lore.kernel.org/r/20260903-fix-stmmac-mtu-change-use-after-free-v1-1-c81dc7d6d18a@uniontech.com
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 24656b35350b14454fb10deced6516eb89e2c0c9..4369e64faf9f878aa20ac5075705044f55c5f8bf 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -4190,6 +4190,7 @@ static int __stmmac_open(struct net_device *dev,
 
 	stmmac_release_ptp(priv);
 init_error:
+	memset(&priv->dma_conf, 0, sizeof(priv->dma_conf));
 	return ret;
 }
 

---
base-commit: a500db7819c50db59e55f1b4fa1c3baa5a2616f3
change-id: 20260903-fix-stmmac-mtu-change-use-after-free-693da6eb4a30

Best regards,
-- 
ZhaoJinming <zhaojinming@uniontech.com>

> __stmmac_open() copies the freshly allocated dma_conf into priv->dma_conf
> via memcpy() before it can fail, e.g. in stmmac_request_irq().  When it
> does fail, the callers free the dma_conf descriptor resources and the
> struct itself, but priv->dma_conf still holds pointers to those freed
> resources, a use-after-free that triggers when a subsequent
> stmmac_release() or another MTU change releases the resources again.
> 
> Undo the memcpy in the error path by zeroing priv->dma_conf, leaving the
> state consistent regardless of the caller.
> 
> Fixes: 30134b7c47bd2 ("net: ethernet: stmicro: stmmac: fix possible memory leak in __stmmac_open")
> Signed-off-by: ZhaoJinming <zhaojinming@uniontech.com>

Hi ZhaoJinming,

I agree the issue is real, but assuming free_dma_desc_resources() always
tolerates a fully zeroed dma_conf struct seems a bit fragile to me.
Is it better to use a pointer for priv->dma_conf that we can set to NULL in
case of error? What do you think?

Regards,
Lorenzo

> ---
> Changes in v2:
> - Move the clearing of priv->dma_conf from stmmac_change_mtu() into
>   __stmmac_open() error path, undoing the memcpy() at the point where it
>   was made and covering both callers.
> - Link to v1: https://lore.kernel.org/r/20260903-fix-stmmac-mtu-change-use-after-free-v1-1-c81dc7d6d18a@uniontech.com
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 24656b35350b14454fb10deced6516eb89e2c0c9..4369e64faf9f878aa20ac5075705044f55c5f8bf 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -4190,6 +4190,7 @@ static int __stmmac_open(struct net_device *dev,
>  
>  	stmmac_release_ptp(priv);
>  init_error:
> +	memset(&priv->dma_conf, 0, sizeof(priv->dma_conf));
>  	return ret;
>  }
>  
> 
> ---
> base-commit: a500db7819c50db59e55f1b4fa1c3baa5a2616f3
> change-id: 20260903-fix-stmmac-mtu-change-use-after-free-693da6eb4a30
> 
> Best regards,
> -- 
> ZhaoJinming <zhaojinming@uniontech.com>
> 
> 

Thanks for the review.

I agree that using NULL as an explicit "no configuration" sentinel is cleaner than relying on an all-zero struct. However, the zeroed struct is currently safe to tear down, and that safety comes from established kernel conventions rather than from anything stmmac-specific:
 - kfree(NULL) is a no-op by definition.
 - dma_free_coherent() bails out early on a NULL cpu_addr (see kernel/dma/mapping.c, dma_free_attrs() -> "if (!cpu_addr) return;").
 - page_pool_destroy() is guarded by "if (rx_q->page_pool)".
 - xdp_rxq_info_unreg() is guarded by xdp_rxq_info_is_reg().
 - The skb/descriptor loops are bounded by dma_tx_size/dma_rx_size, which are zero after the memset, so they never dereference the (now NULL) buffer/descriptor pointers.
So while memset() does lean on those guards, they are not accidental and hold for the current code.

Converting priv->dma_conf to a pointer is a much larger change than it first appears: it is referenced ~120 times across six files (stmmac_main.c, stmmac_selftests.c, stmmac_tc.c, stmmac_ethtool.c, chain_mode.c, ring_mode.c), and would require changing the allocation lifetime in probe/remove plus NULL guards at every teardown site. That feels like net-next cleanup material rather than something to fold into a use-after-free fix that should be backported to stable.

Would you be OK with keeping the memset() here as the minimal fix, and I follow up with a separate net-next series to convert priv->dma_conf to a pointer? If you'd still prefer the pointer approach in this patch, I can do it, but I wanted to flag the scope first.

Regards, 
ZhaoJinming



>> __stmmac_open() copies the freshly allocated dma_conf into priv->dma_conf



>> via memcpy() before it can fail, e.g. in stmmac_request_irq().? When it



>> does fail, the callers free the dma_conf descriptor resources and the



>> struct itself, but priv->dma_conf still holds pointers to those freed



>> resources, a use-after-free that triggers when a subsequent



>> stmmac_release() or another MTU change releases the resources again.



>> 



>> Undo the memcpy in the error path by zeroing priv->dma_conf, leaving the



>> state consistent regardless of the caller.



>> 



>> Fixes: 30134b7c47bd2 ("net: ethernet: stmicro: stmmac: fix possible memory leak in __stmmac_open")



>> Signed-off-by: ZhaoJinming <zhaojinming@uniontech.com>



>



>Hi ZhaoJinming,



>



>I agree the issue is real, but assuming free_dma_desc_resources() always



>tolerates a fully zeroed dma_conf struct seems a bit fragile to me.



>Is it better to use a pointer for priv->dma_conf that we can set to NULL in



>case of error? What do you think?



>



>Regards,



>Lorenzo



>



>> ---



>> Changes in v2:



>> - Move the clearing of priv->dma_conf from stmmac_change_mtu() into



>>?? __stmmac_open() error path, undoing the memcpy() at the point where it



>>?? was made and covering both callers.



>> - Link to v1: https://lore.kernel.org/r/20260903-fix-stmmac-mtu-change-use-after-free-v1-1-c81dc7d6d18a@uniontech.com



>> ---



>>? drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 1 +



>>? 1 file changed, 1 insertion(+)



>> 



>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c



>> index 24656b35350b14454fb10deced6516eb89e2c0c9..4369e64faf9f878aa20ac5075705044f55c5f8bf 100644



>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c



>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c



>> @@ -4190,6 +4190,7 @@ static int __stmmac_open(struct net_device *dev,



>>? 



>>? 	stmmac_release_ptp(priv);



>>? init_error:



>> +	memset(&priv->dma_conf, 0, sizeof(priv->dma_conf));



>>? 	return ret;



>>? }



>>? 



>> 



>> ---



>> base-commit: a500db7819c50db59e55f1b4fa1c3baa5a2616f3



>> change-id: 20260903-fix-stmmac-mtu-change-use-after-free-693da6eb4a30



>> 



>> Best regards,



>> -- 



>> ZhaoJinming <zhaojinming@uniontech.com>



>> 



>>