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 <[email protected]> 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/[email protected]?part=1
