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

[Claude Code Review Bot <claude-review@example.com>]

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