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

* [PATCH] drm/stm: Avoid use-after-free issues with crtc and plane
@ 2023-10-20 10:29 ` Katya Orlova
  0 siblings, 0 replies; 19+ messages in thread
From: Katya Orlova @ 2023-10-20 10:29 UTC (permalink / raw)
  To: Yannick Fertre
  Cc: Katya Orlova, Raphael Gallais-Pou, Philippe Cornu, David Airlie,
	Daniel Vetter, Maxime Coquelin, Alexandre Torgue, Philipp Zabel,
	dri-devel, linux-stm32, linux-arm-kernel, linux-kernel,
	lvc-project

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].

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@ispras.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@st.com>");
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 19+ messages in thread