[PATCH v2] drm/nouveau: validate legacy BIT table bounds

[PATCH v2] drm/nouveau: validate legacy BIT table bounds

[Pengpeng Hou]

The legacy BIT parser finds the BIT signature and then reads the
entry shape and entry array directly from the VBIOS image. The signature
does not prove that the complete header, entry array or referenced table
payload fits in the image.

Validate each extent before use. Preserve the legacy zero-offset meaning
for zero-length entries, but reject non-empty entries with a null or
out-of-range offset.

Fixes: 6ee738610f41 ("drm/nouveau: Add DRM driver for NVIDIA GPUs")
Assisted-by: Codex:gpt-5
Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
Changes since v1: https://lore.kernel.org/all/20260706093856.81984-1-pengpeng@iscas.ac.cn/
- validate the complete BIT header and entry array
- validate each referenced payload extent
- preserve zero-offset semantics for zero-length entries only

The legacy BIT and ROMPTR contracts were reviewed statically; no malformed
VBIOS image was exercised.

 drivers/gpu/drm/nouveau/nouveau_bios.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_bios.c b/drivers/gpu/drm/nouveau/nouveau_bios.c
index 38032bb95826..a4ef62928ba3 100644
--- a/drivers/gpu/drm/nouveau/nouveau_bios.c
+++ b/drivers/gpu/drm/nouveau/nouveau_bios.c
@@ -972,24 +972,39 @@ bit_table(struct drm_device *dev, u8 id, struct bit_entry *bit)
 {
 	struct nouveau_drm *drm = nouveau_drm(dev);
 	struct nvbios *bios = &drm->vbios;
-	u8 entries, *entry;
+	u32 entry_size, entries, offset;
+	u8 *entry;
 
 	if (bios->type != NVBIOS_BIT)
 		return -ENODEV;
 
+	if (bios->offset > bios->length || bios->length - bios->offset < 12)
+		return -EINVAL;
+
+	entry_size = bios->data[bios->offset + 9];
 	entries = bios->data[bios->offset + 10];
-	entry   = &bios->data[bios->offset + 12];
+	if (entry_size < 6 ||
+	    entries > (bios->length - bios->offset - 12) / entry_size)
+		return -EINVAL;
+
+	entry = &bios->data[bios->offset + 12];
 	while (entries--) {
 		if (entry[0] == id) {
 			bit->id = entry[0];
 			bit->version = entry[1];
 			bit->length = ROM16(entry[2]);
 			bit->offset = ROM16(entry[4]);
+
+			offset = bit->offset;
+			if ((bit->length && !offset) || offset > bios->length ||
+			    bit->length > bios->length - offset)
+				return -EINVAL;
+
 			bit->data = ROMPTR(dev, entry[4]);
 			return 0;
 		}
 
-		entry += bios->data[bios->offset + 9];
+		entry += entry_size;
 	}
 
 	return -ENOENT;
-- 
2.50.1 (Apple Git-155)

[PATCH v2] drm/nouveau: validate legacy BIT table bounds

[Pengpeng Hou]

The legacy BIT parser finds the BIT signature and then reads the
entry shape and entry array directly from the VBIOS image. The signature
does not prove that the complete header, entry array or referenced table
payload fits in the image.

Validate each extent before use. Preserve the legacy zero-offset meaning
for zero-length entries, but reject non-empty entries with a null or
out-of-range offset.

Fixes: 6ee738610f41 ("drm/nouveau: Add DRM driver for NVIDIA GPUs")
Assisted-by: Codex:gpt-5
Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
Changes since v1: https://lore.kernel.org/all/20260706093856.81984-1-pengpeng@iscas.ac.cn/
- validate the complete BIT header and entry array
- validate each referenced payload extent
- preserve zero-offset semantics for zero-length entries only

The legacy BIT and ROMPTR contracts were reviewed statically; no malformed
VBIOS image was exercised.

 drivers/gpu/drm/nouveau/nouveau_bios.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_bios.c b/drivers/gpu/drm/nouveau/nouveau_bios.c
index 38032bb95826..a4ef62928ba3 100644
--- a/drivers/gpu/drm/nouveau/nouveau_bios.c
+++ b/drivers/gpu/drm/nouveau/nouveau_bios.c
@@ -972,24 +972,39 @@ bit_table(struct drm_device *dev, u8 id, struct bit_entry *bit)
 {
 	struct nouveau_drm *drm = nouveau_drm(dev);
 	struct nvbios *bios = &drm->vbios;
-	u8 entries, *entry;
+	u32 entry_size, entries, offset;
+	u8 *entry;
 
 	if (bios->type != NVBIOS_BIT)
 		return -ENODEV;
 
+	if (bios->offset > bios->length || bios->length - bios->offset < 12)
+		return -EINVAL;
+
+	entry_size = bios->data[bios->offset + 9];
 	entries = bios->data[bios->offset + 10];
-	entry   = &bios->data[bios->offset + 12];
+	if (entry_size < 6 ||
+	    entries > (bios->length - bios->offset - 12) / entry_size)
+		return -EINVAL;
+
+	entry = &bios->data[bios->offset + 12];
 	while (entries--) {
 		if (entry[0] == id) {
 			bit->id = entry[0];
 			bit->version = entry[1];
 			bit->length = ROM16(entry[2]);
 			bit->offset = ROM16(entry[4]);
+
+			offset = bit->offset;
+			if ((bit->length && !offset) || offset > bios->length ||
+			    bit->length > bios->length - offset)
+				return -EINVAL;
+
 			bit->data = ROMPTR(dev, entry[4]);
 			return 0;
 		}
 
-		entry += bios->data[bios->offset + 9];
+		entry += entry_size;
 	}
 
 	return -ENOENT;
-- 
2.50.1 (Apple Git-155)

Re: [PATCH v2] drm/nouveau: validate legacy BIT table bounds

[sashiko-bot]

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

Pre-existing issues:
- [High] Nested VBIOS table pointers parsed from BIT payloads lack bounds checks before dereference.
--

--- Patch [1]: [PATCH v2] drm/nouveau: validate legacy BIT table bounds ---
commit 3d3dd9aae1a602cb536b23bfafdac9694396793b
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

drm/nouveau: validate legacy BIT table bounds

The legacy BIT parser finds the BIT signature and then reads the
entry shape and entry array directly from the VBIOS image. The signature
does not prove that the complete header, entry array or referenced table
payload fits in the image.

Validate each extent before use. Preserve the legacy zero-offset meaning
for zero-length entries, but reject non-empty entries with a null or
out-of-range offset.

> diff --git a/drivers/gpu/drm/nouveau/nouveau_bios.c b/drivers/gpu/drm/nouveau/nouveau_bios.c
> index 38032bb958262..a4ef62928ba3d 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_bios.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_bios.c
> @@ -972,24 +972,39 @@ bit_table(struct drm_device *dev, u8 id, struct bit_entry *bit)
[ ... ]
> +			offset = bit->offset;
> +			if ((bit->length && !offset) || offset > bios->length ||
> +			    bit->length > bios->length - offset)
> +				return -EINVAL;
> +

[Severity: High]
This isn't a bug introduced by this patch, but since we are hardening VBIOS
parsing here, are there other nested VBIOS table pointers parsed from BIT
payloads that lack bounds checks before dereference?

For example, in parse_bit_A_tbl_entry(), load_table_ptr is extracted and
used directly without checking if it exceeds bios->length:

drivers/gpu/drm/nouveau/nouveau_bios.c:parse_bit_A_tbl_entry() {
	...
	load_table_ptr = ROM16(bios->data[bitentry->offset]);

	if (load_table_ptr == 0x0) {
		NV_DEBUG(drm, "Pointer to BIT loadval table invalid\n");
		return -EINVAL;
	}

	version = bios->data[load_table_ptr];
	...
}

Similarly, in parse_fp_mode_table(), fptablepointer is used as an offset
without validating against bios->length:

drivers/gpu/drm/nouveau/nouveau_bios.c:parse_fp_mode_table() {
	...
	fptable = &bios->data[bios->fp.fptablepointer];
	fptable_ver = fptable[0];
	...
}

Could these out-of-bounds reads cause a kernel panic if they access unmapped
memory pages, especially if a malicious hardware device provides a crafted
VBIOS?

>  			bit->data = ROMPTR(dev, entry[4]);
>  			return 0;
>  		}
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260814080335.22263-1-pengpeng@iscas.ac.cn?part=1