[PATCH 1/3] drm: bridge/panel: Cleanup connector on bridge detach

[PATCH 1/3] drm: bridge/panel: Cleanup connector on bridge detach

[Paul Cercueil]

If we don't call drm_connector_cleanup() manually in
panel_bridge_detach(), the connector will be cleaned up with the other
DRM objects in the call to drm_mode_config_cleanup(). However, since our
drm_connector is devm-allocated, by the time drm_mode_config_cleanup()
will be called, our connector will be long gone. Therefore, the
connector must be cleaned up when the bridge is detached to avoid
use-after-free conditions.

Fixes: 13dfc0540a57 ("drm/bridge: Refactor out the panel wrapper from the lvds-encoder bridge.")
Cc: <stable@vger.kernel.org> # 4.12+
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Jernej Skrabec <jernej.skrabec@siol.net>
Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/gpu/drm/bridge/panel.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
index 0ddc37551194..975d65c14c9c 100644
--- a/drivers/gpu/drm/bridge/panel.c
+++ b/drivers/gpu/drm/bridge/panel.c
@@ -87,6 +87,10 @@ static int panel_bridge_attach(struct drm_bridge *bridge,
 
 static void panel_bridge_detach(struct drm_bridge *bridge)
 {
+	struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
+	struct drm_connector *connector = &panel_bridge->connector;
+
+	drm_connector_cleanup(connector);
 }
 
 static void panel_bridge_pre_enable(struct drm_bridge *bridge)
-- 
2.29.2

Re: [PATCH 1/3] drm: bridge/panel: Cleanup connector on bridge detach

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Sun, Jan 17, 2021 at 11:26:44AM +0000, Paul Cercueil wrote:
> If we don't call drm_connector_cleanup() manually in
> panel_bridge_detach(), the connector will be cleaned up with the other
> DRM objects in the call to drm_mode_config_cleanup(). However, since our
> drm_connector is devm-allocated, by the time drm_mode_config_cleanup()
> will be called, our connector will be long gone. Therefore, the
> connector must be cleaned up when the bridge is detached to avoid
> use-after-free conditions.
> 
> Fixes: 13dfc0540a57 ("drm/bridge: Refactor out the panel wrapper from the lvds-encoder bridge.")
> Cc: <stable@vger.kernel.org> # 4.12+
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Jernej Skrabec <jernej.skrabec@siol.net>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/gpu/drm/bridge/panel.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
> index 0ddc37551194..975d65c14c9c 100644
> --- a/drivers/gpu/drm/bridge/panel.c
> +++ b/drivers/gpu/drm/bridge/panel.c
> @@ -87,6 +87,10 @@ static int panel_bridge_attach(struct drm_bridge *bridge,
>  
>  static void panel_bridge_detach(struct drm_bridge *bridge)
>  {
> +	struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
> +	struct drm_connector *connector = &panel_bridge->connector;
> +
> +	drm_connector_cleanup(connector);

The panel bridge driver only creates the connector if the
DRM_BRIDGE_ATTACH_NO_CONNECTOR flag wasn't set in panel_bridge_attach().
We shouldn't clean up the connector unconditionally.

A better fix would be to stop using the devm_* API, but that's more
complicated.

>  }
>  
>  static void panel_bridge_pre_enable(struct drm_bridge *bridge)

-- 
Regards,

Laurent Pinchart

[PATCH 2/3] drm/ingenic: Register devm action to cleanup encoders

[Paul Cercueil]

Since the encoders have been devm-allocated, they will be freed way
before drm_mode_config_cleanup() is called. To avoid use-after-free
conditions, we then must ensure that drm_encoder_cleanup() is called
before the encoders are freed.

Fixes: c369cb27c267 ("drm/ingenic: Support multiple panels/bridges")
Cc: <stable@vger.kernel.org> # 5.8+
Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index 368bfef8b340..d23a3292a0e0 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -803,6 +803,11 @@ static void __maybe_unused ingenic_drm_release_rmem(void *d)
 	of_reserved_mem_device_release(d);
 }
 
+static void ingenic_drm_encoder_cleanup(void *encoder)
+{
+	drm_encoder_cleanup(encoder);
+}
+
 static int ingenic_drm_bind(struct device *dev, bool has_components)
 {
 	struct platform_device *pdev = to_platform_device(dev);
@@ -1011,6 +1016,11 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
 			return ret;
 		}
 
+		ret = devm_add_action_or_reset(dev, ingenic_drm_encoder_cleanup,
+					       encoder);
+		if (ret)
+			return ret;
+
 		ret = drm_bridge_attach(encoder, bridge, NULL, 0);
 		if (ret) {
 			dev_err(dev, "Unable to attach bridge\n");
-- 
2.29.2

Re: [PATCH 2/3] drm/ingenic: Register devm action to cleanup encoders

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Sun, Jan 17, 2021 at 11:26:45AM +0000, Paul Cercueil wrote:
> Since the encoders have been devm-allocated, they will be freed way
> before drm_mode_config_cleanup() is called. To avoid use-after-free
> conditions, we then must ensure that drm_encoder_cleanup() is called
> before the encoders are freed.

How about allocating the encoder with drmm_encoder_alloc() instead ?

> Fixes: c369cb27c267 ("drm/ingenic: Support multiple panels/bridges")
> Cc: <stable@vger.kernel.org> # 5.8+
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> index 368bfef8b340..d23a3292a0e0 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> @@ -803,6 +803,11 @@ static void __maybe_unused ingenic_drm_release_rmem(void *d)
>  	of_reserved_mem_device_release(d);
>  }
>  
> +static void ingenic_drm_encoder_cleanup(void *encoder)
> +{
> +	drm_encoder_cleanup(encoder);
> +}
> +
>  static int ingenic_drm_bind(struct device *dev, bool has_components)
>  {
>  	struct platform_device *pdev = to_platform_device(dev);
> @@ -1011,6 +1016,11 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>  			return ret;
>  		}
>  
> +		ret = devm_add_action_or_reset(dev, ingenic_drm_encoder_cleanup,
> +					       encoder);
> +		if (ret)
> +			return ret;
> +
>  		ret = drm_bridge_attach(encoder, bridge, NULL, 0);
>  		if (ret) {
>  			dev_err(dev, "Unable to attach bridge\n");

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/3] drm/ingenic: Register devm action to cleanup encoders

[Paul Cercueil]

Hi Laurent,

Le lun. 18 janv. 2021 à 11:43, Laurent Pinchart 
<laurent.pinchart@ideasonboard.com> a écrit :
> Hi Paul,
> 
> Thank you for the patch.
> 
> On Sun, Jan 17, 2021 at 11:26:45AM +0000, Paul Cercueil wrote:
>>  Since the encoders have been devm-allocated, they will be freed way
>>  before drm_mode_config_cleanup() is called. To avoid use-after-free
>>  conditions, we then must ensure that drm_encoder_cleanup() is called
>>  before the encoders are freed.
> 
> How about allocating the encoder with drmm_encoder_alloc() instead ?

That would work, but it is not yet in drm-misc-fixes :(

-Paul

>>  Fixes: c369cb27c267 ("drm/ingenic: Support multiple panels/bridges")
>>  Cc: <stable@vger.kernel.org> # 5.8+
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  ---
>>   drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>> 
>>  diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c 
>> b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>  index 368bfef8b340..d23a3292a0e0 100644
>>  --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>  +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>>  @@ -803,6 +803,11 @@ static void __maybe_unused 
>> ingenic_drm_release_rmem(void *d)
>>   	of_reserved_mem_device_release(d);
>>   }
>> 
>>  +static void ingenic_drm_encoder_cleanup(void *encoder)
>>  +{
>>  +	drm_encoder_cleanup(encoder);
>>  +}
>>  +
>>   static int ingenic_drm_bind(struct device *dev, bool 
>> has_components)
>>   {
>>   	struct platform_device *pdev = to_platform_device(dev);
>>  @@ -1011,6 +1016,11 @@ static int ingenic_drm_bind(struct device 
>> *dev, bool has_components)
>>   			return ret;
>>   		}
>> 
>>  +		ret = devm_add_action_or_reset(dev, ingenic_drm_encoder_cleanup,
>>  +					       encoder);
>>  +		if (ret)
>>  +			return ret;
>>  +
>>   		ret = drm_bridge_attach(encoder, bridge, NULL, 0);
>>   		if (ret) {
>>   			dev_err(dev, "Unable to attach bridge\n");
> 
> --
> Regards,
> 
> Laurent Pinchart

Re: [PATCH 2/3] drm/ingenic: Register devm action to cleanup encoders

[Daniel Vetter]

On Mon, Jan 18, 2021 at 11:37:49AM +0000, Paul Cercueil wrote:
> Hi Laurent,
> 
> Le lun. 18 janv. 2021 à 11:43, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> a écrit :
> > Hi Paul,
> > 
> > Thank you for the patch.
> > 
> > On Sun, Jan 17, 2021 at 11:26:45AM +0000, Paul Cercueil wrote:
> > >  Since the encoders have been devm-allocated, they will be freed way
> > >  before drm_mode_config_cleanup() is called. To avoid use-after-free
> > >  conditions, we then must ensure that drm_encoder_cleanup() is called
> > >  before the encoders are freed.
> > 
> > How about allocating the encoder with drmm_encoder_alloc() instead ?
> 
> That would work, but it is not yet in drm-misc-fixes :(

Well I think then we should only fix this in drm-misc-next. Adding more
broken usage of devm_ isn't really a good idea.

If you want this in -fixes, then I think hand-roll it. But devm_ for drm
objects really is the wrong fix.
-Daniel

> 
> -Paul
> 
> > >  Fixes: c369cb27c267 ("drm/ingenic: Support multiple panels/bridges")
> > >  Cc: <stable@vger.kernel.org> # 5.8+
> > >  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> > >  ---
> > >   drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 10 ++++++++++
> > >   1 file changed, 10 insertions(+)
> > > 
> > >  diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> > > b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> > >  index 368bfef8b340..d23a3292a0e0 100644
> > >  --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> > >  +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> > >  @@ -803,6 +803,11 @@ static void __maybe_unused
> > > ingenic_drm_release_rmem(void *d)
> > >   	of_reserved_mem_device_release(d);
> > >   }
> > > 
> > >  +static void ingenic_drm_encoder_cleanup(void *encoder)
> > >  +{
> > >  +	drm_encoder_cleanup(encoder);
> > >  +}
> > >  +
> > >   static int ingenic_drm_bind(struct device *dev, bool
> > > has_components)
> > >   {
> > >   	struct platform_device *pdev = to_platform_device(dev);
> > >  @@ -1011,6 +1016,11 @@ static int ingenic_drm_bind(struct device
> > > *dev, bool has_components)
> > >   			return ret;
> > >   		}
> > > 
> > >  +		ret = devm_add_action_or_reset(dev, ingenic_drm_encoder_cleanup,
> > >  +					       encoder);
> > >  +		if (ret)
> > >  +			return ret;
> > >  +
> > >   		ret = drm_bridge_attach(encoder, bridge, NULL, 0);
> > >   		if (ret) {
> > >   			dev_err(dev, "Unable to attach bridge\n");
> > 
> > --
> > Regards,
> > 
> > Laurent Pinchart
> 
> 

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

[PATCH 3/3] drm/ingenic: Fix non-OSD mode

[Paul Cercueil]

Even though the JZ4740 did not have the OSD mode, it had (according to
the documentation) two DMA channels, but there is absolutely no
information about how to select the second DMA channel.

Make the ingenic-drm driver work in non-OSD mode by using the
foreground0 plane (which is bound to the DMA0 channel) as the primary
plane, instead of the foreground1 plane, which is the primary plane
when in OSD mode.

Fixes: 3c9bea4ef32b ("drm/ingenic: Add support for OSD mode")
Cc: <stable@vger.kernel.org> # v5.8+
Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index d23a3292a0e0..9d883864e078 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -553,7 +553,7 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
 		height = state->src_h >> 16;
 		cpp = state->fb->format->cpp[0];
 
-		if (priv->soc_info->has_osd && plane->type == DRM_PLANE_TYPE_OVERLAY)
+		if (!priv->soc_info->has_osd || plane->type == DRM_PLANE_TYPE_OVERLAY)
 			hwdesc = &priv->dma_hwdescs->hwdesc_f0;
 		else
 			hwdesc = &priv->dma_hwdescs->hwdesc_f1;
@@ -814,6 +814,7 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
 	const struct jz_soc_info *soc_info;
 	struct ingenic_drm *priv;
 	struct clk *parent_clk;
+	struct drm_plane *primary;
 	struct drm_bridge *bridge;
 	struct drm_panel *panel;
 	struct drm_encoder *encoder;
@@ -928,9 +929,11 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
 	if (soc_info->has_osd)
 		priv->ipu_plane = drm_plane_from_index(drm, 0);
 
-	drm_plane_helper_add(&priv->f1, &ingenic_drm_plane_helper_funcs);
+	primary = priv->soc_info->has_osd ? &priv->f1 : &priv->f0;
 
-	ret = drm_universal_plane_init(drm, &priv->f1, 1,
+	drm_plane_helper_add(primary, &ingenic_drm_plane_helper_funcs);
+
+	ret = drm_universal_plane_init(drm, primary, 1,
 				       &ingenic_drm_primary_plane_funcs,
 				       priv->soc_info->formats_f1,
 				       priv->soc_info->num_formats_f1,
@@ -942,7 +945,7 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
 
 	drm_crtc_helper_add(&priv->crtc, &ingenic_drm_crtc_helper_funcs);
 
-	ret = drm_crtc_init_with_planes(drm, &priv->crtc, &priv->f1,
+	ret = drm_crtc_init_with_planes(drm, &priv->crtc, primary,
 					NULL, &ingenic_drm_crtc_funcs, NULL);
 	if (ret) {
 		dev_err(dev, "Failed to init CRTC: %i\n", ret);
-- 
2.29.2