Nouveau - Aug 2025 - [PATCH v2 1/3] drm/nouveau: fix error path in nvkm_gsp_fwsec_v2

Function nvkm_gsp_fwsec_v2() sets 'ret' if the kmemdup() call fails, but
it never uses or returns 'ret' after that point.  We always need to
release
the firmware regardless, so do that and then check for error.

Fixes: 176fdcbddfd2 ("drm/nouveau/gsp/r535: add support for booting
GSP-RM")
Signed-off-by: Timur Tabi <ttabi at nvidia.com>
---
 drivers/gpu/drm/nouveau/nvkm/subdev/gsp/fwsec.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/fwsec.c
b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/fwsec.c
index 52412965fac1..5b721bd9d799 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/fwsec.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/fwsec.c
@@ -209,11 +209,12 @@ nvkm_gsp_fwsec_v2(struct nvkm_gsp *gsp, const char *name,
 	fw->boot_addr = bld->start_tag << 8;
 	fw->boot_size = bld->code_size;
 	fw->boot = kmemdup(bl->data + hdr->data_offset + bld->code_off,
fw->boot_size, GFP_KERNEL);
-	if (!fw->boot)
-		ret = -ENOMEM;
 
 	nvkm_firmware_put(bl);
 
+	if (!fw->boot)
+		return -ENOMEM;
+
 	/* Patch in interface data. */
 	return nvkm_gsp_fwsec_patch(gsp, fw, desc->InterfaceOffset, init_cmd);
 }
-- 
2.43.0

On Mon, 2025-08-04 at 14:25 -0500, Timur Tabi wrote:
> Function nvkm_gsp_fwsec_v2() sets 'ret' if the kmemdup() call
fails,
> but
> it never uses or returns 'ret' after that point.? We always need to
> release
> the firmware regardless, so do that and then check for error.
> 
> Fixes: 176fdcbddfd2 ("drm/nouveau/gsp/r535: add support for booting
> GSP-RM")
> Signed-off-by: Timur Tabi <ttabi at nvidia.com>
That looks like a bug (leak?) to me.

+Cc stable?

P.
> ---
> ?drivers/gpu/drm/nouveau/nvkm/subdev/gsp/fwsec.c | 5 +++--
> ?1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/fwsec.c
> b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/fwsec.c
> index 52412965fac1..5b721bd9d799 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/fwsec.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/fwsec.c
> @@ -209,11 +209,12 @@ nvkm_gsp_fwsec_v2(struct nvkm_gsp *gsp, const
> char *name,
> ?	fw->boot_addr = bld->start_tag << 8;
> ?	fw->boot_size = bld->code_size;
> ?	fw->boot = kmemdup(bl->data + hdr->data_offset + bld-
> >code_off, fw->boot_size, GFP_KERNEL);
> -	if (!fw->boot)
> -		ret = -ENOMEM;
> ?
> ?	nvkm_firmware_put(bl);
> ?
> +	if (!fw->boot)
> +		return -ENOMEM;
> +
> ?	/* Patch in interface data. */
> ?	return nvkm_gsp_fwsec_patch(gsp, fw, desc->InterfaceOffset,
> init_cmd);
> ?}

On 8/4/25 9:25 PM, Timur Tabi wrote:
> Function nvkm_gsp_fwsec_v2() sets 'ret' if the kmemdup() call
fails, but
> it never uses or returns 'ret' after that point.  We always need to
release
> the firmware regardless, so do that and then check for error.
> 
> Fixes: 176fdcbddfd2 ("drm/nouveau/gsp/r535: add support for booting
GSP-RM")
As mentioned by Philipp, we should Cc stable for this one.
> Signed-off-by: Timur Tabi <ttabi at nvidia.com>
> ---
>   drivers/gpu/drm/nouveau/nvkm/subdev/gsp/fwsec.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/fwsec.c
b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/fwsec.c
> index 52412965fac1..5b721bd9d799 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/fwsec.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/fwsec.c
> @@ -209,11 +209,12 @@ nvkm_gsp_fwsec_v2(struct nvkm_gsp *gsp, const char
*name,
>   	fw->boot_addr = bld->start_tag << 8;
>   	fw->boot_size = bld->code_size;
>   	fw->boot = kmemdup(bl->data + hdr->data_offset +
bld->code_off, fw->boot_size, GFP_KERNEL);
> -	if (!fw->boot)
> -		ret = -ENOMEM;
>   
>   	nvkm_firmware_put(bl);
>   
> +	if (!fw->boot)
> +		return -ENOMEM;
Good catch! It's also good that you moved the return below the
nvkm_firmware_put() call.

But don't we also need to revert the preceding call to
nvkm_falcon_fw_ctor()?
> +
>   	/* Patch in interface data. */
>   	return nvkm_gsp_fwsec_patch(gsp, fw, desc->InterfaceOffset,
init_cmd);
>   }