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

[Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>]

[-- Attachment #1: Type: text/plain, Size: 2228 bytes --]

> __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>
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]