[PATCH v1 1/2] drm/ttm: don't leave bulk_move cursor dangling for unevictable resources

[Samuel Ainsworth]

ttm_resource_add_bulk_move() and ttm_resource_del_bulk_move() both act
only when the resource is evictable (!ttm_resource_unevictable()). A
resource is added to its bo's bulk_move cursor (pos->first / pos->last)
while evictable, but it can become unevictable -- pinned or swapped --
after it has been added.

ttm_resource_del_bulk_move() is reached both when the resource is freed
(ttm_resource_free()) and when the bo's bulk_move is cleared on teardown
(ttm_bo_set_bulk_move()). If the resource has become unevictable by then,
the del is skipped, so pos->first / pos->last are left pointing at it.
Once the resource is freed the cursor dangles, and the next
ttm_resource_add_bulk_move() / ttm_resource_move_to_lru_tail() on that
bulk_move dereferences it: a use-after-free read of
pos->first->bo->base.resv (the WARN_ON in ttm_lru_bulk_move_add())
followed by a list_move() through freed memory that corrupts the LRU
list. With CONFIG_DEBUG_LIST this manifests as a fatal "list_del
corruption" BUG.

On a Framework 13 (AMD Ryzen 7040, gfx1103) this is hit via hibernation:
a buffer object swapped out during hibernate (its resource becomes
unevictable) is later closed after resume (amdgpu_gem_object_close ->
amdgpu_vm_bo_del -> ttm_bo_set_bulk_move()), which skips removing its
resource from the VM's bulk_move cursor; a later GEM allocation on that
cursor then faults. KASAN reports a slab-use-after-free in
ttm_resource_add_bulk_move().

Track whether a resource is actually on the bulk_move cursor with a new
ttm_resource::bulk_move flag, set when it is added, and remove based on
that flag rather than on the resource's current evictability. The del
then always undoes what the add did, regardless of any pin/swap
transition in between.

Fixes: fc5d96670eb2 ("drm/ttm: Move swapped objects off the manager's LRU list")
Cc: stable@xxxxxxxxxxxxxxx
Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/5387
Signed-off-by: Samuel Ainsworth <skainsworth@xxxxxxxxx>
---
 drivers/gpu/drm/ttm/ttm_resource.c | 18 +++++++++++++++---
 include/drm/ttm/ttm_resource.h     |  9 +++++++++
 2 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_resource.c b/drivers/gpu/drm/ttm/ttm_resource.c
index 192fca24f37e..1a031ef151a7 100644
--- a/drivers/gpu/drm/ttm/ttm_resource.c
+++ b/drivers/gpu/drm/ttm/ttm_resource.c
@@ -280,16 +280,27 @@ static bool ttm_resource_unevictable(struct ttm_resource *res, struct ttm_buffer
 void ttm_resource_add_bulk_move(struct ttm_resource *res,
 				struct ttm_buffer_object *bo)
 {
-	if (bo->bulk_move && !ttm_resource_unevictable(res, bo))
+	if (bo->bulk_move && !ttm_resource_unevictable(res, bo)) {
 		ttm_lru_bulk_move_add(bo->bulk_move, res);
+		res->bulk_move = true;
+	}
 }
 
 /* Remove the resource from a bulk move if the BO is configured for it */
 void ttm_resource_del_bulk_move(struct ttm_resource *res,
 				struct ttm_buffer_object *bo)
 {
-	if (bo->bulk_move && !ttm_resource_unevictable(res, bo))
+	/*
+	 * Remove based on whether the resource was actually added, not on its
+	 * current evictability: a resource can become unevictable (pinned or
+	 * swapped) after being added, and must still be taken off the bulk_move
+	 * cursor before it is freed -- otherwise pos->first/last are left
+	 * dangling at freed memory.
+	 */
+	if (res->bulk_move) {
 		ttm_lru_bulk_move_del(bo->bulk_move, res);
+		res->bulk_move = false;
+	}
 }
 
 /* Move a resource to the LRU or bulk tail */
@@ -303,7 +314,7 @@ void ttm_resource_move_to_lru_tail(struct ttm_resource *res)
 	if (ttm_resource_unevictable(res, bo)) {
 		list_move_tail(&res->lru.link, &bdev->unevictable);
 
-	} else if (bo->bulk_move) {
+	} else if (res->bulk_move) {
 		struct ttm_lru_bulk_move_pos *pos =
 			ttm_lru_bulk_move_pos(bo->bulk_move, res);
 
@@ -339,6 +350,7 @@ void ttm_resource_init(struct ttm_buffer_object *bo,
 	res->bus.is_iomem = false;
 	res->bus.caching = ttm_cached;
 	res->bo = bo;
+	res->bulk_move = false;
 
 	man = ttm_manager_type(bo->bdev, place->mem_type);
 	spin_lock(&bo->bdev->lru_lock);
diff --git a/include/drm/ttm/ttm_resource.h b/include/drm/ttm/ttm_resource.h
index 33e80f30b8b8..1fedf75bab96 100644
--- a/include/drm/ttm/ttm_resource.h
+++ b/include/drm/ttm/ttm_resource.h
@@ -274,6 +274,15 @@ struct ttm_resource {
 	 * @lru: Least recently used list, see &ttm_resource_manager.lru
 	 */
 	struct ttm_lru_item lru;
+
+	/**
+	 * @bulk_move: Whether this resource is currently tracked by its bo's
+	 * &ttm_buffer_object.bulk_move cursor. Recorded when the resource is
+	 * added so the matching del removes it even if the resource has since
+	 * become unevictable (pinned or swapped) -- otherwise the cursor would
+	 * be left pointing at this resource after it is freed.
+	 */
+	bool bulk_move;
 };
 
 /**
-- 
2.54.0