Re: [PATCH V2] drm/xe: fix UAF race between exec-queue teardown and GuC CT pause-abort

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Low] drm/xe: missing Fixes trailer for commit 78f3bf00be4f
- [Low] drm/xe: commit message cites out-of-tree assert in
  guc_exec_queue_kill()
--

commit 46bb8a16fcefb342a61773f14635615595ee2bf1
Author: Tejas Upadhyay <tejas.upadhyay@intel.com>

> drm/xe: fix UAF race between exec-queue teardown and GuC CT pause-abort
>
> xe_guc_submit_pause_abort() walks guc->submission_state.exec_queue_lookup
> and unconditionally calls guc_exec_queue_kill() on every entry found,
> without verifying the queue hasn't already begun/finished its own
> independent teardown. guc_exec_queue_fini() (run once a queue's own
> refcount drops to zero via the normal close path) only erases its xarray
> entry partway through its body, via release_guc_id() -- so there is a
> window where a queue's refcount is already 0 but its xarray entry is
> still present.
>
> This was caught as a debug assertion firing on both BMG and CRI:
>
>         xe 0000:00:03.0: [drm] Assertion `kref_read(&q->refcount)` failed!
>         WARNING: drivers/gpu/drm/xe/xe_guc_submit.c:2242 at guc_exec_queue_kill+0x2e2/0x360
>          guc_exec_queue_kill+0x2e2/0x360 [xe]
>          xe_guc_submit_pause_abort+0x8e/0xf0 [xe]
>          guc_submit_fini+0x11f/0x180 [xe]
>          devm_action_release+0x15/0x30
>          ...
>          device_release_driver_internal+0x230/0x270
>          unbind_store+0xac/0xc0                          [BMG: igt@xe_configfs, sysfs unbind]
>          pci_iov_remove_virtfn+0xd1/0x140
>          sriov_disable+0x42/0x100
>          pci_disable_sriov+0x23/0x40
>          xe_pci_sriov_configure+0x2b9/0x1350 [xe]
>          sriov_numvfs_store+0x136/0x1f0           [CRI: igt@xe_pmu, SR-IOV VF disable]

[Severity: Low]
As reported by Matthew Brost on v1 [1], upstream guc_exec_queue_kill() and
its callees do not contain a kref_read(&q->refcount) assertion.

In addition, xe_guc_submit_pause_abort() holds guc->submission_state.lock
across the xa_for_each() loop, while guc_exec_queue_fini() must acquire
the same lock in release_guc_id() -> __release_guc_id() to erase the
xarray entry before freeing q->guc or q.  Could the commit message explain
what upstream issue or use-after-free is being fixed here rather than
citing an out-of-tree assertion?

> Pin the queue with kref_get_unless_zero() before touching it, skipping the
> redundant guc_exec_queue_kill() for entries that have already
> independently reached a zero refcount instead of operating on an object
> that's already being torn down elsewhere.
>
> V2(Sashiko):
> -xe_sched_submission_start(sched) must still run unconditionally for every
> non-duplicate entry regardless of that refcount check
>
> Signed-off-by: Tejas Upadhyay <tejas.upadhyay@intel.com>

[Severity: Low]
As reported by Patchwork on v1 [2], since this fixes the unconditional
guc_exec_queue_kill() call added in xe_guc_submit_pause_abort(), should it
include a Fixes tag?

  Fixes: 78f3bf00be4f ("drm/xe: Always kill exec queues in xe_guc_submit_pause_abort")

[1] https://lore.kernel.org/r/avcy5lJdRge%2FD8qm@gsse-cloud1.jf.intel.com
[2] https://lore.kernel.org/r/179457832098.20475.2771158659080650020@9ac1b63fe3ea

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114035753.3027117-2-tejas.upadhyay@intel.com?part=1