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(-)
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)
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
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
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
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");
dri-devel@lists.freedesktop.org
-
Laurent Pinchart -
Paul Cercueil