[PATCH] misc: fastrpc: reject oversized inline invoke payloads

[PATCH] misc: fastrpc: reject oversized inline invoke payloads

[Yousef Alhouseen]

fastrpc_get_args() computes inline payload spans from 64-bit user
buffer ranges, but stores each span in an int before comparing it with
the remaining DMA buffer space.

A large span can truncate before the bounds check and advance the DMA
write cursor by the wrong amount. Keep the span 64-bit and fail the
invoke when the serialized payload would exceed the buffer.

Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>
---
 drivers/misc/fastrpc.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 42fc128e1..8dd8315d5 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -1079,7 +1079,7 @@ static int fastrpc_get_args(u32 kernel, struct fastrpc_invoke_ctx *ctx)
 	ctx->rpra = rpra;
 
 	for (oix = 0; oix < ctx->nbufs; ++oix) {
-		int mlen;
+		u64 mlen;
 
 		i = ctx->olaps[oix].raix;
 		len = ctx->args[i].length;
@@ -1119,8 +1119,10 @@ static int fastrpc_get_args(u32 kernel, struct fastrpc_invoke_ctx *ctx)
 
 			mlen = ctx->olaps[oix].mend - ctx->olaps[oix].mstart;
 
-			if (rlen < mlen)
+			if (rlen < mlen) {
+				err = -EOVERFLOW;
 				goto bail;
+			}
 
 			rpra[i].buf.pv = args - ctx->olaps[oix].offset;
 			pages[i].addr = ctx->buf->dma_addr -
-- 
2.54.0

Re: [PATCH] misc: fastrpc: reject oversized inline invoke payloads

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Integer overflow in `fastrpc_get_payload_size()` accumulator leads to undersized DMA buffer and massive heap out-of-bounds write.
- [Critical] Heap buffer underflow via overlaps between mapped and unmapped buffers in `fastrpc_get_buff_overlaps()`.
- [High] SMMU isolation bypass via unverified VMA offset calculations.
- [High] Resource leak of `fastrpc_map` objects and DMA attachments for input/output handles.
- [High] Implicit integer truncation to heap buffer overflow on 32-bit architectures.
--

commit a53ab016dbb10f6c8573b1c5e13acf7acf361b1d
Author: Yousef Alhouseen <alhouseenyousef@gmail.com>

misc: fastrpc: reject oversized inline invoke payloads

fastrpc_get_args() computes inline payload spans from 64-bit user
buffer ranges, but stores each span in an int before comparing it with
the remaining DMA buffer space. A large span can truncate before the bounds
check and advance the DMA write cursor by the wrong amount. This patch keeps
the span 64-bit and fails the invoke when the serialized payload would exceed
the buffer.

> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index f3a49384586d1..c60bab4f45c01 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -1067,7 +1067,7 @@ static int fastrpc_get_args(u32 kernel, struct fastrpc_invoke_ctx *ctx)
>  	ctx->rpra = rpra;
>  
>  	for (oix = 0; oix < ctx->nbufs; ++oix) {
> -		int mlen;
> +		u64 mlen;

[Severity: High]
This isn't a bug introduced by this patch, but does the use of 64-bit sizes
lead to truncation issues on 32-bit architectures?

When pkt_size is passed to dma_alloc_coherent() via __fastrpc_buf_alloc(),
the 64-bit size is implicitly truncated to a 32-bit size_t. This can result
in a small kernel allocation for values exceeding 4GB.

The bounds checks use the untruncated 64-bit lengths, and copy_from_user()
will truncate the 64-bit len to a 32-bit unsigned long. Could coordinating
these lengths to wrap around the 32-bit boundary cause copy_from_user() to
copy a large payload into the truncated DMA buffer allocation?

>  
>  		i = ctx->olaps[oix].raix;
>  		len = ctx->args[i].length;

[Severity: High]
This is a pre-existing issue, but is there a risk of SMMU isolation bypass
for mapped buffers?

For mapped DMA buffers, fastrpc_get_args() computes the physical address
offset by subtracting the VMA start address:

drivers/misc/fastrpc.c:fastrpc_get_args() {
    ...
			vma = vma_lookup(current->mm, ctx->args[i].ptr);
			if (vma)
				pages[i].addr += (ctx->args[i].ptr & PAGE_MASK) -
						 vma->vm_start;
    ...
}

Does the code verify that the resolved VMA actually corresponds to the
underlying dma_buf, or that offset + len is within the map's size? If not,
could an attacker forge an arbitrary IOVA offset using a large anonymous VMA
to read or write arbitrary memory mapped within the shared SMMU domain?

[ ... ]

> @@ -1107,8 +1107,10 @@ static int fastrpc_get_args(u32 kernel, struct fastrpc_invoke_ctx *ctx)
>  
>  			mlen = ctx->olaps[oix].mend - ctx->olaps[oix].mstart;
>  
> -			if (rlen < mlen)
> +			if (rlen < mlen) {
> +				err = -EOVERFLOW;
>  				goto bail;

[Severity: Critical]
This is a pre-existing issue, but does rlen bypass this check if pkt_size
wraps around and rlen underflows?

In fastrpc_get_payload_size(), size accumulates the buffer lengths without
checking for wrap-around. An attacker could supply lengths such that the sum
exceeds U64_MAX, resulting in a small positive pkt_size and causing an
undersized DMA allocation.

Then in fastrpc_get_args():
    rlen = pkt_size - metalen;

If pkt_size is smaller than metalen, rlen underflows to a massive 64-bit
value, which trivially bypasses this rlen < mlen check. Could this lead to
copy_from_user() copying the untruncated len into the undersized heap
allocation?

[Severity: High]
This is a pre-existing issue, but could this early exit leak DMA map
attachments for scalar handles?

In fastrpc_create_maps(), map objects are created and attached for all
arguments up to ctx->nscalars. However, when the context is freed:

drivers/misc/fastrpc.c:fastrpc_context_free() {
    ...
	for (i = 0; i < ctx->nbufs; i++)
		fastrpc_map_put(ctx->maps[i]);
    ...
}

The cleanup loop only iterates up to ctx->nbufs, failing to drop references
for handle maps (i >= ctx->nbufs). If this goto bail is hit, do the maps for
scalar handles remain pinned in the user process's list, potentially
exhausting kernel memory and DMA allocations?

> +			}
>  
>  			rpra[i].buf.pv = args - ctx->olaps[oix].offset;

[Severity: Critical]
This is a pre-existing issue, but can this address underflow to a location
before the allocated heap buffer?

If fastrpc_get_buff_overlaps() processes an unmapped inline buffer that
overlaps entirely within a preceding mapped buffer, it sets mstart and
mend to 0, and offset to the distance from the mapped buffer's start.

Because args would not have advanced for this buffer, could subtracting
offset from args here result in an address pointing before
ctx->buf->virt, leading to an out-of-bounds write when copy_from_user()
is called later in fastrpc_get_args()?

>  			pages[i].addr = ctx->buf->dma_addr -

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260624185549.1669-1-alhouseenyousef@gmail.com?part=1