Re: [PATCH v8 4/4] misc: fastrpc: Allow fastrpc_buf_free() to accept NULL

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Exported DMA buffers outlive their fastrpc_user context, leading to a Use-After-Free on dmabuf operations.
- [Critical] In `fastrpc_req_mmap()`, if `copy_to_user()` fails, the allocated buffer is freed via `fastrpc_req_munmap_impl()`, but its node is never removed from `fl->mmaps`.
- [Critical] Timeouts in kernel RPCs cause DMA buffers to be freed while the DSP is still actively using them.
- [High] Interrupted mmaps are unlinked but their memory is never freed during `fastrpc_rpmsg_remove()`.
- [High] Invocation contexts leak references to attached maps for handles, allowing unbounded resource accumulation.
- [High] Failure to restore hypervisor memory permissions in `fastrpc_req_munmap_impl` for remote heap allocations.
--

commit c345df439255945e9c8c222e251d798521775ff6
Author: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>

misc: fastrpc: Allow fastrpc_buf_free() to accept NULL

Make fastrpc_buf_free() a no-op when passed a NULL pointer, allowing
callers to avoid open-coded NULL checks.

> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 1942e74535e52..b4912b443b982 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -424,6 +424,9 @@ static int fastrpc_map_lookup(struct fastrpc_user *fl, int fd,
>  
>  static void fastrpc_buf_free(struct fastrpc_buf *buf)
>  {
> +	if (!buf)
> +		return;
> +
>  	dma_free_coherent(buf->dev, buf->size, buf->virt,
>  			  fastrpc_ipa_to_dma_addr(buf->fl->cctx, buf->dma_addr));

[Severity: Critical]
This is a pre-existing issue, but does fastrpc_buf_free() have a
use-after-free risk when called from the dmabuf release path?

If a user closes the fastrpc device descriptor but keeps the DMA-BUF file
descriptor open, the fastrpc_user (fl) context is freed. When the DMA-BUF
is later released, it calls fastrpc_buf_free(), which dereferences
buf->fl->cctx here to calculate the DMA address.

Since fastrpc_dmabuf_alloc() doesn't take a reference on the fastrpc_user
when exporting the DMA-BUF, can this lead to memory corruption?

[Severity: High]
This isn't a bug introduced by this patch, but does fastrpc_buf_free()
safely handle buffers mapped with ADSP_MMAP_REMOTE_HEAP_ADDR?

When fastrpc_req_mmap() maps a remote heap buffer, it uses
qcom_scm_assign_mem() to grant access to the DSP VM. During unmapping,
fastrpc_req_munmap_impl() calls fastrpc_buf_free() directly.

Will handing this restricted memory back to the kernel's allocator without
first calling qcom_scm_assign_mem() to restore ownership to the HLOS lead
to a hypervisor fault when the kernel tries to use the memory?

[Severity: Critical]
This is a pre-existing issue, but does calling fastrpc_buf_free() on
timeout lead to memory corruption by the DSP?

When fastrpc_internal_invoke() times out, callers like fastrpc_req_mmap()
and fastrpc_init_create_process() treat it as a failure and immediately
call fastrpc_buf_free(buf) to release the kernel memory.

Since there is no mechanism to cancel the pending request on the DSP, won't
the DSP continue processing and access the freed DMA memory, potentially
corrupting memory that has been reallocated by the system?

>  	kfree(buf);
>  }
> @@ -516,8 +519,7 @@ static void fastrpc_user_free(struct kref *ref)
>  	struct fastrpc_map *map, *m;
>  	struct fastrpc_buf *buf, *b;
>  
> -	if (fl->init_mem)
> -		fastrpc_buf_free(fl->init_mem);
> +	fastrpc_buf_free(fl->init_mem);
>  
>  	list_for_each_entry_safe(ctx, n, &fl->pending, node) {

[Severity: Critical]
This isn't a bug introduced by this patch, but does fastrpc_user_free()
risk a use-after-free and double free when cleaning up fl->mmaps?

In fastrpc_req_mmap(), if copy_to_user() fails, the code jumps to
err_assign and calls fastrpc_req_munmap_impl(). This unmaps and frees the
buffer using fastrpc_buf_free(), but leaves its node in the fl->mmaps
list.

When fastrpc_user_free() later loops over fl->mmaps, will it attempt to
access and free this already-freed buffer?

[Severity: High]
This is a pre-existing issue, but does fastrpc_rpmsg_remove() leak
interrupted mmaps when tearing down the channel?

When an invocation returns -ERESTARTSYS, mapped buffers are moved from
fl->mmaps to cctx->invoke_interrupted_mmaps. During fastrpc_rpmsg_remove(),
the elements are unlinked:

    list_for_each_entry_safe(buf, b, &cctx->invoke_interrupted_mmaps, node)
        list_del(&buf->node);

Does skipping the fastrpc_buf_free(buf) call here cause a permanent leak
of the fastrpc_buf and its associated DMA/SMMU memory allocations?

>  		list_del(&ctx->node);
> @@ -562,8 +564,7 @@ static void fastrpc_context_free(struct kref *ref)
>  	for (i = 0; i < ctx->nbufs; i++)
>  		fastrpc_map_put(ctx->maps[i]);
>  

[Severity: High]
This isn't a bug introduced by this patch, but does fastrpc_context_free()
leak references to attached maps for handles?

In fastrpc_create_maps(), the ctx->maps array is populated for all
nscalars. For indices >= nbufs (handles), fastrpc_map_attach() creates
a new map and adds it to fl->maps.

Since this cleanup loop only iterates up to ctx->nbufs, does it fail to
drop the references for the remaining handle maps in ctx->maps, causing
unbounded accumulation in fl->maps?

> -	if (ctx->buf)
> -		fastrpc_buf_free(ctx->buf);
> +	fastrpc_buf_free(ctx->buf);
>  
>  	spin_lock_irqsave(&cctx->lock, flags);
>  	idr_remove(&cctx->ctx_idr, ctx->ctxid >> 4);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609025938.457-1-jianping.li@oss.qualcomm.com?part=4