Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [Critical] accel/qda: premature GEM object put in qda_fastrpc_context_free() on mmap - [High] accel/qda: missing offset bounds check in setup_mmap_pages() for mem_map - [High] accel/qda: uninitialized session PD and leaked mappings in qda_ioctl_mmap() - [Medium] accel/qda: drm_qda_mem_map.attrs is ignored and unused fields are unvalidated - [Medium] accel/qda: DSP mapping leak on signal interruption in qda_fastrpc_invoke() --
commit 395c018ba64267dfbd2bc4c753e6cc750574fd40 Author: Ekansh Gupta <[email protected]> accel/qda: Add remote memory mapping to the DSP address space This commit adds the DRM_IOCTL_QDA_REMOTE_MMAP ioctl to map a buffer handle into the remote processor's address space via QDA_FASTRPC_RMID_INIT_MMAP or QDA_FASTRPC_RMID_INIT_MEM_MAP and return the mapped address to user space. > diff --git a/drivers/accel/qda/qda_fastrpc.c b/drivers/accel/qda/qda_fastrpc.c > index 35ce60f3de2ec..744d7317cbe11 100644 > --- a/drivers/accel/qda/qda_fastrpc.c > +++ b/drivers/accel/qda/qda_fastrpc.c > @@ -3,6 +3,7 @@ > #include <linux/bitfield.h> > #include <linux/capability.h> > #include <linux/dma-mapping.h> > +#include <linux/limits.h> > #include <linux/mm.h> > #include <linux/sched.h> > #include <linux/slab.h> > @@ -251,6 +252,9 @@ void qda_fastrpc_context_free(struct kref *ref) > if (ctx->init_mem_gem_obj) > drm_gem_object_put(&ctx->init_mem_gem_obj->base); > > + if (ctx->map_obj) > + drm_gem_object_put(ctx->map_obj); [Severity: Critical] Could dropping ctx->map_obj here when the invocation context is freed cause a use-after-free while the mapping remains active on the remote processor? In setup_mmap_pages(), get_gem_obj_from_handle() stores the buffer reference in ctx->map_obj and passes qda_gem_obj->dma_addr to the remote processor for QDA_FASTRPC_RMID_INIT_MMAP or QDA_FASTRPC_RMID_INIT_MEM_MAP. However, as soon as the mapping call finishes in qda_fastrpc_invoke(): qda_ioctl_mmap() -> qda_fastrpc_invoke() -> kref_put(&ctx->refcount, qda_fastrpc_context_free) -> drm_gem_object_put(ctx->map_obj) the kernel drops its reference to ctx->map_obj before qda_ioctl_mmap() returns to user space, unlike ctx->init_mem_gem_obj which is transferred to the session via qda_session_set_init_mem(). If user space then closes the buffer handle with DRM_IOCTL_GEM_CLOSE while the remote mapping at vaddrout is still in place, qda_gem_free_object() will free the backing memory and release its address in session->assigned_iommu_dev. Because driver-allocated buffers (such as alloc_kernel_gem() buffers and ctx->msg_gem_obj) allocate from the same session->assigned_iommu_dev context bank, couldn't later ioctls reuse that address and allow remote code to access or corrupt in-flight control messages? Also, should margs->flags be validated in qda_fastrpc_prepare_args_map() before forwarding it to QDA_FASTRPC_RMID_INIT_MMAP? [ ... ] > @@ -890,6 +930,148 @@ static int qda_fastrpc_prepare_args_init_create(struct > qda_fastrpc_invoke_ctx *c > return err; > } > > +/* > + * Find the buffer a mapping request names and describe it to the DSP. The > + * buffer is held by the context until the request is done, so that closing > its > + * handle meanwhile cannot free memory the DSP is being told to map. The > + * region must fit in the buffer, as the DSP would otherwise map past its > end. > + */ > +static int setup_mmap_pages(struct qda_fastrpc_invoke_ctx *ctx, struct > drm_qda_mem_map *margs, > + struct qda_phy_page *pages) > +{ > + struct qda_gem_obj *qda_gem_obj; > + int err; > + > + if (!margs->handle) > + return -EINVAL; > + > + err = get_gem_obj_from_handle(ctx, margs->handle, &ctx->map_obj); > + if (err) > + return err; > + > + qda_gem_obj = to_qda_gem_obj(ctx->map_obj); > + if (margs->size > qda_gem_obj->size) > + return -EINVAL; > + > + pages->addr = qda_gem_obj->dma_addr; > + pages->size = margs->size ? margs->size : qda_gem_obj->size; [Severity: High] Does setup_mmap_pages() need to check margs->offset against qda_gem_obj->size as well? In qda_fastrpc_prepare_args_mem_map(), margs->offset is only checked against INT_MAX before being written to req->offset: if (margs->offset > INT_MAX) return -EINVAL; ... req->offset = margs->offset; And setup_mmap_pages() only verifies that margs->size <= qda_gem_obj->size, unlike process_msg_buffer() which checks both the offset and the remaining length: if (ctx->args[i].offset > qda_gem_obj->size || len > qda_gem_obj->size - ctx->args[i].offset) { When DRM_IOCTL_QDA_REMOTE_MMAP is called with QDA_REQUEST_MEM_MAP and a non-zero margs->offset (for example with margs->size == 0, which sets pages->size to qda_gem_obj->size, or with margs->offset + pages->size exceeding qda_gem_obj->size), won't this allow mapping past the end of qda_gem_obj? > + > + return 0; > +} [ ... ] > +static int qda_fastrpc_prepare_args_mem_map(struct qda_fastrpc_invoke_ctx > *ctx, void *argp) > +{ > + struct drm_qda_mem_map *margs = argp; > + struct qda_fastrpc_mem_map_req_msg *req; > + struct qda_gem_obj *kernel_gem; > + u8 *gv; > + int err; > + > + const size_t off_req = 0; > + const size_t off_pages = off_req + sizeof(*req); > + const size_t off_rsp = off_pages + sizeof(struct qda_phy_page); > + const size_t total_size = off_rsp + sizeof(struct > qda_fastrpc_map_rsp_msg); > + > + /* The wire field is signed */ > + if (margs->offset > INT_MAX) > + return -EINVAL; > + > + ctx->sc = QDA_FASTRPC_SCALARS(QDA_FASTRPC_RMID_INIT_MEM_MAP, 3, 1); > + ctx->handle = QDA_FASTRPC_INIT_HANDLE; > + > + err = alloc_kernel_args(ctx, 4); > + if (err) > + return err; > + > + kernel_gem = alloc_kernel_gem(ctx, total_size); > + if (IS_ERR(kernel_gem)) > + return PTR_ERR(kernel_gem); > + gv = kernel_gem->virt; > + > + req = (struct qda_fastrpc_mem_map_req_msg *)(gv + off_req); > + req->remote_session_id = ctx->session->remote_session_id; > + req->handle = margs->dsp_handle; > + req->offset = margs->offset; > + req->flags = margs->flags; > + req->vaddrin = margs->vaddrin; > + req->num = sizeof(struct qda_phy_page); > + req->data_len = 0; [Severity: Medium] Should margs->attrs be handled or rejected here, and should unused fields in struct drm_qda_mem_map be checked for zero in qda_fastrpc_prepare_args_map()? The comment on struct drm_qda_mem_map in include/uapi/drm/qda_accel.h states: * Makes a buffer visible to the DSP of this file ahead of the calls that use * it. QDA_REQUEST_MMAP maps a whole buffer and uses @handle, @flags, * @vaddrin, @size and @vaddrout. QDA_REQUEST_MEM_MAP maps a region of it with * caller-chosen attributes and uses every field. However, qda_fastrpc_prepare_args_mem_map() never reads or validates margs->attrs, and qda_fastrpc_prepare_args_map() does not reject non-zero values in margs->attrs, margs->offset, or margs->dsp_handle when request is QDA_REQUEST_MMAP. [ ... ] > @@ -953,6 +1135,12 @@ int qda_fastrpc_prepare_args(struct > qda_fastrpc_invoke_ctx *ctx, > ctx->pd = QDA_USER_PD; > err = qda_fastrpc_prepare_args_init_create(ctx, argp); > break; > + case QDA_FASTRPC_RMID_INIT_MMAP: > + err = qda_fastrpc_prepare_args_map(ctx, argp); > + break; > + case QDA_FASTRPC_RMID_INIT_MEM_MAP: > + err = qda_fastrpc_prepare_args_mem_map(ctx, argp); > + break; [Severity: High] What happens if user space calls DRM_IOCTL_QDA_REMOTE_MMAP before creating a session process, or closes the file while mappings are still active? When a file is opened in qda_open(), qda_session_alloc() zero-initializes struct qda_session, leaving session->pd as 0 (QDA_ROOT_PD) and session->init_mem_gem_obj as NULL until DRM_IOCTL_QDA_REMOTE_SESSION_CREATE completes. In qda_fastrpc_prepare_args(), ctx->pd is initialized from READ_ONCE(session->pd), so calling qda_ioctl_mmap() on a freshly opened file will send QDA_FASTRPC_RMID_INIT_MMAP or QDA_FASTRPC_RMID_INIT_MEM_MAP to QDA_ROOT_PD without an initialized session or a capability check. In addition, when the DRM file is closed, qda_postclose() only calls qda_release_dsp_process() when session->init_mem_gem_obj is non-NULL: 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); } and nothing tracks or unmaps active DRM_IOCTL_QDA_REMOTE_MMAP mappings before drm_gem_release() frees the GEM objects and qda_session_release() releases session->assigned_iommu_dev. Doesn't this leak the remote mappings on close? > diff --git a/drivers/accel/qda/qda_ioctl.c b/drivers/accel/qda/qda_ioctl.c > index 99b8a2517726b..cd3cea72f50e8 100644 > --- a/drivers/accel/qda/qda_ioctl.c > +++ b/drivers/accel/qda/qda_ioctl.c > @@ -110,6 +110,8 @@ static int qda_fastrpc_invoke(u32 type, void *data, > struct qda_session *session) > if (err) > goto out_put; > > + qda_fastrpc_return_result(ctx, data); [Severity: Medium] Can a signal interrupting qda_fastrpc_invoke_common() leak the remote mapping in the DSP address space? After qda_rpmsg_send_msg() sends a QDA_FASTRPC_RMID_INIT_MMAP or QDA_FASTRPC_RMID_INIT_MEM_MAP request, qda_rpmsg_wait_for_rsp() waits in wait_for_completion_interruptible(&ctx->work). If a signal arrives before the reply, qda_fastrpc_invoke_common() returns -ERESTARTSYS and qda_fastrpc_invoke() jumps to out_put, skipping qda_fastrpc_return_result(). When the DSP later finishes the mapping and writes rsp->vaddrout into ctx->gem_objs[idx], qda_rpmsg_cb() drops the last reference on ctx and frees the buffer without returning vaddrout to user space or unmapping the region on the DSP. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=17
