Re: [PATCH v13 2/4] misc: fastrpc: Replace hardcoded ctxid mask with GENMASK

[sashiko-bot@kernel.org]

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 <ekansh.gupta@oss.qualcomm.com>

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/20260713052236.3033744-1-ekansh.gupta@oss.qualcomm.com?part=2