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
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: <[email protected]> # 5.8+
Signed-off-by: Paul Cercueil <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
---
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
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: <[email protected]> # 4.12+
Cc: Andrzej Hajda <[email protected]>
Cc: Neil Armstrong <[email protected]>
Cc: Laurent Pinchart <[email protected]>
Cc: Jonas Karlman <[email protected]>
Cc: Jernej Skrabec <[email protected]>
Signed-off-by: Paul Cercueil <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
> ---
> 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
Le dim. 28 mars 2021 ? 1:05, Laurent Pinchart
<[email protected]> 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 <[email protected]>
>
> Reviewed-by: Laurent Pinchart <[email protected]>
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