Re: [PATCH] accel/qaic: Harden checks in BO execute and wait IOCTLs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] accel/qaic: unlocked bo->dbc dereference in
  qaic_wait_bo_ioctl()
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Medium] accel/qaic: missing completion signal in
  send_bo_list_to_device() error path [1]
--

commit f3a92ee30bd740f37f373a9b22ee385311742a39
Author: Youssef Samir <youssef.abdulrahman@oss.qualcomm.com>

accel/qaic: Harden checks in BO execute and wait IOCTLs

This commit adds checks in send_bo_list_to_device() and
qaic_wait_bo_ioctl() to verify that BOs are sliced and attached to the
specified DBC.

> diff --git a/drivers/accel/qaic/qaic_data.c b/drivers/accel/qaic/qaic_data.c
> index ee35e13091b30..be3ceb8c51a09 100644
> --- a/drivers/accel/qaic/qaic_data.c
> +++ b/drivers/accel/qaic/qaic_data.c
> @@ -1256,6 +1256,11 @@ static int send_bo_list_to_device(struct qaic_device *qdev, struct drm_file *fil
>  			goto unlock_bo;
>  		}
>  
> +		if (bo->dbc->id != dbc->id) {
> +			ret = -EINVAL;
> +			goto unlock_bo;
> +		}
> +
>  		if (is_partial && pexec[i].resize > bo->base.size) {
>  			ret = -EINVAL;
>  			goto unlock_bo;

[Severity: Medium]
This problem wasn't introduced by this patch, but can failing on a later BO
in the batch leave waiters on earlier BOs hung indefinitely [1]?

For each BO processed before the failure, send_bo_list_to_device() calls
reinit_completion(&bo->xfer_done) and adds the BO to dbc->xfer_list.  When a
subsequent BO jumps to unlock_bo and failed_to_send_bo, the rollback loop
removes those earlier BOs from dbc->xfer_list without calling
complete_all(&bo->xfer_done):

drivers/accel/qaic/qaic_data.c:send_bo_list_to_device() {
    ...
failed_to_send_bo:
	if (likely(obj))
		drm_gem_object_put(obj);
	for (j = 0; j < i; j++) {
		spin_lock_irqsave(&dbc->xfer_lock, flags);
		bo = list_last_entry(&dbc->xfer_list, struct qaic_bo, xfer_list);
		obj = &bo->base;
		list_del_init(&bo->xfer_list);
		spin_unlock_irqrestore(&dbc->xfer_lock, flags);
		dma_sync_sgtable_for_cpu(&qdev->pdev->dev, bo->sgt, bo->dir);
		drm_gem_object_put(obj);
	}
}

Because the rolled-back BOs are no longer on dbc->xfer_list, the interrupt
handler will never complete them, so any thread waiting in
qaic_wait_bo_ioctl() will block until timeout.

> @@ -1769,6 +1774,17 @@ int qaic_wait_bo_ioctl(struct drm_device *dev, void *data, struct drm_file *file
>  	}
>  
>  	bo = to_qaic_bo(obj);
> +
> +	if (!bo->sliced) {
> +		ret = -EINVAL;
> +		goto put_obj;
> +	}
> +
> +	if (bo->dbc->id != dbc->id) {
> +		ret = -EINVAL;
> +		goto put_obj;
> +	}
> +
>  	timeout = args->timeout ? args->timeout : wait_exec_default_timeout_ms;
>  	timeout = msecs_to_jiffies(timeout);
>  	ret = wait_for_completion_interruptible_timeout(&bo->xfer_done, timeout);

[Severity: High]
Can this dereference a NULL bo->dbc when racing with
qaic_detach_slice_bo_ioctl()?

bo->sliced and bo->dbc are protected by bo->lock, which qaic_wait_bo_ioctl()
does not hold.  A concurrent detach clears bo->dbc in qaic_unprepare_bo()
before clearing bo->sliced in qaic_init_bo():

CPU0                                  CPU1
qaic_wait_bo_ioctl()                  qaic_detach_slice_bo_ioctl()
                                        mutex_lock(&bo->lock)
                                        detach_slice_bo()
                                          qaic_unprepare_bo()
                                            bo->dbc = NULL
  if (!bo->sliced)
  if (bo->dbc->id != dbc->id)  <-- NULL pointer dereference
                                          qaic_init_bo()
                                            bo->sliced = false

Acquiring bo->lock around the bo->sliced and bo->dbc->id checks prevents the
race.

[1] https://sashiko.dev/#/bug/linux-3d10e194-452c-43b5-a7b5-0f068986229f

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106194434.4062205-1-youssef.abdulrahman@oss.qualcomm.com?part=1