From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm: Fix use-after-free on framebuffers and property blobs when calling drm_dev_unplug Date: Sat, 14 Mar 2026 06:45:34 +1000 Message-ID: In-Reply-To: <20260313151728.14990-4-dev@lankhorst.se> References: <20260313151728.14990-3-dev@lankhorst.se> <20260313151728.14990-4-dev@lankhorst.se> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **The `drm_file_free` change (drm_file.c):** The `drm_dev_enter`/`drm_dev_exit` guard is a reasonable approach to skip `= drm_fb_release()` and `drm_property_destroy_user_blobs()` when the device h= as been unplugged: ```c if (drm_core_check_feature(dev, DRIVER_MODESET) && drm_dev_enter(dev, &idx)) { drm_fb_release(file); drm_property_destroy_user_blobs(dev, file); drm_dev_exit(idx); } ``` However, this raises a **resource leak concern**: if `drm_dev_enter` fails = (device unplugged), we skip both `drm_fb_release` and `drm_property_destroy= _user_blobs` entirely. This means the refcounts held by the file's `fbs` li= st and `blobs` list are never dropped. The patch relies on `drm_mode_config= _cleanup()` to handle cleanup instead, but: 1. For framebuffers: `drm_mode_config_cleanup` calls `drm_framebuffer_free`= which bypasses `kref_put` and directly invokes the free path. This works t= o destroy the objects but is unusual =E2=80=94 it forcibly frees regardless= of refcount. 2. For property blobs: the `property_blob_list` iteration in `drm_mode_conf= ig_cleanup` drops the global list reference, but the per-file `head_file` l= ist reference from `file_priv->blobs` is never cleaned up. If a blob has bo= th a global reference and a per-file reference, the per-file `drm_property_= blob_put` is skipped, potentially leaking the blob entirely since `drm_mode= _config_cleanup` only drops the `head_global` reference. **The `drm_mode_config_cleanup` change (drm_mode_config.c):** ```c if (list_empty(&fb->filp_head) || drm_framebuffer_read_refcount(fb) > 1) { struct drm_printer p =3D drm_dbg_printer(dev, DRM_UT_KMS, "[leaked fb]"); drm_printf(&p, "framebuffer[%u]:\n", fb->base.id); drm_framebuffer_print_info(&p, 1, fb); } list_del_init(&fb->filp_head); drm_framebuffer_free(&fb->base.refcount); ``` Several issues here: 1. **The `list_del_init(&fb->filp_head)` is done to satisfy the `drm_WARN_O= N(!list_empty(&fb->filp_head))` in `drm_framebuffer_free`** (drm_framebuffe= r.c:833). This is correct in intent =E2=80=94 these fbs still have a `filp_= head` because `drm_fb_release` was skipped =E2=80=94 but it papers over the= problem by just detaching the node right before the check. 2. **The diagnostic condition is inverted in meaning**: the original code u= nconditionally printed leaked fb info. The new condition `list_empty(&fb->f= ilp_head) || refcount > 1` prints the warning only for fbs that either have= no file association (truly leaked) or have extra references. But fbs that = are on the `filp_head` list with refcount =3D=3D 1 (the normal unplug case)= are silently freed. This seems intentional but the logic is a bit subtle a= nd deserves a comment. 3. **Calling `drm_framebuffer_free(&fb->base.refcount)` directly** bypasses= `kref_put`, so it doesn't check whether the refcount is actually zero. If = any other code path still holds a reference (e.g. an in-flight atomic commi= t), this will cause a double-free. The original code had the same pattern, = so this isn't a regression, but it's worth noting. 4. **No corresponding fix for property blobs**: The cover letter and commit= message mention property blob crashes, and the `drm_file_free` change skip= s `drm_property_destroy_user_blobs`, but there's no change to the blob clea= nup path in `drm_mode_config_cleanup`. The blobs on `file_priv->blobs` are = never cleaned up =E2=80=94 their `head_file` links still point into the (ab= out-to-be-freed) file structure. This seems like an incomplete fix. The blo= b list iteration at `drm_mode_config.c:571` walks `head_global`, so it won'= t encounter the per-file references, meaning blobs created by the user coul= d leak. **Missing `Fixes:` tag**: The cover letter asks whether this should have a = fixes tag. Yes =E2=80=94 this is clearly a bug fix for a crash/oops, so a `= Fixes:` tag pointing to the commit that introduced `drm_dev_unplug` (or the= relevant mode_config cleanup) and `Cc: stable@vger.kernel.org` would be ap= propriate. **Summary of concerns:** - The property blob cleanup appears incomplete =E2=80=94 skipping `drm_prop= erty_destroy_user_blobs` in `drm_file_free` without compensating in `drm_mo= de_config_cleanup` may leak blobs - The approach of directly calling `drm_framebuffer_free` (bypassing kref) = is fragile if any other references exist - A `Fixes:` tag and `Cc: stable` should be added - The conditional leak-warning logic in `drm_mode_config_cleanup` could use= a brief comment explaining the reasoning --- Generated by Claude Code Patch Reviewer