* [PATCH] drm/nouveau: validate MXMS descriptor bounds
@ 2026-07-06 9:39 ` Pengpeng Hou
0 siblings, 0 replies; 3+ messages in thread
From: Pengpeng Hou @ 2026-07-06 9:39 UTC (permalink / raw)
To: Lyude Paul
Cc: Pengpeng Hou, Danilo Krummrich, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Kees Cook,
dri-devel, nouveau, linux-kernel
MXMS data can be provided by the ROM, DSM or WMI paths. The parser only
kept the data pointer, so later validation and descriptor iteration had no
record of the actual provider buffer length. The ROM path also read a
short temporary header before deriving the declared MXMS size.
Keep the provider buffer size with the MXMS data, fetch the complete fixed
header before reading the declared structure length, and validate each
MXMS descriptor header and record array against the remaining MXMS buffer
before dumping or handing the descriptor to consumers.
Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
.../gpu/drm/nouveau/nvkm/subdev/mxm/base.c | 24 ++++++++--
.../gpu/drm/nouveau/nvkm/subdev/mxm/mxms.c | 47 +++++++++++++++----
.../gpu/drm/nouveau/nvkm/subdev/mxm/priv.h | 1 +
3 files changed, 60 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/mxm/base.c b/drivers/gpu/drm/nouveau/nvkm/subdev/mxm/base.c
index 26ec50b66e66..ad74a216a201 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/mxm/base.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/mxm/base.c
@@ -47,7 +47,8 @@ mxm_shadow_rom(struct nvkm_mxm *mxm, u8 version)
struct nvkm_bios *bios = device->bios;
struct nvkm_i2c *i2c = device->i2c;
struct nvkm_i2c_bus *bus = NULL;
- u8 i2cidx, mxms[6], addr, size;
+ u8 i2cidx, mxms[8], addr;
+ u32 size;
i2cidx = mxm_ddc_map(bios, 1 /* LVDS_DDC */) & 0x0f;
if (i2cidx < 0x0f)
@@ -56,22 +57,32 @@ mxm_shadow_rom(struct nvkm_mxm *mxm, u8 version)
return false;
addr = 0x54;
- if (!mxm_shadow_rom_fetch(bus, addr, 0, 6, mxms)) {
+ if (!mxm_shadow_rom_fetch(bus, addr, 0, sizeof(mxms), mxms)) {
addr = 0x56;
- if (!mxm_shadow_rom_fetch(bus, addr, 0, 6, mxms))
+ if (!mxm_shadow_rom_fetch(bus, addr, 0, sizeof(mxms), mxms))
return false;
}
mxm->mxms = mxms;
+ mxm->mxms_size = sizeof(mxms);
size = mxms_headerlen(mxm) + mxms_structlen(mxm);
+ if (size < mxms_headerlen(mxm) || size > U8_MAX) {
+ mxm->mxms = NULL;
+ mxm->mxms_size = 0;
+ return false;
+ }
+
mxm->mxms = kmalloc(size, GFP_KERNEL);
if (mxm->mxms &&
- mxm_shadow_rom_fetch(bus, addr, 0, size, mxm->mxms))
+ mxm_shadow_rom_fetch(bus, addr, 0, size, mxm->mxms)) {
+ mxm->mxms_size = size;
return true;
+ }
kfree(mxm->mxms);
mxm->mxms = NULL;
+ mxm->mxms_size = 0;
return false;
}
@@ -113,6 +124,8 @@ mxm_shadow_dsm(struct nvkm_mxm *mxm, u8 version)
if (obj->type == ACPI_TYPE_BUFFER) {
mxm->mxms = kmemdup(obj->buffer.pointer,
obj->buffer.length, GFP_KERNEL);
+ if (mxm->mxms)
+ mxm->mxms_size = obj->buffer.length;
} else if (obj->type == ACPI_TYPE_INTEGER) {
nvkm_debug(subdev, "DSM MXMS returned 0x%llx\n",
obj->integer.value);
@@ -188,6 +201,8 @@ mxm_shadow_wmi(struct nvkm_mxm *mxm, u8 version)
if (obj->type == ACPI_TYPE_BUFFER) {
mxm->mxms = kmemdup(obj->buffer.pointer,
obj->buffer.length, GFP_KERNEL);
+ if (mxm->mxms)
+ mxm->mxms_size = obj->buffer.length;
}
kfree(obj);
@@ -220,6 +235,7 @@ mxm_shadow(struct nvkm_mxm *mxm, u8 version)
return 0;
kfree(mxm->mxms);
mxm->mxms = NULL;
+ mxm->mxms_size = 0;
}
} while ((++shadow)->name);
return -ENOENT;
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/mxm/mxms.c b/drivers/gpu/drm/nouveau/nvkm/subdev/mxm/mxms.c
index 9abfa5e2fe9f..895a1d76b80f 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/mxm/mxms.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/mxm/mxms.c
@@ -60,14 +60,21 @@ mxms_headerlen(struct nvkm_mxm *mxm)
u16
mxms_structlen(struct nvkm_mxm *mxm)
{
- return *(u16 *)&mxms_data(mxm)[6];
+ if (mxm->mxms_size < mxms_headerlen(mxm))
+ return 0;
+
+ return ROM16(mxms_data(mxm)[6]);
}
bool
mxms_checksum(struct nvkm_mxm *mxm)
{
- u16 size = mxms_headerlen(mxm) + mxms_structlen(mxm);
+ u32 size = mxms_headerlen(mxm) + mxms_structlen(mxm);
u8 *mxms = mxms_data(mxm), sum = 0;
+
+ if (size < mxms_headerlen(mxm) || mxm->mxms_size < size)
+ return false;
+
while (size--)
sum += *mxms++;
if (sum) {
@@ -81,7 +88,13 @@ bool
mxms_valid(struct nvkm_mxm *mxm)
{
u8 *mxms = mxms_data(mxm);
- if (*(u32 *)mxms != 0x5f4d584d) {
+
+ if (mxm->mxms_size < mxms_headerlen(mxm)) {
+ nvkm_debug(&mxm->subdev, "header too short\n");
+ return false;
+ }
+
+ if (ROM32(mxms[0]) != 0x5f4d584d) {
nvkm_debug(&mxm->subdev, "signature invalid\n");
return false;
}
@@ -97,15 +110,21 @@ mxms_foreach(struct nvkm_mxm *mxm, u8 types,
bool (*exec)(struct nvkm_mxm *, u8 *, void *), void *info)
{
struct nvkm_subdev *subdev = &mxm->subdev;
+ u32 remaining = mxms_structlen(mxm);
u8 *mxms = mxms_data(mxm);
u8 *desc = mxms + mxms_headerlen(mxm);
- u8 *fini = desc + mxms_structlen(mxm) - 1;
- while (desc < fini) {
- u8 type = desc[0] & 0x0f;
+
+ if (mxm->mxms_size < mxms_headerlen(mxm) + remaining)
+ return false;
+
+ while (remaining) {
+ u32 desclen;
+ u8 type;
u8 headerlen = 0;
u8 recordlen = 0;
u8 entries = 0;
+ type = desc[0] & 0x0f;
switch (type) {
case 0: /* Output Device Structure */
if (mxms_version(mxm) >= 0x0300)
@@ -119,6 +138,8 @@ mxms_foreach(struct nvkm_mxm *mxm, u8 types,
headerlen = 4;
break;
case 4: /* GPIO Device Structure */
+ if (remaining < sizeof(u32))
+ return false;
headerlen = 4;
recordlen = 2;
entries = (ROM32(desc[0]) & 0x01f00000) >> 20;
@@ -128,6 +149,8 @@ mxms_foreach(struct nvkm_mxm *mxm, u8 types,
break;
case 6: /* Backlight Control Structure */
if (mxms_version(mxm) >= 0x0300) {
+ if (remaining < 2)
+ return false;
headerlen = 4;
recordlen = 8;
entries = (desc[1] & 0xf0) >> 4;
@@ -136,6 +159,8 @@ mxms_foreach(struct nvkm_mxm *mxm, u8 types,
}
break;
case 7: /* Fan Control Structure */
+ if (remaining < 2)
+ return false;
headerlen = 8;
recordlen = 4;
entries = desc[1] & 0x07;
@@ -145,8 +170,13 @@ mxms_foreach(struct nvkm_mxm *mxm, u8 types,
return false;
}
+ if (headerlen > remaining ||
+ (recordlen && entries > (remaining - headerlen) / recordlen))
+ return false;
+
+ desclen = headerlen + entries * recordlen;
if (mxm->subdev.debug >= NV_DBG_DEBUG && (exec == NULL)) {
- static const char * mxms_desc[] = {
+ static const char * const mxms_desc[] = {
"ODS", "SCCS", "TS", "IPS",
"GSD", "VSS", "BCS", "FCS",
};
@@ -171,7 +201,8 @@ mxms_foreach(struct nvkm_mxm *mxm, u8 types,
return false;
}
- desc += headerlen + (entries * recordlen);
+ desc += desclen;
+ remaining -= desclen;
}
return true;
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/mxm/priv.h b/drivers/gpu/drm/nouveau/nvkm/subdev/mxm/priv.h
index fcacb6c6a7f7..ad93a7ceac29 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/mxm/priv.h
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/mxm/priv.h
@@ -9,6 +9,7 @@
struct nvkm_mxm {
struct nvkm_subdev subdev;
u32 action;
+ u32 mxms_size;
u8 *mxms;
};
--
2.43.0
^ permalink raw reply related [flat|nested] 3+ messages in thread* [PATCH] drm/nouveau: validate MXMS descriptor bounds
@ 2026-07-06 9:39 ` Pengpeng Hou
0 siblings, 0 replies; 3+ messages in thread
From: Pengpeng Hou @ 2026-07-06 9:39 UTC (permalink / raw)
To: Lyude Paul
Cc: Pengpeng Hou, Danilo Krummrich, Maarten Lankhorst, Maxime Ripard,
Simona Vetter, Kees Cook, dri-devel, nouveau, linux-kernel
MXMS data can be provided by the ROM, DSM or WMI paths. The parser only
kept the data pointer, so later validation and descriptor iteration had no
record of the actual provider buffer length. The ROM path also read a
short temporary header before deriving the declared MXMS size.
Keep the provider buffer size with the MXMS data, fetch the complete fixed
header before reading the declared structure length, and validate each
MXMS descriptor header and record array against the remaining MXMS buffer
before dumping or handing the descriptor to consumers.
Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
.../gpu/drm/nouveau/nvkm/subdev/mxm/base.c | 24 ++++++++--
.../gpu/drm/nouveau/nvkm/subdev/mxm/mxms.c | 47 +++++++++++++++----
.../gpu/drm/nouveau/nvkm/subdev/mxm/priv.h | 1 +
3 files changed, 60 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/mxm/base.c b/drivers/gpu/drm/nouveau/nvkm/subdev/mxm/base.c
index 26ec50b66e66..ad74a216a201 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/mxm/base.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/mxm/base.c
@@ -47,7 +47,8 @@ mxm_shadow_rom(struct nvkm_mxm *mxm, u8 version)
struct nvkm_bios *bios = device->bios;
struct nvkm_i2c *i2c = device->i2c;
struct nvkm_i2c_bus *bus = NULL;
- u8 i2cidx, mxms[6], addr, size;
+ u8 i2cidx, mxms[8], addr;
+ u32 size;
i2cidx = mxm_ddc_map(bios, 1 /* LVDS_DDC */) & 0x0f;
if (i2cidx < 0x0f)
@@ -56,22 +57,32 @@ mxm_shadow_rom(struct nvkm_mxm *mxm, u8 version)
return false;
addr = 0x54;
- if (!mxm_shadow_rom_fetch(bus, addr, 0, 6, mxms)) {
+ if (!mxm_shadow_rom_fetch(bus, addr, 0, sizeof(mxms), mxms)) {
addr = 0x56;
- if (!mxm_shadow_rom_fetch(bus, addr, 0, 6, mxms))
+ if (!mxm_shadow_rom_fetch(bus, addr, 0, sizeof(mxms), mxms))
return false;
}
mxm->mxms = mxms;
+ mxm->mxms_size = sizeof(mxms);
size = mxms_headerlen(mxm) + mxms_structlen(mxm);
+ if (size < mxms_headerlen(mxm) || size > U8_MAX) {
+ mxm->mxms = NULL;
+ mxm->mxms_size = 0;
+ return false;
+ }
+
mxm->mxms = kmalloc(size, GFP_KERNEL);
if (mxm->mxms &&
- mxm_shadow_rom_fetch(bus, addr, 0, size, mxm->mxms))
+ mxm_shadow_rom_fetch(bus, addr, 0, size, mxm->mxms)) {
+ mxm->mxms_size = size;
return true;
+ }
kfree(mxm->mxms);
mxm->mxms = NULL;
+ mxm->mxms_size = 0;
return false;
}
@@ -113,6 +124,8 @@ mxm_shadow_dsm(struct nvkm_mxm *mxm, u8 version)
if (obj->type == ACPI_TYPE_BUFFER) {
mxm->mxms = kmemdup(obj->buffer.pointer,
obj->buffer.length, GFP_KERNEL);
+ if (mxm->mxms)
+ mxm->mxms_size = obj->buffer.length;
} else if (obj->type == ACPI_TYPE_INTEGER) {
nvkm_debug(subdev, "DSM MXMS returned 0x%llx\n",
obj->integer.value);
@@ -188,6 +201,8 @@ mxm_shadow_wmi(struct nvkm_mxm *mxm, u8 version)
if (obj->type == ACPI_TYPE_BUFFER) {
mxm->mxms = kmemdup(obj->buffer.pointer,
obj->buffer.length, GFP_KERNEL);
+ if (mxm->mxms)
+ mxm->mxms_size = obj->buffer.length;
}
kfree(obj);
@@ -220,6 +235,7 @@ mxm_shadow(struct nvkm_mxm *mxm, u8 version)
return 0;
kfree(mxm->mxms);
mxm->mxms = NULL;
+ mxm->mxms_size = 0;
}
} while ((++shadow)->name);
return -ENOENT;
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/mxm/mxms.c b/drivers/gpu/drm/nouveau/nvkm/subdev/mxm/mxms.c
index 9abfa5e2fe9f..895a1d76b80f 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/mxm/mxms.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/mxm/mxms.c
@@ -60,14 +60,21 @@ mxms_headerlen(struct nvkm_mxm *mxm)
u16
mxms_structlen(struct nvkm_mxm *mxm)
{
- return *(u16 *)&mxms_data(mxm)[6];
+ if (mxm->mxms_size < mxms_headerlen(mxm))
+ return 0;
+
+ return ROM16(mxms_data(mxm)[6]);
}
bool
mxms_checksum(struct nvkm_mxm *mxm)
{
- u16 size = mxms_headerlen(mxm) + mxms_structlen(mxm);
+ u32 size = mxms_headerlen(mxm) + mxms_structlen(mxm);
u8 *mxms = mxms_data(mxm), sum = 0;
+
+ if (size < mxms_headerlen(mxm) || mxm->mxms_size < size)
+ return false;
+
while (size--)
sum += *mxms++;
if (sum) {
@@ -81,7 +88,13 @@ bool
mxms_valid(struct nvkm_mxm *mxm)
{
u8 *mxms = mxms_data(mxm);
- if (*(u32 *)mxms != 0x5f4d584d) {
+
+ if (mxm->mxms_size < mxms_headerlen(mxm)) {
+ nvkm_debug(&mxm->subdev, "header too short\n");
+ return false;
+ }
+
+ if (ROM32(mxms[0]) != 0x5f4d584d) {
nvkm_debug(&mxm->subdev, "signature invalid\n");
return false;
}
@@ -97,15 +110,21 @@ mxms_foreach(struct nvkm_mxm *mxm, u8 types,
bool (*exec)(struct nvkm_mxm *, u8 *, void *), void *info)
{
struct nvkm_subdev *subdev = &mxm->subdev;
+ u32 remaining = mxms_structlen(mxm);
u8 *mxms = mxms_data(mxm);
u8 *desc = mxms + mxms_headerlen(mxm);
- u8 *fini = desc + mxms_structlen(mxm) - 1;
- while (desc < fini) {
- u8 type = desc[0] & 0x0f;
+
+ if (mxm->mxms_size < mxms_headerlen(mxm) + remaining)
+ return false;
+
+ while (remaining) {
+ u32 desclen;
+ u8 type;
u8 headerlen = 0;
u8 recordlen = 0;
u8 entries = 0;
+ type = desc[0] & 0x0f;
switch (type) {
case 0: /* Output Device Structure */
if (mxms_version(mxm) >= 0x0300)
@@ -119,6 +138,8 @@ mxms_foreach(struct nvkm_mxm *mxm, u8 types,
headerlen = 4;
break;
case 4: /* GPIO Device Structure */
+ if (remaining < sizeof(u32))
+ return false;
headerlen = 4;
recordlen = 2;
entries = (ROM32(desc[0]) & 0x01f00000) >> 20;
@@ -128,6 +149,8 @@ mxms_foreach(struct nvkm_mxm *mxm, u8 types,
break;
case 6: /* Backlight Control Structure */
if (mxms_version(mxm) >= 0x0300) {
+ if (remaining < 2)
+ return false;
headerlen = 4;
recordlen = 8;
entries = (desc[1] & 0xf0) >> 4;
@@ -136,6 +159,8 @@ mxms_foreach(struct nvkm_mxm *mxm, u8 types,
}
break;
case 7: /* Fan Control Structure */
+ if (remaining < 2)
+ return false;
headerlen = 8;
recordlen = 4;
entries = desc[1] & 0x07;
@@ -145,8 +170,13 @@ mxms_foreach(struct nvkm_mxm *mxm, u8 types,
return false;
}
+ if (headerlen > remaining ||
+ (recordlen && entries > (remaining - headerlen) / recordlen))
+ return false;
+
+ desclen = headerlen + entries * recordlen;
if (mxm->subdev.debug >= NV_DBG_DEBUG && (exec == NULL)) {
- static const char * mxms_desc[] = {
+ static const char * const mxms_desc[] = {
"ODS", "SCCS", "TS", "IPS",
"GSD", "VSS", "BCS", "FCS",
};
@@ -171,7 +201,8 @@ mxms_foreach(struct nvkm_mxm *mxm, u8 types,
return false;
}
- desc += headerlen + (entries * recordlen);
+ desc += desclen;
+ remaining -= desclen;
}
return true;
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/mxm/priv.h b/drivers/gpu/drm/nouveau/nvkm/subdev/mxm/priv.h
index fcacb6c6a7f7..ad93a7ceac29 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/mxm/priv.h
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/mxm/priv.h
@@ -9,6 +9,7 @@
struct nvkm_mxm {
struct nvkm_subdev subdev;
u32 action;
+ u32 mxms_size;
u8 *mxms;
};
--
2.43.0
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] drm/nouveau: validate MXMS descriptor bounds
2026-07-06 9:39 ` Pengpeng Hou
(?)
@ 2026-07-06 9:55 ` sashiko-bot
-1 siblings, 0 replies; 3+ messages in thread
From: sashiko-bot @ 2026-07-06 9:55 UTC (permalink / raw)
To: Pengpeng Hou; +Cc: dri-devel
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Missing validation of the ACPI WMI evaluation output can cause a NULL pointer dereference.
--
commit d046b4257d93287fd7148175e8bfb0fc2ce27ed6
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>
drm/nouveau: validate MXMS descriptor bounds
This commit adds bounds checking for MXMS descriptors. It retains the provider
buffer size with the MXMS data, validates fixed headers, and verifies
descriptor arrays against the remaining buffer size.
> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/mxm/base.c b/drivers/gpu/drm/nouveau/nvkm/subdev/mxm/base.c
> index 26ec50b66e667..ad74a216a201d 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/mxm/base.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/mxm/base.c
[ ... ]
> @@ -188,6 +201,8 @@ mxm_shadow_wmi(struct nvkm_mxm *mxm, u8 version)
> if (obj->type == ACPI_TYPE_BUFFER) {
[Severity: High]
This is a pre-existing issue, but can wmi_evaluate_method() return success
while leaving retn.pointer as NULL?
If the BIOS WMI implementation returns void, obj (assigned from retn.pointer
just before this block) would be NULL. Does this code risk a NULL pointer
dereference around line 197 when accessing obj->type in mxm_shadow_wmi() in
base.c without verifying the pointer first?
A similar condition appears to exist around line 153 in wmi_wmmx_mxmi() where
retn.pointer is also dereferenced without validation.
> mxm->mxms = kmemdup(obj->buffer.pointer,
> obj->buffer.length, GFP_KERNEL);
> + if (mxm->mxms)
> + mxm->mxms_size = obj->buffer.length;
> }
>
> kfree(obj);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260706093949.82126-1-pengpeng@iscas.ac.cn?part=1
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-07-06 16:44 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-06 9:39 [PATCH] drm/nouveau: validate MXMS descriptor bounds Pengpeng Hou
2026-07-06 9:39 ` Pengpeng Hou
2026-07-06 9:55 ` sashiko-bot
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.