Claude review: drm/dp/mst: fix OOB reads in remote DPCD/I2C sideband reply parsers

Claude review: drm/dp/mst: fix OOB reads in remote DPCD/I2C sideband reply parsers

[Claude Code Review Bot]

Overall Series Review

Subject: drm/dp/mst: fix OOB reads in remote DPCD/I2C sideband reply parsers
Author: Ashutosh Desai <ashutoshdesai993@gmail.com>
Patches: 1
Reviewed: 2026-04-12T10:33:37.154436

---

This is a single-patch fix for out-of-bounds reads in two DP MST sideband reply parsers. The fix is correct, addresses a real vulnerability, and follows the existing validation pattern used throughout the rest of the file. The patch is small and well-scoped.

The main concern is the absence of a `Fixes:` tag and `Cc: stable@vger.kernel.org`, which are expected for security-relevant bug fixes in the kernel.

**Verdict: Looks good with minor process nits.**

---
Generated by Claude Code Patch Reviewer

Claude review: drm/dp/mst: fix OOB reads in remote DPCD/I2C sideband reply parsers

[Claude Code Review Bot]

Patch Review

**Correctness: Good.** The patch addresses two distinct but related issues:

1. **`drm_dp_sideband_parse_remote_dpcd_read()`** — Already had `idx > raw->curlen` bounds checks after each `idx++`, but was missing validation that the `memcpy` length (`num_bytes`) wouldn't cause a read past the received data. The added check:
   ```c
   if (idx + repmsg->u.remote_dpcd_read_ack.num_bytes > raw->curlen)
       goto fail_len;
   ```
   is correct. At this point `idx` is an `int` (value 3), `num_bytes` is `u8` (max 255), so no integer overflow risk. `curlen` is `u8` and the comparison promotes to `int`.

2. **`drm_dp_sideband_parse_remote_i2c_read_ack()`** — Was missing *both* the post-increment `idx > raw->curlen` check *and* the `idx + num_bytes > raw->curlen` check before the `memcpy`. The `/* TODO check */` comment had been there since the code was introduced, so this resolves a known gap. The patch correctly adds both checks and removes the TODO.

**Destination buffer safety:** Both `remote_dpcd_read_ack.bytes` and `remote_i2c_read_ack.bytes` are `u8 bytes[255]` (verified in `drm_dp_mst_helper.h:284,300`), and `num_bytes` is `u8` (max 255), so the destination buffer can never overflow.

**Source buffer safety:** `raw->msg` is 256 bytes (`drm_dp_mst_helper.h:172`), `curlen` is `u8` (max 255). The new checks ensure `idx + num_bytes <= curlen <= 255`, so the `memcpy` stays within bounds of both the valid data and the buffer itself.

**Nits / suggestions:**

- **Missing `Fixes:` tag.** This is a security-relevant fix and should reference the original commit that introduced the code (likely `ad7f8a1f9ced` or similar from the initial MST sideband parser). Kernel maintainers will likely request this before applying.

- **Missing `Cc: stable@vger.kernel.org`.** An OOB read from external input is a security issue that should be backported to stable kernels.

- The commit message says *"reads past the received data into whatever follows in raw->msg[]"* — this is slightly imprecise. With `idx` at 3 and `num_bytes` up to 255, the memcpy could read up to `msg[258]`, which is 2 bytes past the end of `msg[256]` into adjacent struct fields (`curchunk_len`, `curchunk_idx`). So it's not only reading uninitialized *within* `msg[]` but potentially a small struct-bounded OOB. The practical impact is the same (info leak of kernel memory), but the description could be more precise.

- No functional concerns with the code itself — it follows the exact same validation pattern as the rest of the file (e.g., `drm_dp_sideband_parse_enum_path_resources_ack` at line 924).

---
Generated by Claude Code Patch Reviewer

[PATCH v2] drm/dp/mst: fix OOB reads in remote DPCD/I2C sideband reply parsers

[Ashutosh Desai]

drm_dp_sideband_parse_remote_dpcd_read() reads num_bytes from the raw
message and then unconditionally does:

  memcpy(bytes, &raw->msg[idx], num_bytes);

without checking that idx + num_bytes <= raw->curlen. raw->msg[] is
256 bytes; if a malicious or misbehaving MST hub sets num_bytes larger
than the remaining payload, the memcpy reads past the received data
into whatever follows in raw->msg[].

drm_dp_sideband_parse_remote_i2c_read_ack() has the same flaw (noted
with a /* TODO check */ comment since the code was introduced).

Fix both functions by using a single combined check
(idx + num_bytes > curlen) before each memcpy. Since num_bytes is u8,
it is always >= 0, so this strictly subsumes the simpler idx > curlen
form and no separate step is needed.

Cc: stable@vger.kernel.org
Signed-off-by: Ashutosh Desai <ashutoshdesai993@gmail.com>
---
Changes in v2:
- Drop separate idx > curlen check; idx + num_bytes > curlen with u8
  num_bytes (always >= 0) strictly subsumes it (Lyude Paul)

 drivers/gpu/drm/display/drm_dp_mst_topology.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
index 170113520a43..9416a48804c8 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -871,7 +871,7 @@ static bool drm_dp_sideband_parse_remote_dpcd_read(struct drm_dp_sideband_msg_rx
 		goto fail_len;
 	repmsg->u.remote_dpcd_read_ack.num_bytes = raw->msg[idx];
 	idx++;
-	if (idx > raw->curlen)
+	if (idx + repmsg->u.remote_dpcd_read_ack.num_bytes > raw->curlen)
 		goto fail_len;
 
 	memcpy(repmsg->u.remote_dpcd_read_ack.bytes, &raw->msg[idx], repmsg->u.remote_dpcd_read_ack.num_bytes);
@@ -907,7 +907,9 @@ static bool drm_dp_sideband_parse_remote_i2c_read_ack(struct drm_dp_sideband_msg
 		goto fail_len;
 	repmsg->u.remote_i2c_read_ack.num_bytes = raw->msg[idx];
 	idx++;
-	/* TODO check */
+	if (idx + repmsg->u.remote_i2c_read_ack.num_bytes > raw->curlen)
+		goto fail_len;
+
 	memcpy(repmsg->u.remote_i2c_read_ack.bytes, &raw->msg[idx], repmsg->u.remote_i2c_read_ack.num_bytes);
 	return true;
 fail_len:
-- 
2.34.1

Claude review: drm/dp/mst: fix OOB reads in remote DPCD/I2C sideband reply parsers

[Claude Code Review Bot]

Overall Series Review

Subject: drm/dp/mst: fix OOB reads in remote DPCD/I2C sideband reply parsers
Author: Ashutosh Desai <ashutoshdesai993@gmail.com>
Patches: 1
Reviewed: 2026-05-16T16:07:31.754498

---

This is a single-patch series that fixes two out-of-bounds read vulnerabilities in the DP MST sideband reply parsers. The fix is straightforward and correct: it ensures the full range of bytes to be copied (`idx + num_bytes`) is validated against `curlen` before each `memcpy`, rather than just checking the current index position.

The patch is well-motivated — `raw->msg[]` is 256 bytes and `curlen` is `u8` (max 255), so a malicious or misbehaving MST hub could craft a reply with `num_bytes` set larger than the remaining payload, causing the `memcpy` to read past the valid message data. Both destination buffers (`bytes[255]`) are large enough for the max `u8` value, so no destination overflow is possible — the issue is purely about reading past the received data in `raw->msg[]`.

**Verdict: Looks good.** The fix is minimal, correct, and appropriate for stable backport.

---

---
Generated by Claude Code Patch Reviewer

Claude review: drm/dp/mst: fix OOB reads in remote DPCD/I2C sideband reply parsers

[Claude Code Review Bot]

Patch Review

**DPCD read fix (line 883):**
```c
-	if (idx > raw->curlen)
+	if (idx + repmsg->u.remote_dpcd_read_ack.num_bytes > raw->curlen)
```
Correct. The old check only validated that `idx` (the position after reading `num_bytes` from the header) was within bounds. It did not account for the subsequent `memcpy` of `num_bytes` bytes starting at `idx`. Since `idx` is `int` (value ~3 at this point) and `num_bytes` is `u8` (max 255), the addition cannot overflow and the comparison against `u8 curlen` is safe.

**I2C read fix (lines 919-920):**
```c
-	/* TODO check */
+	if (idx + repmsg->u.remote_i2c_read_ack.num_bytes > raw->curlen)
+		goto fail_len;
```
Correct. This resolves a longstanding TODO — the I2C read path had **no** bounds check at all before the `memcpy`. The fix mirrors the DPCD read fix and uses the same `fail_len` label pattern already present in the function.

**Boundary condition correctness:** The check `idx + num_bytes > curlen` is the right comparison. Valid message data occupies indices `0` through `curlen - 1`, so a copy of `num_bytes` starting at `idx` requires `idx + num_bytes <= curlen`. The `>` test correctly rejects violations.

**Minor observation:** The commit message's v2 changelog says "Drop separate `idx > curlen` check" — this accurately describes the DPCD read case where the old check was replaced (since `idx + num_bytes > curlen` with `u8 num_bytes >= 0` subsumes `idx > curlen`). For the I2C case there was nothing to drop (only the TODO comment), but the changelog is adequate.

**No issues found.** The patch is correct, minimal, and appropriate for `Cc: stable`.

---
Generated by Claude Code Patch Reviewer