Re: [PATCH v2] drm/xe: fix UAF around queue destruction

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

On Mon, Sep 23, 2024 at 03:56:48PM +0100, Matthew Auld wrote:
> We currently do stuff like queuing the final destruction step on a
> random system wq, which will outlive the driver instance. With bad
> timing we can teardown the driver with one or more work workqueue still
> being alive leading to various UAF splats. Add a fini step to ensure
> user queues are properly torn down. At this point GuC should already be
> nuked so queue itself should no longer be referenced from hw pov.
> 
> v2 (Matt B)
>  - Looks much safer to use a waitqueue and then just wait for the
>    xa_array to become empty before triggering the drain.
> 
> Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/2317
> Fixes: dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for Intel GPUs")
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>

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

> Cc: <stable@vger.kernel.org> # v6.8+
> ---
>  drivers/gpu/drm/xe/xe_device.c       |  6 +++++-
>  drivers/gpu/drm/xe/xe_device_types.h |  3 +++
>  drivers/gpu/drm/xe/xe_guc_submit.c   | 26 +++++++++++++++++++++++++-
>  drivers/gpu/drm/xe/xe_guc_types.h    |  2 ++
>  4 files changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index cb5a9fd820cf..90b3478ed7cd 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -297,6 +297,9 @@ static void xe_device_destroy(struct drm_device *dev, void *dummy)
>  	if (xe->unordered_wq)
>  		destroy_workqueue(xe->unordered_wq);
>  
> +	if (xe->destroy_wq)
> +		destroy_workqueue(xe->destroy_wq);
> +
>  	ttm_device_fini(&xe->ttm);
>  }
>  
> @@ -360,8 +363,9 @@ struct xe_device *xe_device_create(struct pci_dev *pdev,
>  	xe->preempt_fence_wq = alloc_ordered_workqueue("xe-preempt-fence-wq", 0);
>  	xe->ordered_wq = alloc_ordered_workqueue("xe-ordered-wq", 0);
>  	xe->unordered_wq = alloc_workqueue("xe-unordered-wq", 0, 0);
> +	xe->destroy_wq = alloc_workqueue("xe-destroy-wq", 0, 0);
>  	if (!xe->ordered_wq || !xe->unordered_wq ||
> -	    !xe->preempt_fence_wq) {
> +	    !xe->preempt_fence_wq || !xe->destroy_wq) {
>  		/*
>  		 * Cleanup done in xe_device_destroy via
>  		 * drmm_add_action_or_reset register above
> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> index 5ad96d283a71..515385b916cc 100644
> --- a/drivers/gpu/drm/xe/xe_device_types.h
> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> @@ -422,6 +422,9 @@ struct xe_device {
>  	/** @unordered_wq: used to serialize unordered work, mostly display */
>  	struct workqueue_struct *unordered_wq;
>  
> +	/** @destroy_wq: used to serialize user destroy work, like queue */
> +	struct workqueue_struct *destroy_wq;
> +
>  	/** @tiles: device tiles */
>  	struct xe_tile tiles[XE_MAX_TILES_PER_DEVICE];
>  
> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> index fbbe6a487bbb..ae2f85cc2d08 100644
> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> @@ -276,10 +276,26 @@ static struct workqueue_struct *get_submit_wq(struct xe_guc *guc)
>  }
>  #endif
>  
> +static void xe_guc_submit_fini(struct xe_guc *guc)
> +{
> +	struct xe_device *xe = guc_to_xe(guc);
> +	struct xe_gt *gt = guc_to_gt(guc);
> +	int ret;
> +
> +	ret = wait_event_timeout(
> +		guc->submission_state.fini_wq,
> +		xa_empty(&guc->submission_state.exec_queue_lookup), HZ * 5);
> +
> +	drain_workqueue(xe->destroy_wq);
> +
> +	xe_gt_assert(gt, ret);
> +}
> +
>  static void guc_submit_fini(struct drm_device *drm, void *arg)
>  {
>  	struct xe_guc *guc = arg;
>  
> +	xe_guc_submit_fini(guc);
>  	xa_destroy(&guc->submission_state.exec_queue_lookup);
>  	free_submit_wq(guc);
>  }
> @@ -345,6 +361,8 @@ int xe_guc_submit_init(struct xe_guc *guc, unsigned int num_ids)
>  
>  	xa_init(&guc->submission_state.exec_queue_lookup);
>  
> +	init_waitqueue_head(&guc->submission_state.fini_wq);
> +
>  	primelockdep(guc);
>  
>  	return drmm_add_action_or_reset(&xe->drm, guc_submit_fini, guc);
> @@ -361,6 +379,9 @@ static void __release_guc_id(struct xe_guc *guc, struct xe_exec_queue *q, u32 xa
>  
>  	xe_guc_id_mgr_release_locked(&guc->submission_state.idm,
>  				     q->guc->id, q->width);
> +
> +	if (xa_empty(&guc->submission_state.exec_queue_lookup))
> +		wake_up(&guc->submission_state.fini_wq);
>  }
>  
>  static int alloc_guc_id(struct xe_guc *guc, struct xe_exec_queue *q)
> @@ -1268,13 +1289,16 @@ static void __guc_exec_queue_fini_async(struct work_struct *w)
>  
>  static void guc_exec_queue_fini_async(struct xe_exec_queue *q)
>  {
> +	struct xe_guc *guc = exec_queue_to_guc(q);
> +	struct xe_device *xe = guc_to_xe(guc);
> +
>  	INIT_WORK(&q->guc->fini_async, __guc_exec_queue_fini_async);
>  
>  	/* We must block on kernel engines so slabs are empty on driver unload */
>  	if (q->flags & EXEC_QUEUE_FLAG_PERMANENT || exec_queue_wedged(q))
>  		__guc_exec_queue_fini_async(&q->guc->fini_async);
>  	else
> -		queue_work(system_wq, &q->guc->fini_async);
> +		queue_work(xe->destroy_wq, &q->guc->fini_async);
>  }
>  
>  static void __guc_exec_queue_fini(struct xe_guc *guc, struct xe_exec_queue *q)
> diff --git a/drivers/gpu/drm/xe/xe_guc_types.h b/drivers/gpu/drm/xe/xe_guc_types.h
> index 546ac6350a31..69046f698271 100644
> --- a/drivers/gpu/drm/xe/xe_guc_types.h
> +++ b/drivers/gpu/drm/xe/xe_guc_types.h
> @@ -81,6 +81,8 @@ struct xe_guc {
>  #endif
>  		/** @submission_state.enabled: submission is enabled */
>  		bool enabled;
> +		/** @submission_state.fini_wq: submit fini wait queue */
> +		wait_queue_head_t fini_wq;
>  	} submission_state;
>  	/** @hwconfig: Hardware config state */
>  	struct {
> -- 
> 2.46.1
>