[PATCH] misc: fastrpc: keep copied arguments inside the invoke buffer

[PATCH] misc: fastrpc: keep copied arguments inside the invoke buffer

[Pengpeng Hou]

fastrpc_get_args() derives rpra[i].buf.pv from the overlap offset that
was computed from user-controlled argument pointers and lengths. The
resulting destination pointer is then used for copy_from_user() without
first checking that it still falls inside the allocated invoke buffer.

Validate the overlap-derived destination range before storing it in
rpra[i].buf.pv and before copying inline arguments into the invoke
buffer.
---
 drivers/misc/fastrpc.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 47356a5d5804..d7f53300b899 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -993,6 +993,7 @@ static int fastrpc_get_args(u32 kernel, struct fastrpc_invoke_ctx *ctx)
 	u64 len, rlen, pkt_size;
 	u64 pg_start, pg_end;
 	uintptr_t args;
+	uintptr_t buf_start, buf_end;
 	int metalen;
 
 	inbufs = REMOTE_SCALARS_INBUFS(ctx->sc);
@@ -1016,6 +1017,8 @@ static int fastrpc_get_args(u32 kernel, struct fastrpc_invoke_ctx *ctx)
 	rpra = ctx->buf->virt;
 	list = fastrpc_invoke_buf_start(rpra, ctx->nscalars);
 	pages = fastrpc_phy_page_start(list, ctx->nscalars);
+	buf_start = (uintptr_t)ctx->buf->virt;
+	buf_end = buf_start + pkt_size;
 	args = (uintptr_t)ctx->buf->virt + metalen;
 	rlen = pkt_size - metalen;
 	ctx->rpra = rpra;
@@ -1053,6 +1056,7 @@ static int fastrpc_get_args(u32 kernel, struct fastrpc_invoke_ctx *ctx)
 			pages[i].size = (pg_end - pg_start + 1) * PAGE_SIZE;
 
 		} else {
+			uintptr_t dst;
 
 			if (ctx->olaps[oix].offset == 0) {
 				rlen -= ALIGN(args, FASTRPC_ALIGN) - args;
@@ -1064,7 +1068,18 @@ static int fastrpc_get_args(u32 kernel, struct fastrpc_invoke_ctx *ctx)
 			if (rlen < mlen)
 				goto bail;
 
-			rpra[i].buf.pv = args - ctx->olaps[oix].offset;
+			if (ctx->olaps[oix].offset > args - buf_start) {
+				err = -EINVAL;
+				goto bail;
+			}
+
+			dst = args - ctx->olaps[oix].offset;
+			if (len > buf_end - dst) {
+				err = -EINVAL;
+				goto bail;
+			}
+			rpra[i].buf.pv = dst;
+
 			pages[i].addr = ctx->buf->dma_addr -
 					ctx->olaps[oix].offset +
 					(pkt_size - rlen);
-- 
2.50.1 (Apple Git-155)

Re: [PATCH] misc: fastrpc: keep copied arguments inside the invoke buffer

[Konrad Dybcio]

On 3/24/26 2:44 AM, Pengpeng Hou wrote:
> fastrpc_get_args() derives rpra[i].buf.pv from the overlap offset that
> was computed from user-controlled argument pointers and lengths. The
> resulting destination pointer is then used for copy_from_user() without
> first checking that it still falls inside the allocated invoke buffer.
> 
> Validate the overlap-derived destination range before storing it in
> rpra[i].buf.pv and before copying inline arguments into the invoke
> buffer.
> ---

Your contribution lacks a DCO:

https://docs.kernel.org/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin

without which we can't accept it.

Please run ./scripts/checkpatch.pl on the patch file

Konrad

Claude review: misc: fastrpc: keep copied arguments inside the invoke buffer

[Claude Code Review Bot]

Overall Series Review

Subject: misc: fastrpc: keep copied arguments inside the invoke buffer
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>
Patches: 2
Reviewed: 2026-03-25T07:05:25.189660

---

This is a single-patch series adding bounds validation to `fastrpc_get_args()` in the Qualcomm FastRPC driver. The patch addresses a real issue: the overlap-derived destination pointer (`args - ctx->olaps[oix].offset`) is used for `copy_from_user()` without checking that it remains within the allocated invoke buffer. Since `olaps` values are derived from user-controlled argument pointers/lengths (via `fastrpc_get_buff_overlaps()`), a malicious user could potentially write outside the buffer.

The fix is reasonable and the approach is sound, but there are a couple of issues worth noting.

---
Generated by Claude Code Patch Reviewer

Claude review: misc: fastrpc: keep copied arguments inside the invoke buffer

[Claude Code Review Bot]

Patch Review

**Positive aspects:**
- Correctly identifies a real security issue — the offset subtraction at line 1067 can produce a pointer before the buffer start, and the subsequent `copy_from_user()` at line 1085 would write out-of-bounds.
- The two-check approach is appropriate: first check underflow (`offset > args - buf_start`), then check that the full length fits (`len > buf_end - dst`).

**Issues:**

1. **Type mismatch for `rpra[i].buf.pv`**: The original code assigns to `rpra[i].buf.pv` which is a `u64` (see `union fastrpc_remote_arg`), but the patch stores a `uintptr_t` into it:
```c
rpra[i].buf.pv = dst;
```
The original code was `rpra[i].buf.pv = args - ctx->olaps[oix].offset;` which is the same type arithmetic, so this is fine in practice (implicit widening on 64-bit), but it's worth noting this is consistent with the original 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 to the commit that introduced the vulnerable code (likely the initial fastrpc upstreaming commit) and a `Cc: stable@vger.kernel.org` tag so the fix gets backported.

3. **The `len` used in the second check may be overly broad**: The check `len > buf_end - dst` validates against the full argument length `ctx->args[i].length`. This is correct — `len` is the amount that will be copied 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 > buf_end` (which could happen if the first check passes but `args` itself is beyond `buf_end`), then `buf_end - dst` would wrap since both are `uintptr_t` (unsigned). However, this scenario shouldn't occur because `args` is bounded by `rlen` tracking remaining space — the `rlen < mlen` check at line 1064 prevents `args` from exceeding the buffer. So this is safe in practice.

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` declaration. 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. The patch is minimal and focused, which is appropriate for a security fix.

---
Generated by Claude Code Patch Reviewer