From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: accel/rocket: fix UAF via dangling GEM handle in create_bo
Date: Mon, 25 May 2026 19:45:17 +1000
Message-ID: <review-patch1-20260521165720.2113571-1-tomeu@tomeuvizoso.net>
In-Reply-To: <20260521165720.2113571-1-tomeu@tomeuvizoso.net>
References: <20260521165720.2113571-1-tomeu@tomeuvizoso.net> <20260521165720.2113571-1-tomeu@tomeuvizoso.net>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

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 re=
ference. If a subsequent operation (`drm_gem_shmem_get_pages_sgt`, `drm_mm_=
insert_node_generic`, `iommu_map_sgtable`) fails, the `err:` label calls `d=
rm_gem_shmem_object_free()` which kfree's the object without removing the h=
andle from the file's IDR =E2=80=94 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 =3D 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** =E2=80=94 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 =3D ret`=
), so the unmap size matches what was mapped.

**The missing `drm_mm_insert_node_generic()` error check is a real bug.** P=
reviously the return value was silently overwritten by `iommu_map_sgtable()=
` on the next line. If the insert failed, `rkt_obj->mm` would be uninitiali=
zed 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 =E2=80=94 pre-existing IOMMU domain reference leak (not introdu=
ced by this patch):**

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

```c
	rkt_obj->domain =3D 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 =E2=80=94 it do=
es **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 `rocke=
t_gem_bo_free` =E2=80=94 though that path unconditionally calls `iommu_unma=
p` 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 aut=
hor 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