All of lore.kernel.org
 help / color / mirror / Atom feed
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Paul Cercueil <paul@crapouillou.net>
Cc: Jernej Skrabec <jernej.skrabec@siol.net>,
	Neil Armstrong <narmstrong@baylibre.com>,
	David Airlie <airlied@linux.ie>, Jonas Karlman <jonas@kwiboo.se>,
	linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
	Andrzej Hajda <a.hajda@samsung.com>,
	od@zcrc.me, stable@vger.kernel.org,
	Sam Ravnborg <sam@ravnborg.org>
Subject: Re: [PATCH 1/3] drm: bridge/panel: Cleanup connector on bridge detach
Date: Mon, 18 Jan 2021 11:43:50 +0200	[thread overview]
Message-ID: <YAVYVkb7SPZLAiOZ@pendragon.ideasonboard.com> (raw)
In-Reply-To: <20210117112646.98353-2-paul@crapouillou.net>

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

WARNING: multiple messages have this Message-ID (diff)
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Paul Cercueil <paul@crapouillou.net>
Cc: David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>,
	Sam Ravnborg <sam@ravnborg.org>,
	od@zcrc.me, dri-devel@lists.freedesktop.org,
	linux-kernel@vger.kernel.org, stable@vger.kernel.org,
	Andrzej Hajda <a.hajda@samsung.com>,
	Neil Armstrong <narmstrong@baylibre.com>,
	Jonas Karlman <jonas@kwiboo.se>,
	Jernej Skrabec <jernej.skrabec@siol.net>
Subject: Re: [PATCH 1/3] drm: bridge/panel: Cleanup connector on bridge detach
Date: Mon, 18 Jan 2021 11:43:50 +0200	[thread overview]
Message-ID: <YAVYVkb7SPZLAiOZ@pendragon.ideasonboard.com> (raw)
In-Reply-To: <20210117112646.98353-2-paul@crapouillou.net>

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

  reply	other threads:[~2021-01-18  9:44 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 [this message]
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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YAVYVkb7SPZLAiOZ@pendragon.ideasonboard.com \
    --to=laurent.pinchart@ideasonboard.com \
    --cc=a.hajda@samsung.com \
    --cc=airlied@linux.ie \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=jernej.skrabec@siol.net \
    --cc=jonas@kwiboo.se \
    --cc=linux-kernel@vger.kernel.org \
    --cc=narmstrong@baylibre.com \
    --cc=od@zcrc.me \
    --cc=paul@crapouillou.net \
    --cc=sam@ravnborg.org \
    --cc=stable@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.