Re: [PATCH v4 20/25] accel/qda: Add remote attach to root and sensors protection domains

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] accel/qda: race between qda_ioctl_attach() and qda_ioctl_init_create()
--

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

accel/qda: Add remote attach to root and sensors protection domains

This commit adds an ioctl to attach a calling session to an existing root or
sensors protection domain on the DSP instead of creating a new user process.

> diff --git a/drivers/accel/qda/qda_ioctl.c b/drivers/accel/qda/qda_ioctl.c
> index 999afa9e70c23..9b8cccfea1e3a 100644
> --- a/drivers/accel/qda/qda_ioctl.c
> +++ b/drivers/accel/qda/qda_ioctl.c
> @@ -121,6 +121,8 @@ static int qda_fastrpc_invoke(u32 type, void *data, struct qda_session *session)
>  	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) {
> +		WRITE_ONCE(session->pd, ctx->pd);
>  	}
>  
>  out_put:
> @@ -174,6 +176,40 @@ int qda_release_dsp_process(struct qda_dev *qdev, struct qda_session *session)
>  	return qda_fastrpc_invoke(QDA_FASTRPC_RMID_INIT_RELEASE, NULL, session);
>  }
>  
> +/**
> + * 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
> + *
> + * Return: 0 on success, negative error code on failure
> + */
> +int qda_ioctl_attach(struct drm_device *dev, void *data, struct drm_file *file_priv)
> +{
> +	struct drm_qda_remote_attach *args = data;
> +	struct qda_session *session = file_priv->driver_priv;
> +	int pd;
> +
> +	if (args->pad)
> +		return -EINVAL;
> +
> +	switch (args->pd_type) {
> +	case QDA_ATTACH_ROOT_PD:
> +		pd = QDA_ROOT_PD;
> +		break;
> +	case QDA_ATTACH_SENSORS_PD:
> +		pd = QDA_SENSORS_PD;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (READ_ONCE(session->init_mem_gem_obj))
> +		return -EBUSY;
> +
> +	return qda_fastrpc_invoke(QDA_FASTRPC_RMID_INIT_ATTACH, &pd, session);
> +}

[Severity: High]
Can a concurrent call to qda_ioctl_init_create() race with qda_ioctl_attach()
on the same file descriptor and corrupt session->pd?

As noted in the struct qda_session definition for init_mem_gem_obj, ioctls on
a single file can run concurrently. In qda_ioctl_attach(),
session->init_mem_gem_obj is checked without holding a lock before calling
qda_fastrpc_invoke(), which sleeps in qda_fastrpc_invoke_common() while
waiting for the DSP response.

Meanwhile, qda_ioctl_init_create() -> qda_session_init_create() ->
qda_fastrpc_invoke() only populates session->init_mem_gem_obj via
qda_session_set_init_mem() after its own DSP invocation completes:

	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) {
		WRITE_ONCE(session->pd, ctx->pd);
	}

If thread 1 runs qda_ioctl_init_create() and thread 2 runs qda_ioctl_attach()
before thread 1's RPC finishes, thread 2 sees session->init_mem_gem_obj as
null and proceeds. If thread 1 finishes first and sets
session->init_mem_gem_obj and session->pd, and thread 2 finishes second,
thread 2 will overwrite session->pd with the attached root or sensors domain
(or conversely, qda_ioctl_init_create() can be called on an already-attached
session).

When the file is later closed, qda_postclose() checks
session->init_mem_gem_obj and sends the process release request to
session->pd:

drivers/accel/qda/qda_drv.c:qda_postclose() {
	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);
	}

	qda_session_put(session);
}

Because qda_fastrpc_prepare_args() sets ctx->pd from session->pd, wouldn't
qda_release_dsp_process() send the release message to the wrong protection
domain, leaving the user process running on the DSP while qda_session_put()
frees session->init_mem_gem_obj?

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