[PATCH] drm/omap: dmm_tiler: fix invalid cleanup on probe failure

[PATCH] drm/omap: dmm_tiler: fix invalid cleanup on probe failure

[Guangshuo Li]

omap_dmm_probe() uses omap_dmm_remove() to clean up all probe failures,
but the remove path assumes that the MMIO mapping and IRQ registration
have completed successfully.

If probe fails before the MMIO resource is mapped, omap_dmm_remove()
attempts to disable DMM interrupts through an uninitialized MMIO base,
which can result in an invalid memory access.

There is a similar issue with the IRQ. The IRQ number is stored in
omap_dmm before request_irq() is called, so a later probe failure can
make omap_dmm_remove() call free_irq() even though the IRQ was never
successfully requested.

Initialize the stored IRQ to -1 and keep the IRQ number local until
request_irq() succeeds. Only access the interrupt registers and free
the IRQ when it has been successfully registered. Also avoid unmapping
the MMIO region when it has not been mapped.

Propagate errors returned by platform_get_irq() while at it.

This issue was found by manual code inspection.

Fixes: 71e8831f6407 ("drm/omap: DMM/TILER support for OMAP4+ platform")
Cc: stable@vger.kernel.org
Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
---
 drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 65 ++++++++++++++++++------
 1 file changed, 49 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
index a2c702b831a7..169a158e5e9d 100644
--- a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
+++ b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
@@ -769,14 +769,14 @@ static void omap_dmm_remove(struct platform_device *dev)
 
 static int omap_dmm_probe(struct platform_device *dev)
 {
-	int ret = -EFAULT, i;
+	int ret, i;
 	struct tcm_area area = {0};
 	u32 hwinfo, pat_geom;
 	struct resource *mem;
 
 	omap_dmm = kzalloc_obj(*omap_dmm);
 	if (!omap_dmm)
-		goto fail;
+		return -ENOMEM;
 
 	/* initialize lists */
 	INIT_LIST_HEAD(&omap_dmm->alloc_head);
@@ -791,7 +791,7 @@ static int omap_dmm_probe(struct platform_device *dev)
 		if (!match) {
 			dev_err(&dev->dev, "failed to find matching device node\n");
 			ret = -ENODEV;
-			goto fail;
+			goto err_free_dmm;
 		}
 
 		omap_dmm->plat_data = match->data;
@@ -801,7 +801,8 @@ static int omap_dmm_probe(struct platform_device *dev)
 	mem = platform_get_resource(dev, IORESOURCE_MEM, 0);
 	if (!mem) {
 		dev_err(&dev->dev, "failed to get base address resource\n");
-		goto fail;
+		ret = -ENODEV;
+		goto err_free_dmm;
 	}
 
 	omap_dmm->phys_base = mem->start;
@@ -809,12 +810,15 @@ static int omap_dmm_probe(struct platform_device *dev)
 
 	if (!omap_dmm->base) {
 		dev_err(&dev->dev, "failed to get dmm base address\n");
-		goto fail;
+		ret = -ENOMEM;
+		goto err_free_dmm;
 	}
 
 	omap_dmm->irq = platform_get_irq(dev, 0);
-	if (omap_dmm->irq < 0)
-		goto fail;
+	if (omap_dmm->irq < 0) {
+		ret = omap_dmm->irq;
+		goto err_unmap;
+	}
 
 	omap_dmm->dev = &dev->dev;
 
@@ -864,13 +868,13 @@ static int omap_dmm_probe(struct platform_device *dev)
 	if (!omap_dmm->dummy_page) {
 		dev_err(&dev->dev, "could not allocate dummy page\n");
 		ret = -ENOMEM;
-		goto fail;
+		goto err_workaround;
 	}
 
 	/* set dma mask for device */
 	ret = dma_set_coherent_mask(&dev->dev, DMA_BIT_MASK(32));
 	if (ret)
-		goto fail;
+		goto err_free_dummy;
 
 	omap_dmm->dummy_pa = page_to_phys(omap_dmm->dummy_page);
 
@@ -881,7 +885,7 @@ static int omap_dmm_probe(struct platform_device *dev)
 	if (!omap_dmm->refill_va) {
 		dev_err(&dev->dev, "could not allocate refill memory\n");
 		ret = -ENOMEM;
-		goto fail;
+		goto err_free_dummy;
 	}
 
 	/* alloc engines */
@@ -889,7 +893,7 @@ static int omap_dmm_probe(struct platform_device *dev)
 					 omap_dmm->num_engines);
 	if (!omap_dmm->engines) {
 		ret = -ENOMEM;
-		goto fail;
+		goto err_free_refill;
 	}
 
 	for (i = 0; i < omap_dmm->num_engines; i++) {
@@ -907,7 +911,7 @@ static int omap_dmm_probe(struct platform_device *dev)
 	omap_dmm->tcm = kzalloc_objs(*omap_dmm->tcm, omap_dmm->num_lut);
 	if (!omap_dmm->tcm) {
 		ret = -ENOMEM;
-		goto fail;
+		goto err_free_engines;
 	}
 
 	/* init containers */
@@ -921,7 +925,7 @@ static int omap_dmm_probe(struct platform_device *dev)
 		if (!omap_dmm->tcm[i]) {
 			dev_err(&dev->dev, "failed to allocate container\n");
 			ret = -ENOMEM;
-			goto fail;
+			goto err_free_tcm;
 		}
 
 		omap_dmm->tcm[i]->lut_id = i;
@@ -958,7 +962,7 @@ static int omap_dmm_probe(struct platform_device *dev)
 		dev_err(&dev->dev, "couldn't register IRQ %d, error %d\n",
 			omap_dmm->irq, ret);
 		omap_dmm->irq = -1;
-		goto fail;
+		goto err_free_tcm;
 	}
 
 	/* Enable all interrupts for each refill engine except
@@ -980,8 +984,37 @@ static int omap_dmm_probe(struct platform_device *dev)
 
 	return 0;
 
-fail:
-	omap_dmm_remove(dev);
+err_free_tcm:
+	for (i = 0; i < ARRAY_SIZE(containers); i++)
+		containers[i] = NULL;
+
+	for (i = 0; i < omap_dmm->num_lut; i++)
+		if (omap_dmm->tcm[i])
+			omap_dmm->tcm[i]->deinit(omap_dmm->tcm[i]);
+	kfree(omap_dmm->tcm);
+
+err_free_engines:
+	kfree(omap_dmm->engines);
+
+err_free_refill:
+	dma_free_wc(omap_dmm->dev,
+		    REFILL_BUFFER_SIZE * omap_dmm->num_engines,
+		    omap_dmm->refill_va, omap_dmm->refill_pa);
+
+err_free_dummy:
+	__free_page(omap_dmm->dummy_page);
+
+err_workaround:
+	if (omap_dmm->dmm_workaround)
+		dmm_workaround_uninit(omap_dmm);
+
+err_unmap:
+	iounmap(omap_dmm->base);
+
+err_free_dmm:
+	kfree(omap_dmm);
+	omap_dmm = NULL;
+
 	return ret;
 }
 
-- 
2.43.0

Re: [PATCH] drm/omap: dmm_tiler: fix invalid cleanup on probe failure

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [Medium] The commit message describes an approach to fixing the IRQ bug that does not match the actual code, leaving a dead write.

Pre-existing issues:
- [High] The global `containers` array retains dangling pointers to freed TCM structures after `omap_dmm_remove()` is called.
- [High] Array out-of-bounds access and potential NULL/garbage pointer dereference if DMM hardware registers return 0.
- [High] `omap_dmm_remove()` does not wait for pending DMM engines to finish before freeing the DMA refill buffer.
- [High] The global `omap_dmm` pointer is exposed to the rest of the system before the device is fully initialized.
--

commit 466b29166253ee657c1c421252c56612ce024b71
Author: Guangshuo Li <lgs201920130244@gmail.com>

drm/omap: dmm_tiler: fix invalid cleanup on probe failure

This commit fixes probe cleanup paths in the OMAP DMM tiler driver to
avoid unmapping uninitialized MMIO bases or freeing unrequested IRQs.
It replaces a single omap_dmm_remove() call on error with granular
teardown labels.

Link: stable@vger.kernel.org

> Initialize the stored IRQ to -1 and keep the IRQ number local until
> request_irq() succeeds.

[Severity: Medium]
The commit message describes keeping the IRQ number local until
request_irq() succeeds, but the patch actually stores the result of
platform_get_irq() directly into omap_dmm->irq. 

Could the commit message be updated to match the implemented approach?

> diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
> --- a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
> +++ b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
[ ... ]
> @@ -769,14 +769,14 @@ static void omap_dmm_remove(struct platform_device *dev)
>  
>  static int omap_dmm_probe(struct platform_device *dev)
>  {
[ ... ]
>  	struct resource *mem;
>  
>  	omap_dmm = kzalloc_obj(*omap_dmm);
>  	if (!omap_dmm)
> -		goto fail;
> +		return -ENOMEM;

[Severity: High]
This is a pre-existing issue, but does this expose the global omap_dmm
pointer before any of its fields are configured? 

If a concurrent GEM ioctl or async probe checks dmm_is_available() (which
only verifies omap_dmm is non-NULL), could it immediately invoke
tiler_reserve_2d() and attempt to use the uninitialized global containers
array, leading to a NULL pointer dereference?

[ ... ]
> @@ -958,7 +962,7 @@ static int omap_dmm_probe(struct platform_device *dev)
>  		dev_err(&dev->dev, "couldn't register IRQ %d, error %d\n",
>  			omap_dmm->irq, ret);
>  		omap_dmm->irq = -1;
> -		goto fail;
> +		goto err_free_tcm;

[Severity: Medium]
Because err_free_tcm eventually frees the omap_dmm structure, is this
assignment to -1 now a dead store?

[ ... ]
> -fail:
> -	omap_dmm_remove(dev);

[Severity: High]
This is a pre-existing issue, but when omap_dmm_remove() is called (for
example, during sysfs unbind), it frees the omap_dmm->tcm structures but
doesn't clear the global containers array.

Could this leave dangling pointers in the global containers array? 

Since priv->has_dmm is cached during DRM initialization, could a subsequent
GEM ioctl like DRM_IOCTL_OMAP_GEM_NEW bypass dmm_is_available(), call
tiler_reserve_2d(), and dereference the freed pointers?

[Severity: High]
This is also a pre-existing issue, but does omap_dmm_remove() safely handle
active hardware engines?

In omap_dmm_remove(), the DMA buffer is freed directly:

	if (omap_dmm->refill_va)
		dma_free_wc(omap_dmm->dev,
			    REFILL_BUFFER_SIZE * omap_dmm->num_engines,
			    omap_dmm->refill_va, omap_dmm->refill_pa);

If a hardware engine is actively processing a transaction during driver
unbind, could it continue performing DMA into the freed memory and cause
corruption or IOMMU faults?

[Severity: High]
This is another pre-existing issue in omap_dmm_probe() that isn't directly
in this diff, but can an out-of-bounds access happen if the DMM hardware is
inaccessible and registers return 0?

If DMM_PAT_HWINFO and DMM_PAT_GEOMETRY read as 0, omap_dmm->num_lut and
omap_dmm->lut_height will be 0. 

Since omap_dmm->lut_height (0) != omap_dmm->container_height (128), num_lut
increments to 1, causing the omap_dmm->tcm array to be allocated with a size
of 1.

Later in omap_dmm_probe():

	containers[TILFMT_PAGE] = omap_dmm->tcm[1];
	omap_dmm->tcm[1]->y_offset = OMAP5_LUT_OFFSET;

Would this code overflow the omap_dmm->tcm array when accessing tcm[1] and
cause memory corruption or a kernel panic?

> +err_free_tcm:
> +	for (i = 0; i < ARRAY_SIZE(containers); i++)
> +		containers[i] = NULL;
> +
> +	for (i = 0; i < omap_dmm->num_lut; i++)
> +		if (omap_dmm->tcm[i])
> +			omap_dmm->tcm[i]->deinit(omap_dmm->tcm[i]);
> +	kfree(omap_dmm->tcm);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260913091644.1510151-1-lgs201920130244@gmail.com?part=1