Re: [PATCH v4 22/25] accel/qda: Add remote session attach 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: race between qda_session_attach() and qda_session_init_create()
- [High] accel/qda: use-after-free in qda_session_attach() during rpmsg removal
- [Medium] accel/qda: interrupted wait in qda_attach_audio_pd() resends remote heap
--

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

accel/qda: Add remote session attach to the legacy shim

This commit wires FASTRPC_IOCTL_INIT_ATTACH, FASTRPC_IOCTL_INIT_ATTACH_SNS,
and FASTRPC_IOCTL_INIT_CREATE_STATIC into the legacy FastRPC compatibility
shim by extracting qda_session_attach() and exposing it via
qda_legacy_ops_table.

> diff --git a/drivers/accel/qda/qda_fastrpc_shim.c b/drivers/accel/qda/qda_fastrpc_shim.c
> index c46d2de4f5818..c1464b1adbc2c 100644
> --- a/drivers/accel/qda/qda_fastrpc_shim.c
> +++ b/drivers/accel/qda/qda_fastrpc_shim.c
[ ... ]
> @@ -295,6 +296,23 @@ static long qda_shim_init_create(struct qda_shim_file *sf, void __user *argp)
>  	return err;
>  }
>  
> +static long qda_shim_attach(struct qda_shim_file *sf, u32 pd_type)
> +{
> +	return qda_legacy_ops_table.attach(sf->session, pd_type, NULL, 0);
> +}
> +
> +/* memlen is ignored: the Audio PD heap size comes from the device tree */
> +static long qda_shim_init_create_static(struct qda_shim_file *sf, void __user *argp)
> +{
> +	struct fastrpc_init_create_static args;
> +
> +	if (copy_from_user(&args, argp, sizeof(args)))
> +		return -EFAULT;
> +
> +	return qda_legacy_ops_table.attach(sf->session, QDA_ATTACH_AUDIO_PD,
> +					   u64_to_user_ptr(args.name), args.namelen);
> +}

[Severity: High]
Can qda_shim_attach() and qda_shim_init_create_static() (as well as
qda_ioctl_attach()) trigger a use-after-free if the underlying rpmsg device
is removed while an attach ioctl is in flight?

Both qda_shim_ioctl() and drm_ioctl() only perform a point-in-time
drm_dev_is_unplugged() check on entry without entering a drm_dev_enter()
critical section:

drivers/accel/qda/qda_fastrpc_shim.c:qda_shim_ioctl() {
	if (drm_dev_is_unplugged(&sf->session->qda_dev->drm_dev))
		return -ENODEV;

	return qda_shim_dispatch(sf, cmd, (void __user *)arg);
}

If qda_rpmsg_remove() runs after that check passes (for example while
qda_shim_init_create_static() or qda_attach_audio_pd() sleeps in
copy_from_user() or memdup_user()), drm_dev_unplug() does not wait for the
in-flight qda_session_attach() call before tearing down the device:

drivers/accel/qda/qda_rpmsg.c:qda_rpmsg_remove() {
	drm_dev_unplug(&qdev->drm_dev);
	qdev->rpdev = NULL;
	qda_fastrpc_flush_pending(qdev);
	qda_cb_unpopulate(qdev);
	qda_deinit_device(qdev);
	qda_deinit_remote_heap(qdev);
}

qda_deinit_device() destroys qdev->iommu_mgr->assignment_lock and
qdev->ctx_xa, and devres teardown on &rpdev->dev frees qdev->iommu_mgr
(allocated with devm_kzalloc(qdev->dev, ...) in qda_init_device()). When the
in-flight qda_session_attach() proceeds into:

  qda_fastrpc_invoke()
    -> qda_fastrpc_context_alloc()
    -> qda_fastrpc_prepare_args()
      -> alloc_kernel_gem()
        -> qda_gem_create_object()
          -> qda_memory_manager_alloc()
            -> qda_memory_manager_assign_device()

wouldn't it access the destroyed qdev->ctx_xa and dereference the freed
qdev->iommu_mgr (or have its newly allocated ctx freed early if
qda_fastrpc_flush_pending() runs between qda_fastrpc_context_alloc() and
qda_rpmsg_send_msg())?

> diff --git a/drivers/accel/qda/qda_ioctl.c b/drivers/accel/qda/qda_ioctl.c
> index 41234fc576a9a..aaf3e5e6b5b18 100644
> --- a/drivers/accel/qda/qda_ioctl.c
> +++ b/drivers/accel/qda/qda_ioctl.c
> @@ -183,7 +183,7 @@ int qda_release_dsp_process(struct qda_dev *qdev, struct qda_session *session)
>   * so that a concurrent one cannot skip the heap while the first is still in
>   * flight, and the flag is set only once the DSP has accepted it.
>   */
> -static int qda_attach_audio_pd(struct qda_session *session, struct drm_qda_remote_attach *args)
> +static int qda_attach_audio_pd(struct qda_session *session, const char __user *uname, u32 namelen)
>  {
>  	struct qda_dev *qdev = session->qda_dev;
>  	struct qda_static_pd_args sargs = {};
> @@ -193,20 +193,20 @@ static int qda_attach_audio_pd(struct qda_session *session, struct drm_qda_remot
>  	if (strcmp(qdev->dsp_name, "adsp") || !qdev->remote_heap_size)
>  		return -ENODEV;
>  
> -	if (!args->namelen || args->namelen > QDA_FASTRPC_INIT_NAMELEN_MAX)
> +	if (!namelen || namelen > QDA_FASTRPC_INIT_NAMELEN_MAX)
>  		return -EINVAL;
>  
> -	name = memdup_user(u64_to_user_ptr(args->name), args->namelen);
> +	name = memdup_user(uname, namelen);
>  	if (IS_ERR(name))
>  		return PTR_ERR(name);
>  
> -	if (name[args->namelen - 1]) {
> +	if (name[namelen - 1]) {
>  		err = -EINVAL;
>  		goto out_free;
>  	}
>  
>  	sargs.name = name;
> -	sargs.namelen = args->namelen;
> +	sargs.namelen = namelen;
>  
>  	scoped_guard(mutex, &qdev->remote_heap_lock) {
>  		if (!qdev->audio_init_mem) {

[Severity: Medium]
What happens in qda_attach_audio_pd() if qda_fastrpc_invoke() is interrupted
by a signal after sending the request to the DSP?

Inside the qdev->remote_heap_lock guard:

	scoped_guard(mutex, &qdev->remote_heap_lock) {
		if (!qdev->audio_init_mem) {
			sargs.heap.addr = qdev->remote_heap_addr;
			sargs.heap.size = qdev->remote_heap_size;
		}

		err = qda_fastrpc_invoke(QDA_FASTRPC_RMID_INIT_CREATE_STATIC, &sargs, session);
		if (!err && sargs.heap.size)
			qdev->audio_init_mem = true;
	}

qda_fastrpc_invoke_common() calls qda_rpmsg_send_msg() and then blocks in
qda_rpmsg_wait_for_rsp() via wait_for_completion_interruptible(&ctx->work).
If a signal interrupts the wait before the DSP replies, qda_fastrpc_invoke()
returns -ERESTARTSYS while the first QDA_FASTRPC_RMID_INIT_CREATE_STATIC
request is still in flight on the DSP.

Because err is non-zero, qdev->audio_init_mem remains false when
qdev->remote_heap_lock is released. Could a concurrent or restarted
FASTRPC_IOCTL_INIT_CREATE_STATIC / DRM_IOCTL_QDA_REMOTE_SESSION_ATTACH call
then observe !qdev->audio_init_mem and hand the remote heap to the DSP a
second time?

> @@ -226,38 +226,60 @@ static int qda_attach_audio_pd(struct qda_session *session, struct drm_qda_remot
>  }
>  
>  /**
> - * qda_ioctl_attach() - Attach to a DSP-resident protection domain
> - * @dev: DRM device structure
> - * @data: User-space data (struct drm_qda_remote_attach)
> - * @file_priv: DRM file private data
> + * qda_session_attach() - Attach a session to a DSP-resident protection domain
> + * @session: Session to attach
> + * @pd_type: QDA_ATTACH_ROOT_PD, QDA_ATTACH_SENSORS_PD or QDA_ATTACH_AUDIO_PD
> + * @name: User pointer to the static PD name (QDA_ATTACH_AUDIO_PD only)
> + * @namelen: Length of @name including the NUL (QDA_ATTACH_AUDIO_PD only)
> + *
> + * Shared by the accel UAPI and the legacy shim, so the privilege check lives
> + * here rather than on the ioctl.
>   *
>   * Return: 0 on success, negative error code on failure
>   */
> -int qda_ioctl_attach(struct drm_device *dev, void *data, struct drm_file *file_priv)
> +int qda_session_attach(struct qda_session *session, u32 pd_type, const char __user *name,
> +		       u32 namelen)
>  {
> -	struct drm_qda_remote_attach *args = data;
> -	struct qda_session *session = file_priv->driver_priv;
>  	int pd;
>  
> +	if (!capable(CAP_SYS_ADMIN))
> +		return -EPERM;
> +
>  	if (READ_ONCE(session->init_mem_gem_obj))
>  		return -EBUSY;
>  
> -	switch (args->pd_type) {
> +	switch (pd_type) {
>  	case QDA_ATTACH_ROOT_PD:
>  	case QDA_ATTACH_SENSORS_PD:
> -		if (args->name || args->namelen)
> +		if (name || namelen)
>  			return -EINVAL;
>  
> -		pd = args->pd_type == QDA_ATTACH_ROOT_PD ? QDA_ROOT_PD : QDA_SENSORS_PD;
> +		pd = pd_type == QDA_ATTACH_ROOT_PD ? QDA_ROOT_PD : QDA_SENSORS_PD;
>  
>  		return qda_fastrpc_invoke(QDA_FASTRPC_RMID_INIT_ATTACH, &pd, session);
>  	case QDA_ATTACH_AUDIO_PD:
> -		return qda_attach_audio_pd(session, args);
> +		return qda_attach_audio_pd(session, name, namelen);
>  	default:
>  		return -EINVAL;
>  	}
>  }

[Severity: High]
Does qda_session_attach() race with concurrent qda_session_init_create()
calls on the same session?

Both the accel UAPI and the legacy shim allow concurrent ioctls on the same
open file without holding a per-session lock. In qda_session_attach(), the
check:

	if (READ_ONCE(session->init_mem_gem_obj))
		return -EBUSY;

runs unlocked before qda_fastrpc_invoke() sleeps waiting for the DSP
response, and qda_session_init_create() does not check whether the session
has already attached or is attaching. State updates only happen after the
DSP responds in qda_fastrpc_invoke():

	if (type == QDA_FASTRPC_RMID_INIT_CREATE) {
		qda_session_set_init_mem(session, ctx->init_mem_gem_obj);
		WRITE_ONCE(session->pd, QDA_USER_PD);
	} else if (type == QDA_FASTRPC_RMID_INIT_ATTACH ||
		   type == QDA_FASTRPC_RMID_INIT_CREATE_STATIC) {
		WRITE_ONCE(session->pd, ctx->pd);
	}

If one thread calls qda_session_attach() and another concurrently calls
qda_session_init_create() on the same file descriptor, both can pass
validation and send RPCs to the DSP. If QDA_FASTRPC_RMID_INIT_CREATE finishes
first (setting session->init_mem_gem_obj and session->pd = QDA_USER_PD) and
QDA_FASTRPC_RMID_INIT_ATTACH finishes second, session->pd is overwritten with
QDA_ROOT_PD or QDA_SENSORS_PD while session->init_mem_gem_obj remains
non-NULL.

When the file is later closed, legacy_release() or qda_postclose() sees
session->init_mem_gem_obj is non-NULL and calls qda_release_dsp_process(),
which sends QDA_FASTRPC_RMID_INIT_RELEASE directed at READ_ONCE(session->pd)
(now the attached PD instead of QDA_USER_PD). Wouldn't that leave the User PD
process running on the DSP while qda_session_release() frees
session->init_mem_gem_obj underneath it?

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