Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
Pre-existing issues: - [Critical] Early misc device registration in probe exposes uninitialized context data to userspace, leading to NULL pointer dereferences and a Use-After-Free on error paths. - [High] Permanent memory leak of `fastrpc_buf` structures and DMA buffers when the rpmsg device is removed. - [High] Users holding an open file descriptor can trigger Use-After-Free and NULL pointer dereferences after the rpmsg device is removed. - [High] Refcount leak of the channel context (`cctx`) in the `fastrpc_device_open()` error path. - [High] Use-After-Free vulnerability in `fastrpc_rpmsg_callback()` due to accessing a `fastrpc_invoke_ctx` object without holding a reference count. - [High] Concurrent execution of FASTRPC_IOCTL_INIT_CREATE_STATIC leads to memory leaks and Use-After-Free vulnerabilities. -- commit 89c91d7772abf19a9b1d5b7c02132aeec834f50a 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. This commit adds idr_destroy() to properly clean up the IDR resources. > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > index 1080f9acf70a3..b93267ac30757 100644 > --- a/drivers/misc/fastrpc.c > +++ b/drivers/misc/fastrpc.c > @@ -484,6 +484,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: Critical] This is a pre-existing issue, but does the early misc device registration in fastrpc_rpmsg_probe() expose uninitialized context data to userspace? drivers/misc/fastrpc.c:fastrpc_rpmsg_probe() { ... err = fastrpc_device_register(rdev, data, secure_dsp, domain); if (err) goto err_free_data; ... kref_init(&data->refcount); ... } The device is registered with the misc subsystem and accessible by userspace before essential context variables are initialized. A concurrent open() will access zeroed memory, resulting in crashes. Also, on the error path: drivers/misc/fastrpc.c:fastrpc_rpmsg_probe() { ... err_deregister_fdev: if (data->fdevice) misc_deregister(&data->fdevice->miscdev); ... err_free_data: kfree(data); return err; } The data context is unconditionally freed without checking if any userspace process successfully opened it. misc_deregister() does not revoke existing open file descriptors, so any concurrently opened FD will retain a dangling pointer to the freed context, leading to a Use-After-Free. [Severity: High] This is a pre-existing issue, but does fastrpc_rpmsg_remove() permanently leak fastrpc_buf structures and DMA buffers? drivers/misc/fastrpc.c:fastrpc_rpmsg_remove() { ... list_for_each_entry_safe(buf, b, &cctx->invoke_interrupted_mmaps, node) list_del(&buf->node); ... } The elements are removed from the interrupted mmaps list but never actually freed. It completely fails to call fastrpc_buf_free(buf), which permanently leaks both the struct fastrpc_buf and its associated coherent DMA allocations every time an interrupted channel is torn down. [Severity: High] This is a pre-existing issue, but can users holding an open file descriptor trigger Use-After-Free and NULL pointer dereferences after the rpmsg device is removed? drivers/misc/fastrpc.c:fastrpc_rpmsg_remove() { ... if (cctx->remote_heap) fastrpc_buf_free(cctx->remote_heap); ... } The remote heap buffer is freed, but the pointer cctx->remote_heap is not set to NULL, leaving a dangling pointer. Additionally, in fastrpc_remote_heap_alloc(): drivers/misc/fastrpc.c:fastrpc_remote_heap_alloc() { ... struct device *rdev = &fl->cctx->rpdev->dev; ... } If userspace triggers an allocation after rpmsg_remove sets cctx->rpdev = NULL, this computes an invalid pointer leading to a NULL pointer dereference. [Severity: High] This is a pre-existing issue, but is there a refcount leak of the channel context in the fastrpc_device_open() error path? drivers/misc/fastrpc.c:fastrpc_device_open() { ... fl->sctx = fastrpc_session_alloc(fl); if (!fl->sctx) { dev_err(&cctx->rpdev->dev, "No session available\n"); mutex_destroy(&fl->mutex); kfree(fl); return -EBUSY; } ... } The error path fails to drop the reference acquired by fastrpc_channel_ctx_get(cctx), leaking the context's refcount permanently. Since the open() syscall fails, the release file operation will never execute, preventing the cctx object from ever being freed. [Severity: High] This is a pre-existing issue, but is there a Use-After-Free vulnerability in fastrpc_rpmsg_callback() due to accessing a fastrpc_invoke_ctx object without holding a reference count? drivers/misc/fastrpc.c:fastrpc_rpmsg_callback() { ... spin_lock_irqsave(&cctx->lock, flags); ctx = idr_find(&cctx->ctx_idr, ctxid); spin_unlock_irqrestore(&cctx->lock, flags); if (!ctx) { dev_err(&rpdev->dev, "No context ID matches response\n"); return -ENOENT; } ctx->retval = rsp->retval; ... } The lock is released without elevating the reference count of ctx. A concurrent close can free ctx before ctx->retval is written, resulting in a Use-After-Free write to a freed work structure. [Severity: High] This is a pre-existing issue, but does concurrent execution of FASTRPC_IOCTL_INIT_CREATE_STATIC lead to memory leaks and Use-After-Free vulnerabilities? 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); } ... } The check and subsequent allocation of the shared remote_heap is performed without holding any locks, allowing race conditions between concurrent ioctls. This causes multiple DMA allocations where subsequent threads overwrite fl->cctx->remote_heap, permanently leaking the previously allocated memory. Also, on the error path: drivers/misc/fastrpc.c:fastrpc_init_create_static_process() { ... err_map: fastrpc_buf_free(fl->cctx->remote_heap); fl->cctx->remote_heap = NULL; ... } In the error path, the shared heap is freed and nulled. If another thread is concurrently using it, this leads to a Use-After-Free or dereferencing a NULL pointer. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
