Re: [PATCH v4 16/25] accel/qda: Add DSP process creation and release to the legacy shim

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] accel/qda: TOCTOU race and UAF between legacy_release() and qda_rpmsg_remove()
- [High] accel/qda: missing QDA_FASTRPC_INIT_HANDLE check on user invoke paths
- [Medium] accel/qda: qda_shim_init_create() imports args.filefd when filelen or filefd is 0
--

commit cd276e39d68d699c859df87ba50e8e071d334588
Author: Ekansh Gupta <ekangupt@qti.qualcomm.com>

accel/qda: Add DSP process creation and release to the legacy shim

This commit adds DSP process creation (FASTRPC_IOCTL_INIT_CREATE) and release
upon file close to the legacy /dev/fastrpc-* compatibility shim, enforcing
secure/non-secure node rules and importing the ELF image file descriptor into
the session's context bank.

> diff --git a/drivers/accel/qda/qda_fastrpc_shim.c b/drivers/accel/qda/qda_fastrpc_shim.c
> index 03fce990ccdb9..4a2ba20ee01ac 100644
> --- a/drivers/accel/qda/qda_fastrpc_shim.c
> +++ b/drivers/accel/qda/qda_fastrpc_shim.c
[ ... ]
> @@ -141,6 +154,43 @@ static long qda_shim_alloc_dma_buf(struct qda_shim_file *sf, void __user *argp)
>  	return 0;
>  }
>  
> +/*
> + * As in drivers/misc/fastrpc.c: on a secure channel, a client that came in
> + * through the non-secure node may only create an unsigned PD, and only on a
> + * domain that supports unsigned PD offload.
> + */
> +static bool qda_shim_session_rejected(struct qda_shim_file *sf, bool unsigned_pd)
> +{
> +	if (!sf->secure_node && sf->secure_channel)
> +		return !sf->unsigned_support || !unsigned_pd;
> +
> +	return false;
> +}
> +
> +static long qda_shim_init_create(struct qda_shim_file *sf, void __user *argp)
> +{
> +	struct drm_gem_object *elf_obj = NULL;
> +	struct fastrpc_init_create args;
> +	int err;
> +
> +	if (copy_from_user(&args, argp, sizeof(args)))
> +		return -EFAULT;
> +
> +	if (qda_shim_session_rejected(sf, args.attrs & FASTRPC_MODE_UNSIGNED_MODULE))
> +		return -ECONNREFUSED;

[Severity: High]
Can user space bypass both qda_shim_session_rejected() here and the
CAP_SYS_ADMIN check in qda_fastrpc_prepare_args_init_create() by sending a
QDA_FASTRPC_INIT_HANDLE (0x1) message through FASTRPC_IOCTL_INVOKE or
DRM_IOCTL_QDA_REMOTE_INVOKE?

In drivers/misc/fastrpc.c, fastrpc_internal_invoke() rejects user-space
invocations targeting FASTRPC_INIT_HANDLE with -EPERM when !kernel. Here,
however, neither qda_shim_invoke() -> legacy_invoke() ->
qda_fastrpc_invoke_legacy() nor qda_ioctl_invoke() ->
qda_fastrpc_prepare_args_invoke() blocks handle == QDA_FASTRPC_INIT_HANDLE.

Even more concerning, qda_fastrpc_invoke_pack() checks ctx->handle rather than
ctx->type:

    if (ctx->handle == QDA_FASTRPC_INIT_HANDLE)
        msg->fastrpc.remote_session_id = 0;
    else
        msg->fastrpc.remote_session_id = ctx->session->remote_session_id;

As a result, an unprivileged client on a non-secure node can call
FASTRPC_IOCTL_INIT_CREATE with FASTRPC_MODE_UNSIGNED_MODULE to transition
session->pd to QDA_USER_PD (or leave session->pd at QDA_ROOT_PD) and then issue
FASTRPC_IOCTL_INVOKE with handle = QDA_FASTRPC_INIT_HANDLE and sc encoding
QDA_FASTRPC_RMID_INIT_CREATE, QDA_FASTRPC_RMID_INIT_CREATE_ATTR, or
QDA_FASTRPC_RMID_INIT_RELEASE using user-controlled buffers. Should
QDA_FASTRPC_INIT_HANDLE be rejected on the user-space invoke paths?

> +
> +	elf_obj = shim_import_fd(sf->session, args.filefd);
> +	if (IS_ERR(elf_obj))
> +		return PTR_ERR(elf_obj);

[Severity: Medium]
What happens in qda_shim_init_create() when user space passes a
zero-initialized args.filefd (0) for an empty image (args.filelen == 0) or for
a bare-pointer image (args.file != 0, args.filelen > 0), or when args.filelen
is 0 with a non-negative args.filefd?

In drivers/misc/fastrpc.c, fastrpc_init_create_process() only imports
init.filefd when both init.filelen and init.filefd are non-zero:

    if (init.filelen && init.filefd) {
        ...
    }

And in drivers/accel/qda/qda_legacy_ops.c, legacy_init_create() is written to
expect elf_obj == NULL when no descriptor-backed image is used:

    if (!elf_obj && filelen)
        return -EOPNOTSUPP;

However, shim_import_fd() only treats negative file descriptors as absent:

    if (fd < 0)
        return NULL;

    dmabuf = dma_buf_get(fd);

If a caller zero-initializes struct fastrpc_init_create so args.filefd is 0,
shim_import_fd(sf->session, 0) calls dma_buf_get(0) on standard input, failing
with -EINVAL or -EBADF instead of allowing an empty image or returning
-EOPNOTSUPP for a bare pointer (or silently importing fd 0 if stdin happens to
be a valid DMA-BUF). Should qda_shim_init_create() only call shim_import_fd()
when args.filelen && args.filefd (or should shim_import_fd() treat fd <= 0 as
absent)?

> diff --git a/drivers/accel/qda/qda_legacy_ops.c b/drivers/accel/qda/qda_legacy_ops.c
> index 74ee4995350e2..2b3b3f4fb246b 100644
> --- a/drivers/accel/qda/qda_legacy_ops.c
> +++ b/drivers/accel/qda/qda_legacy_ops.c
[ ... ]
> @@ -38,6 +39,42 @@ static void legacy_session_destroy(struct qda_session *session)
>  	drm_dev_put(drm_dev);
>  }
[ ... ]
> +static int legacy_release(struct qda_session *session)
> +{
> +	/* Only a session that created a process has one to release */
> +	if (!READ_ONCE(session->init_mem_gem_obj))
> +		return 0;
> +
> +	/* Unplug check only; see qda_shim_ioctl() for why not drm_dev_enter() */
> +	if (drm_dev_is_unplugged(&session->qda_dev->drm_dev))
> +		return -ENODEV;
> +
> +	return qda_release_dsp_process(session->qda_dev, session);
> +}

[Severity: High]
Does using drm_dev_is_unplugged() instead of holding a critical section across
qda_release_dsp_process() expose a race and use-after-free with
qda_rpmsg_remove()?

In drivers/accel/qda/qda_rpmsg.c, qda_rpmsg_remove() relies on
drm_dev_unplug() to wait for active callers to finish before flushing pending
contexts and tearing down device resources:

    drm_dev_unplug(&qdev->drm_dev);
    qdev->rpdev = NULL;
    qda_fastrpc_flush_pending(qdev);
    qda_cb_unpopulate(qdev);
    qda_deinit_device(qdev);

Because drm_dev_is_unplugged() immediately drops its SRCU read lock via
drm_dev_exit(), qda_rpmsg_remove() can run concurrently right after
drm_dev_is_unplugged() returns false:

1. In qda_release_dsp_process() -> qda_fastrpc_invoke(),
   qda_fastrpc_context_alloc(qdev) allocates ctx with refcount == 1 and stores
   it in qdev->ctx_xa.
2. Concurrently, qda_rpmsg_remove() calls drm_dev_unplug(&qdev->drm_dev).
   When qda_fastrpc_invoke_common() calls qda_rpmsg_send_msg(), drm_dev_enter()
   now fails with -ENODEV, so the second reference on ctx for the response
   callback is never taken.
3. Meanwhile, qda_fastrpc_flush_pending(qdev) in qda_rpmsg_remove() iterates
   qdev->ctx_xa and calls kref_put(&ctx->refcount, qda_fastrpc_context_free),
   dropping the caller's sole reference and freeing ctx while
   qda_fastrpc_invoke() is still using it and about to call
   kref_put(&ctx->refcount, qda_fastrpc_context_free) a second time at out_put.
4. Furthermore, qda_rpmsg_remove() proceeds to qda_cb_unpopulate(qdev) and
   qda_deinit_device(qdev) (destroying the context-bank platform devices,
   qdev->iommu_mgr, and qdev->ctx_xa) while qda_release_dsp_process() is still
   allocating from qdev->ctx_xa and qdev->iommu_mgr.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-v4-v4-0-88f2f24a5f0b@qti.qualcomm.com?part=16