Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [High] accel/qda: union corruption on zero handle in process_dma_handle() - [High] accel/qda: missing bounds and overflow checks in process_dma_handle() - [High] accel/qda: dangling session->file pointer after qda_postclose() - [High] accel/qda: premature drm_dev_put() while qda_session remains alive - [Medium] accel/qda: unvalidated attr field in qda_fastrpc_prepare_args_invoke() --
commit 0212e9880ce32dca1d81b275fcea22de7515a72a Author: Ekansh Gupta <[email protected]> accel/qda: Add invocation argument validation and packing This commit adds FastRPC invocation argument validation, message payload packing, and per-session message buffer pooling for the QDA driver. It also converts struct qda_session to be reference-counted so in-flight invocations can outlive the open file and clean up DSP-released GEM handles. > diff --git a/drivers/accel/qda/qda_drv.c b/drivers/accel/qda/qda_drv.c > index bdfb23c3c0b1d..99bd37fca4384 100644 > --- a/drivers/accel/qda/qda_drv.c > +++ b/drivers/accel/qda/qda_drv.c [ ... ] > @@ -50,22 +61,47 @@ static int qda_open(struct drm_device *dev, struct > drm_file *file) [ ... ] > static void qda_postclose(struct drm_device *dev, struct drm_file *file) > { > - qda_session_free(file->driver_priv); > + qda_session_put(file->driver_priv); > } [Severity: High] When an invocation outlives the file it was issued on, does session->file become a dangling pointer to a freed struct drm_file after qda_postclose() runs? In qda_open(), session->file is set to the raw struct drm_file pointer, and qda_fastrpc_prepare_args() takes a reference on ctx->session so that the session can outlive file closure if the caller stops waiting for the DSP. However, when the DRM file is closed, drm_file_free() destroys file->object_idr via drm_gem_release(), calls qda_postclose(), and then frees struct drm_file with kfree(file): drm_file_free() drm_gem_release(dev, file) dev->driver->postclose(dev, file) -> qda_postclose() qda_session_put(file->driver_priv) kfree(file) Because qda_postclose() only drops the file owner's session reference without clearing session->file or synchronizing with in-flight invocations, any session kept alive by an invocation retains a non-NULL session->file pointer to freed memory. When the DSP response later triggers qda_fastrpc_cleanup_handlelist(): void qda_fastrpc_cleanup_handlelist(struct qda_fastrpc_invoke_ctx *ctx) { ... for (i = 0; i < QDA_FASTRPC_MAX_HANDLELIST; i++) { if (!handlelist[i]) break; if (ctx->session->file) drm_gem_handle_delete(ctx->session->file, (u32)handlelist[i]); } } checking ctx->session->file and passing it to drm_gem_handle_delete() will dereference the freed struct drm_file and its destroyed handle table. > diff --git a/drivers/accel/qda/qda_fastrpc.c b/drivers/accel/qda/qda_fastrpc.c > index ec37a38e92158..2971f2f9461fc 100644 > --- a/drivers/accel/qda/qda_fastrpc.c > +++ b/drivers/accel/qda/qda_fastrpc.c [ ... ] > @@ -70,3 +283,272 @@ struct qda_fastrpc_invoke_ctx > *qda_fastrpc_context_alloc(struct qda_dev *qdev) [ ... ] > +static int process_dma_handle(struct qda_fastrpc_invoke_ctx *ctx, int i, > + union qda_remote_arg *rpra, > + struct qda_phy_page *pages) > +{ > + struct drm_gem_object *gem_obj; > + int err; > + > + if (!ctx->args[i].handle) { > + rpra[i].buf.addr = ctx->args[i].offset; > + rpra[i].buf.len = ctx->args[i].length; > + return 0; > + } [Severity: High] What prevents user space from bypassing handle validation in process_dma_handle() by passing handle == 0 with a non-zero offset for a DMA-handle argument? In qda_fastrpc_get_args(), arguments at indices ctx->nbufs <= i < ctx->nscalars are DMA-handle arguments, which the DSP interprets as struct qda_remote_dmahandle (rpra[i].dma). Because union qda_remote_arg overlays struct qda_remote_buf with struct qda_remote_dmahandle: union qda_remote_arg { struct qda_remote_buf buf; struct qda_remote_dmahandle dma; }; writing the 64-bit ctx->args[i].offset into rpra[i].buf.addr when ctx->args[i].handle is 0 populates rpra[i].dma.handle with the low 32 bits of ctx->args[i].offset and rpra[i].dma.offset with the high 32 bits, while rpra[i].buf.len overwrites rpra[i].dma.len. In addition, qda_fastrpc_get_args() sets list[i].num = ctx->args[i].length ? 1 : 0 before calling process_dma_handle(), so a zero handle with a non-zero length leaves list[i].num set to 1 while pages[i] remains unpopulated. Checking !ctx->args[i].handle before ctx->gem_objs[i] also skips any pre-populated ctx->gem_objs[i] entry when handle is 0. > + > + if (ctx->gem_objs[i]) { > + gem_obj = ctx->gem_objs[i]; > + } else { > + err = get_gem_obj_from_handle(ctx, ctx->args[i].handle, > &gem_obj); > + if (err) > + return err; > + > + ctx->gem_objs[i] = gem_obj; > + } > + > + setup_pages_from_gem_obj(to_qda_gem_obj(gem_obj), &pages[i]); > + > + /* 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; > + > + return 0; > +} [Severity: High] Should process_dma_handle() validate ctx->args[i].offset and ctx->args[i].length against the GEM object's size and U32_MAX before casting them to u32? In process_msg_buffer(), the user-supplied offset and length are checked against qda_gem_obj->size so that a descriptor cannot reference memory past the end of its buffer in the session's context bank: if (ctx->args[i].offset > qda_gem_obj->size || len > qda_gem_obj->size - ctx->args[i].offset) { ... return -EINVAL; } In process_dma_handle(), however, ctx->args[i].length and ctx->args[i].offset are cast directly to u32 and written into rpra[i].dma.len and rpra[i].dma.offset without checking that the region lies within to_qda_gem_obj(gem_obj)->size or fits within 32 bits. Can user space use a DMA-handle argument to pass a truncated or out-of-bounds region to the DSP? [ ... ] > +static int qda_fastrpc_prepare_args_invoke(struct qda_fastrpc_invoke_ctx > *ctx, void *argp) > +{ > + struct drm_qda_invoke_args *invoke_args = argp; > + struct drm_qda_fastrpc_invoke_args *args; > + u32 nscalars; > + > + ctx->handle = invoke_args->handle; > + ctx->sc = invoke_args->sc; > + > + nscalars = QDA_REMOTE_SCALARS_LENGTH(ctx->sc); > + if (!nscalars) > + return 0; > + > + args = kzalloc_objs(*args, nscalars); > + if (!args) > + return -ENOMEM; > + > + if (copy_from_user(args, u64_to_user_ptr(invoke_args->args), > + nscalars * sizeof(*args))) { > + kfree(args); > + return -EFAULT; > + } > + > + ctx->args = args; > + > + return 0; > +} [Severity: Medium] Should qda_fastrpc_prepare_args_invoke() reject non-zero values in args[i].attr with -EINVAL? struct drm_qda_fastrpc_invoke_args defines __u32 attr in the UAPI header, with no valid attribute flags currently defined, and nothing in qda_fastrpc_prepare_args_invoke() or qda_fastrpc_get_args() checks that args[i].attr is zero. If user space passes uninitialized non-zero values in attr today, won't that prevent future kernel versions from introducing flag bits in attr without breaking existing user space? > diff --git a/drivers/accel/qda/qda_legacy_ops.c > b/drivers/accel/qda/qda_legacy_ops.c > index 3566a10c64880..9639ea84b2007 100644 > --- a/drivers/accel/qda/qda_legacy_ops.c > +++ b/drivers/accel/qda/qda_legacy_ops.c > @@ -33,7 +33,7 @@ static void legacy_session_destroy(struct qda_session > *session) > { > struct drm_device *drm_dev = &session->qda_dev->drm_dev; > > - qda_session_free(session); > + qda_session_put(session); > drm_dev_put(drm_dev); > } [Severity: High] Can dropping the drm_device reference before the last reference to struct qda_session is released cause a use-after-free of struct qda_dev? With this change, struct qda_session is reference-counted and can outlive file closure (both via legacy_session_destroy() and via drm_release() -> qda_postclose()) when an in-flight invocation or its deferred ctx->put_work still holds a reference on ctx->session. However, legacy_session_destroy() still calls drm_dev_put(drm_dev) immediately after qda_session_put(session), and struct qda_session does not hold its own drm_dev_get() reference until qda_session_release(). If the file is closed while an invocation is still in flight and the underlying rpmsg device is unbound before ctx->put_work completes, the final drm_device reference can be dropped and struct qda_dev freed before qda_fastrpc_context_free() and qda_session_release() run: qda_fastrpc_context_free() xa_erase(&ctx->qdev->ctx_xa, ...) qda_session_msg_buf_put(ctx->session, &ctx->msg_gem_obj->base) qda_session_put(ctx->session) -> qda_session_release() qda_session_msg_buf_pool_destroy(session) drm_gem_object_put(entry->gem_obj) At that point, accessing ctx->qdev->ctx_xa and freeing the session's GEM objects will dereference the already-freed struct qda_dev. Should the drm_device reference be held across the lifetime of struct qda_session and released in qda_session_release() instead? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=12
