Re: [PATCH v2] drm/fb-helper: Only consider active CRTCs for vblank sync

[sashiko-bot@kernel.org]

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 <tzimmermann@suse.de>
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/1c9e0e24-9c4a-4259-8700-cf9e5fd60ca3@suse.de/
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/20260622113434.682292-1-tzimmermann@suse.de?part=1