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 <[email protected]> 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/[email protected]?part=11
