linux-kernel - Re: [PATCH 05/29] drm/atomic_state_helper: Fix bridge state initialization

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20250923-dynamic-bumblebee-of-luck-d31a19@penduick>
Date: Tue, 23 Sep 2025 11:33:09 +0200
From: Maxime Ripard <mripard@...nel.org>
To: Thomas Zimmermann <tzimmermann@...e.de>
Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, 
	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>, 
	Jyri Sarha <jyri.sarha@....fi>, Tomi Valkeinen <tomi.valkeinen@...asonboard.com>, 
	Devarsh Thakkar <devarsht@...com>, dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 05/29] drm/atomic_state_helper: Fix bridge state
 initialization

On Mon, Sep 15, 2025 at 03:12:11PM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 15.09.25 um 13:27 schrieb Maxime Ripard:
> > On Tue, Sep 02, 2025 at 03:18:17PM +0200, Thomas Zimmermann wrote:
> > > Hi
> > > 
> > > Am 02.09.25 um 10:32 schrieb Maxime Ripard:
> > > > Bridges implement their state using a drm_private_obj and an
> > > > hand-crafted reset implementation.
> > > > 
> > > > Since drm_private_obj doesn't have a set of reset helper like the other
> > > > states, __drm_atomic_helper_bridge_reset() was initializing both the
> > > > drm_private_state and the drm_bridge_state structures.
> > > > 
> > > > This initialization however was missing the drm_private_state.obj
> > > > pointer to the drm_private_obj the state was allocated for, creating a
> > > > NULL pointer dereference when trying to access it.
> > > > 
> > > > Fixes: 751465913f04 ("drm/bridge: Add a drm_bridge_state object")
> > > > Signed-off-by: Maxime Ripard <mripard@...nel.org>
> > > > ---
> > > >    drivers/gpu/drm/drm_atomic_state_helper.c | 8 ++++++++
> > > >    1 file changed, 8 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> > > > index 7142e163e618ea0d7d9d828e1bd9ff2a6ec0dfeb..b962c342b16aabf4e3bea52a914e5deb1c2080ce 100644
> > > > --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> > > > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> > > > @@ -707,10 +707,17 @@ void drm_atomic_helper_connector_destroy_state(struct drm_connector *connector,
> > > >    	__drm_atomic_helper_connector_destroy_state(state);
> > > >    	kfree(state);
> > > >    }
> > > >    EXPORT_SYMBOL(drm_atomic_helper_connector_destroy_state);
> > > > +static void __drm_atomic_helper_private_obj_reset(struct drm_private_obj *obj,
> > > > +						  struct drm_private_state *state)
> > > > +{
> > > > +	memset(state, 0, sizeof(*state));
> > > This argument is guaranteed to be zero'd, I think. No need for a memset.
> > > 
> > > > +	state->obj = obj;
> > > > +}
> > > > +
> > > >    /**
> > > >     * __drm_atomic_helper_private_obj_duplicate_state - copy atomic private state
> > > >     * @obj: CRTC object
> > > >     * @state: new private object state
> > > >     *
> > > > @@ -796,10 +803,11 @@ EXPORT_SYMBOL(drm_atomic_helper_bridge_destroy_state);
> > > >     */
> > > >    void __drm_atomic_helper_bridge_reset(struct drm_bridge *bridge,
> > > >    				      struct drm_bridge_state *state)
> > > >    {
> > > >    	memset(state, 0, sizeof(*state));
> > > Another unnecessary memset?
> > I guess the two can be seen as redundant, but I'd argue the one in
> > __drm_atomic_helper_private_obj_reset should still be there.
> > 
> > What guarantees that the pointer points to a zero'd structure?
> 
> We only call this helper after allocation AFAICT. And the DRM APIs already
> assume that allocation clears to zero.

Really? Do we have that documented anywhere?

And even then, there's nothing that requires that helper to be called
straight-away after allocation.

More importantly, do we really care about skipping them? Like, all the
other reset helpers are doing it, it's cheap, safe, why should we remove
it?

Maxime

Download attachment "signature.asc" of type "application/pgp-signature" (270 bytes)