[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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing bounds checking on firmware blob offsets allows out-of-bounds kernel memory reads.
--

commit 69b65113e7205835c286bfc4a8d86163010bab77
Author: Dawei Feng <dawei.feng@seu.edu.cn>

nouveau/firmware: fix memory leak on BL load failure

If loading the HS bootloader blob fails, nvkm_falcon_fw_ctor_hs() returns
immediately, skipping the common cleanup path and leaking firmware state.
This patch routes the load failure to the 'done' label to properly clean up
the partially initialized state and clears the original blob pointer.

> diff --git a/drivers/gpu/drm/nouveau/nvkm/falcon/fw.c b/drivers/gpu/drm/nouveau/nvkm/falcon/fw.c
> index 4e8b3f1c7e25d..71f55c5b08374 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);

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a pre-existing 
issue with bounds checking when parsing the firmware blobs.

Does this code safely handle potentially malformed firmware files? When the 
firmware headers are parsed, offsets and sizes like hdr->data_offset, 
desc->code_off, and desc->code_size are extracted without being validated 
against the actual blob->size.

Just after this hunk in nvkm_falcon_fw_ctor_hs(), the unvalidated values 
are used to calculate the source pointer for kmemdup():

    fw->boot = kmemdup(blob->data + hdr->data_offset + desc->code_off,
                       fw->boot_size, GFP_KERNEL);

Could this lead to an out-of-bounds read on the kernel heap if a corrupted 
firmware blob is provided? If the read crosses into unmapped memory, it 
might trigger a kernel oops.

Similarly, earlier in the same function, nvkm_falcon_fw_ctor() is called 
with unchecked offsets:

    ret = nvkm_falcon_fw_ctor(func, name, subdev->device, bl != NULL,
                              blob->data + hdr->data_offset,
                              hdr->data_size, falcon, fw);

Would it be helpful to add validation checks against blob->size before 
these header values are consumed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260605020752.1707562-1-dawei.feng@seu.edu.cn?part=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.