From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: dma-buf: dma-heap: split dma_heap_add Date: Mon, 09 Mar 2026 09:02:39 +1000 Message-ID: In-Reply-To: <20260306-b4-dmabuf-heap-coherent-rmem-v3-2-3d00d36c9bc4@redhat.com> References: <20260306-b4-dmabuf-heap-coherent-rmem-v3-0-3d00d36c9bc4@redhat.com> <20260306-b4-dmabuf-heap-coherent-rmem-v3-2-3d00d36c9bc4@redhat.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 This is the core refactoring, splitting `dma_heap_add()` into `dma_heap_cre= ate()` / `dma_heap_register()` / `dma_heap_destroy()`. **Issue: `heap->name` is not duplicated =E2=80=94 double-free risk** In `dma_heap_create()`: ```c heap->name =3D exp_info->name; ``` The name is stored as a raw pointer from `exp_info`. But `dma_heap_dev_rele= ase()` does: ```c kfree(heap->name); ``` If the caller passed a string literal or stack-allocated name (which `dma_h= eap_add()` callers like system_heap and cma_heap do with string literals), = this `kfree()` will corrupt memory. Looking at the coherent heap (patch 5),= `coh_heap->name =3D kstrdup(rmem->name, GFP_KERNEL)` is passed in, so it's= safe for that caller. But *any existing caller* of `dma_heap_add()` will n= ow have their name kfree'd in the release path. The original code (pre-seri= es) never freed `heap->name` =E2=80=94 the heap struct was freed directly i= n the error path, and heaps were never removed. This is a **regression for = existing callers**. The `dma_heap_create()` should `kstrdup()` the name, or `dma_heap_dev_relea= se()` should not free it. **Issue: `dma_heap_register()` error path calls `dma_heap_destroy()` =E2=80= =94 double destroy possible** In `dma_heap_register()` error paths, `err0` calls `dma_heap_destroy(heap)`: ```c err0: dma_heap_destroy(heap); return ret; ``` And `dma_heap_add()` on register failure also calls: ```c ret =3D dma_heap_register(heap); if (ret) { pr_err("dma_heap: failed to register heap (%d)\n", ret); dma_heap_destroy(heap); return ERR_PTR(ret); } ``` So `dma_heap_destroy()` (which is `put_device()`) is called **twice** =E2= =80=94 once inside `dma_heap_register()` and once in `dma_heap_add()`. Afte= r `device_initialize()`, the refcount is 1. The first `put_device()` drops = it to 0 and triggers the release callback, freeing the heap. The second `pu= t_device()` is a UAF. This is a **bug**. **Issue: `err3` path does `device_destroy()` then falls through to `dma_hea= p_destroy()`** ```c err3: device_destroy(dma_heap_class, heap->heap_devt); err2: cdev_del(&heap->heap_cdev); err1: xa_erase(&dma_heap_minors, minor); err0: dma_heap_destroy(heap); ``` After `device_add()` succeeds, `err3` calls `device_destroy()` which does `= device_unregister()` =E2=86=92 `device_del()` + `put_device()`. Then execut= ion falls through to `err0` which calls `dma_heap_destroy()` =E2=86=92 `put= _device()` again. Since `device_initialize()` sets refcount to 1 and `devic= e_add()` doesn't bump it, the `device_destroy()`'s `put_device()` drops to = 0 and frees, then `dma_heap_destroy()` is UAF. This needs restructuring. **Minor: `kzalloc_obj` usage** ```c heap->heap_dev =3D kzalloc_obj(*heap->heap_dev); ``` This appears to be a custom macro. It works but is unconventional =E2=80=94= `kzalloc(sizeof(*heap->heap_dev), GFP_KERNEL)` is the standard pattern. --- --- Generated by Claude Code Patch Reviewer