Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues: - [Low] The patch uses `FIELD_PREP` and `FIELD_GET` macros which are defined in `<linux/bitfield.h>`, but incorrectly includes `<linux/bitops.h>` instead. Pre-existing issues: - [Critical] Circular reference in `fastrpc_internal_invoke` leaks contexts, DMA memory, and IDR slots on `-ERESTARTSYS`, leading to a system-wide Denial of Service. - [High] Failure in `copy_to_user` during `fastrpc_req_mmap` leaves a freed buffer on the `fl->mmaps` list. - [High] Concurrent allocations of `fl->cctx->remote_heap` lack locking, leading to UAF and memory leaks. - [High] fastrpc_rpmsg_callback dereferences ctx without taking a reference, leading to a race condition and UAF. -- --- Patch [2]: [PATCH v13 2/4] misc: fastrpc: Replace hardcoded ctxid mask with GENMASK --- commit e84380d211cbe45ae646449f6efb6a5a3e342fcc Author: Ekansh Gupta <[email protected]> misc: fastrpc: Replace hardcoded ctxid mask with GENMASK The commit replaces the hardcoded 0xFF0 mask with GENMASK(11, 4) for context ID extraction to follow bitfield conventions. It uses FIELD_PREP and FIELD_GET macros instead of manual shifts. > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > index 02c2765e1188c..41c2ec0bc6281 100644 > --- a/drivers/misc/fastrpc.c > +++ b/drivers/misc/fastrpc.c > @@ -23,6 +23,7 @@ > #include <uapi/misc/fastrpc.h> > #include <linux/of_reserved_mem.h> > #include <linux/bits.h> > +#include <linux/bitops.h> [Severity: Low] Should this code include <linux/bitfield.h> instead of <linux/bitops.h>? The FIELD_PREP and FIELD_GET macros are defined in <linux/bitfield.h>. Not including the proper header may cause compilation failures on architectures that do not transitively include it. [ ... ] > @@ -2547,7 +2548,7 @@ static int fastrpc_rpmsg_callback(struct rpmsg_device > *rpdev, void *data, > if (!cctx) > return -ENODEV; > > - ctxid = ((rsp->ctx & FASTRPC_CTXID_MASK) >> 4); > + ctxid = FIELD_GET(FASTRPC_CTXID_MASK, rsp->ctx); > > spin_lock_irqsave(&cctx->lock, flags); > ctx = idr_find(&cctx->ctx_idr, ctxid); [Severity: High] This isn't a bug introduced by this patch, but could fastrpc_rpmsg_callback() dereference ctx without taking a reference here? If a duplicate or late response is received, the first reply schedules ctx->put_work which invokes fastrpc_context_free(). If a second reply's IDR lookup occurs right before fastrpc_context_free() removes it from the IDR, it might yield a pointer that gets freed immediately after the lock is dropped: 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; complete(&ctx->work); ... } Writing to ctx->retval and calling complete() would then operate on freed memory. Should kref_get_unless_zero() be used before dropping the lock? [ ... ] [Severity: Critical] This isn't a bug introduced by this patch, but is there a circular reference regression on -ERESTARTSYS in fastrpc_internal_invoke()? drivers/misc/fastrpc.c:fastrpc_internal_invoke() { ... if (kernel) { if (!wait_for_completion_timeout(&ctx->work, 10 * HZ)) err = -ETIMEDOUT; } else { err = wait_for_completion_interruptible(&ctx->work); } if (err) goto bail; ... bail: if (err != -ERESTARTSYS && err != -ETIMEDOUT) { /* We are done with this compute context */ spin_lock(&fl->lock); list_del(&ctx->node); spin_unlock(&fl->lock); fastrpc_context_put(ctx); } ... } If a user interrupts FASTRPC_IOCTL_INVOKE with a signal, err becomes -ERESTARTSYS. The context is left on fl->pending without calling fastrpc_context_put(ctx). Since ctx holds a reference to fl, fl->refcount will never reach 0 on device close, permanently leaking contexts and DMA memory. Can this be used to exhaust IDR slots? [ ... ] [Severity: High] This isn't a bug introduced by this patch, but what happens in fastrpc_req_mmap() if copy_to_user() fails after the buffer is added to fl->mmaps? drivers/misc/fastrpc.c:fastrpc_req_mmap() { ... spin_lock(&fl->lock); list_add_tail(&buf->node, &fl->mmaps); spin_unlock(&fl->lock); if (copy_to_user((void __user *)argp, &req, sizeof(req))) { err = -EFAULT; goto err_assign; } ... err_assign: fastrpc_req_munmap_impl(fl, buf); return err; } The fastrpc_req_munmap_impl() function frees the buffer, but it remains linked on fl->mmaps. When the user closes the device, fastrpc_user_free() iterates over fl->mmaps and could double-free this buffer. Should the buffer be removed from the list before returning? [ ... ] [Severity: High] This isn't a bug introduced by this patch, but are concurrent allocations of remote_heap safely handled in fastrpc_init_create_static_process()? 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); if (err) goto err_name; ... } If two threads concurrently call FASTRPC_IOCTL_INIT_CREATE_STATIC, both might observe !fl->cctx->remote_heap as true and call fastrpc_remote_heap_alloc(), overwriting the shared pointer and leaking the first allocation. Could this also lead to a use-after-free if one thread branches to err_map and frees fl->cctx->remote_heap while the other thread is using it? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
