[PATCH] drm/dp/mst: fix buffer overflows in sideband chunk accumulation

[PATCH] drm/dp/mst: fix buffer overflows in sideband chunk accumulation

[Ashutosh Desai]

drm_dp_sideband_append_payload() has three related bugs when processing
device-provided sideband reply data:

1. Zero-length curchunk_len underflow: msg_len is a 6-bit field taken
   directly from the DP sideband header. If a device sends msg_len=0,
   curchunk_len is set to zero. The condition (curchunk_idx >= curchunk_len)
   is immediately true, and curchunk_len-1 wraps to 255 (u8 underflow).
   drm_dp_msg_data_crc4() reads 255 bytes from chunk[48], then memcpy()
   writes 255 bytes into msg[], both far out of bounds.

2. chunk[48] overflow: curchunk_len can reach 63 (6-bit field). chunk[] is
   only 48 bytes. Multi-iteration payload assembly appends 16-byte blocks
   until curchunk_idx reaches curchunk_len, writing up to 15 bytes past
   the end of chunk[] into msg[].

3. msg[256] overflow: each chunk contributes (curchunk_len-1) bytes to
   msg[]. No check ensures curlen + (curchunk_len-1) stays within msg[256],
   so the memcpy can spill into adjacent struct fields.

All three are reachable from any DP MST device that can forge sideband
reply messages on a physical connection.

Cc: stable@vger.kernel.org
Signed-off-by: Ashutosh Desai <ashutoshdesai993@gmail.com>
---
 drivers/gpu/drm/display/drm_dp_mst_topology.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
index f2a7dbc5e..5261a4a54 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -789,6 +789,12 @@ static bool drm_dp_sideband_append_payload(struct drm_dp_sideband_msg_rx *msg,
 {
 	u8 crc4;
 
+	/* curchunk_len must be >= 1 (min 1 CRC byte) and fit in chunk[] */
+	if (!msg->curchunk_len ||
+	    msg->curchunk_len > ARRAY_SIZE(msg->chunk) ||
+	    msg->curchunk_idx + replybuflen > ARRAY_SIZE(msg->chunk))
+		return false;
+
 	memcpy(&msg->chunk[msg->curchunk_idx], replybuf, replybuflen);
 	msg->curchunk_idx += replybuflen;
 
@@ -799,6 +805,9 @@ static bool drm_dp_sideband_append_payload(struct drm_dp_sideband_msg_rx *msg,
 			print_hex_dump(KERN_DEBUG, "wrong crc",
 				       DUMP_PREFIX_NONE, 16, 1,
 				       msg->chunk,  msg->curchunk_len, false);
+		/* Guard against accumulated msg[] overflow */
+		if (msg->curlen + msg->curchunk_len - 1 > ARRAY_SIZE(msg->msg))
+			return false;
 		/* copy chunk into bigger msg */
 		memcpy(&msg->msg[msg->curlen], msg->chunk, msg->curchunk_len - 1);
 		msg->curlen += msg->curchunk_len - 1;
-- 
2.34.1

Claude review: drm/dp/mst: fix buffer overflows in sideband chunk accumulation

[Claude Code Review Bot]

Patch Review

**Commit message accuracy — Bug #1 is already mitigated upstream**

The commit message claims:

> If a device sends msg_len=0, curchunk_len is set to zero. The condition (curchunk_idx >= curchunk_len) is immediately true, and curchunk_len-1 wraps to 255

This is misleading. `msg_len=0` is already rejected in `drm_dp_decode_sideband_msg_hdr()` at line 332:

```c
hdr->msg_len = buf[idx] & 0x3f;
if (hdr->msg_len < 1)		/* min space for body CRC */
    return false;
```

This function returns false before `drm_dp_sideband_msg_set_header()` ever assigns `msg->curchunk_len = hdr->msg_len`. So the zero-length path described in the commit message is not reachable through the normal call chain. The `!msg->curchunk_len` check in the patch is harmless defense-in-depth, but the commit message should not present it as a live vulnerability.

**Bug #2 fix — chunk[48] overflow — Correct**

This is the real fix. `msg_len` is a 6-bit field (max 63), but `chunk[]` is only 48 bytes. The caller accumulates exactly `curchunk_len` bytes total into `chunk[]` via the loop in `drm_dp_get_one_sb_msg()`:

```c
replylen = min(msg->curchunk_len, (u8)(len - hdrlen));
ret = drm_dp_sideband_append_payload(msg, replyblock + hdrlen, replylen);
...
replylen = msg->curchunk_len + msg->curchunk_hdrlen - len;
while (replylen > 0) { ... }
```

When `curchunk_len` is 49–63, the total bytes written to `chunk[48]` will overflow. The check:

```c
msg->curchunk_len > ARRAY_SIZE(msg->chunk)
```

correctly rejects this before any writes occur. The additional per-call guard:

```c
msg->curchunk_idx + replybuflen > ARRAY_SIZE(msg->chunk)
```

provides belt-and-suspenders protection against each individual `memcpy` exceeding the buffer, which is good defensive coding.

**Bug #3 fix — msg[256] overflow — Correct**

The check:

```c
if (msg->curlen + msg->curchunk_len - 1 > ARRAY_SIZE(msg->msg))
    return false;
```

correctly prevents the `memcpy` into `msg[]` from overflowing. The arithmetic is safe: both `curlen` (u8) and `curchunk_len` (u8) are promoted to `int` in the expression, so `255 + 48 - 1 = 302 > 256` evaluates correctly without wraparound.

The `>` (vs `>=`) is correct: writing `curchunk_len - 1` bytes starting at `msg[curlen]` requires `curlen + curchunk_len - 1 <= 256`, i.e. fail when `> 256`.

**Minor concerns:**

1. **No diagnostic logging on the new failure paths.** The callers do log (`drm_dbg_kms(mgr->dev, "sideband msg build failed")`), but adding a log line inside the function itself would help identify *which* bounds check tripped:

```c
if (!msg->curchunk_len ||
    msg->curchunk_len > ARRAY_SIZE(msg->chunk) ||
    msg->curchunk_idx + replybuflen > ARRAY_SIZE(msg->chunk))
+       /* Could add: drm_dbg_kms("sideband chunk bounds exceeded: ...") */
        return false;
```

2. **`curlen` is u8 for a 256-byte buffer** — a pre-existing design concern (not introduced by this patch). If exactly 256 bytes are validly accumulated, `curlen` wraps to 0, which would corrupt message data on subsequent chunks. This is a latent bug in the struct definition, but out of scope for this fix.

3. **The patch may be AI-generated.** The formulaic numbered-bug structure in the commit message, the inaccurate claim about bug #1 (suggesting the author didn't read `drm_dp_decode_sideband_msg_hdr()`), and the Azure VM in the email headers are worth noting. The code itself is fine regardless of provenance, but the inaccurate commit message corroborates this suspicion — a human author who traced the code would have noticed the existing `msg_len < 1` check.

**Recommendation:** Request a v2 with the commit message corrected to acknowledge the existing `msg_len >= 1` validation, and to clarify that the `!curchunk_len` check is defense-in-depth rather than a fix for a live bug. The code changes themselves are correct and worth merging.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/dp/mst: fix buffer overflows in sideband chunk accumulation

[Claude Code Review Bot]

Overall Series Review

Subject: drm/dp/mst: fix buffer overflows in sideband chunk accumulation
Author: Ashutosh Desai <ashutoshdesai993@gmail.com>
Patches: 1
Reviewed: 2026-04-12T10:28:55.630601

---

This is a single-patch series adding bounds checks to `drm_dp_sideband_append_payload()` in the DP MST sideband message handling code. The patch addresses real overflow risks — particularly the `chunk[48]` overflow when `msg_len` (a 6-bit field, max 63) exceeds the buffer size, and the `msg[256]` accumulation overflow. The code changes are correct and the checks are properly placed with correct arithmetic (C integer promotion ensures u8 values are compared as `int`, avoiding wraparound in the comparisons).

However, the commit message overstates the severity by describing three "bugs" when one of them (bug #1, the zero-length `msg_len` case) is already prevented by an existing check in `drm_dp_decode_sideband_msg_hdr()`. This is worth flagging because it may indicate the author didn't fully trace the call chain before writing the description.

**Verdict:** The actual code fix is sound and addresses genuine vulnerabilities, but the commit message needs correction. Likely worth a request for a v2 with an accurate description.

---

---
Generated by Claude Code Patch Reviewer