[PATCH] accel/rocket: fix UAF via dangling GEM handle in create_bo

[PATCH] accel/rocket: fix UAF via dangling GEM handle in create_bo

[Tomeu Vizoso]

From: Dhabaleshwar Das <dhabal123@gmail.com>

rocket_ioctl_create_bo() inserts a GEM handle into the file's IDR via
drm_gem_handle_create() early on, then performs several operations that
can fail (sgt allocation, drm_mm insert, iommu_map). If any fail after
the handle is live, the error path calls drm_gem_shmem_object_free()
which kfree's the object without removing the handle from the IDR.

This leaves a dangling handle pointing to freed slab memory. Any
subsequent ioctl using that handle (PREP_BO, FINI_BO, SUBMIT) calls
drm_gem_object_lookup() and dereferences freed memory (UAF).

Fix by moving drm_gem_handle_create() to after all fallible operations
succeed, matching the pattern used by panfrost, lima, and etnaviv.

Also fix drm_mm_insert_node_generic() whose return value was silently
overwritten by iommu_map_sgtable() on the next line. Add the missing
error check.

[tomeu: Move handle creation to the very end]

Fixes: 658ebeac3351 ("accel/rocket: Add IOCTL for BO creation")
Reported-by: Dhabaleshwar Das <dhabal123@gmail.com>
Signed-off-by: Dhabaleshwar Das <dhabal123@gmail.com>
Reviewed-by: Tomeu Vizoso <tomeu@tomeuvizoso.net>
Signed-off-by: Tomeu Vizoso <tomeu@tomeuvizoso.net>
---
 drivers/accel/rocket/rocket_gem.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/accel/rocket/rocket_gem.c b/drivers/accel/rocket/rocket_gem.c
index c8084719208a..a5fffa51ff35 100644
--- a/drivers/accel/rocket/rocket_gem.c
+++ b/drivers/accel/rocket/rocket_gem.c
@@ -79,11 +79,6 @@ int rocket_ioctl_create_bo(struct drm_device *dev, void *data, struct drm_file *
 	rkt_obj->size = args->size;
 	rkt_obj->offset = 0;
 
-	ret = drm_gem_handle_create(file, gem_obj, &args->handle);
-	drm_gem_object_put(gem_obj);
-	if (ret)
-		goto err;
-
 	sgt = drm_gem_shmem_get_pages_sgt(shmem_obj);
 	if (IS_ERR(sgt)) {
 		ret = PTR_ERR(sgt);
@@ -95,6 +90,8 @@ int rocket_ioctl_create_bo(struct drm_device *dev, void *data, struct drm_file *
 					 rkt_obj->size, PAGE_SIZE,
 					 0, 0);
 	mutex_unlock(&rocket_priv->mm_lock);
+	if (ret)
+		goto err;
 
 	ret = iommu_map_sgtable(rocket_priv->domain->domain,
 				rkt_obj->mm.start,
@@ -112,8 +109,18 @@ int rocket_ioctl_create_bo(struct drm_device *dev, void *data, struct drm_file *
 	args->offset = drm_vma_node_offset_addr(&gem_obj->vma_node);
 	args->dma_address = rkt_obj->mm.start;
 
+	ret = drm_gem_handle_create(file, gem_obj, &args->handle);
+	if (ret)
+		goto err_unmap;
+
+	drm_gem_object_put(gem_obj);
+
 	return 0;
 
+err_unmap:
+	iommu_unmap(rocket_priv->domain->domain,
+		    rkt_obj->mm.start, rkt_obj->size);
+
 err_remove_node:
 	mutex_lock(&rocket_priv->mm_lock);
 	drm_mm_remove_node(&rkt_obj->mm);
-- 
2.54.0

Claude review: accel/rocket: fix UAF via dangling GEM handle in create_bo

[Claude Code Review Bot]

Overall Series Review

Subject: accel/rocket: fix UAF via dangling GEM handle in create_bo
Author: Tomeu Vizoso <tomeu.vizoso@gmail.com>
Patches: 1
Reviewed: 2026-05-25T19:45:17.712730

---

This is a single-patch security fix for a use-after-free vulnerability in the Rocket accelerator's GEM buffer object creation ioctl. The fix is correct and follows the standard DRM driver pattern (panfrost, lima, etnaviv) of deferring handle creation until all fallible operations succeed. The patch also catches a previously missing error check for `drm_mm_insert_node_generic()`. One pre-existing resource leak (IOMMU domain reference) remains unaddressed.

**Verdict: The patch should be accepted, with one pre-existing bug noted for follow-up.**

---

---
Generated by Claude Code Patch Reviewer

Claude review: accel/rocket: fix UAF via dangling GEM handle in create_bo

[Claude Code Review Bot]

Patch Review

**Bug analysis is correct.** In the original code, `drm_gem_handle_create()` was called early, exposing the handle to userspace. After `drm_gem_object_put()` drops the creation reference, the handle's reference is the sole reference. If a subsequent operation (`drm_gem_shmem_get_pages_sgt`, `drm_mm_insert_node_generic`, `iommu_map_sgtable`) fails, the `err:` label calls `drm_gem_shmem_object_free()` which kfree's the object without removing the handle from the file's IDR — classic dangling-pointer UAF.

**The fix is correct.** Moving `drm_gem_handle_create()` to line 112 (after all fallible operations) ensures the handle is never exposed to userspace if any setup step fails:

```c
	ret = drm_gem_handle_create(file, gem_obj, &args->handle);
	if (ret)
		goto err_unmap;

	drm_gem_object_put(gem_obj);

	return 0;
```

**The new error label cascade is correct** — reverse-order cleanup:

```c
err_unmap:
	iommu_unmap(rocket_priv->domain->domain,
		    rkt_obj->mm.start, rkt_obj->size);

err_remove_node:
	mutex_lock(&rocket_priv->mm_lock);
	drm_mm_remove_node(&rkt_obj->mm);
	mutex_unlock(&rocket_priv->mm_lock);

err:
	drm_gem_shmem_object_free(gem_obj);
```

`err_unmap` correctly uses `rkt_obj->size` which was updated to the actual mapped size from `iommu_map_sgtable()` at line 108 (`rkt_obj->size = ret`), so the unmap size matches what was mapped.

**The missing `drm_mm_insert_node_generic()` error check is a real bug.** Previously the return value was silently overwritten by `iommu_map_sgtable()` on the next line. If the insert failed, `rkt_obj->mm` would be uninitialized and the subsequent `iommu_map_sgtable()` would use `rkt_obj->mm.start` as a garbage address. Adding the check is important:

```c
	mutex_unlock(&rocket_priv->mm_lock);
	if (ret)
		goto err;
```

**One issue — pre-existing IOMMU domain reference leak (not introduced by this patch):**

At line 78, `rocket_iommu_domain_get()` acquires a reference on the domain:

```c
	rkt_obj->domain = rocket_iommu_domain_get(rocket_priv);
```

But all error paths reach `err:` which calls `drm_gem_shmem_object_free(gem_obj)`. This function calls `drm_gem_shmem_free()` directly — it does **not** invoke the object's `funcs->free` callback (`rocket_gem_bo_free`). So the matching `rocket_iommu_domain_put()` that lives in `rocket_gem_bo_free()` is never called on any error path. This leaks the domain reference on every failed `create_bo` call.

The fix would be to add `rocket_iommu_domain_put(rkt_obj->domain)` to the `err:` label before calling `drm_gem_shmem_object_free()`, or alternatively use `drm_gem_object_put()` in the error path (which would go through `rocket_gem_bo_free` — though that path unconditionally calls `iommu_unmap` and `drm_mm_remove_node`, which would be problematic if those operations hadn't succeeded yet).

The cleanest approach would be to add an intermediate error label:

```c
err:
	rocket_iommu_domain_put(rkt_obj->domain);
	drm_gem_shmem_object_free(gem_obj);
```

This is a pre-existing bug, not introduced by this patch, but since the author is already reworking the error paths in this function, it would be good to fix it in the same series.

**Summary:** The patch correctly fixes the UAF and the missing error check. The error path structure is sound. Recommend accepting as-is with a follow-up to address the IOMMU domain reference leak.

---
Generated by Claude Code Patch Reviewer