Re: [PATCH v4 12/25] accel/qda: Add invocation argument validation and packing

[sashiko-bot@kernel.org]

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 <ekansh.gupta@oss.qualcomm.com>

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