[PATCH] drm/adp: Free FE IRQ on bind failure

[PATCH] drm/adp: Free FE IRQ on bind failure

[Myeonghun Pak]

adp_drm_bind() requests the front-end IRQ before registering the DRM
device. If drm_dev_register() fails, the function returns immediately
and leaves the IRQ handler registered.

Release the IRQ on the drm_dev_register() error path. The normal unbind
path still frees the IRQ after successful registration.

Fixes: 332122eba628 ("drm: adp: Add Apple Display Pipe driver")
Co-developed-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Myeonghun Pak <mhun512@gmail.com>

---
 drivers/gpu/drm/adp/adp_drv.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/adp/adp_drv.c b/drivers/gpu/drm/adp/adp_drv.c
index 4554cf75565e..20f38543d04e 100644
--- a/drivers/gpu/drm/adp/adp_drv.c
+++ b/drivers/gpu/drm/adp/adp_drv.c
@@ -535,9 +535,13 @@ static int adp_drm_bind(struct device *dev)
 
 	err = drm_dev_register(&adp->drm, 0);
 	if (err)
-		return err;
+		goto err_free_irq;
 
 	return 0;
+
+err_free_irq:
+	free_irq(adp->fe_irq, adp);
+	return err;
 }
 
 static void adp_drm_unbind(struct device *dev)
-- 
2.47.1

Re: [PATCH] drm/adp: Free FE IRQ on bind failure

[sashiko-bot]

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

Pre-existing issues:
- [High] Calling drmm_* initializations inside the component bind callback leads to memory leaks and DRM structure corruption if bind fails and is retried.
- [High] Calling dma_alloc_coherent() with GFP_KERNEL in an atomic context.
- [High] Missing NULL check for the return value of dma_alloc_coherent().
- [High] adp_drm_gem_dumb_create modifies height after validation, causing a 32-bit integer overflow and undersized buffer allocation.
--

commit 8a1bec26bfe0bb63ece93f656fc4dc72b509f897
Author: Myeonghun Pak <mhun512@gmail.com>

drm/adp: Free FE IRQ on bind failure

adp_drm_bind() requests the front-end IRQ before registering the DRM
device. If drm_dev_register() fails, the function returns immediately
and leaves the IRQ handler registered.

> diff --git a/drivers/gpu/drm/adp/adp_drv.c b/drivers/gpu/drm/adp/adp_drv.c
> index ce9dd6222634d..b7fc456043f34 100644
> --- a/drivers/gpu/drm/adp/adp_drv.c
> +++ b/drivers/gpu/drm/adp/adp_drv.c
> @@ -533,9 +533,13 @@ static int adp_drm_bind(struct device *dev)
>  
>  	err = drm_dev_register(&adp->drm, 0);
>  	if (err)
> -		return err;
> +		goto err_free_irq;
>  
>  	return 0;
> +
> +err_free_irq:
> +	free_irq(adp->fe_irq, adp);
> +	return err;
>  }

[Severity: High]
This is a pre-existing issue, but does calling drmm_ initializations inside
the component bind callback lead to memory leaks if bind fails here?

The drm_device is allocated in probe with devm_, so it persists across bind
retries:

drivers/gpu/drm/adp/adp_drv.c:adp_probe() {
    ...
	adp = devm_drm_dev_alloc(&pdev->dev, &adp_driver, struct adp_drv_private, drm);
    ...
}

But adp_drm_bind() calls adp_setup_mode_config(), which initializes drmm_
resources:

drivers/gpu/drm/adp/adp_drv.c:adp_drm_bind() {
    ...
	err = adp_setup_mode_config(adp);
    ...
}

If a later step in bind fails (such as drm_dev_register()) and bind is
retried, will re-running drmm_mode_config_init() on the same drm_device
re-initialize dev->mode_config mutexes and IDRs without destroying the old
ones, leading to memory leaks and corruption?

[Severity: High]
This is a pre-existing issue, but is it safe to use GFP_KERNEL in the atomic
commit tail path?

drivers/gpu/drm/adp/adp_drv.c:adp_crtc_atomic_flush() {
    ...
		if (new_size != 0) {
			adp->mask_buf = dma_alloc_coherent(crtc->dev->dev, new_size,
							   &adp->mask_iova, GFP_KERNEL);
			memset(adp->mask_buf, 0xFF, new_size);
			writel(adp->mask_iova, adp->be + ADBE_MASK_BUF);
		}
    ...
}

Since adp_crtc_atomic_flush() is part of the atomic commit pipeline, it runs
in an atomic context where sleeping is forbidden.

Does calling dma_alloc_coherent() with GFP_KERNEL here allow the function to
sleep, causing kernel warnings or deadlocks?

[Severity: High]
This is a pre-existing issue, but is there a missing NULL check for the
return value of dma_alloc_coherent() here?

drivers/gpu/drm/adp/adp_drv.c:adp_crtc_atomic_flush() {
    ...
			adp->mask_buf = dma_alloc_coherent(crtc->dev->dev, new_size,
							   &adp->mask_iova, GFP_KERNEL);
			memset(adp->mask_buf, 0xFF, new_size);
    ...
}

If memory is exhausted and dma_alloc_coherent() returns NULL, will the
immediate memset() cause a NULL pointer dereference?

[Severity: High]
This is a pre-existing issue, but does aligning args->height after validation
cause an integer overflow?

drivers/gpu/drm/adp/adp_drv.c:adp_drm_gem_dumb_create() {
    ...
	args->height = ALIGN(args->height, 64);
	args->size = args->pitch * args->height;

	return drm_gem_dma_dumb_create_internal(file_priv, drm, args);
}

If userspace requests dimensions that pass the core drm_mode_create_dumb()
checks but overflow when args->height is increased, will the 32-bit
multiplication in drm_gem_dma_dumb_create_internal() wrap around?

Could this result in an undersized DMA buffer allocation that allows
out-of-bounds memory accesses when userspace maps and writes to it?

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