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

[sashiko-bot@kernel.org]

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