[PATCH] drm/panthor: Don't access gpu_contexts of a destroyed heap pool

[PATCH] drm/panthor: Don't access gpu_contexts of a destroyed heap pool

[Jiwoo Kim]

panthor_heap_grow() and panthor_heap_return_chunk() compute the heap
context offset from pool->gpu_contexts before taking pool->lock.

The tiler OOM work holds references on the group, its VM and the heap
pool when it calls these functions, but nothing stops
panthor_vm_destroy() from destroying the pool in the meantime.
panthor_heap_pool_destroy() frees the gpu_contexts BO under pool->lock
and marks the pool as destroyed by clearing pool->vm, but leaves
pool->gpu_contexts pointing to the freed BO. VM destruction doesn't
wait for the work either: panthor_sched_prepare_for_vm_destruction()
only flushes the scheduler tick work, and only when the VM is active,
and evicting the group doesn't help, as group_unbind_locked() uses
cancel_work(), which doesn't wait for a tiler OOM work that is already
running. Such a work can then read the freed BO.

Take pool->lock before dereferencing pool->gpu_contexts, and return
-EINVAL if the pool has been destroyed, as panthor_heap_create()
already does.

The problem was found through AI-assisted source code review. The fix
is compile-tested only and there is no reproducer, as I don't have
Mali CSF hardware.

Fixes: 9cca48fa4f89 ("drm/panthor: Add the heap logical block")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Jiwoo Kim <arpe1618@gmail.com>
---

Notes:
    This conflicts trivially with Osama Abdelkader's pending "drm/panthor:
    Fix race when retrieving the new chunk in panthor_heap_grow()", as both
    change the local variables at the top of panthor_heap_grow(). I can
    rebase on top of it if it lands first.
    
    While looking at this, I also noticed that panthor_heap_create() reads
    pool->vm without pool->lock when allocating the initial chunks, in
    panthor_alloc_heap_chunk(), so a concurrent VM destroy can make
    panthor_kernel_bo_create() hit its drm_WARN_ON(!vm). That doesn't touch
    freed memory and is left for a separate patch.

 drivers/gpu/drm/panthor/panthor_heap.c | 44 +++++++++++++++++++-------
 1 file changed, 33 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_heap.c b/drivers/gpu/drm/panthor/panthor_heap.c
index 99311abdf1..e55f7e648d 100644
--- a/drivers/gpu/drm/panthor/panthor_heap.c
+++ b/drivers/gpu/drm/panthor/panthor_heap.c
@@ -90,7 +90,7 @@ struct panthor_heap_pool {
 	/** @vm: VM this pool is bound to. */
 	struct panthor_vm *vm;
 
-	/** @lock: Lock protecting access to @xa. */
+	/** @lock: Lock protecting access to @xa, @vm and @gpu_contexts. */
 	struct rw_semaphore lock;
 
 	/** @xa: Array storing panthor_heap objects. */
@@ -371,16 +371,27 @@ int panthor_heap_return_chunk(struct panthor_heap_pool *pool,
 			      u64 heap_gpu_va,
 			      u64 chunk_gpu_va)
 {
-	u64 offset = heap_gpu_va - panthor_kernel_bo_gpuva(pool->gpu_contexts);
-	u32 heap_id = (u32)offset / panthor_heap_ctx_stride(pool->ptdev);
 	struct panthor_heap_chunk *chunk, *tmp, *removed = NULL;
 	struct panthor_heap *heap;
+	u32 heap_id;
+	u64 offset;
 	int ret;
 
-	if (offset > U32_MAX || heap_id >= MAX_HEAPS_PER_POOL)
-		return -EINVAL;
-
 	down_read(&pool->lock);
+
+	/* The heap contexts are gone if the pool has been destroyed. */
+	if (!pool->vm) {
+		ret = -EINVAL;
+		goto out_unlock;
+	}
+
+	offset = heap_gpu_va - panthor_kernel_bo_gpuva(pool->gpu_contexts);
+	heap_id = (u32)offset / panthor_heap_ctx_stride(pool->ptdev);
+	if (offset > U32_MAX || heap_id >= MAX_HEAPS_PER_POOL) {
+		ret = -EINVAL;
+		goto out_unlock;
+	}
+
 	heap = xa_load(&pool->xa, heap_id);
 	if (!heap) {
 		ret = -EINVAL;
@@ -435,16 +446,27 @@ int panthor_heap_grow(struct panthor_heap_pool *pool,
 		      u32 pending_frag_count,
 		      u64 *new_chunk_gpu_va)
 {
-	u64 offset = heap_gpu_va - panthor_kernel_bo_gpuva(pool->gpu_contexts);
-	u32 heap_id = (u32)offset / panthor_heap_ctx_stride(pool->ptdev);
 	struct panthor_heap_chunk *chunk;
 	struct panthor_heap *heap;
+	u32 heap_id;
+	u64 offset;
 	int ret;
 
-	if (offset > U32_MAX || heap_id >= MAX_HEAPS_PER_POOL)
-		return -EINVAL;
-
 	down_read(&pool->lock);
+
+	/* The heap contexts are gone if the pool has been destroyed. */
+	if (!pool->vm) {
+		ret = -EINVAL;
+		goto out_unlock;
+	}
+
+	offset = heap_gpu_va - panthor_kernel_bo_gpuva(pool->gpu_contexts);
+	heap_id = (u32)offset / panthor_heap_ctx_stride(pool->ptdev);
+	if (offset > U32_MAX || heap_id >= MAX_HEAPS_PER_POOL) {
+		ret = -EINVAL;
+		goto out_unlock;
+	}
+
 	heap = xa_load(&pool->xa, heap_id);
 	if (!heap) {
 		ret = -EINVAL;

base-commit: bafda823590ddf7031ecb765d673572fae7ab44a
-- 
2.43.0