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 <[email protected]> > 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 <[email protected]> > --- > .../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);
