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

[Matthew Auld <matthew.auld@intel.com>]

On 04/06/2026 12:14, Thomas Hellström wrote:
> 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.

Is it plausible to check for drm_gem_is_imported() in 
ttm_bo_individualize_resv()? If sg && !imported then it should be safe 
to swap out 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);
>>>>>>>>         }
>>>>>>>
>>>>>>
>>>>>