Re: [PATCH v4 11/25] accel/qda: Add RPMsg send and response dispatch

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] accel/qda: use-after-free in qda_rpmsg_cb() due to unlocked xa_load()
- [High] accel/qda: duplicate response refcount underflow in qda_rpmsg_cb()
- [High] accel/qda: unflushed put_work and in-flight contexts in qda_rpmsg_remove()
- [High] accel/qda: missing PD exit sentinel filtering in qda_rpmsg_cb()
--

commit 66df96a5b81c60ce1864c2d544af7d04014ec8e1
Author: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>

accel/qda: Add RPMsg send and response dispatch

This commit adds RPMsg message transmission and response callback handling
for FastRPC invocations, along with the wire message definitions and workqueue
reference cleanup.

> diff --git a/drivers/accel/qda/qda_rpmsg.c b/drivers/accel/qda/qda_rpmsg.c
> index 14034e250555d..43b56ab3f0128 100644
> --- a/drivers/accel/qda/qda_rpmsg.c
> +++ b/drivers/accel/qda/qda_rpmsg.c
[ ... ]
> +static int qda_rpmsg_cb(struct rpmsg_device *rpdev, void *data, int len,
> +			void *priv, u32 src)
> +{
> +	struct qda_dev *qdev = dev_get_drvdata(&rpdev->dev);
> +	struct qda_invoke_rsp *resp_msg = data;
> +	struct qda_fastrpc_invoke_ctx *ctx;
> +	int ret = 0, idx;
> +
> +	if (!qdev)
> +		return -ENODEV;
> +
> +	if (!drm_dev_enter(&qdev->drm_dev, &idx))
> +		return -ENODEV;
> +
> +	if (len < sizeof(*resp_msg)) {
> +		ret = -EINVAL;
> +		goto out_exit;
> +	}
> +
> +	ctx = xa_load(&qdev->ctx_xa, FIELD_GET(QDA_FASTRPC_CTXID_MASK, resp_msg->ctx));
> +	if (!ctx) {
> +		drm_dbg_driver(&qdev->drm_dev, "No context matches response 0x%llx\n",
> +			       resp_msg->ctx);
> +		ret = -ENOENT;
> +		goto out_exit;
> +	}

[Severity: High]
When a protection domain exits on the DSP, the firmware sends a PD exit
notification carrying the sentinel value 0xABCDABCD in resp_msg->ctx (which
drivers/misc/fastrpc.c filters out via FASTRPC_DSP_PD_NOTIFY_CTX in
fastrpc_rpmsg_callback()).

Under the QDA context ID layout, FIELD_GET(QDA_FASTRPC_CTXID_MASK, 0xABCDABCD)
evaluates to 0xCD (205) and FIELD_GET(QDA_FASTRPC_CTXID_SEQ_MASK, 0xABCDABCD)
evaluates to 0xABCD. If a PD exit notification arrives while context slot 0xCD
is occupied by an invocation at sequence number 0xABCD, won't qda_rpmsg_cb()
mistakenly match it, overwrite ctx->retval, wake the caller early via
complete(&ctx->work) while the DSP is still accessing its DMA buffers, and
schedule ctx->put_work?

Additionally, when slot 0xCD is empty, returning -ENOENT here instead of 0
causes SMD-backed RPMsg channels (qcom_smd_channel_recv_single() in
drivers/rpmsg/qcom_smd.c) to stop advancing the RX FIFO tail. Should
qda_rpmsg_cb() filter out 0xABCDABCD early and return 0 when dropping
unmatched responses?

> +
> +	/*
> +	 * Context IDs are only 8 bits wide and are recycled once a context is
> +	 * freed, so a late or duplicate response may name an ID that now belongs
> +	 * to a newer context. The sequence number tells the two apart.
> +	 */
> +	if (FIELD_GET(QDA_FASTRPC_CTXID_SEQ_MASK, resp_msg->ctx) !=
> +	    FIELD_GET(QDA_FASTRPC_CTXID_SEQ_MASK, ctx->ctxid)) {
> +		drm_dbg_driver(&qdev->drm_dev, "Dropping stale response 0x%llx\n",
> +			       resp_msg->ctx);
> +		goto out_exit;
> +	}

[Severity: High]
Can this unlocked xa_load() race with context teardown and cause a
use-after-free in qda_rpmsg_cb()?

When the final reference on ctx drops to zero on another CPU (for example,
when qda_rpmsg_send_msg() fails and drops the callback reference followed by
the caller dropping its reference, or when an invocation finishes),
qda_fastrpc_context_free() removes ctx from qdev->ctx_xa and immediately frees
it without an RCU grace period:

drivers/accel/qda/qda_fastrpc.c:qda_fastrpc_context_free() {
	struct qda_fastrpc_invoke_ctx *ctx =
		container_of(ref, struct qda_fastrpc_invoke_ctx, refcount);

	xa_erase(&ctx->qdev->ctx_xa, FIELD_GET(QDA_FASTRPC_CTXID_MASK, ctx->ctxid));
	kfree(ctx);
}

Because xa_load(&qdev->ctx_xa, ...) only holds rcu_read_lock() internally
during the lookup and returns the raw ctx pointer without holding
qdev->ctx_xa.xa_lock or pinning ctx->refcount via kref_get_unless_zero(), if
a late or duplicate response arrives in qda_rpmsg_cb() just before xa_erase()
and kfree(ctx), qda_rpmsg_cb() will dereference freed memory at ctx->ctxid,
write ctx->retval, and call complete(&ctx->work) and
schedule_work(&ctx->put_work) on the freed object.

Similarly, in qda_fastrpc_context_alloc(), xa_alloc() publishes ctx into
qdev->ctx_xa before ctx->ctxid is initialized:

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

	ctx->ctxid = FIELD_PREP(QDA_FASTRPC_CTXID_MASK, id) |
		     FIELD_PREP(QDA_FASTRPC_CTXID_SEQ_MASK,
				(u64)atomic64_inc_return(&qdev->ctx_seq));
}

which briefly exposes ctx with ctx->ctxid == 0 to concurrent lookups in
qda_rpmsg_cb().

> +
> +	ctx->retval = resp_msg->retval;
> +	complete(&ctx->work);
> +
> +	/*
> +	 * Release the reference taken by qda_rpmsg_send_msg(). This callback
> +	 * runs in atomic context, and if it drops the last reference the
> +	 * context's DMA buffers are freed, which must not happen here.
> +	 */
> +	schedule_work(&ctx->put_work);

[Severity: High]
What prevents a duplicate or unsolicited DSP response from scheduling
ctx->put_work without a corresponding callback reference and underflowing
ctx->refcount?

In qda_fastrpc_context_alloc(), ctx is inserted into qdev->ctx_xa with
refcount = 1 (owned by the caller), and qda_rpmsg_send_msg() takes a single
additional reference via kref_get(&ctx->refcount) for the response handler.
When a valid response arrives in qda_rpmsg_cb(), ctx is neither removed from
qdev->ctx_xa nor atomically marked as already answered; it stays in
qdev->ctx_xa until qda_fastrpc_context_free() runs at refcount == 0.

Once the first put_work begins executing in qda_fastrpc_context_put_wq() and
drops refcount from 2 to 1 while the caller still holds its reference, a
duplicate response from the DSP carrying the same ctx value (matching both the
8-bit ID and the sequence number) will match ctx in qda_rpmsg_cb() a second
time and call schedule_work(&ctx->put_work) again. That second execution of
qda_fastrpc_context_put_wq() drops the caller's reference, freeing ctx while
the caller is still using it and triggering a refcount underflow when the
caller drops its own reference.

Likewise, because ctx is in qdev->ctx_xa before qda_rpmsg_send_msg() is called
(and remains in qdev->ctx_xa after rpmsg_send() fails and drops the callback
reference), an unsolicited response matching ctx->ctxid before send or after a
failed send will also schedule ctx->put_work without a matching callback
reference.

> +
> +out_exit:
> +	drm_dev_exit(idx);
> +
> +	return ret;
> +}
> +
>  static void qda_rpmsg_remove(struct rpmsg_device *rpdev)

[Severity: High]
If the RPMsg device is removed while ctx->put_work is queued or while
invocations are in flight, can qda_fastrpc_context_put_wq() access
qdev->ctx_xa after qda_deinit_device() has destroyed it?

In qda_rpmsg_cb(), schedule_work(&ctx->put_work) queues
qda_fastrpc_context_put_wq() on system_percpu_wq and immediately exits the
drm_dev_enter() critical section via drm_dev_exit(idx). When
qda_rpmsg_remove() runs:

drivers/accel/qda/qda_rpmsg.c:qda_rpmsg_remove() {
	...
	drm_dev_unplug(&qdev->drm_dev);
	qdev->rpdev = NULL;
	qda_cb_unpopulate(qdev);
	qda_deinit_device(qdev);
}

drm_dev_unplug() only waits for active drm_dev_enter() sections; it does not
flush or wait for pending ctx->put_work items before qda_deinit_device(qdev)
destroys qdev->ctx_xa via xa_destroy(&qdev->ctx_xa) (and if all DRM files are
closed, devm frees qdev when qda_rpmsg_remove() returns). When the queued
qda_fastrpc_context_put_wq() worker subsequently runs,
qda_fastrpc_context_free() calls xa_erase(&ctx->qdev->ctx_xa, ...) on the
destroyed XArray and freed qdev.

In addition, if the RPMsg device is removed in this commit while invocations
sent via qda_rpmsg_send_msg() are waiting for a response, qda_rpmsg_remove()
does not complete ctx->work to wake waiters in qda_rpmsg_wait_for_rsp() or
drop the callback references on entries remaining in qdev->ctx_xa before
xa_destroy(). Although a later commit in the series, commit 9e89a77ec2ad
("accel/qda: Add the invocation ioctl and teardown on unplug"), adds
qda_fastrpc_flush_pending(qdev) to qda_rpmsg_remove() to wake and put
remaining entries in ctx_xa, it still does not flush pending ctx->put_work
items that were already scheduled by qda_rpmsg_cb().

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