[PATCH v2] firmware_loader: fix device reference leak in firmware_upload_register()

[PATCH v2] firmware_loader: fix device reference leak in firmware_upload_register()

[Guangshuo Li]

firmware_upload_register()
  -> fw_create_instance()
     -> device_initialize()

After fw_create_instance() succeeds, the lifetime of the embedded struct
device is expected to be managed through the device core reference
counting, since fw_create_instance() has already called
device_initialize().

In firmware_upload_register(), if alloc_lookup_fw_priv() fails after
fw_create_instance() succeeds, the code reaches free_fw_sysfs and frees
fw_sysfs directly instead of releasing the device reference with
put_device(). This may leave the reference count of the embedded struct
device unbalanced, resulting in a refcount leak.

The issue was identified by a static analysis tool I developed and
confirmed by manual review. Fix this by using put_device(fw_dev) in the
failure path and letting fw_dev_release() handle the final cleanup,
instead of freeing the instance directly from the error path.

Fixes: 97730bbb242c ("firmware_loader: Add firmware-upload support")
Cc: stable@vger.kernel.org
Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
---
 drivers/base/firmware_loader/sysfs_upload.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/base/firmware_loader/sysfs_upload.c b/drivers/base/firmware_loader/sysfs_upload.c
index f59a7856934c..a6dab34b22d8 100644
--- a/drivers/base/firmware_loader/sysfs_upload.c
+++ b/drivers/base/firmware_loader/sysfs_upload.c
@@ -351,7 +351,8 @@ firmware_upload_register(struct module *module, struct device *parent,
 	if (ret != 0) {
 		if (ret > 0)
 			ret = -EINVAL;
-		goto free_fw_sysfs;
+		put_device(fw_dev);
+		goto exit_module_put;
 	}
 	fw_priv->is_paged_buf = true;
 	fw_sysfs->fw_priv = fw_priv;
@@ -365,9 +366,6 @@ firmware_upload_register(struct module *module, struct device *parent,
 
 	return fw_upload;
 
-free_fw_sysfs:
-	kfree(fw_sysfs);
-
 free_fw_upload_priv:
 	kfree(fw_upload_priv);
 
-- 
2.43.0

Re: [PATCH v2] firmware_loader: fix device reference leak in firmware_upload_register()

[Russ Weight]

On Sat, Apr 18, 2026 at 03:02:20PM +0800, Guangshuo Li wrote:
> firmware_upload_register()
>   -> fw_create_instance()
>      -> device_initialize()
> 
> After fw_create_instance() succeeds, the lifetime of the embedded struct
> device is expected to be managed through the device core reference
> counting, since fw_create_instance() has already called
> device_initialize().
> 
> In firmware_upload_register(), if alloc_lookup_fw_priv() fails after
> fw_create_instance() succeeds, the code reaches free_fw_sysfs and frees
> fw_sysfs directly instead of releasing the device reference with
> put_device(). This may leave the reference count of the embedded struct
> device unbalanced, resulting in a refcount leak.
> 
> The issue was identified by a static analysis tool I developed and
> confirmed by manual review. Fix this by using put_device(fw_dev) in the
> failure path and letting fw_dev_release() handle the final cleanup,
> instead of freeing the instance directly from the error path.
> 
> Fixes: 97730bbb242c ("firmware_loader: Add firmware-upload support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>

Reviewed-by: Russ Weight <russ.weight@linux.dev>

> ---
>  drivers/base/firmware_loader/sysfs_upload.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/base/firmware_loader/sysfs_upload.c b/drivers/base/firmware_loader/sysfs_upload.c
> index f59a7856934c..a6dab34b22d8 100644
> --- a/drivers/base/firmware_loader/sysfs_upload.c
> +++ b/drivers/base/firmware_loader/sysfs_upload.c
> @@ -351,7 +351,8 @@ firmware_upload_register(struct module *module, struct device *parent,
>  	if (ret != 0) {
>  		if (ret > 0)
>  			ret = -EINVAL;
> -		goto free_fw_sysfs;
> +		put_device(fw_dev);
> +		goto exit_module_put;
>  	}
>  	fw_priv->is_paged_buf = true;
>  	fw_sysfs->fw_priv = fw_priv;
> @@ -365,9 +366,6 @@ firmware_upload_register(struct module *module, struct device *parent,
>  
>  	return fw_upload;
>  
> -free_fw_sysfs:
> -	kfree(fw_sysfs);
> -
>  free_fw_upload_priv:
>  	kfree(fw_upload_priv);
>  
> -- 
> 2.43.0
> 

Re: [PATCH v2] firmware_loader: fix device reference leak in firmware_upload_register()

[Danilo Krummrich]

On Sat Apr 18, 2026 at 9:02 AM CEST, Guangshuo Li wrote:
> diff --git a/drivers/base/firmware_loader/sysfs_upload.c b/drivers/base/firmware_loader/sysfs_upload.c
> index f59a7856934c..a6dab34b22d8 100644
> --- a/drivers/base/firmware_loader/sysfs_upload.c
> +++ b/drivers/base/firmware_loader/sysfs_upload.c
> @@ -351,7 +351,8 @@ firmware_upload_register(struct module *module, struct device *parent,
>  	if (ret != 0) {
>  		if (ret > 0)
>  			ret = -EINVAL;
> -		goto free_fw_sysfs;
> +		put_device(fw_dev);
> +		goto exit_module_put;
>  	}
>  	fw_priv->is_paged_buf = true;
>  	fw_sysfs->fw_priv = fw_priv;

I think this assignment comes too late, by calling put_device() we eventually
end up in fw_upload_free() which expects this to be assigned already.

I didn't think it through entirely, but can we just move the

	fw_sysfs->fw_upload_priv = fw_upload_priv;

assignment to after alloc_lookup_fw_priv(), so fw_dev_release() does not enter
this path in the first place?

Re: [PATCH v2] firmware_loader: fix device reference leak in firmware_upload_register()

[Guangshuo Li]

Hi Danilo, Russ,

Thanks for your reviewing.

On Tue, 5 May 2026 at 03:18, Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Sat Apr 18, 2026 at 9:02 AM CEST, Guangshuo Li wrote:
> > diff --git a/drivers/base/firmware_loader/sysfs_upload.c b/drivers/base/firmware_loader/sysfs_upload.c
> > index f59a7856934c..a6dab34b22d8 100644
> > --- a/drivers/base/firmware_loader/sysfs_upload.c
> > +++ b/drivers/base/firmware_loader/sysfs_upload.c
> > @@ -351,7 +351,8 @@ firmware_upload_register(struct module *module, struct device *parent,
> >       if (ret != 0) {
> >               if (ret > 0)
> >                       ret = -EINVAL;
> > -             goto free_fw_sysfs;
> > +             put_device(fw_dev);
> > +             goto exit_module_put;
> >       }
> >       fw_priv->is_paged_buf = true;
> >       fw_sysfs->fw_priv = fw_priv;
>
> I think this assignment comes too late, by calling put_device() we eventually
> end up in fw_upload_free() which expects this to be assigned already.
>
> I didn't think it through entirely, but can we just move the
>
>         fw_sysfs->fw_upload_priv = fw_upload_priv;
>
> assignment to after alloc_lookup_fw_priv(), so fw_dev_release() does not enter
> this path in the first place?

After manual re-review, I confirmed that the issue Danilo pointed out
does exist. In the current v2 patch, fw_sysfs->fw_upload_priv has
already been assigned before alloc_lookup_fw_priv() succeeds, while
fw_sysfs->fw_priv has not been assigned yet. Therefore, calling
put_device() on this failure path may eventually enter
fw_upload_free(), which expects fw_sysfs->fw_priv to be valid.

I will address this in a v3 patch.

Thanks,
Guangshuo