Re: [PATCH v2] 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 when parsing firmware headers allows out-of-bounds memory accesses.
--

commit 72846afc6e077321cb6e56447e8430295e342e68
Author: Dawei Feng <dawei.feng@seu.edu.cn>

nouveau/firmware: fix memory leak on BL load failure

This patch addresses a memory leak when loading the HS bootloader blob
fails. It routes the failure to the done label so that the partially
initialized firmware state can be properly cleaned up rather than returning
immediately.

> diff --git a/drivers/gpu/drm/nouveau/nvkm/falcon/fw.c b/drivers/gpu/drm/nouveau/nvkm/falcon/fw.c
> --- a/drivers/gpu/drm/nouveau/nvkm/falcon/fw.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/falcon/fw.c

[ ... ]

> @@ -277,19 +277,20 @@ nvkm_falcon_fw_ctor_hs(const struct nvkm_falcon_fw_func *func, const char *name,
>  	fw->dmem_sign = loc - lhdr->data_dma_base;
>  
>  	if (bl) {
> -		nvkm_firmware_put(blob);
> +		const struct firmware *blob_bl;
>  
> -		ret = nvkm_firmware_load_name(subdev, bl, "", ver, &blob);
> +		ret = nvkm_firmware_load_name(subdev, bl, "", ver, &blob_bl);
>  		if (ret)
> -			return ret;
> +			goto done;
>  
> -		hdr = nvfw_bin_hdr(subdev, blob->data);
> -		desc = nvfw_bl_desc(subdev, blob->data + hdr->header_offset);
> +		hdr = nvfw_bin_hdr(subdev, blob_bl->data);
> +		desc = nvfw_bl_desc(subdev, blob_bl->data + hdr->header_offset);
>  
>  		fw->boot_addr = desc->start_tag << 8;
>  		fw->boot_size = desc->code_size;
> -		fw->boot = kmemdup(blob->data + hdr->data_offset + desc->code_off,
> +		fw->boot = kmemdup(blob_bl->data + hdr->data_offset + desc->code_off,
>  				   fw->boot_size, GFP_KERNEL);

[Severity: High]
This isn't a bug introduced by this patch, but does the firmware parsing
code in nvkm_falcon_fw_ctor_hs() lack bounds checking?

The offsets (such as hdr->header_offset, hdr->data_offset, and
desc->code_off) and sizes (like desc->code_size) are read directly from the
firmware blob. Because functions like nvfw_bin_hdr() and nvfw_bl_desc() do
not take the buffer size as an argument, they cannot perform structural
validation.

Could a malformed or corrupted firmware file cause an out-of-bounds memory
access when kmemdup() tries to copy data, or trigger an unusually large
allocation if desc->code_size is excessively large without being validated
against the total firmware size (blob_bl->size)?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260610025037.4115412-1-dawei.feng@seu.edu.cn?part=1