Re: [PATCH 1/2] RFC: drm/xe: Fix pinned list UAF panic

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] xe: use-after-free race in xe_bo_apply_to_pinned() due to 0-refcount BOs
- [High] xe: shrinker accounting leak when pinned BO is destroyed
- [High] xe: unconditional panic due to NULL bdev in ___xe_bo_create error path
--

commit 2ba63aa4ec480f34e429a19652b5cfeb92d01204
Author: Kornel Dulęba <korneld@google.com>
RFC: drm/xe: Fix pinned list UAF panic

This commit attempts to fix a list corruption panic observed when a
buffer object is destroyed while still pinned. It modifies
xe_ttm_bo_destroy() to check and remove the buffer object from the
pinned list before it is freed.

> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> index 6921b6967330d..98c7c17cc46cc 100644
> --- a/drivers/gpu/drm/xe/xe_bo.c
> +++ b/drivers/gpu/drm/xe/xe_bo.c
> @@ -1885,6 +1885,11 @@ static void xe_ttm_bo_destroy(struct ttm_buffer_object *ttm_bo)
>  	if (bo->parent_obj)
>  		xe_bo_put(bo->parent_obj);
>  
> +	spin_lock(&xe->pinned.lock);
> +	if (WARN_ON_ONCE(!list_empty(&bo->pinned_link)))
> +		list_del_init(&bo->pinned_link);
> +	spin_unlock(&xe->pinned.lock);

[Severity: High]
Does this expose a use-after-free race with xe_bo_apply_to_pinned()?

When ttm_bo_release() drops the object's kref to 0, the object remains
on the pinned list until xe_ttm_bo_destroy() acquires the xe->pinned.lock
and removes it here.

Concurrently, xe_bo_apply_to_pinned() can iterate over the pinned list,
find the object, and unconditionally call xe_bo_get(bo). Since the refcount
is already 0, xe_bo_get() will call kref_get() on a 0-refcount object,
which triggers a refcount underflow warning and leads to a use-after-free
panic.

Should xe_bo_apply_to_pinned() use xe_bo_get_unless_zero() to safely
skip objects undergoing destruction?

[Severity: High]
Does removing the buffer object from the pinned list here bypass shrinker
accounting?

When a buffer object is pinned via xe_bo_pin_external(), it calls
xe_ttm_tt_account_subtract() to remove its pages from the shrinker's
shrinkable_pages pool.

By handling the cleanup directly in xe_ttm_bo_destroy() instead of the
normal xe_bo_unpin_external() path, xe_ttm_tt_account_add() is never
called to return the pages to the shrinker.

Furthermore, for native buffer objects, the teardown path in
xe_ttm_tt_unpopulate() will still execute and call
xe_ttm_tt_account_subtract() again. Because the addition was missed and a
double-subtraction occurs, this creates a permanent negative delta for
shrinkable_pages. Due to unsigned integer math, this underflow converts
into a massive positive value, potentially forcing the kernel into an
infinite memory reclaim loop.

[Severity: High]
Can this spin_lock() cause a kernel panic if reached from the
___xe_bo_create() error path?

In ___xe_bo_create(), if __xe_bo_placement_for_flags() fails, the error
path directly calls xe_ttm_bo_destroy(&bo->ttm).

At this point, ttm_bo_init_reserved() has not yet been called, meaning
bo->ttm.bdev is NULL. At the top of xe_ttm_bo_destroy(), it computes:

    struct xe_device *xe = ttm_to_xe_device(ttm_bo->bdev);

This yields an invalid pointer (NULL - offsetof(struct xe_device, ttm)).
When the new code unconditionally attempts to lock xe->pinned.lock, it
will dereference this invalid xe pointer and trigger a kernel panic.

Should the error path in ___xe_bo_create() free the buffer object directly
or pass the xe device context explicitly, rather than relying on the
uninitialized bdev?

> +
>  	mutex_lock(&xe->mem_access.vram_userfault.lock);
>  	if (!list_empty(&bo->vram_userfault_link))
>  		list_del(&bo->vram_userfault_link);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261006175741.3582911-1-fnu.vishwanatha@intel.com?part=1