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

quoted

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

quoted

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

quoted

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

quoted

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

quoted

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

quoted

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

quoted

quoted

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

quoted

state consistent regardless of the caller.

quoted

quoted

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

quoted

Signed-off-by: ZhaoJinming <redacted>

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

quoted

---

quoted

Changes in v2:

quoted

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

quoted

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

quoted

?? was made and covering both callers.

quoted

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

quoted

---

quoted

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

quoted

? 1 file changed, 1 insertion(+)

quoted

quoted

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

quoted

index 24656b35350b14454fb10deced6516eb89e2c0c9..4369e64faf9f878aa20ac5075705044f55c5f8bf 100644

quoted

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

quoted

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

quoted

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

quoted

? 

quoted

? 	stmmac_release_ptp(priv);

quoted

? init_error:

quoted

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

quoted

? 	return ret;

quoted

? }

quoted

? 

quoted

quoted

---

quoted

base-commit: a500db7819c50db59e55f1b4fa1c3baa5a2616f3

quoted

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

quoted

quoted

Best regards,

quoted

-- 

quoted

ZhaoJinming [off-list ref]

quoted

quoted