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: misc: fastrpc: keep copied arguments inside the invoke buffer
Date: Wed, 25 Mar 2026 07:05:25 +1000
Message-ID: <review-patch1-20260324014459.93364-1-pengpeng@iscas.ac.cn>
In-Reply-To: <20260324014459.93364-1-pengpeng@iscas.ac.cn>
References: <20260324014459.93364-1-pengpeng@iscas.ac.cn> <20260324014459.93364-1-pengpeng@iscas.ac.cn>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

**Positive aspects:**
- Correctly identifies a real security issue =E2=80=94 the offset subtracti=
on at line 1067 can produce a pointer before the buffer start, and the subs=
equent `copy_from_user()` at line 1085 would write out-of-bounds.
- The two-check approach is appropriate: first check underflow (`offset > a=
rgs - buf_start`), then check that the full length fits (`len > buf_end - d=
st`).

**Issues:**

1. **Type mismatch for `rpra[i].buf.pv`**: The original code assigns to `rp=
ra[i].buf.pv` which is a `u64` (see `union fastrpc_remote_arg`), but the pa=
tch stores a `uintptr_t` into it:
```c
rpra[i].buf.pv =3D dst;
```
The original code was `rpra[i].buf.pv =3D args - ctx->olaps[oix].offset;` w=
hich is the same type arithmetic, so this is fine in practice (implicit wid=
ening on 64-bit), but it's worth noting this is consistent with the origina=
l code's implicit conversion.

2. **Missing Fixes/Cc stable tags**: This is a security fix for an existing=
 vulnerability. The commit message should include a `Fixes:` tag pointing t=
o the commit that introduced the vulnerable code (likely the initial fastrp=
c upstreaming commit) and a `Cc: stable@vger.kernel.org` tag so the fix get=
s backported.

3. **The `len` used in the second check may be overly broad**: The check `l=
en > buf_end - dst` validates against the full argument length `ctx->args[i=
].length`. This is correct =E2=80=94 `len` is the amount that will be copie=
d by `copy_from_user()` at line 1085, so the entire range `[dst, dst+len)` =
must be within the buffer. This check is sound.

4. **Potential integer underflow in `buf_end - dst`**: If somehow `dst > bu=
f_end` (which could happen if the first check passes but `args` itself is b=
eyond `buf_end`), then `buf_end - dst` would wrap since both are `uintptr_t=
` (unsigned). However, this scenario shouldn't occur because `args` is boun=
ded by `rlen` tracking remaining space =E2=80=94 the `rlen < mlen` check at=
 line 1064 prevents `args` from exceeding the buffer. So this is safe in pr=
actice.

5. **The blank line removal is cosmetic but fine**: The original code had a=
 blank line after `} else {` which the patch replaces with the `dst` declar=
ation. Minor style point, not an issue.

**Overall**: This is a valid security fix. It should include `Fixes:` and `=
Cc: stable` tags. The validation logic itself is correct and well-placed. T=
he patch is minimal and focused, which is appropriate for a security fix.

---
Generated by Claude Code Patch Reviewer