linux-kernel - Re: [PATCH v2 30/34] drm/bridge: imx8qxp-pixel-combiner: convert to 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] [thread-next>] [day] [month] [year] [list]
Message-ID: <20250430112944.1b39caab@booty>
Date: Wed, 30 Apr 2025 11:29:44 +0200
From: Luca Ceresoli <luca.ceresoli@...tlin.com>
To: Liu Ying <victor.liu@....com>
Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard
 <mripard@...nel.org>, 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>, Jagan Teki
 <jagan@...rulasolutions.com>, Shawn Guo <shawnguo@...nel.org>, Sascha Hauer
 <s.hauer@...gutronix.de>, Pengutronix Kernel Team <kernel@...gutronix.de>,
 Fabio Estevam <festevam@...il.com>, Douglas Anderson
 <dianders@...omium.org>, Chun-Kuang Hu <chunkuang.hu@...nel.org>, Krzysztof
 Kozlowski <krzk@...nel.org>, Anusha Srivatsa <asrivats@...hat.com>, Paul
 Kocialkowski <paulk@...-base.io>, Dmitry Baryshkov <lumag@...nel.org>, Hui
 Pu <Hui.Pu@...ealthcare.com>, Thomas Petazzoni
 <thomas.petazzoni@...tlin.com>, dri-devel@...ts.freedesktop.org,
 asahi@...ts.linux.dev, linux-kernel@...r.kernel.org,
 chrome-platform@...ts.linux.dev, imx@...ts.linux.dev,
 linux-arm-kernel@...ts.infradead.org, linux-mediatek@...ts.infradead.org,
 linux-amlogic@...ts.infradead.org, linux-renesas-soc@...r.kernel.org,
 platform-driver-x86@...r.kernel.org, linux-samsung-soc@...r.kernel.org,
 linux-arm-msm@...r.kernel.org, freedreno@...ts.freedesktop.org,
 linux-stm32@...md-mailman.stormreply.com
Subject: Re: [PATCH v2 30/34] drm/bridge: imx8qxp-pixel-combiner: convert to
 devm_drm_bridge_alloc() API

Hello Liu,

On Tue, 29 Apr 2025 10:10:55 +0800
Liu Ying <victor.liu@....com> wrote:

> Hi,
> 
> On 04/25/2025, Luca Ceresoli wrote:
> > This is the new API for allocating DRM bridges.
> > 
> > This driver embeds an array of channels in the main struct, and each
> > channel embeds a drm_bridge. This prevents dynamic, refcount-based
> > deallocation of the bridges.
> > 
> > To make the new, dynamic bridge allocation possible:
> > 
> >  * change the array of channels into an array of channel pointers
> >  * allocate each channel using devm_drm_bridge_alloc()
> >  * adapt the code wherever using the channels
> > 
> > Signed-off-by: Luca Ceresoli <luca.ceresoli@...tlin.com>

[...]

> > @@ -345,8 +351,8 @@ static int imx8qxp_pc_bridge_probe(struct platform_device *pdev)
> >  free_child:
> >  	of_node_put(child);
> >  
> > -	if (i == 1 && pc->ch[0].next_bridge)
> > -		drm_bridge_remove(&pc->ch[0].bridge);
> > +	if (i == 1 && pc->ch[0]->next_bridge)  
> 
> Since this patch makes pc->ch[0] and pc->ch[1] be allocated separately,
> pc->ch[0] could be NULL if channel0 is not available, hence a NULL pointer
> dereference here...

See below for this.

> > +		drm_bridge_remove(&pc->ch[0]->bridge);
> >  
> >  	pm_runtime_disable(dev);
> >  	return ret;
> > @@ -359,7 +365,7 @@ static void imx8qxp_pc_bridge_remove(struct platform_device *pdev)
> >  	int i;
> >  
> >  	for (i = 0; i < 2; i++) {
> > -		ch = &pc->ch[i];
> > +		ch = pc->ch[i];
> >  
> >  		if (!ch->is_available)  
> 
> ...and here too.

This is indeed a bug, I should have checked the pointer for being
non-NULL.

Looking at that more closely, I think the is_available flag can be
entirely removed now. The allocation itself (ch != NULL) now is
equivalent. Do you think my reasoning is correct?

Ouch! After writing the previous paragraph I realized you proposed this
a few lines below! OK, removing is_available. :)

[...]

> On top of this patch series, this issue doesn't happen if I apply the below
> change:

[...]

> @@ -351,7 +349,7 @@ static int imx8qxp_pc_bridge_probe(struct platform_device *pdev)
>  free_child:
>         of_node_put(child);
>  
> -       if (i == 1 && pc->ch[0]->next_bridge)
> +       if (i == 1 && pc->ch[0])
>                 drm_bridge_remove(&pc->ch[0]->bridge);

Unrelated to this patch, but as I looked at it more in depth now, I'm
not sure this whole logic is robust, even in the original code.

The 'i == 1' check here seems to mean "if some error happened when
handling channel@1, that means channel@0 was successfully initialized,
so let's clean up channel 0".

However my understanding of the bindings is that device tree is allowed
to have the channel@1 node before the channel@0 node (or even channel@1
without channel@0, but that's less problematic here).

In such case (channel@1 before channel@0), this would happen:

 1. alloc and init ch[1], all OK
 2. alloc and init ch[0], an error happens
    (e.g. of_graph_get_remote_node() fails)

So we'd reach the free_child: label, and we should call
drm_bridge_remove() for ch[1]->bridge, but there's no code to do that.

To be robust in such a case, I think both channels need to be checked
independently, as the status of one does not imply the status of the
other. E.g.:

  for (i = 0; i < 2; i++)
      if (pc->ch[i] && pc->ch[i]->next_bridge)
          drm_bridge_remove(&pc->ch[i]->bridge);

(which is similar to what .remove() does after the changes discussed in
this thread, and which I have queued for v3)

What's your opinion? Do you think I missed anything?

Thanks for taking the time to dig into this!

Best regards,
Luca

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