linux-kernel - Re: [PATCH 24/29] drm/bridge: Provide a helper to get the global state from a bridge state

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z4juJy7kKPbI2BDb@phenom.ffwll.local>
Date: Thu, 16 Jan 2025 12:31:51 +0100
From: Simona Vetter <simona.vetter@...ll.ch>
To: Maxime Ripard <mripard@...nel.org>
Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
	Thomas Zimmermann <tzimmermann@...e.de>,
	David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
	Andrzej Hajda <andrzej.hajda@...el.com>,
	Neil Armstrong <neil.armstrong@...aro.org>,
	Robert Foss <rfoss@...nel.org>,
	Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
	Jonas Karlman <jonas@...boo.se>,
	Jernej Skrabec <jernej.skrabec@...il.com>,
	Douglas Anderson <dianders@...omium.org>,
	dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 24/29] drm/bridge: Provide a helper to get the global
 state from a bridge state

On Wed, Jan 15, 2025 at 10:05:31PM +0100, Maxime Ripard wrote:
> We have access to the global drm_atomic_state from a drm_bridge_state,
> but since it's fairly indirect it's not as obvious as it can be for
> other KMS entities.
> 
> Provide a helper to make it easier to figure out.
> 
> Signed-off-by: Maxime Ripard <mripard@...nel.org>
> ---
>  include/drm/drm_atomic.h | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 31ca88deb10d262fb3a3f8e14d2afe24f8410cb1..bd7959ae312c99c0a0034d36378ae44f04f6a374 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -1183,10 +1183,26 @@ static inline struct drm_bridge_state *
>  drm_priv_to_bridge_state(struct drm_private_state *priv)
>  {
>  	return container_of(priv, struct drm_bridge_state, base);
>  }
>  
> +/**
> + * @drm_bridge_state_get_atomic_state() - Get the atomic state from a bridge state
> + * @bridge_state: bridge state object
> + *
> + * RETURNS:
> + * The global atomic state @bridge_state is a part of, or NULL if there is none.
> + */
> +static inline struct drm_atomic_state *
> +drm_bridge_state_get_atomic_state(struct drm_bridge_state *bridge_state)

So this one is nasty, because we clear out these backpointers once we push
the states into obj->state (because they can then outlive the
drm_atomic_state). Which means you can't use this in commit callbacks. Or
the bridge code has a really bad use-after-free when it doesn't clear out
these backpointers when we swap in the new states in
drm_atomic_helper_swap_state().

The better pattern is to just ditch passing individual states to callbacks
and just pass the entire drm_atomic_state container, and let callbacks
fish out what exactly they need. And also provide all necessary helpers to
find the right states and all that stuff.

We should probably also document that design approach in the kerneldoc for
drm_atomic_state, or wherever there's a good place for that.

See also my other reply for some of the history of why we have this mess.

Cheers, Sima

> +{
> +	if (!bridge_state)
> +		return NULL;
> +
> +	return bridge_state->base.state;
> +}
> +
>  struct drm_bridge_state *
>  drm_atomic_get_bridge_state(struct drm_atomic_state *state,
>  			    struct drm_bridge *bridge);
>  struct drm_bridge_state *
>  drm_atomic_get_old_bridge_state(const struct drm_atomic_state *state,
> 
> -- 
> 2.47.1
> 

-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch