Skip to content

Commit

Permalink
drm/xe: fix UAF around queue destruction
Browse files Browse the repository at this point in the history
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: dd08ebf ("drm/xe: Introduce a new DRM driver for Intel GPUs")
Signed-off-by: Matthew Auld <[email protected]>
Cc: Matthew Brost <[email protected]>
Cc: <[email protected]> # v6.8+
Reviewed-by: Matthew Brost <[email protected]>
Link: https://patchwork.freedesktop.org/patch/msgid/[email protected]
(cherry picked from commit 8611086)
Signed-off-by: Lucas De Marchi <[email protected]>
  • Loading branch information
matt-auld authored and lucasdemarchi committed Oct 3, 2024
1 parent 790533e commit 2d2be27
Show file tree
Hide file tree
Showing 4 changed files with 35 additions and 2 deletions.
6 changes: 5 additions & 1 deletion drivers/gpu/drm/xe/xe_device.c
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,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);
}

Expand Down Expand Up @@ -363,8 +366,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
Expand Down
3 changes: 3 additions & 0 deletions drivers/gpu/drm/xe/xe_device_types.h
Original file line number Diff line number Diff line change
Expand Up @@ -396,6 +396,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];

Expand Down
26 changes: 25 additions & 1 deletion drivers/gpu/drm/xe/xe_guc_submit.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -351,6 +367,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);
Expand All @@ -367,6 +385,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)
Expand Down Expand Up @@ -1274,13 +1295,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)
Expand Down
2 changes: 2 additions & 0 deletions drivers/gpu/drm/xe/xe_guc_types.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down

0 comments on commit 2d2be27

Please sign in to comment.