[PATCH] drm/nouveau/gsp: fix potential leak of memory used during acpi init

[PATCH] drm/nouveau/gsp: fix potential leak of memory used during acpi init

[Ben Skeggs]

If any of the ACPI calls fail, memory allocated for the input buffer
would be leaked.  Fix failure paths to free allocated memory.

Also add checks to ensure the allocations succeeded in the first place.

Reported-by: Danilo Krummrich <dakr@kernel.org>
Fixes: 176fdcbddfd2 ("drm/nouveau/gsp/r535: add support for booting GSP-RM")
Signed-off-by: Ben Skeggs <bskeggs@nvidia.com>
---
 .../drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c | 20 +++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
index baf42339f93e..b098a7555fde 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
@@ -719,7 +719,6 @@ r535_gsp_acpi_caps(acpi_handle handle, CAPS_METHOD_DATA *caps)
 	union acpi_object argv4 = {
 		.buffer.type    = ACPI_TYPE_BUFFER,
 		.buffer.length  = 4,
-		.buffer.pointer = kmalloc(argv4.buffer.length, GFP_KERNEL),
 	}, *obj;
 
 	caps->status = 0xffff;
@@ -727,17 +726,22 @@ r535_gsp_acpi_caps(acpi_handle handle, CAPS_METHOD_DATA *caps)
 	if (!acpi_check_dsm(handle, &NVOP_DSM_GUID, NVOP_DSM_REV, BIT_ULL(0x1a)))
 		return;
 
+	argv4.buffer.pointer = kmalloc(argv4.buffer.length, GFP_KERNEL);
+	if (!argv4.buffer.pointer)
+		return;
+
 	obj = acpi_evaluate_dsm(handle, &NVOP_DSM_GUID, NVOP_DSM_REV, 0x1a, &argv4);
 	if (!obj)
-		return;
+		goto done;
 
 	if (WARN_ON(obj->type != ACPI_TYPE_BUFFER) ||
 	    WARN_ON(obj->buffer.length != 4))
-		return;
+		goto done;
 
 	caps->status = 0;
 	caps->optimusCaps = *(u32 *)obj->buffer.pointer;
 
+done:
 	ACPI_FREE(obj);
 
 	kfree(argv4.buffer.pointer);
@@ -754,24 +758,28 @@ r535_gsp_acpi_jt(acpi_handle handle, JT_METHOD_DATA *jt)
 	union acpi_object argv4 = {
 		.buffer.type    = ACPI_TYPE_BUFFER,
 		.buffer.length  = sizeof(caps),
-		.buffer.pointer = kmalloc(argv4.buffer.length, GFP_KERNEL),
 	}, *obj;
 
 	jt->status = 0xffff;
 
+	argv4.buffer.pointer = kmalloc(argv4.buffer.length, GFP_KERNEL);
+	if (!argv4.buffer.pointer)
+		return;
+
 	obj = acpi_evaluate_dsm(handle, &JT_DSM_GUID, JT_DSM_REV, 0x1, &argv4);
 	if (!obj)
-		return;
+		goto done;
 
 	if (WARN_ON(obj->type != ACPI_TYPE_BUFFER) ||
 	    WARN_ON(obj->buffer.length != 4))
-		return;
+		goto done;
 
 	jt->status = 0;
 	jt->jtCaps = *(u32 *)obj->buffer.pointer;
 	jt->jtRevId = (jt->jtCaps & 0xfff00000) >> 20;
 	jt->bSBIOSCaps = 0;
 
+done:
 	ACPI_FREE(obj);
 
 	kfree(argv4.buffer.pointer);
-- 
2.49.0

Re: [PATCH] drm/nouveau/gsp: fix potential leak of memory used during acpi init

[Philipp Stanner]

On Tue, 2025-06-17 at 14:00 +1000, Ben Skeggs wrote:
> If any of the ACPI calls fail, memory allocated for the input buffer
> would be leaked.  Fix failure paths to free allocated memory.
> 
> Also add checks to ensure the allocations succeeded in the first
> place.
> 
> Reported-by: Danilo Krummrich <dakr@kernel.org>
> Fixes: 176fdcbddfd2 ("drm/nouveau/gsp/r535: add support for booting
> GSP-RM")

Needs to +Cc the stable kernel

But, see below


> Signed-off-by: Ben Skeggs <bskeggs@nvidia.com>
> ---
>  .../drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c | 20 +++++++++++++----
> --
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
> b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
> index baf42339f93e..b098a7555fde 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c

This seems to be based on a code move that is not yet in mainline.

Therefore, backporting the bugfix to stable seems difficult. Since that
code move is already in drm-misc-next, it would seem that it can only
be solved with two distinct patches for stable and for -next.

But this needs to be judged by a maintainer.

> @@ -719,7 +719,6 @@ r535_gsp_acpi_caps(acpi_handle handle,
> CAPS_METHOD_DATA *caps)
>  	union acpi_object argv4 = {
>  		.buffer.type    = ACPI_TYPE_BUFFER,
>  		.buffer.length  = 4,
> -		.buffer.pointer = kmalloc(argv4.buffer.length,
> GFP_KERNEL),
>  	}, *obj;
>  
>  	caps->status = 0xffff;
> @@ -727,17 +726,22 @@ r535_gsp_acpi_caps(acpi_handle handle,
> CAPS_METHOD_DATA *caps)
>  	if (!acpi_check_dsm(handle, &NVOP_DSM_GUID, NVOP_DSM_REV,
> BIT_ULL(0x1a)))
>  		return;
>  
> +	argv4.buffer.pointer = kmalloc(argv4.buffer.length,
> GFP_KERNEL);
> +	if (!argv4.buffer.pointer)
> +		return;
> +

This could be done immediately after the creation of argv4. That way
it's more difficult to have the leak again if something is inserted
later on.

>  	obj = acpi_evaluate_dsm(handle, &NVOP_DSM_GUID,
> NVOP_DSM_REV, 0x1a, &argv4);
>  	if (!obj)
> -		return;
> +		goto done;
>  
>  	if (WARN_ON(obj->type != ACPI_TYPE_BUFFER) ||
>  	    WARN_ON(obj->buffer.length != 4))
> -		return;
> +		goto done;
>  
>  	caps->status = 0;
>  	caps->optimusCaps = *(u32 *)obj->buffer.pointer;
>  
> +done:
>  	ACPI_FREE(obj);
>  
>  	kfree(argv4.buffer.pointer);
> @@ -754,24 +758,28 @@ r535_gsp_acpi_jt(acpi_handle handle,
> JT_METHOD_DATA *jt)
>  	union acpi_object argv4 = {
>  		.buffer.type    = ACPI_TYPE_BUFFER,
>  		.buffer.length  = sizeof(caps),
> -		.buffer.pointer = kmalloc(argv4.buffer.length,
> GFP_KERNEL),
>  	}, *obj;
>  
>  	jt->status = 0xffff;
>  
> +	argv4.buffer.pointer = kmalloc(argv4.buffer.length,
> GFP_KERNEL);
> +	if (!argv4.buffer.pointer)
> +		return;
> +
>  	obj = acpi_evaluate_dsm(handle, &JT_DSM_GUID, JT_DSM_REV,
> 0x1, &argv4);
>  	if (!obj)
> -		return;
> +		goto done;
>  
>  	if (WARN_ON(obj->type != ACPI_TYPE_BUFFER) ||
>  	    WARN_ON(obj->buffer.length != 4))
> -		return;
> +		goto done;
>  
>  	jt->status = 0;
>  	jt->jtCaps = *(u32 *)obj->buffer.pointer;
>  	jt->jtRevId = (jt->jtCaps & 0xfff00000) >> 20;
>  	jt->bSBIOSCaps = 0;
>  
> +done:

'done' seems like a bad name considering that the operations are
aborted with a WARN_ON above. Better 'abort' or sth like that.


P.

>  	ACPI_FREE(obj);
>  
>  	kfree(argv4.buffer.pointer);

Re: [PATCH] drm/nouveau/gsp: fix potential leak of memory used during acpi init

[Danilo Krummrich]

On Tue, Jun 17, 2025 at 01:29:20PM +0200, Philipp Stanner wrote:
> On Tue, 2025-06-17 at 14:00 +1000, Ben Skeggs wrote:
> > diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
> > b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
> > index baf42339f93e..b098a7555fde 100644
> > --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
> > +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
> 
> This seems to be based on a code move that is not yet in mainline.

It is, it did land with v6.16-rc1.

> Therefore, backporting the bugfix to stable seems difficult. Since that
> code move is already in drm-misc-next, it would seem that it can only
> be solved with two distinct patches for stable and for -next.

drm-misc-fixes is the relevant target branch and given the above, it contains
the code move as well.

However, you're right that this fix won't apply to anything before v6.16-rc1.
Given that, it makes sense to leave a note below the '---' line that this fix
won't apply before v6.16-rc1 and that a backported patch will be sent to stable
once this one hit Linus' tree.

> But this needs to be judged by a maintainer.
> 
> > @@ -719,7 +719,6 @@ r535_gsp_acpi_caps(acpi_handle handle,
> > CAPS_METHOD_DATA *caps)
> >  	union acpi_object argv4 = {
> >  		.buffer.type    = ACPI_TYPE_BUFFER,
> >  		.buffer.length  = 4,
> > -		.buffer.pointer = kmalloc(argv4.buffer.length,
> > GFP_KERNEL),
> >  	}, *obj;
> >  
> >  	caps->status = 0xffff;
> > @@ -727,17 +726,22 @@ r535_gsp_acpi_caps(acpi_handle handle,
> > CAPS_METHOD_DATA *caps)
> >  	if (!acpi_check_dsm(handle, &NVOP_DSM_GUID, NVOP_DSM_REV,
> > BIT_ULL(0x1a)))
> >  		return;
> >  
> > +	argv4.buffer.pointer = kmalloc(argv4.buffer.length,
> > GFP_KERNEL);
> > +	if (!argv4.buffer.pointer)
> > +		return;
> > +
> 
> This could be done immediately after the creation of argv4. That way
> it's more difficult to have the leak again if something is inserted
> later on.

I think the idea was to avoid a potential unwind path after acpi_check_dsm().

> >  	obj = acpi_evaluate_dsm(handle, &NVOP_DSM_GUID,
> > NVOP_DSM_REV, 0x1a, &argv4);
> >  	if (!obj)
> > -		return;
> > +		goto done;
> >  
> >  	if (WARN_ON(obj->type != ACPI_TYPE_BUFFER) ||
> >  	    WARN_ON(obj->buffer.length != 4))
> > -		return;
> > +		goto done;
> >  
> >  	caps->status = 0;
> >  	caps->optimusCaps = *(u32 *)obj->buffer.pointer;
> >  
> > +done:
> >  	ACPI_FREE(obj);
> >  
> >  	kfree(argv4.buffer.pointer);
> > @@ -754,24 +758,28 @@ r535_gsp_acpi_jt(acpi_handle handle,
> > JT_METHOD_DATA *jt)
> >  	union acpi_object argv4 = {
> >  		.buffer.type    = ACPI_TYPE_BUFFER,
> >  		.buffer.length  = sizeof(caps),
> > -		.buffer.pointer = kmalloc(argv4.buffer.length,
> > GFP_KERNEL),
> >  	}, *obj;
> >  
> >  	jt->status = 0xffff;
> >  
> > +	argv4.buffer.pointer = kmalloc(argv4.buffer.length,
> > GFP_KERNEL);
> > +	if (!argv4.buffer.pointer)
> > +		return;
> > +
> >  	obj = acpi_evaluate_dsm(handle, &JT_DSM_GUID, JT_DSM_REV,
> > 0x1, &argv4);
> >  	if (!obj)
> > -		return;
> > +		goto done;
> >  
> >  	if (WARN_ON(obj->type != ACPI_TYPE_BUFFER) ||
> >  	    WARN_ON(obj->buffer.length != 4))
> > -		return;
> > +		goto done;
> >  
> >  	jt->status = 0;
> >  	jt->jtCaps = *(u32 *)obj->buffer.pointer;
> >  	jt->jtRevId = (jt->jtCaps & 0xfff00000) >> 20;
> >  	jt->bSBIOSCaps = 0;
> >  
> > +done:
> 
> 'done' seems like a bad name considering that the operations are
> aborted with a WARN_ON above. Better 'abort' or sth like that.

I think some neutral name is fine, since we also enter this code path when
everything went well, maybe 'out_free' or just 'free'?

> P.
> 
> >  	ACPI_FREE(obj);
> >  
> >  	kfree(argv4.buffer.pointer);
> 

Re: [PATCH] drm/nouveau/gsp: fix potential leak of memory used during acpi init

[Danilo Krummrich]

On Tue, Jun 17, 2025 at 03:05:10PM +0200, Danilo Krummrich wrote:
> On Tue, Jun 17, 2025 at 01:29:20PM +0200, Philipp Stanner wrote:
> > On Tue, 2025-06-17 at 14:00 +1000, Ben Skeggs wrote:
> > > diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
> > > b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
> > > index baf42339f93e..b098a7555fde 100644
> > > --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
> > > +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
> > 
> > This seems to be based on a code move that is not yet in mainline.
> 
> It is, it did land with v6.16-rc1.
> 
> > Therefore, backporting the bugfix to stable seems difficult. Since that
> > code move is already in drm-misc-next, it would seem that it can only
> > be solved with two distinct patches for stable and for -next.
> 
> drm-misc-fixes is the relevant target branch and given the above, it contains
> the code move as well.
> 
> However, you're right that this fix won't apply to anything before v6.16-rc1.
> Given that, it makes sense to leave a note below the '---' line that this fix
> won't apply before v6.16-rc1 and that a backported patch will be sent to stable
> once this one hit Linus' tree.

Nevermind, I think this file wasn't split up and is correctly considered as
moved by git, so I think the patch should apply as is.

Re: [PATCH] drm/nouveau/gsp: fix potential leak of memory used during acpi init

[Philipp Stanner]

On Tue, 2025-06-17 at 14:00 +1000, Ben Skeggs wrote:
> If any of the ACPI calls fail, memory allocated for the input buffer
> would be leaked.  Fix failure paths to free allocated memory.
> 
> Also add checks to ensure the allocations succeeded in the first
> place.

If you've got a kmemleak trace, it would also be good to put it here in
the commit message so that we can distinguish this bug from potential
other leaks.


P.


> 
> Reported-by: Danilo Krummrich <dakr@kernel.org>
> Fixes: 176fdcbddfd2 ("drm/nouveau/gsp/r535: add support for booting
> GSP-RM")
> Signed-off-by: Ben Skeggs <bskeggs@nvidia.com>
> ---
>  .../drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c | 20 +++++++++++++----
> --
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
> b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
> index baf42339f93e..b098a7555fde 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
> @@ -719,7 +719,6 @@ r535_gsp_acpi_caps(acpi_handle handle,
> CAPS_METHOD_DATA *caps)
>  	union acpi_object argv4 = {
>  		.buffer.type    = ACPI_TYPE_BUFFER,
>  		.buffer.length  = 4,
> -		.buffer.pointer = kmalloc(argv4.buffer.length,
> GFP_KERNEL),
>  	}, *obj;
>  
>  	caps->status = 0xffff;
> @@ -727,17 +726,22 @@ r535_gsp_acpi_caps(acpi_handle handle,
> CAPS_METHOD_DATA *caps)
>  	if (!acpi_check_dsm(handle, &NVOP_DSM_GUID, NVOP_DSM_REV,
> BIT_ULL(0x1a)))
>  		return;
>  
> +	argv4.buffer.pointer = kmalloc(argv4.buffer.length,
> GFP_KERNEL);
> +	if (!argv4.buffer.pointer)
> +		return;
> +
>  	obj = acpi_evaluate_dsm(handle, &NVOP_DSM_GUID,
> NVOP_DSM_REV, 0x1a, &argv4);
>  	if (!obj)
> -		return;
> +		goto done;
>  
>  	if (WARN_ON(obj->type != ACPI_TYPE_BUFFER) ||
>  	    WARN_ON(obj->buffer.length != 4))
> -		return;
> +		goto done;
>  
>  	caps->status = 0;
>  	caps->optimusCaps = *(u32 *)obj->buffer.pointer;
>  
> +done:
>  	ACPI_FREE(obj);
>  
>  	kfree(argv4.buffer.pointer);
> @@ -754,24 +758,28 @@ r535_gsp_acpi_jt(acpi_handle handle,
> JT_METHOD_DATA *jt)
>  	union acpi_object argv4 = {
>  		.buffer.type    = ACPI_TYPE_BUFFER,
>  		.buffer.length  = sizeof(caps),
> -		.buffer.pointer = kmalloc(argv4.buffer.length,
> GFP_KERNEL),
>  	}, *obj;
>  
>  	jt->status = 0xffff;
>  
> +	argv4.buffer.pointer = kmalloc(argv4.buffer.length,
> GFP_KERNEL);
> +	if (!argv4.buffer.pointer)
> +		return;
> +
>  	obj = acpi_evaluate_dsm(handle, &JT_DSM_GUID, JT_DSM_REV,
> 0x1, &argv4);
>  	if (!obj)
> -		return;
> +		goto done;
>  
>  	if (WARN_ON(obj->type != ACPI_TYPE_BUFFER) ||
>  	    WARN_ON(obj->buffer.length != 4))
> -		return;
> +		goto done;
>  
>  	jt->status = 0;
>  	jt->jtCaps = *(u32 *)obj->buffer.pointer;
>  	jt->jtRevId = (jt->jtCaps & 0xfff00000) >> 20;
>  	jt->bSBIOSCaps = 0;
>  
> +done:
>  	ACPI_FREE(obj);
>  
>  	kfree(argv4.buffer.pointer);

Re: [PATCH] drm/nouveau/gsp: fix potential leak of memory used during acpi init

[Danilo Krummrich]

On 7/7/25 10:27 AM, Philipp Stanner wrote:
> On Tue, 2025-06-17 at 14:00 +1000, Ben Skeggs wrote:
>> If any of the ACPI calls fail, memory allocated for the input buffer
>> would be leaked.  Fix failure paths to free allocated memory.
>>
>> Also add checks to ensure the allocations succeeded in the first
>> place.
> 
> If you've got a kmemleak trace, it would also be good to put it here in
> the commit message so that we can distinguish this bug from potential
> other leaks.

unreferenced object 0xffff8ed5029bfb28 (size 8):
   comm "(udev-worker)", pid 464, jiffies 4294672444
   hex dump (first 8 bytes):
     7c b4 d4 79 01 59 36 6c                          |..y.Y6l
   backtrace (crc 4461fdb7):
     __kmalloc_cache_noprof+0x31b/0x410
     r535_gsp_acpi_jt+0x7c/0x110 [nouveau]
     r535_gsp_set_system_info+0x153/0x390 [nouveau]
     r535_gsp_oneinit+0xa4d/0xf50 [nouveau]
     tu102_gsp_oneinit+0x124/0x440 [nouveau]
     nvkm_subdev_oneinit_+0x3f/0x90 [nouveau]
     nvkm_subdev_init_+0x33/0xa0 [nouveau]
     nvkm_subdev_init+0x46/0x60 [nouveau]
     nvkm_device_init+0x167/0x1f0 [nouveau]
     nvkm_udevice_init+0x4b/0x70 [nouveau]
     nvkm_object_init+0x3a/0x110 [nouveau]
     nvkm_ioctl_new+0x13a/0x200 [nouveau]
     nvkm_ioctl+0x9f/0x140 [nouveau]
     nvif_object_ctor+0x11a/0x1a0 [nouveau]
     nvif_device_ctor+0x2a/0x80 [nouveau]
     nouveau_drm_device_new+0x157/0x2e0 [nouveau]
unreferenced object 0xffff8ed502a37580 (size 32):
   comm "(udev-worker)", pid 464, jiffies 4294672444
   hex dump (first 32 bytes):
     01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
   backtrace (crc f1da05aa):
     __kmalloc_noprof+0x3ac/0x500
     acpi_ut_initialize_buffer+0x67/0xc0
     acpi_evaluate_object+0x272/0x2c0
     acpi_evaluate_dsm+0xb4/0x120
     r535_gsp_acpi_jt+0xa3/0x110 [nouveau]
     r535_gsp_set_system_info+0x153/0x390 [nouveau]
     r535_gsp_oneinit+0xa4d/0xf50 [nouveau]
     tu102_gsp_oneinit+0x124/0x440 [nouveau]
     nvkm_subdev_oneinit_+0x3f/0x90 [nouveau]
     nvkm_subdev_init_+0x33/0xa0 [nouveau]
     nvkm_subdev_init+0x46/0x60 [nouveau]
     nvkm_device_init+0x167/0x1f0 [nouveau]
     nvkm_udevice_init+0x4b/0x70 [nouveau]
     nvkm_object_init+0x3a/0x110 [nouveau]
     nvkm_ioctl_new+0x13a/0x200 [nouveau]
     nvkm_ioctl+0x9f/0x140 [nouveau]
unreferenced object 0xffff8ed5029bf1c0 (size 8):
   comm "(udev-worker)", pid 464, jiffies 4294672446
   hex dump (first 8 bytes):
     cc bb d4 79 01 59 3c 84                          ...y.Y<.
   backtrace (crc 30e1d939):
     __kmalloc_cache_noprof+0x31b/0x410
     r535_gsp_acpi_caps+0x7e/0x120 [nouveau]
     r535_gsp_set_system_info+0x162/0x390 [nouveau]
     r535_gsp_oneinit+0xa4d/0xf50 [nouveau]
     tu102_gsp_oneinit+0x124/0x440 [nouveau]
     nvkm_subdev_oneinit_+0x3f/0x90 [nouveau]
     nvkm_subdev_init_+0x33/0xa0 [nouveau]
     nvkm_subdev_init+0x46/0x60 [nouveau]
     nvkm_device_init+0x167/0x1f0 [nouveau]
     nvkm_udevice_init+0x4b/0x70 [nouveau]
     nvkm_object_init+0x3a/0x110 [nouveau]
     nvkm_ioctl_new+0x13a/0x200 [nouveau]
     nvkm_ioctl+0x9f/0x140 [nouveau]
     nvif_object_ctor+0x11a/0x1a0 [nouveau]
     nvif_device_ctor+0x2a/0x80 [nouveau]
     nouveau_drm_device_new+0x157/0x2e0 [nouveau]

Re: [PATCH] drm/nouveau/gsp: fix potential leak of memory used during acpi init

[Danilo Krummrich]

On 6/17/25 6:00 AM, Ben Skeggs wrote:
> If any of the ACPI calls fail, memory allocated for the input buffer
> would be leaked.  Fix failure paths to free allocated memory.
> 
> Also add checks to ensure the allocations succeeded in the first place.
> 
> Reported-by: Danilo Krummrich <dakr@kernel.org>
> Fixes: 176fdcbddfd2 ("drm/nouveau/gsp/r535: add support for booting GSP-RM")
> Signed-off-by: Ben Skeggs <bskeggs@nvidia.com>

Applied to drm-misc-fixes, thanks!

Re: [PATCH] drm/nouveau/gsp: fix potential leak of memory used during acpi init

[Philipp Stanner]

On Mon, 2025-07-07 at 16:31 +0200, Danilo Krummrich wrote:
> On 7/7/25 10:27 AM, Philipp Stanner wrote:
> > On Tue, 2025-06-17 at 14:00 +1000, Ben Skeggs wrote:
> > > If any of the ACPI calls fail, memory allocated for the input buffer
> > > would be leaked.  Fix failure paths to free allocated memory.
> > > 
> > > Also add checks to ensure the allocations succeeded in the first
> > > place.
> > 
> > If you've got a kmemleak trace, it would also be good to put it here in
> > the commit message so that we can distinguish this bug from potential
> > other leaks.
> 
> unreferenced object 0xffff8ed5029bfb28 (size 8):
>    comm "(udev-worker)", pid 464, jiffies 4294672444
>    hex dump (first 8 bytes):
>      7c b4 d4 79 01 59 36 6c                          |..y.Y6l
>    backtrace (crc 4461fdb7):
>      __kmalloc_cache_noprof+0x31b/0x410
>      r535_gsp_acpi_jt+0x7c/0x110 [nouveau]
>      r535_gsp_set_system_info+0x153/0x390 [nouveau]
>      r535_gsp_oneinit+0xa4d/0xf50 [nouveau]
>      tu102_gsp_oneinit+0x124/0x440 [nouveau]
>      nvkm_subdev_oneinit_+0x3f/0x90 [nouveau]
>      nvkm_subdev_init_+0x33/0xa0 [nouveau]
>      nvkm_subdev_init+0x46/0x60 [nouveau]
>      nvkm_device_init+0x167/0x1f0 [nouveau]
>      nvkm_udevice_init+0x4b/0x70 [nouveau]
>      nvkm_object_init+0x3a/0x110 [nouveau]
>      nvkm_ioctl_new+0x13a/0x200 [nouveau]
>      nvkm_ioctl+0x9f/0x140 [nouveau]
>      nvif_object_ctor+0x11a/0x1a0 [nouveau]
>      nvif_device_ctor+0x2a/0x80 [nouveau]
>      nouveau_drm_device_new+0x157/0x2e0 [nouveau]
> unreferenced object 0xffff8ed502a37580 (size 32):
>    comm "(udev-worker)", pid 464, jiffies 4294672444
>    hex dump (first 32 bytes):
>      01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>      00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>    backtrace (crc f1da05aa):
>      __kmalloc_noprof+0x3ac/0x500
>      acpi_ut_initialize_buffer+0x67/0xc0
>      acpi_evaluate_object+0x272/0x2c0
>      acpi_evaluate_dsm+0xb4/0x120
>      r535_gsp_acpi_jt+0xa3/0x110 [nouveau]
>      r535_gsp_set_system_info+0x153/0x390 [nouveau]
>      r535_gsp_oneinit+0xa4d/0xf50 [nouveau]
>      tu102_gsp_oneinit+0x124/0x440 [nouveau]
>      nvkm_subdev_oneinit_+0x3f/0x90 [nouveau]
>      nvkm_subdev_init_+0x33/0xa0 [nouveau]
>      nvkm_subdev_init+0x46/0x60 [nouveau]
>      nvkm_device_init+0x167/0x1f0 [nouveau]
>      nvkm_udevice_init+0x4b/0x70 [nouveau]
>      nvkm_object_init+0x3a/0x110 [nouveau]
>      nvkm_ioctl_new+0x13a/0x200 [nouveau]
>      nvkm_ioctl+0x9f/0x140 [nouveau]
> unreferenced object 0xffff8ed5029bf1c0 (size 8):
>    comm "(udev-worker)", pid 464, jiffies 4294672446
>    hex dump (first 8 bytes):
>      cc bb d4 79 01 59 3c 84                          ...y.Y<.
>    backtrace (crc 30e1d939):
>      __kmalloc_cache_noprof+0x31b/0x410
>      r535_gsp_acpi_caps+0x7e/0x120 [nouveau]
>      r535_gsp_set_system_info+0x162/0x390 [nouveau]
>      r535_gsp_oneinit+0xa4d/0xf50 [nouveau]
>      tu102_gsp_oneinit+0x124/0x440 [nouveau]
>      nvkm_subdev_oneinit_+0x3f/0x90 [nouveau]
>      nvkm_subdev_init_+0x33/0xa0 [nouveau]
>      nvkm_subdev_init+0x46/0x60 [nouveau]
>      nvkm_device_init+0x167/0x1f0 [nouveau]
>      nvkm_udevice_init+0x4b/0x70 [nouveau]
>      nvkm_object_init+0x3a/0x110 [nouveau]
>      nvkm_ioctl_new+0x13a/0x200 [nouveau]
>      nvkm_ioctl+0x9f/0x140 [nouveau]
>      nvif_object_ctor+0x11a/0x1a0 [nouveau]
>      nvif_device_ctor+0x2a/0x80 [nouveau]
>      nouveau_drm_device_new+0x157/0x2e0 [nouveau]
> 

Hm, interesting. That's not the memory leak we have observed. I've got
this trace for leaks of similar size:

unreferenced object 0xff110001518f9348 (size 8):
  comm "kworker/0:4", pid 141, jiffies 4294719397
  hex dump (first 8 bytes):
    98 93 36 c3 c1 b9 80 3e                          ..6....>
  backtrace (crc a04f7c42):
    kmalloc_trace+0x28a/0x380
    r535_gsp_rpc_set_system_info+0x7e8/0x17f0 [nouveau]
    r535_gsp_oneinit+0x1ab0/0x2820 [nouveau]
    nvkm_subdev_oneinit_.part.0+0x91/0x1a0 [nouveau]
    nvkm_subdev_init_+0xfe/0x2b0 [nouveau]
    nvkm_subdev_init+0xa7/0xc0 [nouveau]
    nvkm_device_init+0x347/0x540 [nouveau]
    nvkm_udevice_init+0x97/0xf0 [nouveau]
    nvkm_object_init+0xc3/0x3e0 [nouveau]
    nvkm_ioctl_new+0x36c/0x6b0 [nouveau]
    nvkm_ioctl+0x1c0/0x4a0 [nouveau]
    nvif_object_ctor+0x3c9/0x740 [nouveau]
    nvif_device_ctor+0x2e/0x100 [nouveau]
    nouveau_drm_device_new+0x367/0x910 [nouveau]
    nouveau_drm_probe+0x10c/0x450 [nouveau]
    local_pci_probe+0xe8/0x1a0
unreferenced object 0xff11000142e1bf50 (size 8):
  comm "kworker/0:4", pid 141, jiffies 4294719397
  hex dump (first 8 bytes):
    88 b2 58 d0 d2 d7 ac 26                          ..X....&
  backtrace (crc 8a38119a):
    kmalloc_trace+0x28a/0x380
    r535_gsp_rpc_set_system_info+0xa8f/0x17f0 [nouveau]
    r535_gsp_oneinit+0x1ab0/0x2820 [nouveau]
    nvkm_subdev_oneinit_.part.0+0x91/0x1a0 [nouveau]
    nvkm_subdev_init_+0xfe/0x2b0 [nouveau]
    nvkm_subdev_init+0xa7/0xc0 [nouveau]
    nvkm_device_init+0x347/0x540 [nouveau]
    nvkm_udevice_init+0x97/0xf0 [nouveau]
    nvkm_object_init+0xc3/0x3e0 [nouveau]
    nvkm_ioctl_new+0x36c/0x6b0 [nouveau]
    nvkm_ioctl+0x1c0/0x4a0 [nouveau]
    nvif_object_ctor+0x3c9/0x740 [nouveau]
    nvif_device_ctor+0x2e/0x100 [nouveau]
    nouveau_drm_device_new+0x367/0x910 [nouveau]
    nouveau_drm_probe+0x10c/0x450 [nouveau]


Ben, could those be related?


P.

Re: [PATCH] drm/nouveau/gsp: fix potential leak of memory used during acpi init

[Philipp Stanner]

On Tue, 2025-06-17 at 14:00 +1000, Ben Skeggs wrote:
> If any of the ACPI calls fail, memory allocated for the input buffer
> would be leaked.  Fix failure paths to free allocated memory.
> 
> Also add checks to ensure the allocations succeeded in the first place.
> 
> Reported-by: Danilo Krummrich <dakr@kernel.org>
> Fixes: 176fdcbddfd2 ("drm/nouveau/gsp/r535: add support for booting GSP-RM")
> Signed-off-by: Ben Skeggs <bskeggs@nvidia.com>

Tested-by: Philipp Stanner <phasta@kernel.org>


This

Closes: https://www.spinics.net/lists/nouveau/msg16319.html



P.

> ---
>  .../drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c | 20 +++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
> index baf42339f93e..b098a7555fde 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
> @@ -719,7 +719,6 @@ r535_gsp_acpi_caps(acpi_handle handle, CAPS_METHOD_DATA *caps)
>  	union acpi_object argv4 = {
>  		.buffer.type    = ACPI_TYPE_BUFFER,
>  		.buffer.length  = 4,
> -		.buffer.pointer = kmalloc(argv4.buffer.length, GFP_KERNEL),
>  	}, *obj;
>  
>  	caps->status = 0xffff;
> @@ -727,17 +726,22 @@ r535_gsp_acpi_caps(acpi_handle handle, CAPS_METHOD_DATA *caps)
>  	if (!acpi_check_dsm(handle, &NVOP_DSM_GUID, NVOP_DSM_REV, BIT_ULL(0x1a)))
>  		return;
>  
> +	argv4.buffer.pointer = kmalloc(argv4.buffer.length, GFP_KERNEL);
> +	if (!argv4.buffer.pointer)
> +		return;
> +
>  	obj = acpi_evaluate_dsm(handle, &NVOP_DSM_GUID, NVOP_DSM_REV, 0x1a, &argv4);
>  	if (!obj)
> -		return;
> +		goto done;
>  
>  	if (WARN_ON(obj->type != ACPI_TYPE_BUFFER) ||
>  	    WARN_ON(obj->buffer.length != 4))
> -		return;
> +		goto done;
>  
>  	caps->status = 0;
>  	caps->optimusCaps = *(u32 *)obj->buffer.pointer;
>  
> +done:
>  	ACPI_FREE(obj);
>  
>  	kfree(argv4.buffer.pointer);
> @@ -754,24 +758,28 @@ r535_gsp_acpi_jt(acpi_handle handle, JT_METHOD_DATA *jt)
>  	union acpi_object argv4 = {
>  		.buffer.type    = ACPI_TYPE_BUFFER,
>  		.buffer.length  = sizeof(caps),
> -		.buffer.pointer = kmalloc(argv4.buffer.length, GFP_KERNEL),
>  	}, *obj;
>  
>  	jt->status = 0xffff;
>  
> +	argv4.buffer.pointer = kmalloc(argv4.buffer.length, GFP_KERNEL);
> +	if (!argv4.buffer.pointer)
> +		return;
> +
>  	obj = acpi_evaluate_dsm(handle, &JT_DSM_GUID, JT_DSM_REV, 0x1, &argv4);
>  	if (!obj)
> -		return;
> +		goto done;
>  
>  	if (WARN_ON(obj->type != ACPI_TYPE_BUFFER) ||
>  	    WARN_ON(obj->buffer.length != 4))
> -		return;
> +		goto done;
>  
>  	jt->status = 0;
>  	jt->jtCaps = *(u32 *)obj->buffer.pointer;
>  	jt->jtRevId = (jt->jtCaps & 0xfff00000) >> 20;
>  	jt->bSBIOSCaps = 0;
>  
> +done:
>  	ACPI_FREE(obj);
>  
>  	kfree(argv4.buffer.pointer);