Re: [PATCH v4 17/25] accel/qda: Add remote memory mapping to the DSP address space

[sashiko-bot@kernel.org]

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 <ekangupt@qti.qualcomm.com>

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/20261102-v4-v4-0-88f2f24a5f0b@qti.qualcomm.com?part=17