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: Fri, 27 Feb 2026 13:40:53 +1000 Message-ID: In-Reply-To: <20260225101532.13260-4-dev@lankhorst.se> References: <20260225101532.13260-3-dev@lankhorst.se> <20260225101532.13260-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 **Positive aspects:** - The bug report with stack traces is excellent and makes the problem clear. - The SRCU-based argument for safety between `drm_file_free()` and the unpl= ug path is sound: after `synchronize_srcu()`, no new `drm_dev_enter()` can = succeed, so the guarded block in `drm_file_free()` can't run concurrently w= ith `drm_modeset_unplug_all()`. **Issue 1 (Medium): Missing locking in `drm_framebuffer_unplug()` and `drm_= property_blob_unplug()`** `drm_framebuffer_unplug()` iterates `dev->mode_config.fb_list` without hold= ing `fb_lock`, and does `list_del_init(&fb->filp_head)` without holding the= owning `file_priv->fbs_lock`: ```c void drm_framebuffer_unplug(struct drm_device *dev) { struct drm_framebuffer *fb, *next; list_for_each_entry_safe(fb, next, &dev->mode_config.fb_list, head) { if (!list_empty(&fb->filp_head)) { list_del_init(&fb->filp_head); drm_framebuffer_put(fb); } } } ``` While the SRCU synchronization prevents concurrent `drm_fb_release()` / `dr= m_property_destroy_user_blobs()` via `drm_file_free()`, it does **not** pre= vent concurrent ioctls. `drm_dev_is_unplugged()` in `drm_ioctl()` uses `drm= _dev_enter()`/`drm_dev_exit()` but exits the SRCU read section immediately = =E2=80=94 it doesn't protect the full ioctl duration. So an in-flight `drm_= mode_addfb2()` could be doing `list_add(&fb->filp_head, &file_priv->fbs)` u= nder `fbs_lock` while `drm_framebuffer_unplug()` concurrently does `list_de= l_init(&fb->filp_head)` on the same per-file list without `fbs_lock`. Simil= arly, `drm_framebuffer_init()` modifies `fb_list` under `fb_lock` while `dr= m_framebuffer_unplug()` iterates it without that lock. The same concern applies to `drm_property_blob_unplug()` which iterates `pr= operty_blob_list` and modifies `head_file` without holding `blob_lock`. At minimum, `drm_framebuffer_unplug()` should hold `fb_lock` while iteratin= g `fb_list`, and `drm_property_blob_unplug()` should hold `blob_lock`. The = `filp_head` / `head_file` manipulation is trickier since the per-file lock = isn't easily accessible when iterating by device, but at least the global l= ist iteration should be protected. **Issue 2 (Minor): `drm_framebuffer_put()` may call driver destroy during u= nplug** If the file reference is the last reference on a framebuffer, `drm_framebuf= fer_put()` will call `fb->funcs->destroy()`, which invokes driver-specific = cleanup (e.g., `intel_user_framebuffer_destroy()`). At unplug time, driver = state may be partially torn down. The comment says: ``` * There is no need to call drm_framebuffer_remove, as the driver * is now gone and will perform it's own cleanup of framebuffers. ``` This seems to assume the put won't be the last reference, but that's not gu= aranteed for fbs that aren't actively displayed. It would be worth verifyin= g that the driver's destroy callback is safe to call during unplug. **Issue 3 (Nit): Typo in comment** ```c * is now gone and will perform it's own cleanup of framebuffers. ``` Should be "its own" (possessive), not "it's" (contraction of "it is"). **Issue 4 (Nit): Trailing blank line** ```c void drm_modeset_unplug_all(struct drm_device *dev) { /* With drm_dev_enter() now failing, it's safe to unplug all file_priv->fb= 's and user mode blobs */ drm_framebuffer_unplug(dev); drm_property_blob_unplug(dev); } ``` There's a spurious blank line before the closing brace. **Issue 5 (Question): Fixes tag / Cc stable** The author asks whether this needs a Fixes tag or Cc stable. Given this is = a UAF crash triggered by a real workflow (module reload with running compos= itor), it should have both. The Fixes tag should reference the commit that = introduced `drm_dev_unplug()` (likely `3f04e0a6` or similar), and `Cc: stab= le@vger.kernel.org` would be appropriate since it's fixing a crash. **Summary:** The overall approach is correct and addresses a real bug. The = main concern is the missing locking in the new unplug helpers =E2=80=94 at = minimum the global list locks (`fb_lock`, `blob_lock`) should be held while= iterating. The ioctl race window is narrow but should be acknowledged, and= the author should consider whether there's a better synchronization point = (or at least document why the race is benign). --- Generated by Claude Code Patch Reviewer