linux-kernel - Re: [PATCH 3/3] drm/bridge: add warning for bridges not using devm_drm_bridge

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] [day] [month] [year] [list]

Message-ID: <20250620175750.2e8e5203@booty>
Date: Fri, 20 Jun 2025 17:57:50 +0200
From: Luca Ceresoli <luca.ceresoli@...tlin.com>
To: Maxime Ripard <mripard@...nel.org>
Cc: 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>, Maarten
 Lankhorst <maarten.lankhorst@...ux.intel.com>, Thomas Zimmermann
 <tzimmermann@...e.de>, David Airlie <airlied@...il.com>, Simona Vetter
 <simona@...ll.ch>, Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>,
 Anusha Srivatsa <asrivats@...hat.com>, Thomas Petazzoni
 <thomas.petazzoni@...tlin.com>, dri-devel@...ts.freedesktop.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] drm/bridge: add warning for bridges not using
 devm_drm_bridge_alloc()

Hi Maxime,

On Fri, 20 Jun 2025 13:41:48 +0200
Maxime Ripard <mripard@...nel.org> wrote:

> Hi Luca,
> 
> On Fri, Jun 20, 2025 at 11:32:08AM +0200, Luca Ceresoli wrote:
> > To the best of my knowledge, all drivers in the mainline kernel adding a
> > DRM bridge are now converted to using devm_drm_bridge_alloc() for
> > allocation and initialization. Among others this ensures initialization of
> > the bridge refcount, allowing dynamic allocation lifetime.
> > 
> > devm_drm_bridge_alloc() is now mandatory for all new bridges. Code using
> > the old pattern ([devm_]kzalloc + filling the struct fields +
> > drm_bridge_add) is not allowed anymore.
> > 
> > Any drivers that might have been missed during the conversion, patches in
> > flight towards mainline and out-of-tre drivers still using the old pattern
> > will already be caught by a warning looking like:
> > 
> >   ------------[ cut here ]------------
> >   refcount_t: addition on 0; use-after-free.
> >   WARNING: CPU: 2 PID: 83 at lib/refcount.c:25 refcount_warn_saturate+0x120/0x148
> >   [...]
> >   Call trace:
> >    refcount_warn_saturate+0x120/0x148 (P)
> >    drm_bridge_get.part.0+0x70/0x98 [drm]
> >    drm_bridge_add+0x34/0x108 [drm]
> >    sn65dsi83_probe+0x200/0x480 [ti_sn65dsi83]
> >    [...]
> > 
> > This warning comes from the refcount code and happens because
> > drm_bridge_add() is increasing the refcount, which is uninitialized and
> > thus initially zero.
> > 
> > Having a warning and the corresponding stack trace is surely useful, but
> > the warning text does not clarify the root problem nor how to fix it.
> > 
> > Add a DRM_WARN() just before increasing the refcount, so the log will be
> > much more readable:
> > 
> >   [drm] DRM bridge corrupted or not allocated by devm_drm_bridge_alloc()
> >   ------------[ cut here ]------------
> >   refcount_t: addition on 0; use-after-free.
> >   [...etc...]
> > 
> > A DRM_WARN is used because drm_warn and drm_WARN require a struct
> > drm_device pointer which is not yet available when adding a bridge.
> > 
> > Do not print the dev_name() in the warning because struct drm_bridge has no
> > pointer to the struct device. The affected driver should be easy to catch
> > based on the following stack trace however.
> > 
> > Signed-off-by: Luca Ceresoli <luca.ceresoli@...tlin.com>
> > 
> > ---
> > 
> > This patch was added in v8
> > ---
> >  drivers/gpu/drm/drm_bridge.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index f001bbe95559aabf0aac9f25f89250ad4e1ad9c8..7d511c28608f1d8ea8fbb81d00efa9e227b02a13 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -295,6 +295,9 @@ EXPORT_SYMBOL(__devm_drm_bridge_alloc);
> >   */
> >  void drm_bridge_add(struct drm_bridge *bridge)
> >  {
> > +	if (kref_read(&bridge->refcount) == 0)
> > +		DRM_WARN("DRM bridge corrupted or not allocated by devm_drm_bridge_alloc()\n");
> > +  
> 
> I'm fine with it on principle, but I wonder if using bridge->container
> is set wouldn't be a more obvious way to check it?

Good point. Indeed the refcount check is potentially fragile because by
its own nature the refcount gets changed at various moments by various
parts of the kernel, unlike the container which is supposed to be never
modified after allocation.

I'll change this for v9.

Ouch, I just realized this does not show as v8 as it should (see cover
letter). Sorry about that, I messed up with my b4 branches, fixing both
things in, well, v9.

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com