[PATCH] drm/nouveau/iccsense: fix memory leak in nvkm_iccsense_oneinit

[PATCH] drm/nouveau/iccsense: fix memory leak in nvkm_iccsense_oneinit

[Muhammad Bilal]

nvbios_iccsense_parse() allocates stbl.rail via kmalloc_objs() to hold
the per-entry power-rail table parsed out of the vbios ICCSENSE table
(drivers/gpu/drm/nouveau/nvkm/subdev/bios/iccsense.c). The only
consumer of that table, nvkm_iccsense_oneinit(), copies the fields it
needs into freshly allocated struct nvkm_iccsense_rail nodes but never
frees stbl.rail itself, on either the normal return path or the
-ENOMEM error path taken when a rail node allocation fails.

nvkm_iccsense_dtor() only walks and frees iccsense->rails (the copied
nodes) and iccsense->sensors -- it has no reference to the transient
stbl.rail array, so that allocation is unrecoverably leaked every time
oneinit() runs.

Observed with kmemleak on a KASAN build:

  unreferenced object 0xffff888104cbe480 (size 96)
  comm "(udev-worker)" pid 526
  backtrace:
    nvbios_iccsense_parse+0x217/0x740 [nouveau]
    nvkm_iccsense_oneinit+0x140/0xdd0

Free stbl.rail once we're done consuming it, via a common exit path
that also covers the -ENOMEM case.

Note: ret is reset to 0 immediately before the loop rather than at
declaration time, since it is already in use a few lines earlier for
the unrelated nvbios_power_budget_header()/nvbios_power_budget_entry()
return codes. Returning it unreset from the done: label would leak
that unrelated (and commonly non-zero, e.g. on boards without a power
budget table) status code out of oneinit() on the success path.

Fixes: b71c0892631a ("drm/nouveau/iccsense: implement for ina209, ina219 and ina3221")
Cc: stable@vger.kernel.org
Signed-off-by: Muhammad Bilal <meatuni001@gmail.com>
---
 drivers/gpu/drm/nouveau/nvkm/subdev/iccsense/base.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/iccsense/base.c b/drivers/gpu/drm/nouveau/nvkm/subdev/iccsense/base.c
index 3ccdbbe2fad0..6bfe4913dcb6 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/iccsense/base.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/iccsense/base.c
@@ -239,6 +239,7 @@ nvkm_iccsense_oneinit(struct nvkm_subdev *subdev)
 		return 0;
 
 	iccsense->data_valid = true;
+	ret = 0;
 	for (i = 0; i < stbl.nr_entry; ++i) {
 		struct pwr_rail_t *pwr_rail = &stbl.rail[i];
 		struct nvkm_iccsense_sensor *sensor;
@@ -280,8 +281,10 @@ nvkm_iccsense_oneinit(struct nvkm_subdev *subdev)
 			}
 
 			rail = kmalloc_obj(*rail);
-			if (!rail)
-				return -ENOMEM;
+			if (!rail) {
+				ret = -ENOMEM;
+				goto done;
+			}
 
 			rail->read = read;
 			rail->sensor = sensor;
@@ -291,7 +294,10 @@ nvkm_iccsense_oneinit(struct nvkm_subdev *subdev)
 			list_add_tail(&rail->head, &iccsense->rails);
 		}
 	}
-	return 0;
+
+done:
+	kfree(stbl.rail);
+	return ret;
 }
 
 static int
-- 
2.55.0

Re: [PATCH] drm/nouveau/iccsense: fix memory leak in nvkm_iccsense_oneinit

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Branching on uninitialized memory triggers undefined behavior and potential out-of-bounds heap read.
--

--- Patch [1]: [PATCH] drm/nouveau/iccsense: fix memory leak in nvkm_iccsense_oneinit ---
commit db41910973f19f5f6b73007149784b3d74245e81
Author: Muhammad Bilal <meatuni001@gmail.com>

drm/nouveau/iccsense: fix memory leak in nvkm_iccsense_oneinit

This commit fixes a memory leak in nvkm_iccsense_oneinit() where the
stbl.rail array allocated by nvbios_iccsense_parse() was never freed.
The array is now correctly freed in a common exit path that handles both
success and error conditions.

> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/iccsense/base.c b/drivers/gpu/drm/nouveau/nvkm/subdev/iccsense/base.c
> index 3ccdbbe2fad09..6bfe4913dcb66 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/iccsense/base.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/iccsense/base.c
> @@ -239,6 +239,7 @@ nvkm_iccsense_oneinit(struct nvkm_subdev *subdev)
>  		return 0;
>  
>  	iccsense->data_valid = true;
> +	ret = 0;
>  	for (i = 0; i < stbl.nr_entry; ++i) {
>  		struct pwr_rail_t *pwr_rail = &stbl.rail[i];

[Severity: High]
This is a pre-existing issue, but could this read uninitialized memory?

In drivers/gpu/drm/nouveau/nvkm/subdev/bios/iccsense.c, the function
nvbios_iccsense_parse() allocates the rail array using kmalloc_objs(),
which does not zero memory:

    iccsense->rail = kmalloc_objs(struct pwr_rail_t, cnt);

Inside its parsing loop, if nvbios_extdev_parse() fails, it skips
initialization of the remaining fields:

    if (nvbios_extdev_parse(bios, rail->extdev_id, &extdev))
        continue;

This leaves rail->resistor_count and rail->resistors uninitialized.

Then, back here in nvkm_iccsense_oneinit(), we read
pwr_rail->resistor_count:

    if (pwr_rail->mode != 1 || !pwr_rail->resistor_count)
        continue;

If the uninitialized resistor_count happens to be non-zero, this loop
could execute using that garbage value and access memory out of bounds:

    for (r = 0; r < pwr_rail->resistor_count; ++r) {
        ...
        struct pwr_rail_resistor_t *res = &pwr_rail->resistors[r];

Should nvbios_iccsense_parse() zero the allocated memory to prevent this?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803215022.172201-1-meatuni001@gmail.com?part=1