Re: [PATCH v9 04/10] drm/atomic: Allow filling a commit with pristine object states

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] drm/atomic: race on plane->state in
  drm_atomic_commit_fill_with_defaults()
- [High] drm/atomic: preserve normalized_zpos in
  drm_atomic_commit_fill_with_defaults()
- [Medium] drm/atomic: unleased connector pull in
  drm_atomic_commit_fill_with_defaults()
--

commit 0fce0c540da05484e00eeed0228bcc4903e9743e
Author: Maxime Ripard <mripard@kernel.org>

drm/atomic: Allow filling a commit with pristine object states

This commit adds drm_atomic_commit_fill_with_defaults() to populate an
atomic commit with pristine default states for CRTCs, connectors, planes,
and color operations via atomic_create_state().

> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 7a77051096c91..b469a8169ca6d 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c

[ ... ]

> +static int drm_atomic_commit_fill_connector_with_defaults(struct drm_file *file_priv,
> +							  struct drm_atomic_commit *commit,
> +							  struct drm_connector *connector)
> +{

[ ... ]

> +	} else {
> +		/*
> +		 * The connector isn't part of the lease so it keeps its
> +		 * current state. If it is bound to a leased CRTC though,
> +		 * that CRTC is being reset under it and comes with an
> +		 * empty connector mask, so
> +		 * drm_atomic_add_affected_connectors() won't find the
> +		 * connector during the check. Pull it in here so that its
> +		 * routing and encoder are properly handled.
> +		 *
> +		 * The leased CRTCs are all part of the commit by now. An
> +		 * unleased one might not be yet, but it keeps its
> +		 * connector mask, so the helpers will pull the connector
> +		 * in on their own if that CRTC ends up being changed.
> +		 */
> +		if (!connector->state->crtc)
> +			return 0;
> +
> +		crtc_state = drm_atomic_get_new_crtc_state(commit,
> +							   connector->state->crtc);
> +		if (!crtc_state)
> +			return 0;
> +
> +		connector_state = drm_atomic_get_connector_state(commit,
> +								 connector);
> +		if (IS_ERR(connector_state))
> +			return PTR_ERR(connector_state);
> +
> +		/*
> +		 * The connector can't stay bound to a CRTC that the reset
> +		 * disables, whether it is a leased one being reset or an
> +		 * unleased one that just lost its last leased connector:
> +		 * update_connector_routing() would keep it attached to a
> +		 * disabled CRTC. Detach it like userspace setting CRTC_ID
> +		 * to 0 would.
> +		 */
> +		if (!crtc_state->enable) {
> +			ret = drm_atomic_set_crtc_for_connector(connector_state,
> +								NULL);
> +			if (ret)
> +				return ret;
> +		}
> +	}

[Severity: Medium]
Should this check !crtc_state->enable before calling
drm_atomic_get_connector_state(), like the unleased plane loop below does?

When an unleased CRTC drives both a leased connector and an unleased
connector in clone mode, and the leased connector precedes the unleased
one in connector_list, the leased connector pulls the unleased CRTC into
the commit while leaving it enabled because the unleased connector's bit
remains in crtc_state->connector_mask.  Calling
drm_atomic_get_connector_state() here then adds the unleased connector to
the commit before drm_atomic_helper_check_modeset() runs, which marks it
as added_by_user in user_connectors_mask.  If that unleased connector was
hot-unplugged and unregistered while the unleased CRTC stays active,
update_connector_routing() rejects the lessee's reset commit with -EINVAL:

drivers/gpu/drm/drm_atomic_helper.c:update_connector_routing() {
	...
	if (!state->duplicated && drm_connector_is_unregistered(connector) &&
	    added_by_user && crtc_state->active) {
		drm_dbg_atomic(connector->dev,
			       "[CONNECTOR:%d:%s] is not registered\n",
			       connector->base.id, connector->name);
		return -EINVAL;
	}
	...
}

Also, the comment notes that an unleased connector could be bound to an
unleased CRTC that just lost its last leased connector and was disabled,
which cannot happen while this unleased connector's bit is still set in
crtc_state->connector_mask.

[ ... ]

> +			plane_state = plane->funcs->atomic_create_state(plane);
> +			if (IS_ERR(plane_state))
> +				return PTR_ERR(plane_state);
> +
> +			/*
> +			 * Same thing for the planes: report a color pipeline change
> +			 * compared to the current state.
> +			 */
> +			plane_state->color_mgmt_changed =
> +				plane->state->color_pipeline != plane_state->color_pipeline;
> +
> +			drm_atomic_commit_set_plane_state(commit, plane, plane_state);

[Severity: High]
Does plane_state also need to carry over plane->state->normalized_zpos,
just as connector_state->best_encoder and crtc_state->adjusted_mode are
preserved above?

__drm_atomic_helper_plane_state_init() initializes normalized_zpos to the
un-normalized default zpos value, and drm_atomic_plane_set_property() only
updates state->zpos when userspace sets the zpos property.  During atomic
check, drm_atomic_normalize_zpos() only recalculates normalized_zpos if
plane_mask or a plane's zpos changed between old and new states:

drivers/gpu/drm/drm_blend.c:drm_atomic_normalize_zpos() {
	...
	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
		crtc = new_plane_state->crtc;
		if (!crtc)
			continue;
		if (old_plane_state->zpos != new_plane_state->zpos) {
			new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
			new_crtc_state->zpos_changed = true;
		}
	}

	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
		if (old_crtc_state->plane_mask != new_crtc_state->plane_mask ||
		    new_crtc_state->zpos_changed) {
			ret = drm_atomic_helper_crtc_normalize_zpos(crtc,
								    new_crtc_state);
	...
}

When a reset commit re-applies the active plane_mask and zpos values on a
CRTC, drm_atomic_normalize_zpos() skips normalization and leaves
new_plane_state->normalized_zpos at its un-normalized default, corrupting
plane z-ordering or causing a NULL pointer dereference in drivers such as
sun4i_backend_atomic_check() that index an array by normalized_zpos and
dereference plane_states[i]->fb.

> +		} else if (plane->state->crtc) {
> +			/*
> +			 * An unleased plane keeps its current state, unless
> +			 * the CRTC it is enabled on is disabled by the reset:
> +			 * that CRTC is either leased and comes with a
> +			 * pristine state, or unleased and just lost its last
> +			 * leased connector. The plane can't remain enabled on
> +			 * a disabled CRTC: the atomic helpers would pull it in
> +			 * with its current state during the check and reject
> +			 * the commit. Disable it like userspace setting
> +			 * CRTC_ID and FB_ID to 0 would.
> +			 */
> +			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);
> +			if (IS_ERR(plane_state))
> +				return PTR_ERR(plane_state);
> +
> +			ret = drm_atomic_set_crtc_for_plane(plane_state, NULL);
> +			if (ret)
> +				return ret;
> +
> +			drm_atomic_set_fb_for_plane(plane_state, NULL);
> +		}

[Severity: High]
Can reading plane->state->crtc without holding plane->mutex here cause a
use-after-free or NULL pointer dereference when a concurrent commit updates
an unleased plane?

Unlike the leased branch above, which locks plane->mutex before reading
plane->state->crtc, the unleased branch dereferences plane->state twice
before drm_atomic_get_plane_state() acquires plane->mutex.  A concurrent
non-modeset plane update or page flip on an unleased plane and unleased
CRTC does not take connection_mutex, so it can swap and free the old
drm_plane_state or clear plane->state->crtc in parallel:

CPU0 (lessee reset)               CPU1 (lessor plane update)
drm_atomic_commit_fill_with_defaults()
  else if (plane->state->crtc)
                                  drm_mode_atomic_ioctl()
                                    ...
                                    drm_atomic_helper_swap_state()
                                      /* plane->state->crtc = NULL */
                                    drm_atomic_commit_put()
                                      ...
                                      atomic_destroy_state(old_plane_state)
  drm_atomic_get_new_crtc_state(commit,
                                plane->state->crtc)
    drm_crtc_index(NULL)  <-- NULL pointer dereference (or UAF on old state)

In addition, if the unleased plane was on a CRTC being disabled when
checked unlocked, but a concurrent commit moves it onto an active unleased
CRTC before drm_atomic_get_plane_state() takes plane->mutex, this branch
will detach and disable the unleased plane on that active CRTC.  Locking
plane->mutex before reading plane->state->crtc avoids these races.

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