[PATCH v4 0/3] Fixes to bridge/panel and ingenic-drm

[PATCH v4 0/3] Fixes to bridge/panel and ingenic-drm

[Paul Cercueil]

Hi,

This is the V4 of my patchset that attempts to fix use-after-free errors
in bridge/panel.c and in the ingenic-drm driver.

Changes from v3:
- [1/3]: the code now checks (connector->dev) instead of
  (!!panel_bridge->connector.dev)
- [2/3]: the macro is now called drmm_plain_encoder_alloc(), and moved
  to <drm/drm_encoder.h>. It also takes funcs/name parameters to be more
  similar to drmm_encoder_alloc(), although these parameters can very
  well be NULL.
- [3/3] uses the new macro.

V3 had a 4th patch, which was already applied as it received a
reviewed-by tag and could be applied independently.

Cheers,
-Paul

Paul Cercueil (3):
  drm: bridge/panel: Cleanup connector on bridge detach
  drm/encoder: Add macro drmm_plain_encoder_alloc()
  drm/ingenic: Register devm action to cleanup encoders

 drivers/gpu/drm/bridge/panel.c            | 12 ++++++++++++
 drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 17 +++++++----------
 include/drm/drm_encoder.h                 | 18 ++++++++++++++++++
 3 files changed, 37 insertions(+), 10 deletions(-)

-- 
2.30.2

[PATCH v4 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.

v2: Cleanup connector only if it was created

v3: Add FIXME

v4: (Use connector->dev) directly in if() block

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>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/bridge/panel.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
index 0ddc37551194..c916f4b8907e 100644
--- a/drivers/gpu/drm/bridge/panel.c
+++ b/drivers/gpu/drm/bridge/panel.c
@@ -87,6 +87,18 @@ 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;
+
+	/*
+	 * Cleanup the connector if we know it was initialized.
+	 *
+	 * FIXME: This wouldn't be needed if the panel_bridge structure was
+	 * allocated with drmm_kzalloc(). This might be tricky since the
+	 * drm_device pointer can only be retrieved when the bridge is attached.
+	 */
+	if (connector->dev)
+		drm_connector_cleanup(connector);
 }
 
 static void panel_bridge_pre_enable(struct drm_bridge *bridge)
-- 
2.30.2

[PATCH v4 2/3] drm/encoder: Add macro drmm_plain_encoder_alloc()

[Paul Cercueil]

This performs the same operation as drmm_encoder_alloc(), but
only allocates and returns a struct drm_encoder instance.

v4: Rename macro drmm_plain_encoder_alloc() and move to
    <drm/drm_encoder.h>. Since it's not "simple" anymore it
    will now take funcs/name arguments as well.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 include/drm/drm_encoder.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
index 5bf78b5bcb2b..6e91a0280f31 100644
--- a/include/drm/drm_encoder.h
+++ b/include/drm/drm_encoder.h
@@ -224,6 +224,24 @@ void *__drmm_encoder_alloc(struct drm_device *dev,
 				      offsetof(type, member), funcs, \
 				      encoder_type, name, ##__VA_ARGS__))
 
+/**
+ * drmm_plain_encoder_alloc - Allocate and initialize an encoder
+ * @dev: drm device
+ * @funcs: callbacks for this encoder (optional)
+ * @encoder_type: user visible type of the encoder
+ * @name: printf style format string for the encoder name, or NULL for default name
+ *
+ * This is a simplified version of drmm_encoder_alloc(), which only allocates
+ * and returns a struct drm_encoder instance, with no subclassing.
+ *
+ * Returns:
+ * Pointer to the new drm_encoder struct, or ERR_PTR on failure.
+ */
+#define drmm_plain_encoder_alloc(dev, funcs, encoder_type, name, ...) \
+	((struct drm_encoder *) \
+	 __drmm_encoder_alloc(dev, sizeof(struct drm_encoder), \
+			      0, funcs, encoder_type, name, ##__VA_ARGS__))
+
 /**
  * drm_encoder_index - find the index of a registered encoder
  * @encoder: encoder to find index for
-- 
2.30.2

Re: [PATCH v4 2/3] drm/encoder: Add macro drmm_plain_encoder_alloc()

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Sat, Mar 27, 2021 at 11:57:41AM +0000, Paul Cercueil wrote:
> This performs the same operation as drmm_encoder_alloc(), but
> only allocates and returns a struct drm_encoder instance.
> 
> v4: Rename macro drmm_plain_encoder_alloc() and move to
>     <drm/drm_encoder.h>. Since it's not "simple" anymore it
>     will now take funcs/name arguments as well.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  include/drm/drm_encoder.h | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
> index 5bf78b5bcb2b..6e91a0280f31 100644
> --- a/include/drm/drm_encoder.h
> +++ b/include/drm/drm_encoder.h
> @@ -224,6 +224,24 @@ void *__drmm_encoder_alloc(struct drm_device *dev,
>  				      offsetof(type, member), funcs, \
>  				      encoder_type, name, ##__VA_ARGS__))
>  
> +/**
> + * drmm_plain_encoder_alloc - Allocate and initialize an encoder
> + * @dev: drm device
> + * @funcs: callbacks for this encoder (optional)
> + * @encoder_type: user visible type of the encoder
> + * @name: printf style format string for the encoder name, or NULL for default name
> + *
> + * This is a simplified version of drmm_encoder_alloc(), which only allocates
> + * and returns a struct drm_encoder instance, with no subclassing.
> + *
> + * Returns:
> + * Pointer to the new drm_encoder struct, or ERR_PTR on failure.
> + */
> +#define drmm_plain_encoder_alloc(dev, funcs, encoder_type, name, ...) \
> +	((struct drm_encoder *) \
> +	 __drmm_encoder_alloc(dev, sizeof(struct drm_encoder), \
> +			      0, funcs, encoder_type, name, ##__VA_ARGS__))
> +
>  /**
>   * drm_encoder_index - find the index of a registered encoder
>   * @encoder: encoder to find index for

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 2/3] drm/encoder: Add macro drmm_plain_encoder_alloc()

[Paul Cercueil]

Le dim. 28 mars 2021 à 1:05, Laurent Pinchart 
<laurent.pinchart@ideasonboard.com> a écrit :
> Hi Paul,
> 
> Thank you for the patch.
> 
> On Sat, Mar 27, 2021 at 11:57:41AM +0000, Paul Cercueil wrote:
>>  This performs the same operation as drmm_encoder_alloc(), but
>>  only allocates and returns a struct drm_encoder instance.
>> 
>>  v4: Rename macro drmm_plain_encoder_alloc() and move to
>>      <drm/drm_encoder.h>. Since it's not "simple" anymore it
>>      will now take funcs/name arguments as well.
>> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Patchset applied to drm-misc-next.

Thanks!

-Paul

>>  ---
>>   include/drm/drm_encoder.h | 18 ++++++++++++++++++
>>   1 file changed, 18 insertions(+)
>> 
>>  diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
>>  index 5bf78b5bcb2b..6e91a0280f31 100644
>>  --- a/include/drm/drm_encoder.h
>>  +++ b/include/drm/drm_encoder.h
>>  @@ -224,6 +224,24 @@ void *__drmm_encoder_alloc(struct drm_device 
>> *dev,
>>   				      offsetof(type, member), funcs, \
>>   				      encoder_type, name, ##__VA_ARGS__))
>> 
>>  +/**
>>  + * drmm_plain_encoder_alloc - Allocate and initialize an encoder
>>  + * @dev: drm device
>>  + * @funcs: callbacks for this encoder (optional)
>>  + * @encoder_type: user visible type of the encoder
>>  + * @name: printf style format string for the encoder name, or NULL 
>> for default name
>>  + *
>>  + * This is a simplified version of drmm_encoder_alloc(), which 
>> only allocates
>>  + * and returns a struct drm_encoder instance, with no subclassing.
>>  + *
>>  + * Returns:
>>  + * Pointer to the new drm_encoder struct, or ERR_PTR on failure.
>>  + */
>>  +#define drmm_plain_encoder_alloc(dev, funcs, encoder_type, name, 
>> ...) \
>>  +	((struct drm_encoder *) \
>>  +	 __drmm_encoder_alloc(dev, sizeof(struct drm_encoder), \
>>  +			      0, funcs, encoder_type, name, ##__VA_ARGS__))
>>  +
>>   /**
>>    * drm_encoder_index - find the index of a registered encoder
>>    * @encoder: encoder to find index for
> 
> --
> Regards,
> 
> Laurent Pinchart

[PATCH v4 3/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.

v2: Use the new __drmm_simple_encoder_alloc() function

v3: Use the new drmm_plain_simple_encoder_alloc() macro

v4: Use drmm_plain_encoder_alloc() macro

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

diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index d60e1eefc9d1..29742ec5ab95 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -24,6 +24,7 @@
 #include <drm/drm_crtc.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_drv.h>
+#include <drm/drm_encoder.h>
 #include <drm/drm_gem_cma_helper.h>
 #include <drm/drm_fb_cma_helper.h>
 #include <drm/drm_fb_helper.h>
@@ -37,7 +38,6 @@
 #include <drm/drm_plane.h>
 #include <drm/drm_plane_helper.h>
 #include <drm/drm_probe_helper.h>
-#include <drm/drm_simple_kms_helper.h>
 #include <drm/drm_vblank.h>
 
 struct ingenic_dma_hwdesc {
@@ -1024,20 +1024,17 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
 			bridge = devm_drm_panel_bridge_add_typed(dev, panel,
 								 DRM_MODE_CONNECTOR_DPI);
 
-		encoder = devm_kzalloc(dev, sizeof(*encoder), GFP_KERNEL);
-		if (!encoder)
-			return -ENOMEM;
+		encoder = drmm_plain_encoder_alloc(drm, NULL, DRM_MODE_ENCODER_DPI, NULL);
+		if (IS_ERR(encoder)) {
+			ret = PTR_ERR(encoder);
+			dev_err(dev, "Failed to init encoder: %d\n", ret);
+			return ret;
+		}
 
 		encoder->possible_crtcs = 1;
 
 		drm_encoder_helper_add(encoder, &ingenic_drm_encoder_helper_funcs);
 
-		ret = drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_DPI);
-		if (ret) {
-			dev_err(dev, "Failed to init encoder: %d\n", ret);
-			return ret;
-		}
-
 		ret = drm_bridge_attach(encoder, bridge, NULL, 0);
 		if (ret) {
 			dev_err(dev, "Unable to attach bridge\n");
-- 
2.30.2