Re: [PATCH] drm/xe: prevent UAF around preempt fence

[Matthew Brost <matthew.brost@intel.com>]

On Wed, Aug 14, 2024 at 12:01:30PM +0100, Matthew Auld wrote:
> The fence lock is part of the queue, therefore in the current design
> anything locking the fence should then also hold a ref to the queue to
> prevent the queue from being freed.
> 
> However, currently it looks like we signal the fence and then drop the
> queue ref, but if something is waiting on the fence, the waiter is
> kicked to wake up at some later point, where upon waking up it first
> grabs the lock before checking the fence state. But if we have already
> dropped the queue ref, then the lock might already be freed as part of
> the queue, leading to uaf.
> 
> To prevent this, move the fence lock into the fence itself so we don't
> run into lifetime issues. Alternative might be to have device level
> lock, or only release the queue in the fence release callback, however
> that might require pushing to another worker to avoid locking issues.
> 
> Fixes: dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for Intel GPUs")
> References: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/2454
> References: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/2342
> References: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/2020
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>

Good catch. This is indeed a problem, and you have it coded probably in
the safest way possible.

With that:
Reviewed-by: Matthew Brost <matthew.brost@intel.com>

> Cc: <stable@vger.kernel.org> # v6.8+
> ---
>  drivers/gpu/drm/xe/xe_exec_queue.c          | 1 -
>  drivers/gpu/drm/xe/xe_exec_queue_types.h    | 2 --
>  drivers/gpu/drm/xe/xe_preempt_fence.c       | 3 ++-
>  drivers/gpu/drm/xe/xe_preempt_fence_types.h | 2 ++
>  4 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_exec_queue.c b/drivers/gpu/drm/xe/xe_exec_queue.c
> index 971e1234b8ea..0f610d273fb6 100644
> --- a/drivers/gpu/drm/xe/xe_exec_queue.c
> +++ b/drivers/gpu/drm/xe/xe_exec_queue.c
> @@ -614,7 +614,6 @@ int xe_exec_queue_create_ioctl(struct drm_device *dev, void *data,
>  
>  		if (xe_vm_in_preempt_fence_mode(vm)) {
>  			q->lr.context = dma_fence_context_alloc(1);
> -			spin_lock_init(&q->lr.lock);
>  
>  			err = xe_vm_add_compute_exec_queue(vm, q);
>  			if (XE_IOCTL_DBG(xe, err))
> diff --git a/drivers/gpu/drm/xe/xe_exec_queue_types.h b/drivers/gpu/drm/xe/xe_exec_queue_types.h
> index 1408b02eea53..fc2a1a20b7e4 100644
> --- a/drivers/gpu/drm/xe/xe_exec_queue_types.h
> +++ b/drivers/gpu/drm/xe/xe_exec_queue_types.h
> @@ -126,8 +126,6 @@ struct xe_exec_queue {
>  		u32 seqno;
>  		/** @lr.link: link into VM's list of exec queues */
>  		struct list_head link;
> -		/** @lr.lock: preemption fences lock */
> -		spinlock_t lock;
>  	} lr;
>  
>  	/** @ops: submission backend exec queue operations */
> diff --git a/drivers/gpu/drm/xe/xe_preempt_fence.c b/drivers/gpu/drm/xe/xe_preempt_fence.c
> index 56e709d2fb30..83fbeea5aa20 100644
> --- a/drivers/gpu/drm/xe/xe_preempt_fence.c
> +++ b/drivers/gpu/drm/xe/xe_preempt_fence.c
> @@ -134,8 +134,9 @@ xe_preempt_fence_arm(struct xe_preempt_fence *pfence, struct xe_exec_queue *q,
>  {
>  	list_del_init(&pfence->link);
>  	pfence->q = xe_exec_queue_get(q);
> +	spin_lock_init(&pfence->lock);
>  	dma_fence_init(&pfence->base, &preempt_fence_ops,
> -		      &q->lr.lock, context, seqno);
> +		      &pfence->lock, context, seqno);
>  
>  	return &pfence->base;
>  }
> diff --git a/drivers/gpu/drm/xe/xe_preempt_fence_types.h b/drivers/gpu/drm/xe/xe_preempt_fence_types.h
> index b54b5c29b533..312c3372a49f 100644
> --- a/drivers/gpu/drm/xe/xe_preempt_fence_types.h
> +++ b/drivers/gpu/drm/xe/xe_preempt_fence_types.h
> @@ -25,6 +25,8 @@ struct xe_preempt_fence {
>  	struct xe_exec_queue *q;
>  	/** @preempt_work: work struct which issues preemption */
>  	struct work_struct preempt_work;
> +	/** @lock: dma-fence fence lock */
> +	spinlock_t lock;
>  	/** @error: preempt fence is in error state */
>  	int error;
>  };
> -- 
> 2.46.0
>