From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/dp/mst: fix OOB reads on 2-byte fields in sideband reply parsers
Date: Sun, 12 Apr 2026 10:31:51 +1000
Message-ID: <review-patch1-20260410040026.2436280-1-ashutoshdesai993@gmail.com>
In-Reply-To: <20260410040026.2436280-1-ashutoshdesai993@gmail.com>
References: <20260410040026.2436280-1-ashutoshdesai993@gmail.com> <20260410040026.2436280-1-ashutoshdesai993@gmail.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

**Correctness: The fix is sound.** Each of the three functions now validate=
s that 2 bytes are available before performing the 2-byte read. The removal=
 of the post-read `idx +=3D 2` and trailing bounds check on the last field =
in each function is correct =E2=80=94 nothing uses `idx` afterward, and the=
 pre-check is sufficient.

**Issue 1: Missing `Fixes:` tag and `Cc: stable`.**
This is a bug fix for an OOB read. It should identify the commit that intro=
duced the vulnerable code and include:
```
Fixes: <sha> ("<original commit subject>")
Cc: stable@vger.kernel.org
```
The vulnerable pattern has existed since at least the original MST topology=
 code introduction. A `Fixes:` tag is important for stable kernel backports.

**Issue 2: Redundant bounds checks in `enum_path_resources_ack` and `query_=
payload_ack`.**

In `drm_dp_sideband_parse_enum_path_resources_ack`, the patch produces:
```c
idx++;
if (idx > raw->curlen)
    goto fail_len;
if (idx + 2 > raw->curlen)
    goto fail_len;
```
The check `idx + 2 > raw->curlen` strictly subsumes `idx > raw->curlen` (if=
 `idx > curlen`, then `idx + 2 > curlen` is trivially true). The first chec=
k is now redundant. The same pattern appears in `drm_dp_sideband_parse_quer=
y_payload_ack`.

In `drm_dp_sideband_parse_allocate_payload_ack`, the situation is slightly =
different =E2=80=94 the `idx > raw->curlen` check validates the prior singl=
e-byte `vcpi` read, so it's arguably more defensible to keep, though it's s=
till technically subsumed.

Consider collapsing the adjacent checks into a single `idx + 2 > raw->curle=
n` check in each function, or at minimum acknowledge this is intentional fo=
r readability.

**Issue 3: Commit message overstates the OOB consequence.**

The commit message states:
> raw->msg[idx+1] is one byte past the received message data, reading into =
the following struct fields (curchunk_len, curchunk_idx, curlen).

Since `msg` is a 256-byte array (`u8 msg[256]`) and `curlen` is `u8` (max 2=
55), in the common case where `curlen < 255`, the OOB read stays within the=
 `msg` array =E2=80=94 it reads stale/uninitialized bytes from the buffer, =
not the subsequent struct fields. Reading into `curchunk_len` et al. would =
only happen in the edge case where `curlen =3D=3D 255` and `idx+1 =3D=3D 25=
6`, which overruns the array. The commit message should say "reading past t=
he valid message data" rather than implying it always reads into the struct=
 fields that follow `msg[]`.

**Issue 4 (pre-existing, not introduced by this patch):** All three functio=
ns read `raw->msg[1]` at `idx =3D 1` before any bounds check. If `curlen < =
2`, this is also an OOB read (within the buffer, reading stale data). This =
is a pre-existing issue and outside the scope of this patch, but worth noti=
ng for a follow-up.

**Summary:** The fix is correct and addresses a real problem. Please add `F=
ixes:`/`Cc: stable` tags and consider cleaning up the redundant adjacent ch=
ecks before merging.

---
Generated by Claude Code Patch Reviewer