[v2] drm/stm: Avoid use-after-free issues with crtc and plane

  ltdc_load() calls functions drm_crtc_init_with_planes(),
drm_universal_plane_init() and drm_encoder_init(). These functions
should not be called with parameters allocated with devm_kzalloc()
to avoid use-after-free issues [1].

Use allocations managed by the DRM framework.

Found by Linux Verification Center (linuxtesting.org).

[1]
https://lore.kernel.org/lkml/u366i76e3qhh3ra5oxrtngjtm2u5lterkekcz6y2jkndhuxzli@diujon4h7qwb/

Signed-off-by: Katya Orlova <e.orlova@ispras.ru>
---
v2: use allocations managed by the DRM as
Raphael Gallais-Pou <raphael.gallais-pou@foss.st.com> suggested.
Also add a fix for encoder.
 drivers/gpu/drm/stm/drv.c  |  3 +-
 drivers/gpu/drm/stm/ltdc.c | 68 +++++++++-----------------------------
 2 files changed, 18 insertions(+), 53 deletions(-)
  

On 11/24/23 11:04, Katya Orlova wrote:
> ltdc_load() calls functions drm_crtc_init_with_planes(),
> drm_universal_plane_init() and drm_encoder_init(). These functions
> should not be called with parameters allocated with devm_kzalloc()
> to avoid use-after-free issues [1].
>
> Use allocations managed by the DRM framework.
>
> Found by Linux Verification Center (linuxtesting.org).
>
> [1]
> https://lore.kernel.org/lkml/u366i76e3qhh3ra5oxrtngjtm2u5lterkekcz6y2jkndhuxzli@diujon4h7qwb/
>
> Signed-off-by: Katya Orlova <e.orlova@ispras.ru>
> ---
> v2: use allocations managed by the DRM as
> Raphael Gallais-Pou <raphael.gallais-pou@foss.st.com> suggested.
> Also add a fix for encoder.
>  drivers/gpu/drm/stm/drv.c  |  3 +-
>  drivers/gpu/drm/stm/ltdc.c | 68 +++++++++-----------------------------
>  2 files changed, 18 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/gpu/drm/stm/drv.c b/drivers/gpu/drm/stm/drv.c
> index e8523abef27a..152bec2c0238 100644
> --- a/drivers/gpu/drm/stm/drv.c
> +++ b/drivers/gpu/drm/stm/drv.c
> @@ -25,6 +25,7 @@
>  #include <drm/drm_module.h>
>  #include <drm/drm_probe_helper.h>
>  #include <drm/drm_vblank.h>
> +#include <drm/drm_managed.h>
>  
>  #include "ltdc.h"
>  
> @@ -75,7 +76,7 @@ static int drv_load(struct drm_device *ddev)
>  
>  	DRM_DEBUG("%s\n", __func__);
>  
> -	ldev = devm_kzalloc(ddev->dev, sizeof(*ldev), GFP_KERNEL);
> +	ldev = drmm_kzalloc(ddev, sizeof(*ldev), GFP_KERNEL);
>  	if (!ldev)
>  		return -ENOMEM;
>  
> diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
> index 5576fdae4962..02a7c8375f44 100644
> --- a/drivers/gpu/drm/stm/ltdc.c
> +++ b/drivers/gpu/drm/stm/ltdc.c
> @@ -36,6 +36,7 @@
>  #include <drm/drm_probe_helper.h>
>  #include <drm/drm_simple_kms_helper.h>
>  #include <drm/drm_vblank.h>
> +#include <drm/drm_managed.h>
>  
>  #include <video/videomode.h>
>  
> @@ -1199,7 +1200,6 @@ static void ltdc_crtc_atomic_print_state(struct drm_printer *p,
>  }
>  
>  static const struct drm_crtc_funcs ltdc_crtc_funcs = {
> -	.destroy = drm_crtc_cleanup,
>  	.set_config = drm_atomic_helper_set_config,
>  	.page_flip = drm_atomic_helper_page_flip,
>  	.reset = drm_atomic_helper_crtc_reset,
> @@ -1212,7 +1212,6 @@ static const struct drm_crtc_funcs ltdc_crtc_funcs = {
>  };
>  
>  static const struct drm_crtc_funcs ltdc_crtc_with_crc_support_funcs = {
> -	.destroy = drm_crtc_cleanup,
>  	.set_config = drm_atomic_helper_set_config,
>  	.page_flip = drm_atomic_helper_page_flip,
>  	.reset = drm_atomic_helper_crtc_reset,
> @@ -1545,7 +1544,6 @@ static void ltdc_plane_atomic_print_state(struct drm_printer *p,
>  static const struct drm_plane_funcs ltdc_plane_funcs = {
>  	.update_plane = drm_atomic_helper_update_plane,
>  	.disable_plane = drm_atomic_helper_disable_plane,
> -	.destroy = drm_plane_cleanup,
>  	.reset = drm_atomic_helper_plane_reset,
>  	.atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
>  	.atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
> @@ -1572,7 +1570,6 @@ static struct drm_plane *ltdc_plane_create(struct drm_device *ddev,
>  	const u64 *modifiers = ltdc_format_modifiers;
>  	u32 lofs = index * LAY_OFS;
>  	u32 val;
> -	int ret;
>  
>  	/* Allocate the biggest size according to supported color formats */
>  	formats = devm_kzalloc(dev, (ldev->caps.pix_fmt_nb +
> @@ -1613,14 +1610,10 @@ static struct drm_plane *ltdc_plane_create(struct drm_device *ddev,
>  		}
>  	}
>  
> -	plane = devm_kzalloc(dev, sizeof(*plane), GFP_KERNEL);
> -	if (!plane)
> -		return NULL;
> -
> -	ret = drm_universal_plane_init(ddev, plane, possible_crtcs,
> -				       &ltdc_plane_funcs, formats, nb_fmt,
> -				       modifiers, type, NULL);
> -	if (ret < 0)
> +	plane = drmm_universal_plane_alloc(ddev, struct drm_plane, dev,
> +				       possible_crtcs, &ltdc_plane_funcs, formats, nb_fmt,
> +				       modifiers, type, NULL);

Hi Katya,

Thanks for your submission, and sorry for the delay.


There is several alignment style problems, such as the lines above.

You can use "--strict" option with checkpatch script to show you all the faulty
alignment before sending a patch.


Other than that this patch looks pretty good to me.

Regards,

Raphaël

> +	if (IS_ERR(plane))
>  		return NULL;
>  
>  	if (ldev->caps.ycbcr_input) {
> @@ -1643,15 +1636,6 @@ static struct drm_plane *ltdc_plane_create(struct drm_device *ddev,
>  	return plane;
>  }
>  
> -static void ltdc_plane_destroy_all(struct drm_device *ddev)
> -{
> -	struct drm_plane *plane, *plane_temp;
> -
> -	list_for_each_entry_safe(plane, plane_temp,
> -				 &ddev->mode_config.plane_list, head)
> -		drm_plane_cleanup(plane);
> -}
> -
>  static int ltdc_crtc_init(struct drm_device *ddev, struct drm_crtc *crtc)
>  {
>  	struct ltdc_device *ldev = ddev->dev_private;
> @@ -1677,14 +1661,14 @@ static int ltdc_crtc_init(struct drm_device *ddev, struct drm_crtc *crtc)
>  
>  	/* Init CRTC according to its hardware features */
>  	if (ldev->caps.crc)
> -		ret = drm_crtc_init_with_planes(ddev, crtc, primary, NULL,
> +		ret = drmm_crtc_init_with_planes(ddev, crtc, primary, NULL,
>  						&ltdc_crtc_with_crc_support_funcs, NULL);
>  	else
> -		ret = drm_crtc_init_with_planes(ddev, crtc, primary, NULL,
> +		ret = drmm_crtc_init_with_planes(ddev, crtc, primary, NULL,
>  						&ltdc_crtc_funcs, NULL);
>  	if (ret) {
>  		DRM_ERROR("Can not initialize CRTC\n");
> -		goto cleanup;
> +		return ret;
>  	}
>  
>  	drm_crtc_helper_add(crtc, &ltdc_crtc_helper_funcs);
> @@ -1698,9 +1682,8 @@ static int ltdc_crtc_init(struct drm_device *ddev, struct drm_crtc *crtc)
>  	for (i = 1; i < ldev->caps.nb_layers; i++) {
>  		overlay = ltdc_plane_create(ddev, DRM_PLANE_TYPE_OVERLAY, i);
>  		if (!overlay) {
> -			ret = -ENOMEM;
>  			DRM_ERROR("Can not create overlay plane %d\n", i);
> -			goto cleanup;
> +			return -ENOMEM;
>  		}
>  		if (ldev->caps.dynamic_zorder)
>  			drm_plane_create_zpos_property(overlay, i, 0, ldev->caps.nb_layers - 1);
> @@ -1713,10 +1696,6 @@ static int ltdc_crtc_init(struct drm_device *ddev, struct drm_crtc *crtc)
>  	}
>  
>  	return 0;
> -
> -cleanup:
> -	ltdc_plane_destroy_all(ddev);
> -	return ret;
>  }
>  
>  static void ltdc_encoder_disable(struct drm_encoder *encoder)
> @@ -1776,23 +1755,19 @@ static int ltdc_encoder_init(struct drm_device *ddev, struct drm_bridge *bridge)
>  	struct drm_encoder *encoder;
>  	int ret;
>  
> -	encoder = devm_kzalloc(ddev->dev, sizeof(*encoder), GFP_KERNEL);
> -	if (!encoder)
> -		return -ENOMEM;
> +	encoder = drmm_simple_encoder_alloc(ddev, struct drm_encoder, dev,
> +										DRM_MODE_ENCODER_DPI);
Nit: bad alignment.
> +	if (IS_ERR(encoder))
> +		return PTR_ERR(encoder);
>  
>  	encoder->possible_crtcs = CRTC_MASK;
>  	encoder->possible_clones = 0;	/* No cloning support */
>  
> -	drm_simple_encoder_init(ddev, encoder, DRM_MODE_ENCODER_DPI);
> -
>  	drm_encoder_helper_add(encoder, &ltdc_encoder_helper_funcs);
>  
>  	ret = drm_bridge_attach(encoder, bridge, NULL, 0);
> -	if (ret) {
> -		if (ret != -EPROBE_DEFER)
> -			drm_encoder_cleanup(encoder);
> +	if (ret)
>  		return ret;
> -	}
>  
>  	DRM_DEBUG_DRIVER("Bridge encoder:%d created\n", encoder->base.id);
>  
> @@ -1962,8 +1937,7 @@ int ltdc_load(struct drm_device *ddev)
>  			goto err;
>  
>  		if (panel) {
> -			bridge = drm_panel_bridge_add_typed(panel,
> -							    DRM_MODE_CONNECTOR_DPI);
> +			bridge = drmm_panel_bridge_add(ddev, panel);
>  			if (IS_ERR(bridge)) {
>  				DRM_ERROR("panel-bridge endpoint %d\n", i);
>  				ret = PTR_ERR(bridge);
> @@ -2045,7 +2019,7 @@ int ltdc_load(struct drm_device *ddev)
>  		}
>  	}
>  
> -	crtc = devm_kzalloc(dev, sizeof(*crtc), GFP_KERNEL);
> +	crtc = drmm_kzalloc(ddev, sizeof(*crtc), GFP_KERNEL);
>  	if (!crtc) {
>  		DRM_ERROR("Failed to allocate crtc\n");
>  		ret = -ENOMEM;
> @@ -2072,8 +2046,6 @@ int ltdc_load(struct drm_device *ddev)
>  
>  	return 0;
>  err:
> -	for (i = 0; i < nb_endpoints; i++)
> -		drm_of_panel_bridge_remove(ddev->dev->of_node, 0, i);
>  
>  	clk_disable_unprepare(ldev->pixel_clk);
>  
> @@ -2082,16 +2054,8 @@ int ltdc_load(struct drm_device *ddev)
>  
>  void ltdc_unload(struct drm_device *ddev)
>  {
> -	struct device *dev = ddev->dev;
> -	int nb_endpoints, i;
> -
>  	DRM_DEBUG_DRIVER("\n");
>  
> -	nb_endpoints = of_graph_get_endpoint_count(dev->of_node);
> -
> -	for (i = 0; i < nb_endpoints; i++)
> -		drm_of_panel_bridge_remove(ddev->dev->of_node, 0, i);
> -
>  	pm_runtime_disable(ddev->dev);
>  }
>