Re: [PATCH] misc: fastrpc: Add cache maintenance for non-coherent platforms

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] misc: fastrpc: missing error handling for dma_buf_begin_cpu_access()
- [High] misc: fastrpc: cache coherency bypass due to user pointer overlap detection
- [High] misc: fastrpc: missing max_raix update causes incorrect do_cmo evaluation
- [Low] misc: fastrpc: dead code in RPC control header overlap check
--

commit 131a71612f32d48a67415e31fef5d7317be532ce
Author: Abhinav Parihar <abhinav.parihar@oss.qualcomm.com>
Subject: misc: fastrpc: Add cache maintenance for non-coherent platforms

This commit adds cache maintenance for fastrpc argument buffers on
non-coherent platforms. It introduces fastrpc_flush_args() and
fastrpc_inv_args() to flush input buffers and invalidate output buffers based
on the "dma-coherent" property.

> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index d4fac2caca867..83d04b9d2ea02 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
[ ... ]
> @@ -675,6 +679,9 @@ static void fastrpc_get_buff_overlaps(struct fastrpc_invoke_ctx *ctx)
>  			if (ctx->olaps[i].end > max_end) {
>  				max_end = ctx->olaps[i].end;

[Severity: High]
Does this code miss updating max_raix when max_end is extended?

If a partially overlapping buffer extends max_end, max_raix remains stale.
Subsequent fully subsumed buffers might then evaluate their boundary crossing
(do_cmo) against this stale max_raix, potentially skipping necessary cache
maintenance.

>  			} else {
> +				if ((max_raix < inbufs && ctx->olaps[i].raix + 1 > inbufs) ||
> +					(ctx->olaps[i].raix < inbufs && max_raix + 1 > inbufs))
> +					ctx->olaps[i].do_cmo = true;
>  				ctx->olaps[i].mend = 0;
>  				ctx->olaps[i].mstart = 0;
>  			}
[ ... ]
> @@ -1067,6 +1075,64 @@ static int fastrpc_create_maps(struct fastrpc_invoke_ctx *ctx)
>  	return 0;
>  }
>  
> +static void fastrpc_flush_args(struct fastrpc_invoke_ctx *ctx)
> +{
> +	union fastrpc_remote_arg *rpra = ctx->rpra;
> +	int i, inbufs, outbufs;
> +
> +	inbufs = REMOTE_SCALARS_INBUFS(ctx->sc);
> +	outbufs = REMOTE_SCALARS_OUTBUFS(ctx->sc);
> +
> +	for (i = 0; i < inbufs + outbufs; ++i) {
> +		int raix = ctx->olaps[i].raix;
> +		struct fastrpc_map *map = ctx->maps[raix];
> +
> +		if (raix + 1 > inbufs)
> +			continue;
> +		if (!map || !map->buf)
> +			continue;
> +
> +		if (rpra[raix].buf.len && (ctx->olaps[i].mstart || ctx->olaps[i].do_cmo)) {

[Severity: High]
Can cache maintenance be incorrectly skipped here due to relying on mstart?

The fastrpc_get_buff_overlaps() function calculates overlaps and sets mstart
using the user-provided virtual address (ctx->args[i].ptr), rather than the
underlying dma-buf identity. 

If a user passes the same ptr value for two distinct dma-buf fds, the overlap
logic merges them and sets mstart = 0 for the second buffer. Additionally, if
ptr == 0, mstart also evaluates to 0. This could allow untrusted userspace to
bypass cache maintenance, leading to silent data corruption or coherency
issues.

> +			dma_buf_begin_cpu_access(map->buf, DMA_TO_DEVICE);

[Severity: High]
Is it safe to ignore the return value of dma_buf_begin_cpu_access() here?

This function can fail (e.g., returning -ERESTARTSYS if interrupted by a
signal while waiting on fences). 

Ignoring the failure causes the driver to proceed with unstabilized buffers
(which could lead to data corruption) and unconditionally calls
dma_buf_end_cpu_access() on the error path, violating API pairing and
potentially causing exporter crashes or state corruption.

> +			dma_buf_end_cpu_access(map->buf, DMA_TO_DEVICE);
> +		}
> +	}
> +}
> +
> +static void fastrpc_inv_args(struct fastrpc_invoke_ctx *ctx)
> +{
> +	union fastrpc_remote_arg *rpra = ctx->rpra;
> +	int i, inbufs, outbufs;
> +
> +	inbufs = REMOTE_SCALARS_INBUFS(ctx->sc);
> +	outbufs = REMOTE_SCALARS_OUTBUFS(ctx->sc);
> +
> +	for (i = 0; i < inbufs + outbufs; ++i) {
> +		int raix = ctx->olaps[i].raix;
> +		struct fastrpc_map *map = ctx->maps[raix];
> +
> +		if (raix + 1 <= inbufs)
> +			continue;
> +		if (!rpra[raix].buf.len)
> +			continue;
> +		if (!map || !map->buf)
> +			continue;
> +
> +		/*
> +		 * Skip invalidation if the argument overlaps with the
> +		 * RPC control header page.
> +		 */
> +		if (((uintptr_t)rpra & PAGE_MASK) ==
> +			((uintptr_t)rpra[raix].buf.pv & PAGE_MASK))
> +			continue;

[Severity: Low]
Does this code perform a meaningless comparison?

The overlap check compares a kernel virtual address (rpra) with a user-space
virtual address (rpra[raix].buf.pv). 

Because of the `if (!map || !map->buf) continue;` check above it, this code
is only reached for explicit user dma-buf mappings, where pv is explicitly set
to the user-space ptr. These addresses will virtually never match, making this
dead code.

> +
> +		if (ctx->olaps[i].mstart || ctx->olaps[i].do_cmo) {
> +			dma_buf_begin_cpu_access(map->buf, DMA_FROM_DEVICE);

[Severity: High]
Should this check the return value of dma_buf_begin_cpu_access() and handle
errors appropriately before proceeding and calling dma_buf_end_cpu_access()?

> +			dma_buf_end_cpu_access(map->buf, DMA_FROM_DEVICE);
> +		}
> +	}
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260916185850.3686010-1-abhinav.parihar@oss.qualcomm.com?part=1