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

[Claude Code Review Bot <claude-review@example.com>]

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