thr3ads.net - Nouveau - [PATCH] drm/nouveau/gsp: fix potential leak of memory used during acpi init [Jun 2025]

If this information is useful, please help other people find it:
Share via:

Philipp Stanner

2025-Jun-17 11:29 UTC

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

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 at 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 at 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);

Danilo Krummrich

2025-Jun-17 13:05 UTC

head link

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

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);
>

Nouveau - Jun 2025 - [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

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