Re: [PATCH] drm/xe: Fix UAF in xe_gem_prime_import() on attach failure

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

Hi,

On Thu, 2026-06-04 at 04:54 +0000, Gote, Nitin R wrote:
> Hi, 
> 
> > -----Original Message-----
> > From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf
> > Of Gote, Nitin
> > R
> > Sent: Monday, June 1, 2026 8:57 PM
> > To: Christian König <christian.koenig@amd.com>; Auld, Matthew
> > <matthew.auld@intel.com>; intel-xe@lists.freedesktop.org; Christian
> > König
> > <ckoenig.leichtzumerken@gmail.com>
> > Cc: stable@vger.kernel.org; Thomas Hellstrom
> > <thomas.hellstrom@linux.intel.com>; Brost, Matthew
> > <matthew.brost@intel.com>; Prosyak, Vitaly <Vitaly.Prosyak@amd.com>
> > Subject: RE: [PATCH] drm/xe: Fix UAF in xe_gem_prime_import() on
> > attach failure
> > 
> > Hi Christian,
> > 
> > > -----Original Message-----
> > > From: Christian König <christian.koenig@amd.com>
> > > Sent: Monday, June 1, 2026 5:47 PM
> > > To: Auld, Matthew <matthew.auld@intel.com>; Gote, Nitin R
> > > <nitin.r.gote@intel.com>; intel-xe@lists.freedesktop.org;
> > > Christian
> > > König <ckoenig.leichtzumerken@gmail.com>
> > > Cc: stable@vger.kernel.org; Thomas Hellstrom
> > > <thomas.hellstrom@linux.intel.com>; Brost, Matthew
> > > <matthew.brost@intel.com>; Prosyak, Vitaly
> > > <Vitaly.Prosyak@amd.com>
> > > Subject: Re: [PATCH] drm/xe: Fix UAF in xe_gem_prime_import() on
> > > attach failure
> > > 
> > > On 6/1/26 14:01, Matthew Auld wrote:
> > > > On 01/06/2026 12:39, Christian König wrote:
> > > > > 
> > > > > 
> > > > > On 6/1/26 12:46, Matthew Auld wrote:
> > > > > > On 01/06/2026 11:15, Nitin Gote wrote:
> > > > > > > xe_dma_buf_create_obj() creates the importer BO with obj-
> > > > > > > >resv
> > > > > > > pointing at the exporter's dma_buf->resv. When
> > > > > > > dma_buf_dynamic_attach() fails, no dma_buf reference is
> > > > > > > held so
> > > > > > > the exporter can be freed immediately. Since
> > > > > > > ttm_bo_release() now
> > > > > > > always defers cleanup for ttm_bo_type_sg BOs to the TTM
> > > > > > > workqueue, the worker later calls
> > > > > > > dma_resv_lock() on the already-freed exporter resv,
> > > > > > > causing a UAF.
> > > > > > > 
> > > > > > > Reset obj->resv to the BO's private _resv before calling
> > > > > > > xe_bo_put() in the error path. The BO is not yet
> > > > > > > published
> > > > > > > (attach
> > > > > > > failed) and carries no fences, so the switch is safe.
> > > > > > > 
> > > > > > > Observed with igt@xe_live_ktest@xe_dma_buf_kunit on BMG
> > > > > > > (QEMU):
> > > > > > > 
> > > > > > >     Oops: general protection fault, probably for non-
> > > > > > > canonical
> > > > > > > address 0x6b6b6b6b6b6b6b9c
> > > > > > >     Workqueue: ttm ttm_bo_delayed_delete [ttm]
> > > > > > >     RIP: 0010:mutex_can_spin_on_owner+0x3f/0xc0
> > > > > > >     Call Trace:
> > > > > > >      <TASK>
> > > > > > >      ? __ww_mutex_lock.constprop.0+0x2dd/0x18e0
> > > > > > >      ? ttm_bo_delayed_delete+0x41/0xc0 [ttm]
> > > > > > >      ww_mutex_lock+0x3c/0xb0
> > > > > > >      ttm_bo_delayed_delete+0x41/0xc0 [ttm]
> > > > > > >      process_one_work+0x239/0x740
> > > > > > >      worker_thread+0x200/0x3f0
> > > > > > >      kthread+0x10d/0x150
> > > > > > >      ret_from_fork+0x3bd/0x470
> > > > > > >      ret_from_fork_asm+0x1a/0x30
> > > > > > >      </TASK>
> > > > > > > 
> > > > > > > Closes:
> > > > > > > https://gitlab.freedesktop.org/drm/xe/kernel/-/work_items/8023
> > > > > > > Fixes: d99fbd9aab62 ("drm/ttm: Always take the bo delayed
> > > > > > > cleanup
> > > > > > > path for imported bos")
> > > > > > > Cc: stable@vger.kernel.org # v6.8+
> > > > > > > Cc: Thomas Hellstrom <thomas.hellstrom@linux.intel.com>
> > > > > > > Cc: Matthew Brost <matthew.brost@intel.com>
> > > > > > > Cc: Matthew Auld <matthew.auld@intel.com>
> > > > > > > Signed-off-by: Nitin Gote <nitin.r.gote@intel.com>
> > > > > > > ---
> > > > > > >    drivers/gpu/drm/xe/xe_dma_buf.c | 8 ++++++++
> > > > > > >    1 file changed, 8 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/xe/xe_dma_buf.c
> > > > > > > b/drivers/gpu/drm/xe/xe_dma_buf.c index
> > > > > > > 8a920e58245c..6d944bd4065c
> > > > > > > 100644
> > > > > > > --- a/drivers/gpu/drm/xe/xe_dma_buf.c
> > > > > > > +++ b/drivers/gpu/drm/xe/xe_dma_buf.c
> > > > > > > @@ -384,6 +384,14 @@ struct drm_gem_object
> > > > > > > *xe_gem_prime_import(struct drm_device *dev,
> > > > > > >          attach = dma_buf_dynamic_attach(dma_buf, dev-
> > > > > > > >dev,
> > > > > > > attach_ops, obj);
> > > > > > >        if (IS_ERR(attach)) {
> > > > > > > +        /*
> > > > > > > +         * The BO was created with resv = dma_buf->resv
> > > > > > > +(exporter's
> > > > > > > +         * resv). Since attach failed, no dma_buf
> > > > > > > reference is
> > > > > > > +held and
> > > > > > > +         * the exporter may be freed before TTM's
> > > > > > > delayed_delete
> > > > > > > +worker
> > > > > > > +         * runs. Switch to the BO's own resv to prevent
> > > > > > > a UAF
> > > > > > > +when
> > > > > > > +         * ttm_bo_delayed_delete() tries to lock the
> > > > > > > stale pointer.
> > > > > > > +         */
> > > > > > > +        obj->resv = &obj->_resv;
> > > > > > 
> > > > > > +Christian, does amdgpu not have the type of same issue
> > > > > > here? Also
> > > > > > +any
> > > thoughts here?
> > > > > 
> > > > > Oh, good catch. Yeah I think we have the same problem on
> > > > > amdgpu as well.
> > > > 
> > > > Maybe dumb question, but why does the
> > > > ttm_bo_individualize_resv()
> > > > skip the
> > > final switch of the resv for type_sg?
> > > 
> > > Because we need the original resv object for cleaning up the
> > > mapping
> > > should the initial attach and then map have succeed.
> > > 
> > > > It goes through the trouble of copying the fences across?
> > > 
> > > Because we need to know when the import can be cleaned up.
> > > 
> > > In other words TTM takes a copy of the current fences and only
> > > unmap,
> > > detach and then do the final cleanup after we are sure that the
> > > set of
> > > fences which was active on destruction is now signaled.
> > > 
> > > If new fences are added to the resv object (maybe by the exporter
> > > itself or other
> > > importers) after our reference count got down to zero then we
> > > don't
> > > care about that.
> > > > If we do need to handle this here, do we also need to grab the
> > > > lru
> > > > lock, like we
> > > do in ttm_bo_individualize_resv() when doing the swap?
> > > 
> > > Good question, of hand I would say yes but I clearly need to
> > > check the
> > > source code as well.
> > > 
> > > Might be better to switch the type of the BO on error so that the
> > > normal cleanup will just switch over to the local dma_resv
> > > object.
> > > 
> > 
> > -               obj->resv = &obj->_resv;
> > +               gem_to_xe_bo(obj)->ttm.type = ttm_bo_type_kernel;
> > 
> > Switching the type to ttm_bo_type_kernel lets
> > ttm_bo_individualize_resv() swap
> > resv to the BO's private _resv under lru_lock, which prevents UAF
> > without
> > needing any manual locking.

The lru lock is IIRC only needed and safe when the ttm refcount is zero
(in the TTM destruction path) to protect against a racing LRU walk
trylock succeeds against the incorrect resv.

I wonder whether this was actually why xe code initially took care not
to publish the bo on the LRUs until the attachment succeeded.

A TTM LRU walker may pick up the exporting resv as soon as the resource
is published on the LRU, and then try to lock it using
ttm_lru_walk_ticketlock(). The lru lock doesn't protect against that.
 
So we have a sort of moment22, since with that approach move_notify()
could be called without the bo being fully initialized.

One way to move forward would perhaps be to, for now, reinstate that
and have move_notify check if the bo is a stub or fully initialized
before doing anything.

Also perhaps we should in the future consider allowing dma-buf
attachment removal under a separate lower-level lock than the resv.

Thanks,
Thomas


> 
> Checked all bo->type readers (xe_evict_flags(), xe_bo_move(),
> xe_bo_can_migrate()) and found they can be called concurrently by the
> shrinker or eviction paths without any synchronization, making the
> bo->type change unsafe.
> 
> Switching resv to &obj->_resv under lru_lock, mirroring
> ttm_bo_individualize_resv(), is the more reasonable. 
> I'll send this as v2, along with a separate patch fixing the same
> issue in amdgpu.
> 
> - Nitin
> 
> > > Since we don't need the original dma_resv for the cleanup that
> > > should work
> > fine.
> > > 
> > > > Ideally xe and amdgpu can just have identical solutions here.
> > > 
> > > Yeah completely agree.
> > > 
> > > Regards,
> > > Christian.
> > > 
> > > > 
> > > > > 
> > > > > How the heck did you found that? Do we have a dummy driver
> > > > > (VGEM?)
> > > > > which
> > > could be made to always fail attachment for a test case?
> > 
> > The bug was found via the existing KUnit test (xe_dma_buf_kunit),
> > which was
> > failing on a BMG VM device. The test runs 20 parameter
> > combinations.
> > the failing ones use force_different_devices=true +
> > mem_mask=XE_BO_FLAG_VRAM0 + nop2p_attach_ops, where
> > dma_buf_dynamic_attach() returns -EOPNOTSUPP, hitting the error
> > path.
> > 
> > On bare metal BMG the race window is too narrow to hit the issue.
> > To make it
> > more deterministic, added a small msleep(100) in
> > ttm_bo_delayed_delete() just
> > before the dma_resv_lock() call, which widened the race window.
> > With KASAN enabled, that gave a clear slab-use-after-free in
> > __ww_mutex_lock
> > — the 0x6b6b6b6b SLUB poison pattern in the faulting address
> > confirmed the
> > UAF.
> > 
> > Thanks,
> > Nitin
> > 
> > > > > 
> > > > > @Vitaly can you take a look and try to come up with a test
> > > > > case for that?
> > > Thanks in advance.
> > > > > 
> > > > > Thanks for the notice,
> > > > > Christian.
> > > > > 
> > > > > > 
> > > > > > >            xe_bo_put(gem_to_xe_bo(obj));
> > > > > > >            return ERR_CAST(attach);
> > > > > > >        }
> > > > > > 
> > > > > 
> > > >