Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [Critical] accel/qda: DMA UAF of init_mem_gem_obj on signal in qda_fastrpc_invoke() - [Critical] accel/qda: race in qda_session_set_init_mem() frees active process memory - [Critical] accel/qda: unprivileged userspace can invoke QDA_FASTRPC_INIT_HANDLE - [High] accel/qda: deadlock in qda_postclose() holding drm_dev_enter() across RPC - [High] accel/qda: default session->pd of QDA_ROOT_PD allows unattached root PD invoke --
commit 981b12f498982e2e454ee8c3d214b24f64472c31 Author: Ekansh Gupta <[email protected]> accel/qda: Add DSP process creation and release This commit adds FastRPC requests and the DRM_IOCTL_QDA_REMOTE_SESSION_CREATE ioctl to create a DSP process on a session and release it when the session closes. It allocates per-session memory for the DSP process image and tracks the session's protection domain for subsequent requests. > diff --git a/drivers/accel/qda/qda_drv.c b/drivers/accel/qda/qda_drv.c > index 7d44342d3f67b..983c6a3dcfb45 100644 > --- a/drivers/accel/qda/qda_drv.c > +++ b/drivers/accel/qda/qda_drv.c [ ... ] > @@ -101,7 +105,16 @@ EXPORT_SYMBOL_FOR_MODULES(qda_session_put, > "qda_fastrpc_shim"); > > static void qda_postclose(struct drm_device *dev, struct drm_file *file) > { > - qda_session_put(file->driver_priv); > + struct qda_session *session = file->driver_priv; > + int idx; > + > + /* Only a session that created a process has one to release */ > + if (READ_ONCE(session->init_mem_gem_obj) && drm_dev_enter(dev, &idx)) { > + qda_release_dsp_process(session->qda_dev, session); > + drm_dev_exit(idx); > + } [Severity: High] Can holding drm_dev_enter() across qda_release_dsp_process() in qda_postclose() deadlock with qda_rpmsg_remove() if the DSP crashes or the RPMsg transport disconnects while the release request is in flight? qda_release_dsp_process() calls qda_fastrpc_invoke() -> qda_fastrpc_invoke_common(), which sends the QDA_FASTRPC_RMID_INIT_RELEASE message (inside qda_rpmsg_send_msg(), which already uses its own short drm_dev_enter()/drm_dev_exit() section around rpmsg_send()) and then blocks in qda_rpmsg_wait_for_rsp() via wait_for_completion_interruptible(&ctx->work). If the RPMsg device is removed before the DSP replies, qda_rpmsg_remove() runs: drivers/accel/qda/qda_rpmsg.c:qda_rpmsg_remove() { drm_dev_unplug(&qdev->drm_dev); qdev->rpdev = NULL; qda_fastrpc_flush_pending(qdev); ... } drm_dev_unplug() calls synchronize_srcu(&drm_unplug_srcu) and waits for qda_postclose() to call drm_dev_exit(idx), while qda_postclose() is blocked in qda_rpmsg_wait_for_rsp() waiting for qda_fastrpc_flush_pending() to complete ctx->work (and when qda_postclose() runs during process exit after exit_signals() or from the delayed_fput workqueue, signals cannot interrupt the wait). Should qda_postclose() check !drm_dev_is_unplugged(dev) instead of holding drm_dev_enter() across the blocking RPC wait? [ ... ] > diff --git a/drivers/accel/qda/qda_fastrpc.c b/drivers/accel/qda/qda_fastrpc.c > index 693dd6f035d2a..4672405ea4b79 100644 > --- a/drivers/accel/qda/qda_fastrpc.c > +++ b/drivers/accel/qda/qda_fastrpc.c [ ... ] > @@ -511,9 +572,14 @@ int qda_fastrpc_invoke_pack(struct > qda_fastrpc_invoke_ctx *ctx, struct qda_msg * > /* Ensure the payload is visible to the DSP before the doorbell */ > dma_wmb(); > > - msg->fastrpc.remote_session_id = ctx->session->remote_session_id; > + /* The session has no process yet to identify while one is being > created */ > + if (ctx->handle == QDA_FASTRPC_INIT_HANDLE) > + msg->fastrpc.remote_session_id = 0; > + else > + msg->fastrpc.remote_session_id = > ctx->session->remote_session_id; > + > msg->fastrpc.tid = current->pid; > - msg->fastrpc.ctx = ctx->ctxid | QDA_USER_PD; > + msg->fastrpc.ctx = ctx->ctxid | ctx->pd; > msg->fastrpc.handle = ctx->handle; > msg->fastrpc.sc = ctx->sc; > msg->fastrpc.addr = msg->phys; [Severity: Critical] What prevents unprivileged user space from invoking QDA_FASTRPC_INIT_HANDLE directly via DRM_IOCTL_QDA_REMOTE_INVOKE or the legacy invoke path? In qda_fastrpc_invoke_pack(), any invocation with ctx->handle == QDA_FASTRPC_INIT_HANDLE is formatted with msg->fastrpc.remote_session_id = 0. However, neither qda_fastrpc_prepare_args_invoke() nor qda_fastrpc_invoke_legacy() rejects user-supplied requests where handle == QDA_FASTRPC_INIT_HANDLE: drivers/accel/qda/qda_fastrpc.c:qda_fastrpc_prepare_args_invoke() { ctx->handle = invoke_args->handle; ctx->sc = invoke_args->sc; ... } drivers/accel/qda/qda_fastrpc.c:qda_fastrpc_invoke_legacy() { ... ctx->type = QDA_FASTRPC_RMID_INVOKE_DYNAMIC; ctx->legacy = true; ctx->pd = READ_ONCE(session->pd); ctx->handle = handle; ctx->sc = sc; ... } Doesn't this allow an unprivileged caller to forge kernel initialisation requests to QDA_FASTRPC_INIT_HANDLE with remote_session_id = 0—for example, invoking QDA_FASTRPC_RMID_INIT_CREATE / QDA_FASTRPC_RMID_INIT_CREATE_ATTR for a signed process without CAP_SYS_ADMIN, supplying arbitrary physical page descriptors in argument 3, or sending QDA_FASTRPC_RMID_INIT_RELEASE with another session's remote_session_id? [ ... ] > @@ -740,8 +913,23 @@ int qda_fastrpc_prepare_args(struct > qda_fastrpc_invoke_ctx *ctx, > */ > qda_session_get(session); > ctx->session = session; > - > - err = qda_fastrpc_prepare_args_invoke(ctx, argp); > + ctx->pd = READ_ONCE(session->pd); > + > + switch (ctx->type) { > + case QDA_FASTRPC_RMID_INIT_RELEASE: > + err = qda_fastrpc_prepare_args_release_process(ctx); > + break; > + case QDA_FASTRPC_RMID_INIT_CREATE: > + /* The new process is in the user PD, and so is the request > creating it */ > + ctx->pd = QDA_USER_PD; > + err = qda_fastrpc_prepare_args_init_create(ctx, argp); > + break; > + case QDA_FASTRPC_RMID_INVOKE_DYNAMIC: > + err = qda_fastrpc_prepare_args_invoke(ctx, argp); > + break; > + default: > + return -EINVAL; > + } [Severity: High] Because qda_session_alloc() zero-initializes struct qda_session, session->pd starts as 0 (QDA_ROOT_PD). If user space calls DRM_IOCTL_QDA_REMOTE_INVOKE (via qda_ioctl_invoke() -> qda_fastrpc_prepare_args()) or FASTRPC_IOCTL_INVOKE (via qda_fastrpc_invoke_legacy()) before creating a DSP process on the session, ctx->pd is set to READ_ONCE(session->pd) == QDA_ROOT_PD, and qda_fastrpc_invoke_pack() packs QDA_ROOT_PD into msg->fastrpc.ctx. Can an unprivileged caller use this to direct invocations at QDA_ROOT_PD before any protection domain has been created or attached? [ ... ] > diff --git a/drivers/accel/qda/qda_ioctl.c b/drivers/accel/qda/qda_ioctl.c > index e9f4151e344af..f51ad0bc9e9cc 100644 > --- a/drivers/accel/qda/qda_ioctl.c > +++ b/drivers/accel/qda/qda_ioctl.c > @@ -75,7 +75,22 @@ 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) > +/* > + * A process created again replaces the first, so the memory held for the > + * previous one is dropped. Swapped atomically, as ioctls on one file can run > + * concurrently. > + */ > +static void qda_session_set_init_mem(struct qda_session *session, struct > qda_gem_obj *mem) > +{ > + struct qda_gem_obj *old; > + > + drm_gem_object_get(&mem->base); > + old = xchg(&session->init_mem_gem_obj, mem); > + if (old) > + drm_gem_object_put(&old->base); > +} [Severity: Critical] As reported by Sashiko on v1 [1], is an atomic xchg() in qda_session_set_init_mem() sufficient when two threads call DRM_IOCTL_QDA_REMOTE_SESSION_CREATE concurrently on the same file? In qda_fastrpc_invoke(), qda_fastrpc_invoke_common() and qda_session_set_init_mem() run without a per-session lock serializing process creation. If Thread A (with memA) and Thread B (with memB) issue QDA_FASTRPC_RMID_INIT_CREATE concurrently: 1. The DSP processes Thread A first and Thread B second, so the active DSP process is replaced and runs in memB. 2. On the CPU, Thread B runs qda_session_set_init_mem(session, memB) before Thread A runs qda_session_set_init_mem(session, memA). 3. Thread A swaps session->init_mem_gem_obj to memA and calls drm_gem_object_put(&memB->base). 4. When Thread B drops ctx->refcount at out_put, qda_fastrpc_context_free() drops the last reference to memB, unmapping and freeing memB while the DSP process is actively running in it. Should process creation on a session be serialized with a mutex (or rejected if a process already exists)? > + > +static int qda_fastrpc_invoke(u32 type, void *data, struct qda_session > *session) > { > struct qda_dev *qdev = session->qda_dev; > struct qda_fastrpc_invoke_ctx *ctx; > @@ -85,11 +100,26 @@ static int qda_fastrpc_invoke(void *data, struct > qda_session *session) > if (IS_ERR(ctx)) > return PTR_ERR(ctx); > > + ctx->type = type; > + > err = qda_fastrpc_prepare_args(ctx, session, data); > if (err) > goto out_put; > > err = qda_fastrpc_invoke_common(qdev, ctx); > + if (err) > + goto out_put; > + > + /* > + * The session only changes once the DSP has accepted the request, so a > + * failed create leaves the previous process, if any, as it was. The > + * memory is freed with the session, which the invocation keeps alive > + * until it is done, so a release needs no reference of its own. > + */ > + if (type == QDA_FASTRPC_RMID_INIT_CREATE) { > + qda_session_set_init_mem(session, ctx->init_mem_gem_obj); > + WRITE_ONCE(session->pd, QDA_USER_PD); > + } > > out_put: > kref_put(&ctx->refcount, qda_fastrpc_context_free); [Severity: Critical] What happens if a thread calling DRM_IOCTL_QDA_REMOTE_SESSION_CREATE is interrupted by a signal while qda_fastrpc_invoke_common() is waiting in qda_rpmsg_wait_for_rsp()? After qda_rpmsg_send_msg() has sent the QDA_FASTRPC_RMID_INIT_CREATE request to the DSP, a signal causes wait_for_completion_interruptible(&ctx->work) to return -ERESTARTSYS. Here in qda_fastrpc_invoke(), the error check jumps to out_put without calling qda_session_set_init_mem(session, ctx->init_mem_gem_obj). The DSP still processes the in-flight create request, loads the ELF image into ctx->init_mem_gem_obj, starts the process, and replies. When qda_rpmsg_cb() receives the reply and runs qda_fastrpc_context_put_wq() -> qda_fastrpc_context_free(): drivers/accel/qda/qda_fastrpc.c:qda_fastrpc_context_free() { ... if (ctx->init_mem_gem_obj) drm_gem_object_put(&ctx->init_mem_gem_obj->base); ... } Because qda_session_set_init_mem() never took a reference for the session, qda_fastrpc_context_free() drops the only reference to ctx->init_mem_gem_obj and frees the DMA buffer while the DSP process is running in it. In addition, session->init_mem_gem_obj remains NULL, so qda_postclose() later skips qda_release_dsp_process() and qda_session_release() releases session->assigned_iommu_dev while the orphaned DSP process is still using that context bank. Could ctx->init_mem_gem_obj be retained by the session whenever an INIT_CREATE request has been sent to the DSP (or transferred when the DSP replies with status 0)? [1] https://lore.kernel.org/r/[email protected] -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=15
