public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH v2] drm/panthor: Avoid potential UAF due to memory reclaim
@ 2026-04-13  8:02 Akash Goel
  2026-04-13  8:43 ` Claude review: " Claude Code Review Bot
  2026-04-13  8:43 ` Claude Code Review Bot
  0 siblings, 2 replies; 3+ messages in thread
From: Akash Goel @ 2026-04-13  8:02 UTC (permalink / raw)
  To: boris.brezillon, liviu.dudau, steven.price
  Cc: dri-devel, linux-kernel, maarten.lankhorst, mripard, tzimmermann,
	airlied, daniel, nd, Akash Goel

Recent changes to add shrinker support introduced a use after free
vulnerability.
When a BO is evicted from the shrinker callback, all its CPU and GPU
mappings are invalidated. It can happen that another GPU mapping is
created for the BO after the eviction. Because of the new GPU mapping,
BO will be added back to one of the reclaim list but the state of
corresponding vm_bo will not be changed.
If vm_bo remains in evicted state and shrinker callback is invoked
again then the new GPU mapping won't be invalidated. As a result the
backing pages, which were acquired on the creation of new GPU mapping,
can get reclaimed and reused whilst they are still mapped to the GPU.

To prevent the use after free possibility, this commit removes the
evicted check for vm_bo so that all GPU mappings are checked for
invalidation.

v2:
 - Update comment and add a newline in
   panthor_vm_evict_bo_mappings_locked().

Fixes: fb42964e2a76 ("drm/panthor: Add a GEM shrinker")
Suggested-by: Boris Brezillon <boris.brezillon@collabora.com>
Signed-off-by: Akash Goel <akash.goel@arm.com>
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/gpu/drm/panthor/panthor_mmu.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
index fa8b31df85c9..592a6829bee6 100644
--- a/drivers/gpu/drm/panthor/panthor_mmu.c
+++ b/drivers/gpu/drm/panthor/panthor_mmu.c
@@ -2350,14 +2350,20 @@ int panthor_vm_evict_bo_mappings_locked(struct panthor_gem_object *bo)
 		struct panthor_vm *vm = container_of(vm_bo->vm, struct panthor_vm, base);
 		struct drm_gpuva *va;
 
-		/* Skip already evicted GPU mappings. */
-		if (vm_bo->evicted)
-			continue;
-
 		if (!mutex_trylock(&vm->op_lock))
 			return -EDEADLK;
 
-		drm_gpuvm_bo_evict(vm_bo, true);
+		/* It can be that the vm_bo was already evicted but a new
+		 * mapping pointing to this BO got created in the meantime,
+		 * thus turning the vm_bo in partially evicted state. In that case
+		 * we don't call drm_gpuvm_bo_evict() again because this would
+		 * mess up with the internal gpuvm lists, but we do walk the
+		 * VAs on this vm_bo to make sure the non-evicted ones are
+		 * torn down.
+		 */
+		if (!vm_bo->evicted)
+			drm_gpuvm_bo_evict(vm_bo, true);
+
 		drm_gpuvm_bo_for_each_va(va, vm_bo) {
 			struct panthor_vma *vma = container_of(va, struct panthor_vma, base);
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Claude review: drm/panthor: Avoid potential UAF due to memory reclaim
  2026-04-13  8:02 [PATCH v2] drm/panthor: Avoid potential UAF due to memory reclaim Akash Goel
@ 2026-04-13  8:43 ` Claude Code Review Bot
  2026-04-13  8:43 ` Claude Code Review Bot
  1 sibling, 0 replies; 3+ messages in thread
From: Claude Code Review Bot @ 2026-04-13  8:43 UTC (permalink / raw)
  To: dri-devel-reviews

Overall Series Review

Subject: drm/panthor: Avoid potential UAF due to memory reclaim
Author: Akash Goel <akash.goel@arm.com>
Patches: 1
Reviewed: 2026-04-13T18:43:07.425815

---

This is a single-patch fix for a use-after-free vulnerability in the panthor GPU driver's shrinker path. The bug was introduced by the GEM shrinker support (commit fb42964e2a76). The fix is minimal, well-targeted, and already carries a Reviewed-by from Boris Brezillon (the panthor co-maintainer who also suggested the approach).

**Assessment: The patch looks correct and ready to merge.** It addresses a real security-relevant bug with the smallest reasonable change. The logic is sound and the commit message clearly explains both the problem and the solution.

---

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Claude review: drm/panthor: Avoid potential UAF due to memory reclaim
  2026-04-13  8:02 [PATCH v2] drm/panthor: Avoid potential UAF due to memory reclaim Akash Goel
  2026-04-13  8:43 ` Claude review: " Claude Code Review Bot
@ 2026-04-13  8:43 ` Claude Code Review Bot
  1 sibling, 0 replies; 3+ messages in thread
From: Claude Code Review Bot @ 2026-04-13  8:43 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**The Bug:**
When the shrinker evicts a BO, all CPU and GPU mappings are invalidated and `vm_bo->evicted` is set to `true`. If a *new* GPU mapping is subsequently created for that BO, the `vm_bo` ends up in a "partially evicted" state — it's back on a reclaim list but `vm_bo->evicted` is still `true`. On the next shrinker invocation, the old code would skip this `vm_bo` entirely:

```c
/* Skip already evicted GPU mappings. */
if (vm_bo->evicted)
    continue;
```

This means the newly created mapping is never torn down, so its backing pages can be reclaimed and reused while still mapped to the GPU — a classic use-after-free.

**The Fix:**
The patch replaces the early `continue` with a conditional guard on `drm_gpuvm_bo_evict()`, while always walking the VAs to tear down any non-evicted mappings:

```c
if (!vm_bo->evicted)
    drm_gpuvm_bo_evict(vm_bo, true);

drm_gpuvm_bo_for_each_va(va, vm_bo) {
```

This is correct — it ensures every VA on the `vm_bo` is checked for invalidation regardless of the evicted flag.

**Minor observation on the comment:** The comment states that calling `drm_gpuvm_bo_evict()` again "would mess up with the internal gpuvm lists." Looking at the underlying `__drm_gpuvm_bo_list_add()` in `drivers/gpu/drm/drm_gpuvm.c`:

```c
static void
__drm_gpuvm_bo_list_add(struct drm_gpuvm *gpuvm, spinlock_t *lock,
			struct list_head *entry, struct list_head *list)
{
	cond_spin_lock(lock, !!lock);
	if (list_empty(entry))
		list_add_tail(entry, list);
	cond_spin_unlock(lock, !!lock);
}
```

The `list_empty()` guard actually protects against double-adds, so a duplicate `drm_gpuvm_bo_evict(vm_bo, true)` call wouldn't corrupt the list — it would just be a redundant no-op. The guard `if (!vm_bo->evicted)` is still good practice (avoids pointless locking and makes the intent clear), but the comment slightly overstates the danger. This is very minor and not worth holding up the patch.

**Verdict:** The fix is correct, minimal, has proper `Fixes:` tag targeting the right commit, and is already reviewed by the relevant maintainer. No issues blocking merge.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2026-04-13  8:43 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-04-13  8:02 [PATCH v2] drm/panthor: Avoid potential UAF due to memory reclaim Akash Goel
2026-04-13  8:43 ` Claude review: " Claude Code Review Bot
2026-04-13  8:43 ` Claude Code Review Bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox