Re: [PATCH v3] misc: fastrpc: fix memory leak in fastrpc_channel_ctx_free

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] DMA buffers moved to `invoke_interrupted_mmaps` are permanently leaked.
- [High] Premature exposure of the misc device in probe allows uninitialized access, Use-After-Free, and Double Free.
- [High] Concurrent device removal causes a NULL pointer dereference in `fastrpc_invoke_send`.
- [High] Potential Use-After-Free of `cctx->remote_heap` after `fastrpc_rpmsg_remove` frees it.
- [High] Maps allocated for handle arguments are never freed during context teardown.
- [High] Memory assigned to the DSP hypervisor is returned to the Linux page allocator without restoring permissions, causing an XPU fault.
- [High] A double free of dmabuf attachments and refcount underflow of `dma_buf` occurs in the error path of `fastrpc_map_attach`.
- [Medium] Double allocation of `remote_heap` due to missing synchronization.
- [Medium] The channel context refcount is leaked in the error path of `fastrpc_device_open`.
--

commit 7b667747a11655158ab5dcea5700acfc6656da6d
Author: Eddie Lin <eddie.lin@oss.qualcomm.com>

misc: fastrpc: fix memory leak in fastrpc_channel_ctx_free
    
The 'ctx_idr' is initialized but never destroyed when
the channel context is freed, leading to a memory leak.
Add idr_destroy() to properly clean up the IDR resources.

> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index f3a49384586d1..1a35380a1557a 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -492,6 +492,7 @@ static void fastrpc_channel_ctx_free(struct kref *ref)
>  
>  	cctx = container_of(ref, struct fastrpc_channel_ctx, refcount);
>  
> +	idr_destroy(&cctx->ctx_idr);
>  	kfree(cctx);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does the driver permanently
leak dma buffers moved to invoke_interrupted_mmaps?

In fastrpc_internal_invoke(), buffers are moved to the interrupted list to
prevent them from being freed while the dsp is active:

drivers/misc/fastrpc.c:fastrpc_internal_invoke() {
    ...
    list_add_tail(&buf->node, &fl->cctx->invoke_interrupted_mmaps);
    ...
}

However, during device teardown in fastrpc_rpmsg_remove(), the list nodes
are unlinked but the associated memory appears to be abandoned, without
calling fastrpc_buf_free():

drivers/misc/fastrpc.c:fastrpc_rpmsg_remove() {
    ...
    list_for_each_entry_safe(buf, b, &cctx->invoke_interrupted_mmaps, node)
        list_del(&buf->node);
    ...
}

Could a user space program exhaust system dma memory by repeatedly triggering
signals during invocation?

[Severity: High]
This is a pre-existing issue, but does the premature exposure of the misc
device in probe allow uninitialized access or a use-after-free?

In fastrpc_rpmsg_probe(), the misc device is registered before the subsequent
kref_init(&data->refcount) is called:

drivers/misc/fastrpc.c:fastrpc_rpmsg_probe() {
    ...
    err = fastrpc_device_register(rdev, data, true, domain);
    ...
}

If probe subsequently fails, the structure is freed unconditionally without
checking if an open file has taken a reference to it:

drivers/misc/fastrpc.c:fastrpc_rpmsg_probe() {
    ...
    kfree(data);
    ...
}

Could a racing userspace open access the uninitialized refcount via
fastrpc_device_open() and continue to issue ioctls against freed memory if
probe aborts?

[Severity: High]
This isn't a bug introduced by this patch, but can concurrent device removal
cause a null pointer dereference?

In fastrpc_invoke_send(), the cctx->rpdev pointer is dereferenced without
holding locks:

drivers/misc/fastrpc.c:fastrpc_invoke_send() {
    ...
    ret = rpmsg_send(cctx->rpdev->ept, (void *)msg, sizeof(*msg));
    ...
}

Concurrently, fastrpc_rpmsg_remove() sets cctx->rpdev to null while ioctls
might still be running:

drivers/misc/fastrpc.c:fastrpc_rpmsg_remove() {
    ...
    cctx->rpdev = NULL;
    ...
}

Could this cause a kernel panic if user space ioctls run concurrently with a
device unbind or module unload event?

[Severity: High]
This is a pre-existing issue, but is there a potential use-after-free of
cctx->remote_heap?

In fastrpc_rpmsg_remove(), the buffer is freed but leaves cctx->remote_heap
as a dangling pointer:

drivers/misc/fastrpc.c:fastrpc_rpmsg_remove() {
    ...
    if (cctx->remote_heap)
        fastrpc_buf_free(cctx->remote_heap);
    ...
}

Lingering open file descriptors can execute
fastrpc_init_create_static_process(), pass the !fl->cctx->remote_heap check,
and dereference the dangling pointer's fields:

drivers/misc/fastrpc.c:fastrpc_init_create_static_process() {
    ...
    pages[0].addr = fl->cctx->remote_heap->dma_addr;
    ...
}

Could userspace intentionally hold an open file descriptor after the device
is removed to trigger memory corruption?

[Severity: Medium]
This isn't a bug introduced by this patch, but does missing synchronization
lead to a double allocation of remote_heap?

In fastrpc_init_create_static_process(), the check and allocation are
unsynchronized:

drivers/misc/fastrpc.c:fastrpc_init_create_static_process() {
    ...
    if (!fl->cctx->remote_heap) {
        err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen,
                                        &fl->cctx->remote_heap);
    ...
}

Can multiple concurrent threads pass the condition and allocate memory
simultaneously, leaking the dma buffer?

[Severity: High]
This is a pre-existing issue, but are maps allocated for handle arguments
abandoned during context teardown?

In fastrpc_create_maps(), maps for handle arguments are created up to
ctx->nscalars (where indices >= nbufs represent handle arguments):

drivers/misc/fastrpc.c:fastrpc_create_maps() {
    ...
    else
        err = fastrpc_map_attach(ctx->fl, ctx->args[i].fd,
                                 ctx->args[i].length, ctx->args[i].attr,
                                 &ctx->maps[i]);
    ...
}

However, in fastrpc_context_free(), the cleanup loop terminates early at
nbufs instead of nscalars, leaving the attached map pointers:

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

Can userspace continually leak map structures and dma_buf references by
providing handle arguments?

[Severity: High]
This isn't a bug introduced by this patch, but is memory assigned to the
dsp hypervisor returned to the Linux page allocator without restoring
permissions?

In fastrpc_rpmsg_remove(), the remote_heap buffer is freed without reclaiming
scm permissions:

drivers/misc/fastrpc.c:fastrpc_rpmsg_remove() {
    ...
    if (cctx->remote_heap)
        fastrpc_buf_free(cctx->remote_heap);
    ...
}

Similarly, in fastrpc_req_munmap_impl(), mapped user buffers are freed
without reclaiming scm permissions:

drivers/misc/fastrpc.c:fastrpc_req_munmap_impl() {
    ...
    fastrpc_buf_free(buf);
    ...
}

Will returning memory with stale xpu protections to the system page allocator
cause a system crash via hypervisor access control exceptions?

[Severity: High]
This is a pre-existing issue, but does a double free of dmabuf attachments
and a refcount underflow occur in the error path of fastrpc_map_attach()?

The error path performs manual detach and put operations before handing off
to the standard cleanup function which repeats them since map->table was left
non-null:

drivers/misc/fastrpc.c:fastrpc_map_attach() {
    ...
map_err:
    dma_buf_detach(map->buf, map->attach);
attach_err:
    dma_buf_put(map->buf);
get_err:
    fastrpc_map_put(map);
    ...
}

If an error occurs after map->table is populated, fastrpc_free_map() executes
its teardown logic and repeats the same dma_buf_detach() and dma_buf_put()
calls on the same pointers.

Does this lead to memory corruption?

[Severity: Medium]
This isn't a bug introduced by this patch, but is the channel context
refcount leaked in the error path of fastrpc_device_open()?

The error path exits early without dropping the cctx reference:

drivers/misc/fastrpc.c:fastrpc_device_open() {
    ...
    if (!fl->sctx) {
        dev_err(&cctx->rpdev->dev, "No session available\n");
        mutex_destroy(&fl->mutex);
        kfree(fl);

        return -EBUSY;
    }
    ...
}

If fastrpc_session_alloc() fails, does the function forget to call
fastrpc_channel_ctx_put() to balance the fastrpc_channel_ctx_get() called
earlier, allowing a user to exhaust the session count and leak references?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701-fastrpc-cctx-cleanup-v3-1-3a73c2e4ce1a@oss.qualcomm.com?part=1