* [PATCH 0/3] Fixes to bridge/panel and ingenic-drm
@ 2021-01-17 11:26 ` Paul Cercueil
0 siblings, 0 replies; 16+ messages in thread
From: Paul Cercueil @ 2021-01-17 11:26 UTC (permalink / raw
To: David Airlie, Daniel Vetter
Cc: Sam Ravnborg, od, dri-devel, linux-kernel, Paul Cercueil
Hi,
Here are three independent fixes. The first one addresses a
use-after-free in bridge/panel.c; the second one addresses a
use-after-free in the ingenic-drm driver; finally, the third one makes
the ingenic-drm driver work again on older Ingenic SoCs.
Cheers,
-Paul
Paul Cercueil (3):
drm: bridge/panel: Cleanup connector on bridge detach
drm/ingenic: Register devm action to cleanup encoders
drm/ingenic: Fix non-OSD mode
drivers/gpu/drm/bridge/panel.c | 4 ++++
drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 21 +++++++++++++++++----
2 files changed, 21 insertions(+), 4 deletions(-)
--
2.29.2
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 0/3] Fixes to bridge/panel and ingenic-drm
@ 2021-01-17 11:26 ` Paul Cercueil
0 siblings, 0 replies; 16+ messages in thread
From: Paul Cercueil @ 2021-01-17 11:26 UTC (permalink / raw
To: David Airlie, Daniel Vetter
Cc: Paul Cercueil, od, Sam Ravnborg, linux-kernel, dri-devel
Hi,
Here are three independent fixes. The first one addresses a
use-after-free in bridge/panel.c; the second one addresses a
use-after-free in the ingenic-drm driver; finally, the third one makes
the ingenic-drm driver work again on older Ingenic SoCs.
Cheers,
-Paul
Paul Cercueil (3):
drm: bridge/panel: Cleanup connector on bridge detach
drm/ingenic: Register devm action to cleanup encoders
drm/ingenic: Fix non-OSD mode
drivers/gpu/drm/bridge/panel.c | 4 ++++
drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 21 +++++++++++++++++----
2 files changed, 21 insertions(+), 4 deletions(-)
--
2.29.2
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/3] drm: bridge/panel: Cleanup connector on bridge detach
2021-01-17 11:26 ` Paul Cercueil
@ 2021-01-17 11:26 ` Paul Cercueil
-1 siblings, 0 replies; 16+ messages in thread
From: Paul Cercueil @ 2021-01-17 11:26 UTC (permalink / raw
To: David Airlie, Daniel Vetter
Cc: Sam Ravnborg, od, dri-devel, linux-kernel, Paul Cercueil, stable,
Andrzej Hajda, Neil Armstrong, Laurent Pinchart, Jonas Karlman,
Jernej Skrabec
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 1/3] drm: bridge/panel: Cleanup connector on bridge detach
@ 2021-01-17 11:26 ` Paul Cercueil
0 siblings, 0 replies; 16+ messages in thread
From: Paul Cercueil @ 2021-01-17 11:26 UTC (permalink / raw
To: David Airlie, Daniel Vetter
Cc: Jernej Skrabec, Neil Armstrong, Jonas Karlman, linux-kernel,
stable, Paul Cercueil, Andrzej Hajda, od, dri-devel, Sam Ravnborg,
Laurent Pinchart
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/3] drm/ingenic: Register devm action to cleanup encoders
2021-01-17 11:26 ` Paul Cercueil
@ 2021-01-17 11:26 ` Paul Cercueil
-1 siblings, 0 replies; 16+ messages in thread
From: Paul Cercueil @ 2021-01-17 11:26 UTC (permalink / raw
To: David Airlie, Daniel Vetter
Cc: Sam Ravnborg, od, dri-devel, linux-kernel, Paul Cercueil, stable
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/3] drm/ingenic: Register devm action to cleanup encoders
@ 2021-01-17 11:26 ` Paul Cercueil
0 siblings, 0 replies; 16+ messages in thread
From: Paul Cercueil @ 2021-01-17 11:26 UTC (permalink / raw
To: David Airlie, Daniel Vetter
Cc: linux-kernel, stable, Paul Cercueil, od, dri-devel, Sam Ravnborg
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 3/3] drm/ingenic: Fix non-OSD mode
2021-01-17 11:26 ` Paul Cercueil
@ 2021-01-17 11:26 ` Paul Cercueil
-1 siblings, 0 replies; 16+ messages in thread
From: Paul Cercueil @ 2021-01-17 11:26 UTC (permalink / raw
To: David Airlie, Daniel Vetter
Cc: Sam Ravnborg, od, dri-devel, linux-kernel, Paul Cercueil, stable
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 3/3] drm/ingenic: Fix non-OSD mode
@ 2021-01-17 11:26 ` Paul Cercueil
0 siblings, 0 replies; 16+ messages in thread
From: Paul Cercueil @ 2021-01-17 11:26 UTC (permalink / raw
To: David Airlie, Daniel Vetter
Cc: linux-kernel, stable, Paul Cercueil, od, dri-devel, Sam Ravnborg
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] drm/ingenic: Register devm action to cleanup encoders
2021-01-17 11:26 ` Paul Cercueil
@ 2021-01-18 9:43 ` Laurent Pinchart
-1 siblings, 0 replies; 16+ messages in thread
From: Laurent Pinchart @ 2021-01-18 9:43 UTC (permalink / raw
To: Paul Cercueil
Cc: David Airlie, linux-kernel, stable, od, dri-devel, Sam Ravnborg
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] drm/ingenic: Register devm action to cleanup encoders
@ 2021-01-18 9:43 ` Laurent Pinchart
0 siblings, 0 replies; 16+ messages in thread
From: Laurent Pinchart @ 2021-01-18 9:43 UTC (permalink / raw
To: Paul Cercueil
Cc: David Airlie, Daniel Vetter, linux-kernel, stable, od, dri-devel,
Sam Ravnborg
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] drm: bridge/panel: Cleanup connector on bridge detach
2021-01-17 11:26 ` Paul Cercueil
@ 2021-01-18 9:43 ` Laurent Pinchart
-1 siblings, 0 replies; 16+ messages in thread
From: Laurent Pinchart @ 2021-01-18 9:43 UTC (permalink / raw
To: Paul Cercueil
Cc: Jernej Skrabec, Neil Armstrong, David Airlie, Jonas Karlman,
linux-kernel, dri-devel, Andrzej Hajda, od, stable, Sam Ravnborg
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] drm: bridge/panel: Cleanup connector on bridge detach
@ 2021-01-18 9:43 ` Laurent Pinchart
0 siblings, 0 replies; 16+ messages in thread
From: Laurent Pinchart @ 2021-01-18 9:43 UTC (permalink / raw
To: Paul Cercueil
Cc: David Airlie, Daniel Vetter, Sam Ravnborg, od, dri-devel,
linux-kernel, stable, Andrzej Hajda, Neil Armstrong,
Jonas Karlman, Jernej Skrabec
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] drm/ingenic: Register devm action to cleanup encoders
2021-01-18 9:43 ` Laurent Pinchart
@ 2021-01-18 11:37 ` Paul Cercueil
-1 siblings, 0 replies; 16+ messages in thread
From: Paul Cercueil @ 2021-01-18 11:37 UTC (permalink / raw
To: Laurent Pinchart
Cc: David Airlie, Daniel Vetter, linux-kernel, stable, od, dri-devel,
Sam Ravnborg
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] drm/ingenic: Register devm action to cleanup encoders
@ 2021-01-18 11:37 ` Paul Cercueil
0 siblings, 0 replies; 16+ messages in thread
From: Paul Cercueil @ 2021-01-18 11:37 UTC (permalink / raw
To: Laurent Pinchart
Cc: David Airlie, linux-kernel, stable, od, dri-devel, Sam Ravnborg
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] drm/ingenic: Register devm action to cleanup encoders
2021-01-18 11:37 ` Paul Cercueil
@ 2021-01-18 12:52 ` Daniel Vetter
-1 siblings, 0 replies; 16+ messages in thread
From: Daniel Vetter @ 2021-01-18 12:52 UTC (permalink / raw
To: Paul Cercueil
Cc: Laurent Pinchart, David Airlie, Daniel Vetter, linux-kernel,
stable, od, dri-devel, Sam Ravnborg
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] drm/ingenic: Register devm action to cleanup encoders
@ 2021-01-18 12:52 ` Daniel Vetter
0 siblings, 0 replies; 16+ messages in thread
From: Daniel Vetter @ 2021-01-18 12:52 UTC (permalink / raw
To: Paul Cercueil
Cc: David Airlie, dri-devel, linux-kernel, stable, od,
Laurent Pinchart, Sam Ravnborg
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2021-01-19 8:38 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-01-17 11:26 [PATCH 0/3] Fixes to bridge/panel and ingenic-drm Paul Cercueil
2021-01-17 11:26 ` Paul Cercueil
2021-01-17 11:26 ` [PATCH 1/3] drm: bridge/panel: Cleanup connector on bridge detach Paul Cercueil
2021-01-17 11:26 ` Paul Cercueil
2021-01-18 9:43 ` Laurent Pinchart
2021-01-18 9:43 ` Laurent Pinchart
2021-01-17 11:26 ` [PATCH 2/3] drm/ingenic: Register devm action to cleanup encoders Paul Cercueil
2021-01-17 11:26 ` Paul Cercueil
2021-01-18 9:43 ` Laurent Pinchart
2021-01-18 9:43 ` Laurent Pinchart
2021-01-18 11:37 ` Paul Cercueil
2021-01-18 11:37 ` Paul Cercueil
2021-01-18 12:52 ` Daniel Vetter
2021-01-18 12:52 ` Daniel Vetter
2021-01-17 11:26 ` [PATCH 3/3] drm/ingenic: Fix non-OSD mode Paul Cercueil
2021-01-17 11:26 ` Paul Cercueil
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.