[PATCH] nouveau/firmware: fix memory leak on BL load failure

[PATCH] nouveau/firmware: fix memory leak on BL load failure

[Dawei Feng]

If loading the HS bootloader blob fails, nvkm_falcon_fw_ctor_hs() returns
immediately. This skips the common cleanup path and leaks the firmware
state allocated by nvkm_falcon_fw_ctor() and nvkm_falcon_fw_sign().

Fix this by routing the load failure to the 'done' label so
nvkm_falcon_fw_dtor() can properly clean up the partially initialized
state. Also clear the original 'blob' pointer after releasing it so the
final nvkm_firmware_put() remains balanced after a failed bootloader
reload.

The bug was first flagged by an experimental analysis tool we are
developing for kernel memory-management bugs while analyzing
v6.13-rc1. The tool is still under development and is not yet publicly
available. Manual inspection confirms that the bug is still present in
v7.1-rc6.

An x86_64 allyesconfig build showed no new warnings. As we do not have a
supported NVIDIA GPU with the required firmware to test this path, no
runtime testing was able to be performed.

Fixes: 2541626cfb79 ("drm/nouveau/acr: use common falcon HS FW code for ACR FWs")
Cc: stable@vger.kernel.org
Signed-off-by: Zilin Guan <zilin@seu.edu.cn>
Signed-off-by: Dawei Feng <dawei.feng@seu.edu.cn>
---
 drivers/gpu/drm/nouveau/nvkm/falcon/fw.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/nouveau/nvkm/falcon/fw.c b/drivers/gpu/drm/nouveau/nvkm/falcon/fw.c
index 4e8b3f1c7e25..71f55c5b0837 100644
--- a/drivers/gpu/drm/nouveau/nvkm/falcon/fw.c
+++ b/drivers/gpu/drm/nouveau/nvkm/falcon/fw.c
@@ -278,10 +278,11 @@ nvkm_falcon_fw_ctor_hs(const struct nvkm_falcon_fw_func *func, const char *name,
 
 	if (bl) {
 		nvkm_firmware_put(blob);
+		blob = NULL;
 
 		ret = nvkm_firmware_load_name(subdev, bl, "", ver, &blob);
 		if (ret)
-			return ret;
+			goto done;
 
 		hdr = nvfw_bin_hdr(subdev, blob->data);
 		desc = nvfw_bl_desc(subdev, blob->data + hdr->header_offset);
-- 
2.34.1

Re: [PATCH] nouveau/firmware: fix memory leak on BL load failure

[Timur Tabi]

On Fri, 2026-06-05 at 10:07 +0800, Dawei Feng wrote:
>  	if (bl) {
>  		nvkm_firmware_put(blob);
> +		blob = NULL;
>  

I think it would be cleaner to instead delete this nvkm_firmware_put(blob) call here, and just rely
on the call to nvkm_firmware_put() at the end of nvkm_falcon_fw_ctor_hs().  Then you won't need
"blob = NULL".

Re: [PATCH] nouveau/firmware: fix memory leak on BL load failure

[Dawei Feng]

Hi Timur,

On Fri, Jun 05, 2026 at 06:22:41PM +0000, Timur Tabi wrote:
> I think it would be cleaner to instead delete this
> nvkm_firmware_put(blob) call here, and just rely on the call to
> nvkm_firmware_put() at the end of nvkm_falcon_fw_ctor_hs(). Then you
> won't need "blob = NULL".

Thanks for your review.

I don't think we can drop the nvkm_firmware_put(blob) here. At this
point, blob still points to the image firmware loaded at the beginning of
nvkm_falcon_fw_ctor_hs(). The later nvkm_firmware_load_name(..., &blob)
call overwrites blob with the bootloader firmware pointer on success.

If we only rely on the final nvkm_firmware_put(blob), the success path
would release the bootloader firmware, but the original image firmware
pointer would be lost and leaked.

Best regards,
Dawei

Re: [PATCH] nouveau/firmware: fix memory leak on BL load failure

[Timur Tabi]

On Mon, 2026-06-08 at 21:52 +0800, Dawei Feng wrote:
> Hi Timur,
> 
> On Fri, Jun 05, 2026 at 06:22:41PM +0000, Timur Tabi wrote:
> > I think it would be cleaner to instead delete this
> > nvkm_firmware_put(blob) call here, and just rely on the call to
> > nvkm_firmware_put() at the end of nvkm_falcon_fw_ctor_hs(). Then you
> > won't need "blob = NULL".
> 
> Thanks for your review.
> 
> I don't think we can drop the nvkm_firmware_put(blob) here. At this
> point, blob still points to the image firmware loaded at the beginning of
> nvkm_falcon_fw_ctor_hs(). The later nvkm_firmware_load_name(..., &blob)
> call overwrites blob with the bootloader firmware pointer on success.
> 
> If we only rely on the final nvkm_firmware_put(blob), the success path
> would release the bootloader firmware, but the original image firmware
> pointer would be lost and leaked.

Ah yes, you're right.  

So now I think a better fix might be to have two different `blob` variables, so that there is no
longer any confusion.  Because right now, the nvkm_firmware_put() call at the end of the function
releases a different `blob` depending on whether `bl` is NULL or not.

What do you think about this:

	nvkm_firmware_put(blob);
	if (bl) {
		const struct firmware *blob_bl;

		ret = nvkm_firmware_load_name(subdev, bl, "", ver, &blob_bl);
		if (ret)
			goto done;
		...
		nvkm_firmware_put(blob_bl);
		if (!fw->boot)
			ret = -ENOMEM;
	} else {
		fw->boot_addr = fw->nmem_base;
	}

done:
	if (ret)
		nvkm_falcon_fw_dtor(fw);

	return ret;

This way, there is no confusion between the two blobs, and the bootloader blob exists only inside
the if-block that needs it.

Re: [PATCH] nouveau/firmware: fix memory leak on BL load failure

[Dawei Feng]

Hi, Timur,

On Fri, Jun 05, 2026 at 06:22:41PM +0000, Timur Tabi wrote:
> Ah yes, you're right.
>
> So now I think a better fix might be to have two different `blob`
> variables, so that there is no longer any confusion. Because right now,
> the nvkm_firmware_put() call at the end of the function releases a
> different `blob` depending on whether `bl` is NULL or not.
>
> What do you think about this:
>
>     nvkm_firmware_put(blob);
>     if (bl) {
>             const struct firmware *blob_bl;
>
>             ret = nvkm_firmware_load_name(subdev, bl, "", ver, &blob_bl);
>             if (ret)
>                     goto done;
>             ...
>             nvkm_firmware_put(blob_bl);
>             if (!fw->boot)
>                     ret = -ENOMEM;
>     } else {
>             fw->boot_addr = fw->nmem_base;
>     }
>
> done:
>     if (ret)
>             nvkm_falcon_fw_dtor(fw);
>
>     return ret;

Yes, using a separate pointer `blob_bl` for the bootloader firmware is a
cleaner approach. 

However, we must keep the final nvkm_firmware_put(blob) under the done
label. Moving it earlier would cause memory leaks in prior error paths
like nvkm_falcon_fw_ctor(), which jump directly to done. 

A safer approach is to manage blob_bl locally inside the if (bl) block,
while leaving the original blob cleanup at the end. 

What do you think about this:

        if (bl) {
                const struct firmware *blob_bl;

                ret = nvkm_firmware_load_name(subdev, bl, "", ver, &blob_bl);
                if (ret)
                        goto done;

                ...
                nvkm_firmware_put(blob_bl);
                if (!fw->boot)
                        ret = -ENOMEM;
        } else {
                fw->boot_addr = fw->nmem_base;
        }

done:
        if (ret)
                nvkm_falcon_fw_dtor(fw);

        nvkm_firmware_put(blob);
        return ret;

Regards,
Dawei

Re: [PATCH] nouveau/firmware: fix memory leak on BL load failure

[Timur Tabi]

On Tue, 2026-06-09 at 16:57 +0800, Dawei Feng wrote:
>         if (bl) {
>                 const struct firmware *blob_bl;
> 
>                 ret = nvkm_firmware_load_name(subdev, bl, "", ver, &blob_bl);
>                 if (ret)
>                         goto done;
> 
>                 ...
>                 nvkm_firmware_put(blob_bl);
>                 if (!fw->boot)
>                         ret = -ENOMEM;
>         } else {
>                 fw->boot_addr = fw->nmem_base;
>         }
> 
> done:
>         if (ret)
>                 nvkm_falcon_fw_dtor(fw);
> 
>         nvkm_firmware_put(blob);
>         return ret;

Yes, this is good.  Thanks.