Re: [PATCH v4 13/25] accel/qda: Add the invocation ioctl and teardown on unplug

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] accel/qda: unconditional kref_put() in qda_fastrpc_flush_pending()
- [Critical] accel/qda: premature teardown in qda_rpmsg_remove() while ctxs active
- [Critical] accel/qda: missing init handle check in qda_fastrpc_prepare_args_invoke()
- [Critical] accel/qda: missing bounds check and union misuse in process_dma_handle()
- [Medium] accel/qda: handle list leak on DSP error in qda_fastrpc_invoke_common()
--

commit 9e89a77ec2ad7f5231cb9a9e8fb15165b01e374a
Author: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>

accel/qda: Add the invocation ioctl and teardown on unplug

This commit adds the DRM_IOCTL_QDA_REMOTE_INVOKE ioctl to allocate an
invocation context, prepare arguments, pack and send the payload over
RPMsg, and wait for the DSP response. It also adds
qda_fastrpc_flush_pending() to fail and wake pending invocations when
the RPMsg device is removed.

> diff --git a/drivers/accel/qda/qda_fastrpc.c b/drivers/accel/qda/qda_fastrpc.c
> index 2971f2f9461fc..3a1959e2c53ee 100644
> --- a/drivers/accel/qda/qda_fastrpc.c
> +++ b/drivers/accel/qda/qda_fastrpc.c
> @@ -10,6 +10,7 @@
>  #include "qda_drv.h"
>  #include "qda_fastrpc.h"
>  #include "qda_gem.h"
> +#include "qda_rpmsg.h"
>  
>  /*
>   * The caller owns the returned reference and must drop it with
> @@ -484,6 +485,72 @@ int qda_fastrpc_invoke_pack(struct qda_fastrpc_invoke_ctx *ctx, struct qda_msg *
>  	return 0;
>  }
>  
> +/**
> + * qda_fastrpc_flush_pending() - Fail every invocation still in flight
> + * @qdev: Device whose invocations should be failed
> + *
> + * The DSP has gone away and will never answer, so stand in for the response
> + * handler: wake each waiter with an error and drop the reference the send
> + * path took for that handler. Each waiter then tears its invocation down,
> + * releasing the buffers it holds.
> + *
> + * Must run after drm_dev_unplug() has drained the response handler, so this
> + * is the only path completing an invocation.
> + */
> +void qda_fastrpc_flush_pending(struct qda_dev *qdev)
> +{
> +	struct qda_fastrpc_invoke_ctx *ctx;
> +	unsigned long idx;
> +
> +	xa_for_each(&qdev->ctx_xa, idx, ctx) {
> +		ctx->retval = -EPIPE;
> +		complete(&ctx->work);
> +		kref_put(&ctx->refcount, qda_fastrpc_context_free);
> +	}
> +}

[Severity: Critical]
Can qda_fastrpc_flush_pending() drop a reference that was never taken (or was
already queued to be dropped), causing a reference count underflow and
use-after-free of struct qda_fastrpc_invoke_ctx?

In qda_fastrpc_invoke(), qda_fastrpc_context_alloc() initializes
ctx->refcount to 1 and immediately inserts ctx into qdev->ctx_xa before
qda_fastrpc_prepare_args(), qda_fastrpc_invoke_pack(), and
qda_rpmsg_send_msg() run:

drivers/accel/qda/qda_fastrpc.c:qda_fastrpc_context_alloc() {
	...
	kref_init(&ctx->refcount);
	...
	ret = xa_alloc(&qdev->ctx_xa, &id, ctx, XA_LIMIT(1, QDA_FASTRPC_CTXID_MAX),
		       GFP_KERNEL);
}

Because drm_ioctl() only checks drm_dev_is_unplugged() at entry and does not
hold a drm_dev_enter() read lock across qda_ioctl_invoke(), drm_dev_unplug()
in qda_rpmsg_remove() does not wait for in-flight qda_fastrpc_invoke() calls.
The second reference for the response handler is only acquired inside
qda_rpmsg_send_msg() if drm_dev_enter() succeeds, qdev->rpdev is non-NULL,
and rpmsg_send() succeeds:

drivers/accel/qda/qda_rpmsg.c:qda_rpmsg_send_msg() {
	if (!drm_dev_enter(&qdev->drm_dev, &idx))
		return -ENODEV;

	if (!qdev->rpdev) {
		ret = -ENODEV;
		goto out_exit;
	}

	kref_get(&ctx->refcount);

	ret = rpmsg_send(qdev->rpdev->ept, &msg->fastrpc, sizeof(msg->fastrpc));
	if (ret) {
		drm_err_ratelimited(&qdev->drm_dev, "Failed to send rpmsg: %d\n", ret);
		kref_put(&ctx->refcount, qda_fastrpc_context_free);
	}
}

Similarly, if qda_rpmsg_cb() handled a response just before unplug, it
schedules ctx->put_work to drop the response reference asynchronously while
ctx remains in qdev->ctx_xa until qda_fastrpc_context_free() runs:

drivers/accel/qda/qda_rpmsg.c:qda_rpmsg_cb() {
	...
	ctx->retval = resp_msg->retval;
	complete(&ctx->work);
	schedule_work(&ctx->put_work);
}

When qda_fastrpc_flush_pending() iterates qdev->ctx_xa unlocked and
unconditionally calls kref_put(&ctx->refcount, qda_fastrpc_context_free), it
drops the caller's or worker's reference in both windows, freeing ctx while
qda_fastrpc_invoke() or qda_fastrpc_context_put_wq() is still using it. In
addition, because xa_for_each() runs without locking or pinning ctx, a
concurrent caller or put_work worker can free ctx right after xa_for_each()
loads the pointer, causing qda_fastrpc_flush_pending() to write ctx->retval
and call complete() and kref_put() on freed memory.

> +
> +/**
> + * qda_fastrpc_invoke_common() - Shared tail of a FastRPC invocation
> + * @qdev: Device to invoke on
> + * @ctx: Invocation whose arguments are already prepared
> + *
> + * Packs the invocation into a message, sends it, waits for the DSP, and acts
> + * on the answer. Common to the accel UAPI and the legacy shim, once each has
> + * prepared ctx->args and ctx->gem_objs by its own means.
> + *
> + * Return: 0 on success, negative error code on failure
> + */
> +int qda_fastrpc_invoke_common(struct qda_dev *qdev, struct qda_fastrpc_invoke_ctx *ctx)
> +{
> +	struct qda_msg msg = {};
> +	int err;
> +
> +	err = qda_fastrpc_invoke_pack(ctx, &msg);
> +	if (err)
> +		return err;

[Severity: Critical]
When qda_fastrpc_invoke_common() calls qda_fastrpc_invoke_pack() ->
qda_fastrpc_get_args() -> process_dma_handle() for handle arguments
(i >= ctx->nbufs), can user space forge DSP DMA handle descriptors or trigger
out-of-bounds DSP memory accesses?

In process_dma_handle():

drivers/accel/qda/qda_fastrpc.c:process_dma_handle() {
	if (!ctx->args[i].handle) {
		rpra[i].buf.addr = ctx->args[i].offset;
		rpra[i].buf.len = ctx->args[i].length;
		return 0;
	}
	...
	/* The DSP-facing offset and length fields are 32-bit by wire format */
	rpra[i].dma.handle = ctx->args[i].handle;
	rpra[i].dma.len = (u32)ctx->args[i].length;
	rpra[i].dma.offset = (u32)ctx->args[i].offset;
}

First, rpra[i] is a union qda_remote_arg, which the DSP interprets as struct
qda_remote_dmahandle dma (s32 handle; u32 offset; u32 len;) for handle
arguments (i >= ctx->nbufs). When ctx->args[i].handle is 0, writing the
64-bit ctx->args[i].offset into rpra[i].buf.addr aliases rpra[i].dma.handle
with the lower 32 bits and rpra[i].dma.offset with the upper 32 bits,
allowing user space to inject an arbitrary non-zero DSP handle and offset
while bypassing get_gem_obj_from_handle() validation.

Second, when ctx->args[i].handle is non-zero, process_dma_handle() looks up
gem_obj via get_gem_obj_from_handle(), but unlike process_msg_buffer(), it
never verifies that ctx->args[i].offset and ctx->args[i].length fit within
to_qda_gem_obj(gem_obj)->size (or within u32 before truncating them to
rpra[i].dma.offset and rpra[i].dma.len).

> +
> +	err = qda_rpmsg_send_msg(qdev, &msg);
> +	if (err)
> +		return err;
> +
> +	err = qda_rpmsg_wait_for_rsp(ctx);
> +	if (err)
> +		return err;
> +
> +	/* Ensure the results written by the DSP are visible before reading */
> +	dma_rmb();
> +
> +	err = ctx->retval;
> +	if (err) {
> +		drm_dbg_driver(&qdev->drm_dev, "DSP returned status %d\n", ctx->retval);
> +		return err;
> +	}
> +
> +	qda_fastrpc_cleanup_handlelist(ctx);
> +
> +	return 0;
> +}

[Severity: Medium]
Does returning early when ctx->retval is non-zero in
qda_fastrpc_invoke_common() leak the GEM handles listed in the DSP's response
handle list?

When a remote DSP invocation fails with a non-zero status code, the DSP
framework can still populate the handle release list in the payload buffer
(as addressed in drivers/misc/fastrpc.c by commit 1c8093591d1e ("misc:
fastrpc: Clean buffers on remote invocation failures")). Returning before
qda_fastrpc_cleanup_handlelist(ctx) leaves those GEM handles open in
ctx->session->file.

[ ... ]

> diff --git a/drivers/accel/qda/qda_ioctl.c b/drivers/accel/qda/qda_ioctl.c
> index b11743ab791e7..e9f4151e344af 100644
> --- a/drivers/accel/qda/qda_ioctl.c
> +++ b/drivers/accel/qda/qda_ioctl.c
> @@ -6,6 +6,7 @@
>  #include <drm/drm_gem.h>
>  #include <drm/qda_accel.h>
>  #include "qda_drv.h"
> +#include "qda_fastrpc.h"
>  #include "qda_gem.h"
>  #include "qda_ioctl.h"
>  
> @@ -73,3 +74,38 @@ int qda_ioctl_gem_mmap_offset(struct drm_device *dev, void *data, struct drm_fil
>  
>  	return drm_gem_dumb_map_offset(file_priv, dev, args->handle, &args->offset);
>  }
> +
> +static int qda_fastrpc_invoke(void *data, struct qda_session *session)
> +{
> +	struct qda_dev *qdev = session->qda_dev;
> +	struct qda_fastrpc_invoke_ctx *ctx;
> +	int err;
> +
> +	ctx = qda_fastrpc_context_alloc(qdev);
> +	if (IS_ERR(ctx))
> +		return PTR_ERR(ctx);
> +
> +	err = qda_fastrpc_prepare_args(ctx, session, data);
> +	if (err)
> +		goto out_put;
> +
> +	err = qda_fastrpc_invoke_common(qdev, ctx);

[Severity: Critical]
Should qda_fastrpc_invoke() or qda_fastrpc_prepare_args_invoke() reject
user-space invocations targeting handle 1 (QDA_FASTRPC_INIT_HANDLE)?

In qda_fastrpc_prepare_args() -> qda_fastrpc_prepare_args_invoke():

drivers/accel/qda/qda_fastrpc.c:qda_fastrpc_prepare_args_invoke() {
	...
	ctx->handle = invoke_args->handle;
	ctx->sc = invoke_args->sc;
}

In the FastRPC protocol, handle 1 is reserved for kernel-issued process and
protection domain management calls. Unlike fastrpc_internal_invoke() in
drivers/misc/fastrpc.c, which rejects user-space calls with handle ==
FASTRPC_INIT_HANDLE with -EPERM (CVE-2019-2308), DRM_IOCTL_QDA_REMOTE_INVOKE
allows unprivileged user space to invoke handle 1 directly.

[ ... ]

> diff --git a/drivers/accel/qda/qda_rpmsg.c b/drivers/accel/qda/qda_rpmsg.c
> index 43b56ab3f0128..7e58df3eb2e17 100644
> --- a/drivers/accel/qda/qda_rpmsg.c
> +++ b/drivers/accel/qda/qda_rpmsg.c
> @@ -160,6 +160,7 @@ static void qda_rpmsg_remove(struct rpmsg_device *rpdev)
>  	 */
>  	drm_dev_unplug(&qdev->drm_dev);
>  	qdev->rpdev = NULL;
> +	qda_fastrpc_flush_pending(qdev);
>  	qda_cb_unpopulate(qdev);
>  	qda_deinit_device(qdev);
>  }

[Severity: Critical]
Does qda_rpmsg_remove() tear down qdev->cb_devs, qdev->iommu_mgr, and
qdev->ctx_xa while woken invocations and pending ctx->put_work items are
still using them?

When an invocation is waiting in qda_rpmsg_wait_for_rsp(), ctx->refcount is 2
(one held by the caller in qda_fastrpc_invoke() and one taken by
qda_rpmsg_send_msg()). Calling qda_fastrpc_flush_pending() wakes the waiter
and drops one reference, reducing ctx->refcount to 1, while ctx remains in
qdev->ctx_xa until the woken caller thread (or a pending ctx->put_work
worker) runs qda_fastrpc_context_free():

drivers/accel/qda/qda_fastrpc.c:qda_fastrpc_context_free() {
	...
	xa_erase(&ctx->qdev->ctx_xa, FIELD_GET(QDA_FASTRPC_CTXID_MASK, ctx->ctxid));
	...
	if (ctx->msg_gem_obj)
		qda_session_msg_buf_put(ctx->session, &ctx->msg_gem_obj->base);

	if (ctx->session)
		qda_session_put(ctx->session);
	...
}

Because qda_rpmsg_remove() immediately proceeds to qda_cb_unpopulate(qdev)
and qda_deinit_device(qdev) without waiting for in-flight invocations or
flushing pending ctx->put_work items:

1. qda_cb_unpopulate() destroys and frees the child context-bank platform
devices via of_platform_device_destroy(&entry->pdev->dev, NULL) while GEM
objects held by active invocations and sessions still reference
&entry->pdev->dev via gem_obj->iommu_dev->dev, leading to a use-after-free in
qda_dma_free() when dma_free_coherent() runs.

2. qda_deinit_device() calls qda_memory_manager_exit(qdev->iommu_mgr) and
xa_destroy(&qdev->ctx_xa) while contexts are still present in qdev->ctx_xa;
when woken threads or ctx->put_work workers subsequently execute
qda_fastrpc_context_free(), they call xa_erase() on the destroyed XArray.

3. If an interrupted caller already returned and closed its DRM file while
ctx->put_work is still queued (since neither qda_session nor
qda_fastrpc_invoke_ctx holds a drm_dev_get() reference on qdev->drm_dev),
finishing qda_rpmsg_remove() allows devm to release qdev before
qda_fastrpc_context_put_wq() runs qda_fastrpc_context_free() and dereferences
ctx->qdev.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-v4-v4-0-88f2f24a5f0b@qti.qualcomm.com?part=13