Re: [PATCH] EDAC/versalnet: fix memory leak on device registration failure

[Shubhrajyoti Datta]

On Sun, Sep 20, 2026 at 11:25 AM Guangshuo Li <lgs201920130244@xxxxxxxxx> wrote:
>
> init_one_mc() allocates dev with kzalloc() and registers it with
> device_register(). If device_register() fails, the device has already
> been initialized and holds its initial device reference.
>
> The current error path eventually frees dev directly with kfree().
> This bypasses the device release path and can leak driver-core
> resources associated with the initialized device. The existing
> versal_edac_release() callback is responsible for freeing dev once the
> device reference reaches zero.
>
> Call put_device() when device_register() fails so the initialized
> device reference is dropped and versal_edac_release() performs the
> proper cleanup. Return after freeing mci to avoid falling through to
> the direct kfree() path, which remains necessary for failures that
> occur before device_register() is called.
>
> The issue was identified by a static analysis tool I developed and
> confirmed by manual review.
>
> Fixes: d5fe2fec6c40 ("EDAC: Add a driver for the AMD Versal NET DDR controller")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Guangshuo Li <lgs201920130244@xxxxxxxxx>
> ---
>  drivers/edac/versalnet_edac.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c
> index 3a06b41c1d84..db3bf5a1345b 100644
> --- a/drivers/edac/versalnet_edac.c
> +++ b/drivers/edac/versalnet_edac.c
> @@ -829,8 +829,10 @@ static int init_one_mc(struct mc_priv *priv, struct platform_device *pdev, int i
>         dev->release = versal_edac_release;
>
>         rc = device_register(dev);
> -       if (rc)
> +       if (rc) {
> +               put_device(dev);
>                 goto err_mc_free;
> +       }

There was a comment earlier from sashiko
 failure path calls put_device() with dev->init_name still pointing at a stack
buffer before device_add() copies it. That's unsafe in
principle (dev_name() would follow init_name)
>
>         mci->pdev = dev;
>         mc_init(mci, dev);
> @@ -852,9 +854,9 @@ static int init_one_mc(struct mc_priv *priv, struct platform_device *pdev, int i
>         device_unregister(mci->pdev);
>  err_mc_free:
>         edac_mc_free(mci);
> +       return rc;
>  err_dev_free:
>         kfree(dev);
> -
>         return rc;
>  }
>
> --
> 2.43.0
>
>