Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Critical] Lockless dereference of atomic state pointers (`plane->state` and `crtc->state`) in `find_crtc_index_atomic` can lead to Use-After-Free during concurrent atomic commits. --
commit 789de6442f862dfb55a96bcc0ed2477d4a69bfd2 Author: Thomas Zimmermann <[email protected]> Subject: drm/fb-helper: Only consider active CRTCs for vblank sync This patch modifies the fbdev emulation to only synchronize to the vblank of an active CRTC. It introduces separate lookup paths for atomic and non-atomic drivers to find the correct CRTC index. Link: https://lore.kernel.org/dri-devel/[email protected]/ Closes: https://bugs.debian.org/1138033 > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index 7b11a582f8ec4..cbf0a9a7b8e51 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -225,16 +225,85 @@ static void drm_fb_helper_resume_worker(struct > work_struct *work) [ ... ] > +static int find_crtc_index_atomic(struct drm_fb_helper *helper) > +{ > + struct drm_device *dev = helper->dev; > + struct drm_plane *plane; > + > + drm_for_each_plane(plane, dev) { > + const struct drm_plane_state *plane_state; > + const struct drm_crtc *crtc; > + > + if (plane->type != DRM_PLANE_TYPE_PRIMARY) > + continue; > + > + plane_state = plane->state; > + if (plane_state->fb != helper->fb || !plane_state->crtc) > + continue; /* plane doesn't display fbdev emulation */ [Severity: Critical] Is it safe to access plane->state here without holding the appropriate plane locks? Since dev->mode_config.mutex is held but plane->mutex is not, could a concurrent atomic commit from another DRM master swap and free plane->state while this function is executing? > + > + crtc = plane_state->crtc; > + if (!crtc->state->active) > + continue; [Severity: Critical] If a use-after-free occurs on plane->state, could it lead to reading a fake crtc pointer, dereferencing its state, and returning an arbitrary crtc->index? > + if (drm_WARN_ON_ONCE(dev, crtc->index > INT_MAX)) > + continue; /* driver bug */ > + > + return crtc->index; > + } > + > + return -EINVAL; > +} [ ... ] > static void drm_fb_helper_fb_dirty(struct drm_fb_helper *helper) > { [ ... ] > mutex_lock(&helper->lock); > - drm_client_modeset_wait_for_vblank(&helper->client, 0); > + crtc_index = drm_fb_helper_find_crtc_index(helper); > + if (crtc_index >= 0) > + drm_client_modeset_wait_for_vblank(&helper->client, crtc_index); > mutex_unlock(&helper->lock); [Severity: Critical] Because drm_fb_helper_find_crtc_index() is called before checking if we still own the DRM master (which happens inside drm_client_modeset_wait_for_vblank via drm_master_internal_acquire), could an unprivileged user writing to /dev/fb0 trigger this lookup concurrently with an active userspace compositor's atomic commits, exposing the race condition mentioned above? Furthermore, if an attacker can control the returned crtc_index, could it lead to an out-of-bounds array read in drm_client_modeset_wait_for_vblank() when it does: crtc = client->modesets[crtc_index].crtc; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
