Re: [PATCH] misc: fastrpc: reject overflowing invoke buffer ranges

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] The kernel trusts pointer and length values read directly from the DSP-shared DMA buffer, allowing an arbitrary kernel read/write vulnerability.
- [High] Context is published to the IDR before its refcount is initialized, allowing a concurrent DSP callback to trigger a Use-After-Free.
- [High] `cctx->rpdev` is read locklessly after an initial check, leading to a NULL pointer dereference if `fastrpc_rpmsg_remove` runs concurrently.
- [High] Integer truncation of `mlen` breaks buffer serialization and leads to DMA memory corruption.
--

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

misc: fastrpc: reject overflowing invoke buffer ranges

This commit adds an overflow check to fastrpc_get_buff_overlaps(). It
verifies that adding the buffer length to its start address does not
overflow, returning -EOVERFLOW if it does. This prevents the use of
wrapped end addresses that could misstate payload sizes or feed invalid
ranges into invoke metadata.

> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index f3a49384586d1..ba4ade8747833 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c

[ ... ]

> @@ -675,7 +681,13 @@ static struct fastrpc_invoke_ctx *fastrpc_context_alloc(
>  			return ERR_PTR(-ENOMEM);
>  		}
>  		ctx->args = args;
> -		fastrpc_get_buff_overlaps(ctx);
> +		ret = fastrpc_get_buff_overlaps(ctx);
> +		if (ret) {
> +			kfree(ctx->olaps);
> +			kfree(ctx->maps);
> +			kfree(ctx);
> +			return ERR_PTR(ret);
> +		}
>  	}
>  
>  	/* Released in fastrpc_context_put() */

[Severity: High]
This isn't a bug introduced by this patch, but since we are modifying
fastrpc_context_alloc(), could the context be published to the IDR
before it is fully initialized?

Further down in this function, the context is published:

	ret = idr_alloc_cyclic(&cctx->ctx_idr, ctx, 1,
			       FASTRPC_CTX_MAX, GFP_ATOMIC);

But the reference count is initialized later:

	kref_init(&ctx->refcount);

If an asynchronous callback like fastrpc_rpmsg_callback() retrieves this
newly published context from the IDR and drops its reference, could it
free the context before kref_init() executes, leading to a
use-after-free?


[Severity: High]
This is a pre-existing issue, but looking at fastrpc_internal_invoke(),
is there a lockless check that could lead to a NULL pointer dereference?

	if (!fl->cctx->rpdev)
		return -EPIPE;

If this check passes, and a concurrent device unbind clears the pointer
in fastrpc_rpmsg_remove() under a spinlock:

	spin_lock_irqsave(&cctx->lock, flags);
	cctx->rpdev = NULL;
	...
	spin_unlock_irqrestore(&cctx->lock, flags);

Could the invocation thread then dereference the NULL pointer when it
calls fastrpc_invoke_send()?

	ret = rpmsg_send(cctx->rpdev->ept, (void *)msg, sizeof(*msg));


[Severity: Critical]
This is a pre-existing issue, but does fastrpc_put_args() trust pointers
and lengths read directly from the DSP-shared DMA buffer without
validation?

In fastrpc_put_args():

			void *src = (void *)(uintptr_t)rpra[i].buf.pv;
			void *dst = (void *)(uintptr_t)ctx->args[i].ptr;
			u64 len = rpra[i].buf.len;

			if (!kernel) {
				if (copy_to_user((void __user *)dst, src, len)) {

If a compromised DSP supplies arbitrary addresses in this shared region,
could this result in an arbitrary kernel read or write?


[Severity: High]
This is a pre-existing issue, but does fastrpc_get_args() truncate the
64-bit overlap size into a 32-bit signed integer?

	for (oix = 0; oix < ctx->nbufs; ++oix) {
		int mlen;
		...
			mlen = ctx->olaps[oix].mend - ctx->olaps[oix].mstart;

			if (rlen < mlen)
				goto bail;

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

If the overlap exceeds 2GB, mlen could truncate to a negative value or a
small positive value. Could this cause unsigned promotion to erroneously
reject the buffer, or fail to advance the payload pointer sufficiently,
overwriting subsequent buffers?

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