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

[sashiko-bot@kernel.org]

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