Message-ID: <8ec28237-8641-4c4c-b448-824ac01f313b@foss.st.com>
Date:   Mon, 23 Oct 2023 10:19:10 +0200
From:   Raphael Gallais-Pou <raphael.gallais-pou@...s.st.com>
To:     Katya Orlova <e.orlova@...ras.ru>,
        Yannick Fertre <yannick.fertre@...s.st.com>
CC:     Philippe Cornu <philippe.cornu@...s.st.com>,
        David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>,
        Maxime Coquelin <mcoquelin.stm32@...il.com>,
        Alexandre Torgue <alexandre.torgue@...s.st.com>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        <dri-devel@...ts.freedesktop.org>,
        <linux-stm32@...md-mailman.stormreply.com>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <lvc-project@...uxtesting.org>
Subject: Re: [PATCH] drm/stm: Avoid use-after-free issues with crtc and plane

Hello Katya,


Thanks for your submission.


Please see drivers/gpu/drm/cr

On 10/20/23 12:29, Katya Orlova wrote:
> ltdc_load() calls functions drm_crtc_init_with_planes() and
> drm_universal_plane_init(). These functions should not be called with
> parameters allocated with devm_kzalloc() to avoid use-after-free issues [1].

What about drmm_kzalloc() ? By doing so the drm device is properly destroyed
using DRM internals, rather than handling the allocation within the LTDC driver.

This way has two benefits IMHO : you don't have to implement the .destroy hook
for CRTC and planes and since the allocation is managed by the DRM framework it
is less likely to make resources allocation errors on future patches.


You would then have to use drmm_universal_plane_init() and
drmm_crtc_init_with_planes().

see include/drm/drm_plane.h:779 and drivers/gpu/drm/drm_crtc.c:409


Regards,

Raphaël Gallais-Pou

>
> The patch replaces managed resource allocations with regular ones.
>
> Found by Linux Verification Center (linuxtesting.org).
>
> [1]
> https://lore.kernel.org/lkml/u366i76e3qhh3ra5oxrtngjtm2u5lterkekcz6y2jkndhuxzli@diujon4h7qwb/
>
> Signed-off-by: Katya Orlova <e.orlova@...ras.ru>
> ---
>  drivers/gpu/drm/stm/drv.c  | 11 ++++--
>  drivers/gpu/drm/stm/ltdc.c | 72 ++++++++++++++++++++++++++------------
>  2 files changed, 58 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/stm/drv.c b/drivers/gpu/drm/stm/drv.c
> index cb4404b3ce62..409f26d3e400 100644
> --- a/drivers/gpu/drm/stm/drv.c
> +++ b/drivers/gpu/drm/stm/drv.c
> @@ -74,7 +74,7 @@ static int drv_load(struct drm_device *ddev)
>  
>  	DRM_DEBUG("%s\n", __func__);
>  
> -	ldev = devm_kzalloc(ddev->dev, sizeof(*ldev), GFP_KERNEL);
> +	ldev = kzalloc(sizeof(*ldev), GFP_KERNEL);
>  	if (!ldev)
>  		return -ENOMEM;
>  
> @@ -82,7 +82,7 @@ static int drv_load(struct drm_device *ddev)
>  
>  	ret = drmm_mode_config_init(ddev);
>  	if (ret)
> -		return ret;
> +		goto err;
>  
>  	/*
>  	 * set max width and height as default value.
> @@ -98,7 +98,7 @@ static int drv_load(struct drm_device *ddev)
>  
>  	ret = ltdc_load(ddev);
>  	if (ret)
> -		return ret;
> +		goto err;
>  
>  	drm_mode_config_reset(ddev);
>  	drm_kms_helper_poll_init(ddev);
> @@ -106,6 +106,9 @@ static int drv_load(struct drm_device *ddev)
>  	platform_set_drvdata(pdev, ddev);
>  
>  	return 0;
> +err:
> +	kfree(ldev);
> +	return ret;
>  }
>  
>  static void drv_unload(struct drm_device *ddev)
> @@ -114,6 +117,8 @@ static void drv_unload(struct drm_device *ddev)
>  
>  	drm_kms_helper_poll_fini(ddev);
>  	ltdc_unload(ddev);
> +	kfree(ddev->dev_private);
> +	ddev->dev_private = NULL;
>  }
>  
>  static __maybe_unused int drv_suspend(struct device *dev)
> diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
> index b8be4c1db423..ec3bc3637a63 100644
> --- a/drivers/gpu/drm/stm/ltdc.c
> +++ b/drivers/gpu/drm/stm/ltdc.c
> @@ -1120,6 +1120,12 @@ static const struct drm_crtc_helper_funcs ltdc_crtc_helper_funcs = {
>  	.get_scanout_position = ltdc_crtc_get_scanout_position,
>  };
>  
> +static void ltdc_crtc_destroy(struct drm_crtc *crtc)
> +{
> +	drm_crtc_cleanup(crtc);
> +	kfree(crtc);
> +}
> +
>  static int ltdc_crtc_enable_vblank(struct drm_crtc *crtc)
>  {
>  	struct ltdc_device *ldev = crtc_to_ltdc(crtc);
> @@ -1200,7 +1206,7 @@ static void ltdc_crtc_atomic_print_state(struct drm_printer *p,
>  }
>  
>  static const struct drm_crtc_funcs ltdc_crtc_funcs = {
> -	.destroy = drm_crtc_cleanup,
> +	.destroy = ltdc_crtc_destroy,
>  	.set_config = drm_atomic_helper_set_config,
>  	.page_flip = drm_atomic_helper_page_flip,
>  	.reset = drm_atomic_helper_crtc_reset,
> @@ -1213,7 +1219,7 @@ 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,
> +	.destroy = ltdc_crtc_destroy,
>  	.set_config = drm_atomic_helper_set_config,
>  	.page_flip = drm_atomic_helper_page_flip,
>  	.reset = drm_atomic_helper_crtc_reset,
> @@ -1543,10 +1549,16 @@ static void ltdc_plane_atomic_print_state(struct drm_printer *p,
>  	fpsi->counter = 0;
>  }
>  
> +static void ltdc_plane_destroy(struct drm_plane *plane)
> +{
> +	drm_plane_cleanup(plane);
> +	kfree(plane);
> +}
> +
>  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,
> +	.destroy = ltdc_plane_destroy,
>  	.reset = drm_atomic_helper_plane_reset,
>  	.atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
>  	.atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
> @@ -1565,7 +1577,6 @@ static struct drm_plane *ltdc_plane_create(struct drm_device *ddev,
>  {
>  	unsigned long possible_crtcs = CRTC_MASK;
>  	struct ltdc_device *ldev = ddev->dev_private;
> -	struct device *dev = ddev->dev;
>  	struct drm_plane *plane;
>  	unsigned int i, nb_fmt = 0;
>  	u32 *formats;
> @@ -1576,7 +1587,7 @@ static struct drm_plane *ltdc_plane_create(struct drm_device *ddev,
>  	int ret;
>  
>  	/* Allocate the biggest size according to supported color formats */
> -	formats = devm_kzalloc(dev, (ldev->caps.pix_fmt_nb +
> +	formats = kzalloc((ldev->caps.pix_fmt_nb +
>  			       ARRAY_SIZE(ltdc_drm_fmt_ycbcr_cp) +
>  			       ARRAY_SIZE(ltdc_drm_fmt_ycbcr_sp) +
>  			       ARRAY_SIZE(ltdc_drm_fmt_ycbcr_fp)) *
> @@ -1614,15 +1625,20 @@ static struct drm_plane *ltdc_plane_create(struct drm_device *ddev,
>  		}
>  	}
>  
> -	plane = devm_kzalloc(dev, sizeof(*plane), GFP_KERNEL);
> -	if (!plane)
> +	plane = kzalloc(sizeof(*plane), GFP_KERNEL);
> +	if (!plane) {
> +		kfree(formats);
>  		return NULL;
> +	}
>  
>  	ret = drm_universal_plane_init(ddev, plane, possible_crtcs,
>  				       &ltdc_plane_funcs, formats, nb_fmt,
>  				       modifiers, type, NULL);
> -	if (ret < 0)
> +	kfree(formats);
> +	if (ret < 0) {
> +		kfree(plane);
>  		return NULL;
> +	}
>  
>  	if (ldev->caps.ycbcr_input) {
>  		if (val & (LXCR_C1R_YIA | LXCR_C1R_YSPA | LXCR_C1R_YFPA))
> @@ -1650,7 +1666,7 @@ static void ltdc_plane_destroy_all(struct drm_device *ddev)
>  
>  	list_for_each_entry_safe(plane, plane_temp,
>  				 &ddev->mode_config.plane_list, head)
> -		drm_plane_cleanup(plane);
> +		ltdc_plane_destroy(plane);
>  }
>  
>  static int ltdc_crtc_init(struct drm_device *ddev, struct drm_crtc *crtc)
> @@ -1936,7 +1952,7 @@ int ltdc_load(struct drm_device *ddev)
>  	if (!nb_endpoints)
>  		return -ENODEV;
>  
> -	ldev->pixel_clk = devm_clk_get(dev, "lcd");
> +	ldev->pixel_clk = clk_get(dev, "lcd");
>  	if (IS_ERR(ldev->pixel_clk)) {
>  		if (PTR_ERR(ldev->pixel_clk) != -EPROBE_DEFER)
>  			DRM_ERROR("Unable to get lcd clock\n");
> @@ -1982,7 +1998,7 @@ int ltdc_load(struct drm_device *ddev)
>  		}
>  	}
>  
> -	rstc = devm_reset_control_get_exclusive(dev, NULL);
> +	rstc = reset_control_get_exclusive(dev, NULL);
>  
>  	mutex_init(&ldev->err_lock);
>  
> @@ -1993,25 +2009,25 @@ int ltdc_load(struct drm_device *ddev)
>  	}
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	ldev->regs = devm_ioremap_resource(dev, res);
> +	ldev->regs = ioremap(res->start, resource_size(res));
>  	if (IS_ERR(ldev->regs)) {
>  		DRM_ERROR("Unable to get ltdc registers\n");
>  		ret = PTR_ERR(ldev->regs);
>  		goto err;
>  	}
>  
> -	ldev->regmap = devm_regmap_init_mmio(&pdev->dev, ldev->regs, &stm32_ltdc_regmap_cfg);
> +	ldev->regmap = regmap_init_mmio(&pdev->dev, ldev->regs, &stm32_ltdc_regmap_cfg);
>  	if (IS_ERR(ldev->regmap)) {
>  		DRM_ERROR("Unable to regmap ltdc registers\n");
>  		ret = PTR_ERR(ldev->regmap);
> -		goto err;
> +		goto err_iounmap;
>  	}
>  
>  	ret = ltdc_get_caps(ddev);
>  	if (ret) {
>  		DRM_ERROR("hardware identifier (0x%08x) not supported!\n",
>  			  ldev->caps.hw_version);
> -		goto err;
> +		goto err_regmap_exit;
>  	}
>  
>  	/* Disable interrupts */
> @@ -2034,49 +2050,57 @@ int ltdc_load(struct drm_device *ddev)
>  		irq = platform_get_irq(pdev, i);
>  		if (irq < 0) {
>  			ret = irq;
> -			goto err;
> +			goto err_regmap_exit;
>  		}
>  
> -		ret = devm_request_threaded_irq(dev, irq, ltdc_irq,
> +		ret = request_threaded_irq(irq, ltdc_irq,
>  						ltdc_irq_thread, IRQF_ONESHOT,
>  						dev_name(dev), ddev);
>  		if (ret) {
>  			DRM_ERROR("Failed to register LTDC interrupt\n");
> -			goto err;
> +			goto err_regmap_exit;
>  		}
>  	}
>  
> -	crtc = devm_kzalloc(dev, sizeof(*crtc), GFP_KERNEL);
> +	crtc = kzalloc(sizeof(*crtc), GFP_KERNEL);
>  	if (!crtc) {
>  		DRM_ERROR("Failed to allocate crtc\n");
>  		ret = -ENOMEM;
> -		goto err;
> +		goto err_regmap_exit;
>  	}
>  
>  	ret = ltdc_crtc_init(ddev, crtc);
>  	if (ret) {
>  		DRM_ERROR("Failed to init crtc\n");
> -		goto err;
> +		goto free_crtc;
>  	}
>  
>  	ret = drm_vblank_init(ddev, NB_CRTC);
>  	if (ret) {
>  		DRM_ERROR("Failed calling drm_vblank_init()\n");
> -		goto err;
> +		goto free_crtc;
>  	}
>  
>  	clk_disable_unprepare(ldev->pixel_clk);
> +	clk_put(ldev->pixel_clk);
>  
>  	pinctrl_pm_select_sleep_state(ddev->dev);
>  
>  	pm_runtime_enable(ddev->dev);
>  
>  	return 0;
> +free_crtc:
> +	kfree(crtc);
> +err_regmap_exit:
> +	regmap_exit(ldev->regmap);
> +err_iounmap:
> +	iounmap(ldev->regs);
>  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);
> +	clk_put(ldev->pixel_clk);
>  
>  	return ret;
>  }
> @@ -2084,6 +2108,7 @@ int ltdc_load(struct drm_device *ddev)
>  void ltdc_unload(struct drm_device *ddev)
>  {
>  	struct device *dev = ddev->dev;
> +	struct ltdc_device *ldev = ddev->dev_private;
>  	int nb_endpoints, i;
>  
>  	DRM_DEBUG_DRIVER("\n");
> @@ -2094,6 +2119,9 @@ void ltdc_unload(struct drm_device *ddev)
>  		drm_of_panel_bridge_remove(ddev->dev->of_node, 0, i);
>  
>  	pm_runtime_disable(ddev->dev);
> +
> +	regmap_exit(ldev->regmap);
> +	iounmap(ldev->regs);
>  }
>  
>  MODULE_AUTHOR("Philippe Cornu <philippe.cornu@...com>");

Powered by blists - more mailing lists