From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: accel/rocket: Fix drm_mm UAF on close vs in-flight job Date: Tue, 28 Apr 2026 15:18:19 +1000 Message-ID: In-Reply-To: <20260426103758.1373137-1-gye976@gmail.com> References: <20260426103758.1373137-1-gye976@gmail.com> <20260426103758.1373137-1-gye976@gmail.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Positive aspects:** 1. The commit message is excellent =E2=80=94 it includes the KASAN splat, c= learly explains the root cause and the fix approach, and has a proper `Fixe= s:` tag. 2. The lifetime model is now correct: `rocket_vm` is kref-managed, and both= `rocket_gem_object` (via `bo->vm`) and `rocket_job` (via `job->vm`) hold r= eferences, so the `drm_mm` stays alive as long as any BO or job needs it. 3. The `rocket_vm_destroy()` correctly tears down in the right order =E2=80= =94 `drm_mm_takedown`, `mutex_destroy`, then `iommu_domain_free`: ```c drm_mm_takedown(&vm->mm); mutex_destroy(&vm->lock); iommu_domain_free(vm->domain); vm->domain =3D NULL; kfree(vm); ``` 4. Initializing `drm_mm` inside `rocket_vm_create()` from the IOMMU geometr= y aperture is cleaner than the previous approach in `rocket_open()`: ```c start =3D vm->domain->geometry.aperture_start; end =3D vm->domain->geometry.aperture_end; drm_mm_init(&vm->mm, start, end - start + 1); ``` **Issues:** 1. **Bug (pre-existing, carried forward): Missing error check on `drm_mm_in= sert_node_generic`** =E2=80=94 In `rocket_gem.c:94-98`, the return value fr= om `drm_mm_insert_node_generic()` is assigned to `ret`, but it is immediate= ly overwritten by the `iommu_map_sgtable()` call on line 100 without checki= ng for failure: ```c mutex_lock(&vm->lock); ret =3D drm_mm_insert_node_generic(&vm->mm, &rkt_obj->mm, rkt_obj->size, PAGE_SIZE, 0, 0); mutex_unlock(&vm->lock); ret =3D iommu_map_sgtable(vm->domain, rkt_obj->mm.start, shmem_obj->sgt, IOMMU_READ | IOMMU_WRITE); ``` If `drm_mm_insert_node_generic()` fails, `rkt_obj->mm` is not allocated, ye= t `iommu_map_sgtable()` will use `rkt_obj->mm.start` (which is uninitialize= d/zero) and the `err_remove_node` path will try to remove a node that was n= ever inserted. This is a pre-existing bug, but since this patch is reworkin= g the function, it should add the missing check: ```c if (ret) goto err; ``` 2. **Bug (pre-existing, carried forward): Missing `rocket_vm_put` in the `e= rr` path of `rocket_ioctl_create_bo`** =E2=80=94 At line 78-79, a `rocket_v= m_get()` reference is taken and stored in `rkt_obj->vm`: ```c vm =3D rocket_vm_get(rocket_priv); rkt_obj->vm =3D vm; ``` If any subsequent step fails and we jump to `err:`, the code calls `drm_gem= _shmem_object_free(gem_obj)` =E2=80=94 but that calls `rocket_gem_bo_free()= ` only if the GEM object's free callback is wired up, and the BO destructor= path already calls `rocket_vm_put()`. However, looking more carefully: aft= er `drm_gem_handle_create()` + `drm_gem_object_put()` on line 83-84, the dr= iver drops its reference. If `drm_gem_handle_create()` succeeded but a late= r step fails, the GEM handle still exists, and jumping to `err:` calls `drm= _gem_shmem_object_free()` without first closing the handle =E2=80=94 this l= eaks the GEM handle. This is also a pre-existing bug but worth fixing in th= is series. 3. **Minor: `void *err` type in `rocket_vm_create`** =E2=80=94 The error re= turn variable is declared as `void *err` rather than the more typical `stru= ct rocket_vm *err` or just using `ERR_PTR` inline. This is carried forward = from the original code but is slightly unusual =E2=80=94 no functional impa= ct. 4. **Nit: `vm->domain =3D NULL` before `kfree(vm)`** =E2=80=94 In `rocket_v= m_destroy()`, setting `vm->domain =3D NULL` just before `kfree(vm)` is poin= tless since the memory is about to be freed. This was also present in the o= riginal code. **Summary**: The core fix is sound and addresses the reported UAF correctly= . The `drm_mm_insert_node_generic` missing error check should be fixed =E2= =80=94 either in this patch or as a clearly-marked follow-up =E2=80=94 sinc= e it can cause further UAFs and undefined behavior on the new `vm->mm`. The= other issues are lower priority. --- Generated by Claude Code Patch Reviewer