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

[sashiko-bot@kernel.org]

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 <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. 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/20260617-fastrpc-cctx-cleanup-v2-1-be87c021114a@oss.qualcomm.com?part=1