Re: [PATCH v9 09/10] drm/atomic-uapi: Add DRM_MODE_ATOMIC_RESET flag

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] drm/atomic: unlocked plane->state->crtc read on unleased planes
  on reset
- [Medium] drm/atomic: TOCTOU race with lease revocation in
  prepare_signaling()
--

commit f689cd02961e3a01034ef22d1f4351321e14445b
Author: Sebastian Wick <sebastian.wick@redhat.com>

drm/atomic-uapi: Add DRM_MODE_ATOMIC_RESET flag

This commit introduces the DRM_MODE_ATOMIC_RESET flag and the
DRM_CAP_ATOMIC_RESET capability to allow userspace to reset all KMS object
states to their defaults in an atomic commit before applying new property
values.

> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 1eebcf6f5e05b..1a89175318d02 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -1440,10 +1440,41 @@ static int prepare_signaling(struct drm_device *dev,
>  
>  	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
>  		s32 __user *fence_ptr;
> +		bool page_flip_event;
>  
>  		fence_ptr = get_out_fence_for_crtc(crtc_state->state, crtc);
>  
> -		if (arg->flags & DRM_MODE_PAGE_FLIP_EVENT || fence_ptr) {
> +		page_flip_event = arg->flags & DRM_MODE_PAGE_FLIP_EVENT;
> +
> +		/*
> +		 * A commit with the DRM_MODE_ATOMIC_RESET flag contains
> +		 * every CRTC of the device or lease, even the ones that are
> +		 * disabled before and after the commit. Those can't report
> +		 * a page flip, and drm_atomic_crtc_check() would reject the
> +		 * whole commit, so don't attach any event to them.
> +		 */
> +		if (page_flip_event && !fence_ptr &&
> +		    (arg->flags & DRM_MODE_ATOMIC_RESET) &&
> +		    !crtc_state->active) {
> +			struct drm_crtc_state *old_crtc_state;
> +
> +			old_crtc_state = drm_atomic_get_old_crtc_state(state, crtc);
> +			page_flip_event = old_crtc_state->active;
> +		}
> +
> +		/*
> +		 * A lessee commit with the DRM_MODE_ATOMIC_RESET flag also
> +		 * contains unleased CRTCs, pulled in because a leased plane
> +		 * or connector is bound to them. Those CRTCs belong to
> +		 * another master, so the lessee can't get a page flip from
> +		 * them: don't attach any event to them, and don't count
> +		 * them towards the CRTCs that can report one.
> +		 */
> +		if (page_flip_event && (arg->flags & DRM_MODE_ATOMIC_RESET) &&
> +		    !drm_lease_held(file_priv, crtc->base.id))
> +			page_flip_event = false;

[Severity: Medium]
Can a concurrent lease revocation during prepare_signaling() cause a
multi-CRTC reset commit to drop page-flip events for a subset of leased
CRTCs?

drm_atomic_commit_fill_with_defaults() checks drm_lease_revoked() at the
end of filling the state, but idr_mutex is only held inside individual
drm_lease_held() calls.  If the lessor revokes the lease after that check:

CPU0 (lessee)                           CPU1 (lessor)
drm_mode_atomic_ioctl()
  drm_atomic_commit_fill_with_defaults()
    drm_lease_revoked() -> false
  ...
  prepare_signaling()
    /* crtc 0 (leased) */
    drm_lease_held(crtc 0) -> true
    create_vblank_event(crtc 0)
    c++
                                        drm_mode_revoke_lease_ioctl()
                                          drm_lease_revoke()
                                            _drm_lease_revoke()
                                              idr_destroy(&master->leases)
    /* crtc 1 (leased) */
    drm_lease_held(crtc 1) -> false
    page_flip_event = false             <-- event skipped for crtc 1
  drm_atomic_commit()

Because c > 0, prepare_signaling() succeeds and the commit is applied to
both leased CRTCs, but crtc 1 never emits its requested vblank event,
leaving userspace waiting for an event that never arrives.  If the lease
is revoked before the loop starts, all leased CRTCs fail drm_lease_held()
and prepare_signaling() fails with -EINVAL instead of -ENOENT.

Checking drm_lease_revoked(file_priv) after the CRTC loop in
prepare_signaling() (as drm_atomic_commit_fill_with_defaults() does) would
catch a revocation that happened before or during the loop.

[ ... ]

> @@ -1688,6 +1727,17 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>  	fence_state = NULL;
>  	num_fences = 0;
>  
> +	if (arg->flags & DRM_MODE_ATOMIC_RESET) {
> +		if (!drm_atomic_implements_create_state(dev)) {
> +			ret = -EOPNOTSUPP;
> +			goto out;
> +		}
> +
> +		ret = drm_atomic_commit_fill_with_defaults(file_priv, state);
> +		if (ret)
> +			goto out;
> +	}

[Severity: High]
Can calling drm_atomic_commit_fill_with_defaults() from a lessee trigger a
use-after-free or NULL pointer dereference on plane->state?

For unleased planes, drm_atomic_commit_fill_with_defaults() reads
plane->state->crtc twice before acquiring plane->mutex:

drivers/gpu/drm/drm_atomic.c:drm_atomic_commit_fill_with_defaults() {
    ...
		} else if (plane->state->crtc) {
			crtc_state = drm_atomic_get_new_crtc_state(commit,
								   plane->state->crtc);
			if (!crtc_state || crtc_state->enable)
				continue;

			plane_state = drm_atomic_get_plane_state(commit, plane);
    ...
}

When an unleased plane is bound to an unleased CRTC, the lessee's reset
commit holds neither plane->mutex nor that CRTC's crtc->mutex.  The lessor
(or another lessee) can concurrently commit a plane update on that
unleased plane:

CPU0 (lessee)                           CPU1 (lessor)
drm_mode_atomic_ioctl()
  drm_atomic_commit_fill_with_defaults()
    if (plane->state->crtc)
                                        drm_atomic_helper_swap_state()
                                          /* sets plane->state->crtc = NULL
                                           * and frees old plane->state
                                           */
    drm_atomic_get_new_crtc_state(...,
      plane->state->crtc)
      drm_crtc_index(NULL)              <-- NULL pointer dereference
                                            (or UAF of old plane->state)

Locking plane->mutex before inspecting plane->state in the unleased branch
as well would prevent concurrent state swaps while reading plane->state->crtc.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-drm-reset-state-flag-v9-0-ddb0e26324a0@kernel.org?part=9