| Message ID | 20231116063028.35871-1-shum.sdl@nppct.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | therm.c: Adding an array index check before accessing an element. | expand |
> Subject: therm.c: Adding an array index check before accessing an element. As $ git log --oneline drivers/gpu/drm/nouveau/nvkm/subdev/bios/therm.c a215721fb64e drm/nouveau/bios/therm: pointers are 32-bit 46484438ab7d drm/nouveau/bios: convert to new-style nvkm_subdev 7f5f518fd70b drm/nouveau/bios: remove object accessor functions 60b29d207179 drm/nouveau/bios: switch to subdev printk macros 9ace404b1098 drm/nouveau/device: include core/device.h automatically for subdevs/engines d390b48027f8 drm/nouveau/bios: namespace + nvidia gpu names (no binary change) c39f472e9f14 drm/nouveau: remove symlinks, move core/ to nvkm/ (no code changes) shows, a better prefix should be drm/nouveau/bios: or drm/nouveau/bios/therm: and there should not be a dot at the end. e.g. drm/nouveau/bios: avoid invalid memory memory access in nvbios_therm_fan_parse() On 16.11.2023 09:30, Andrey Shumilin wrote: > It is possible to access an element at index -1 if at the first iteration of the loop the result of switch is equal to 0x25 > If nvbios_rd08(bios, entry + 0) returns 0x25 before 0x24, buffer underrun happens as far as &fan->trip[fan->nr_fan_trip - 1] points to invalid memory. > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > Signed-off-by: Andrey Shumilin <shum.sdl@nppct.ru> > --- > drivers/gpu/drm/nouveau/nvkm/subdev/bios/therm.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/bios/therm.c b/drivers/gpu/drm/nouveau/nvkm/subdev/bios/therm.c > index 5babc5a7c7d5..78387053f214 100644 > --- a/drivers/gpu/drm/nouveau/nvkm/subdev/bios/therm.c > +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/bios/therm.c > @@ -180,6 +180,8 @@ nvbios_therm_fan_parse(struct nvkm_bios *bios, struct nvbios_therm_fan *fan) > cur_trip->fan_duty = duty_lut[(value & 0xf000) >> 12]; > break; > case 0x25: > + if (fan->nr_fan_trip == 0) > + fan->nr_fan_trip++; I would suggest to return -EINVAL if the assumption on valid nr_fan_trip is failed. > cur_trip = &fan->trip[fan->nr_fan_trip - 1]; > cur_trip->fan_duty = value; > break; > -- Alexey
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/bios/therm.c b/drivers/gpu/drm/nouveau/nvkm/subdev/bios/therm.c index 5babc5a7c7d5..78387053f214 100644 --- a/drivers/gpu/drm/nouveau/nvkm/subdev/bios/therm.c +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/bios/therm.c @@ -180,6 +180,8 @@ nvbios_therm_fan_parse(struct nvkm_bios *bios, struct nvbios_therm_fan *fan) cur_trip->fan_duty = duty_lut[(value & 0xf000) >> 12]; break; case 0x25: + if (fan->nr_fan_trip == 0) + fan->nr_fan_trip++; cur_trip = &fan->trip[fan->nr_fan_trip - 1]; cur_trip->fan_duty = value; break;
It is possible to access an element at index -1 if at the first iteration of the loop the result of switch is equal to 0x25 Found by Linux Verification Center (linuxtesting.org) with SVACE. Signed-off-by: Andrey Shumilin <shum.sdl@nppct.ru> --- drivers/gpu/drm/nouveau/nvkm/subdev/bios/therm.c | 2 ++ 1 file changed, 2 insertions(+)