Re: [PATCH v2] net: stmmac: clear dma_conf in __stmmac_open error path

[赵金明]

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@xxxxxxxxxxxxx>



>



>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@xxxxxxxxxxxxx



>> ---



>>? 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@xxxxxxxxxxxxx>



>> 



>>