mbox series

[v2,0/6] media: Use devm_of_reserved_mem_device_init()

Message ID 20260902200703.2016410-1-mukesh.ojha@oss.qualcomm.com (mailing list archive)
Headers
Series media: Use devm_of_reserved_mem_device_init() |

Message

Mukesh Ojha Sept. 2, 2026, 8:06 p.m. UTC 
Several media drivers manually manage reserved memory lifetime by calling
of_reserved_mem_device_init() in probe and of_reserved_mem_device_release()
in remove. Drivers that fail to release on every error path between the two
calls leak the reserved memory region until the driver is unloaded.

devm_of_reserved_mem_device_init() was recently introduced to tie the
release to the device's devres lifetime automatically. Convert the affected
media drivers to use it, removing the manual release calls and any wrapper
devres actions.

Changes since v1: https://lore.kernel.org/lkml/20260703193855.110619-2-mukesh.ojha@oss.qualcomm.com/
 - Carried R-b tag if any.
 - Rebased it.

Mukesh Ojha (6):
  media: synopsys: hdmirx: Use devm_of_reserved_mem_device_init()
  media: aspeed: Use devm_of_reserved_mem_device_init()
  media: nuvoton: npcm-video: Use devm_of_reserved_mem_device_init()
  media: arm: mali-c55: Use devm_of_reserved_mem_device_init()
  media: mediatek: vpu: Use devm_of_reserved_mem_device_init()
  staging: media: cedrus: Use devm_of_reserved_mem_device_init()

 .../media/platform/arm/mali-c55/mali-c55-core.c  | 12 +++---------
 drivers/media/platform/aspeed/aspeed-video.c     | 12 ++++--------
 drivers/media/platform/mediatek/vpu/mtk_vpu.c    |  3 +--
 drivers/media/platform/nuvoton/npcm-video.c      |  9 ++-------
 .../media/platform/synopsys/hdmirx/snps_hdmirx.c | 16 ++--------------
 drivers/staging/media/sunxi/cedrus/cedrus_hw.c   |  9 ++-------
 6 files changed, 14 insertions(+), 47 deletions(-)

Comments

sashiko-bot@kernel.org Sept. 2, 2026, 8:17 p.m. UTC | #1 
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Potential use-after-free of the driver's private `video` structure in the interrupt handler due to devres lifecycle mismatch.
- [High] V4L2 lifecycle mismatch causes a use-after-free of the embedded `video_device` structure.
- [High] Device-managed allocations are incorrectly attached to an external child device, causing memory leaks and preventing reprobing.
--

commit 5d2eb460720d228622a94970ff2802d5f5591843
Author: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>

media: nuvoton: npcm-video: Use devm_of_reserved_mem_device_init()

Switch to devm_of_reserved_mem_device_init() so the reserved memory
region is released automatically on probe failure or device removal.
Remove the four explicit of_reserved_mem_device_release() calls and
collapse the now-empty err_release_mem label into err_free.

> diff --git a/drivers/media/platform/nuvoton/npcm-video.c b/drivers/media/platform/nuvoton/npcm-video.c
> index 52505af35c087..6786736c8f07c 100644
> --- a/drivers/media/platform/nuvoton/npcm-video.c
> +++ b/drivers/media/platform/nuvoton/npcm-video.c
> @@ -1715,17 +1715,15 @@ static int npcm_video_init(struct npcm_video *video)
>  		return rc;
>  	}
>  
> -	of_reserved_mem_device_init(dev);
> +	devm_of_reserved_mem_device_init(dev);
>  	rc = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
>  	if (rc) {
>  		dev_err(dev, "Failed to set DMA mask\n");
> -		of_reserved_mem_device_release(dev);
>  		return rc;
>  	}
>  
>  	rc = npcm_video_ece_init(video);

[Severity: High]
This is a pre-existing issue, but could this leak resources when the
npcm-video driver is unloaded?

In npcm_video_ece_init() in drivers/media/platform/nuvoton/npcm-video.c,
the driver fetches the external device node ece_pdev and uses its device
pointer for devres allocations:

	regs = devm_platform_ioremap_resource(ece_pdev, 0);
	...
	video->ece.reset = devm_reset_control_get(ece_dev, NULL);

Since these allocations are tied to the external nuvoton,ece device's
lifecycle rather than the driver's device, the resources won't be released
when the npcm-video driver unbinds. If the driver is re-probed, could the
ioremap request fail with -EBUSY and permanently break the driver?

>  	if (rc) {
> -		of_reserved_mem_device_release(dev);
>  		dev_err(dev, "Failed to initialize ECE\n");
>  		return rc;
>  	}

[ ... ]

> @@ -1789,13 +1787,11 @@ static int npcm_video_probe(struct platform_device *pdev)
>  
>  	rc = npcm_video_setup_video(video);
>  	if (rc)
> -		goto err_release_mem;
> +		goto err_free;
>  
>  	dev_info(video->dev, "NPCM video driver probed\n");
>  	return 0;
>  
> -err_release_mem:
> -	of_reserved_mem_device_release(&pdev->dev);
>  err_free:
>  	kfree(video);

[Severity: High]
This is a pre-existing issue, but can this cause a use-after-free of the
driver's private video structure in the interrupt handler?

If npcm_video_probe() in drivers/media/platform/nuvoton/npcm-video.c fails
here, the driver manually calls kfree(video). However, earlier in
npcm_video_init(), the interrupt was registered using
devm_request_threaded_irq().

Because devres cleanup happens after the probe function returns, the IRQ
remains registered and capable of firing while the video structure has already
been freed. If an interrupt fires during this window, won't the handler
execute and access the freed structure?

>  	return rc;
> @@ -1814,7 +1810,6 @@ static void npcm_video_remove(struct platform_device *pdev)
>  	if (video->ece.enable)
>  		npcm_video_ece_stop(video);
>  	kfree(video);

[Severity: High]
This is a pre-existing issue, but can this kfree() also trigger a
use-after-free due to the devm-managed IRQ lifecycle, similar to the probe
error path?

In npcm_video_remove() in drivers/media/platform/nuvoton/npcm-video.c, the
devres cleanup won't disable the interrupt until after the remove function
finishes. This leaves a window where the IRQ can fire and access the freed
video structure.

[Severity: High]
This isn't a bug introduced by this patch, but can this cause a
use-after-free if userspace still has the V4L2 device open?

In npcm_video_setup_video() in drivers/media/platform/nuvoton/npcm-video.c,
the driver embeds struct video_device inside its dynamically allocated
struct npcm_video and sets its .release callback to
video_device_release_empty.

Since video_unregister_device() in npcm_video_remove() does not wait for file
descriptors to be closed, could subsequent userspace operations access the
embedded vdev after this kfree() has freed the enclosing structure?

> -	of_reserved_mem_device_release(dev);
>  }
sashiko-bot@kernel.org Sept. 2, 2026, 8:17 p.m. UTC | #2 
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Pre-existing race condition leading to a use-after-free. The driver uses `devm_request_irq()` to manage its interrupt, but manually destroys the workqueue (`vpu->wdt.wq`) in `mtk_vpu_remove()`. Because devres cleans up the IRQ only after `remove()` completes, the interrupt remains active while the workqueue is freed.
--

commit 246438dea9de2810275787060608206f1c3a4872
Author: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>

media: mediatek: vpu: Use devm_of_reserved_mem_device_init()

This patch simplifies the driver by replacing manual
of_reserved_mem_device_init() and of_reserved_mem_device_release()
calls with the managed devm_of_reserved_mem_device_init() equivalent.

> diff --git a/drivers/media/platform/mediatek/vpu/mtk_vpu.c b/drivers/media/platform/mediatek/vpu/mtk_vpu.c
> --- a/drivers/media/platform/mediatek/vpu/mtk_vpu.c
> +++ b/drivers/media/platform/mediatek/vpu/mtk_vpu.c
[ ... ]
> @@ -930,7 +930,6 @@ static int mtk_vpu_probe(struct platform_device *pdev)
>  free_d_mem:
>  	vpu_free_ext_mem(vpu, D_FW);
>  remove_debugfs:
> -	of_reserved_mem_device_release(dev);

[Severity: High]
This is a pre-existing issue, but while reviewing this devres conversion, I
noticed a potential use-after-free race condition related to device teardown.

In mtk_vpu_probe(), the interrupt is registered as managed using
devm_request_irq(), meaning it will remain active until devres cleanup runs
after mtk_vpu_remove() completes.

However, the workqueue is manually destroyed during mtk_vpu_remove():

drivers/media/platform/mediatek/vpu/mtk_vpu.c:mtk_vpu_remove() {
    ...
    if (vpu->wdt.wq)
        destroy_workqueue(vpu->wdt.wq);
    ...
}

If a hardware interrupt fires after destroy_workqueue() executes but before
the devres cleanup disables the IRQ, vpu_irq_handler() will attempt to
access the freed workqueue structure:

drivers/media/platform/mediatek/vpu/mtk_vpu.c:vpu_irq_handler() {
    ...
    queue_work(vpu->wdt.wq, &vpu->wdt.ws);
    ...
}

Can this sequence cause a use-after-free if the hardware triggers an
interrupt late during the unbind process?
sashiko-bot@kernel.org Sept. 2, 2026, 8:19 p.m. UTC | #3 
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Race condition between runtime PM autosuspend and manual power-off during device removal and probe error paths.
- [High] The IRQ handler is never freed if the device is removed while in the active runtime PM state.
- [High] Media nodes are registered and exposed to userspace before the device's IRQ is requested, breaking initial video capture.
--

commit c75be1887c52469e855778c08a0a6679f1af97b0
Author: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>

media: arm: mali-c55: Use devm_of_reserved_mem_device_init()

This commit refactors the driver to use the devres-managed
devm_of_reserved_mem_device_init() instead of manual initialization and
cleanup.

> diff --git a/drivers/media/platform/arm/mali-c55/mali-c55-core.c b/drivers/media/platform/arm/mali-c55/mali-c55-core.c
> index f28e9f4354aca..3d37cd4465838 100644
> --- a/drivers/media/platform/arm/mali-c55/mali-c55-core.c
> +++ b/drivers/media/platform/arm/mali-c55/mali-c55-core.c
> @@ -803,14 +803,12 @@ static int mali_c55_probe(struct platform_device *pdev)
>  	if (ret)
>  		return dev_err_probe(dev, ret, "failed to acquire resets\n");
>  
> -	of_reserved_mem_device_init(dev);
> +	devm_of_reserved_mem_device_init(dev);
>  	vb2_dma_contig_set_max_seg_size(dev, UINT_MAX);
>  
>  	ret = __mali_c55_power_on(mali_c55);

[Severity: High]
This is a pre-existing issue, but are the media nodes registered and exposed
to userspace before the device's IRQ is requested?

Later in mali_c55_probe():

	ret = mali_c55_media_frameworks_init(mali_c55);
	if (ret)
		goto err_pm_runtime_disable;

	pm_runtime_idle(&pdev->dev);

	mali_c55->irqnum = platform_get_irq(pdev, 0);

The device is explicitly set to RPM_ACTIVE during probe, but the IRQ handler
is only requested inside mali_c55_runtime_resume().

If userspace opens the device and begins streaming before the autosuspend
delay forces a suspend cycle, the device will process frames without an IRQ
handler. Will this cause video streaming to hang indefinitely because the
capture buffers are never completed?

> -	if (ret) {
> -		dev_err_probe(dev, ret, "failed to power on\n");
> -		goto err_release_mem;
> -	}
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to power on\n");
>  
>  	ret = mali_c55_check_hwcfg(mali_c55);
>  	if (ret)

[ ... ]

> @@ -849,9 +847,6 @@ static int mali_c55_probe(struct platform_device *pdev)
>  	kfree(mali_c55->context.registers);
>  err_power_off:
>  	__mali_c55_power_off(mali_c55);

[Severity: High]
This is a pre-existing issue, but is there a risk of a double power-off
here if the device successfully auto-suspended before pm_runtime_disable()
is called?

If the autosuspend timer triggered, calling __mali_c55_power_off() again
could lead to imbalanced clock disables (calling clk_bulk_disable_unprepare()
without a matching enable) and reset assertions, potentially causing kernel
warnings in the clock and reset cores.

> -err_release_mem:
> -	of_reserved_mem_device_release(dev);
> -
>  	return ret;
>  }
>  
> @@ -866,7 +861,6 @@ static void mali_c55_remove(struct platform_device *pdev)
>  	}

[Severity: High]
This is a pre-existing issue, but does this manual power-off in
mali_c55_remove() create a race window with concurrent autosuspend?

In mali_c55_remove(), if the device is not suspended:

	if (!pm_runtime_suspended(&pdev->dev)) {
		__mali_c55_power_off(mali_c55);
		pm_runtime_set_suspended(&pdev->dev);
	}

Because pm_runtime_disable() is called after this check, an asynchronous
autosuspend could execute concurrently. Could this result in both threads
calling __mali_c55_power_off() at the same time?

[Severity: High]
This is a pre-existing issue, but is it possible that bypassing the runtime
suspend callback here causes an IRQ resource leak?

The driver dynamically registers its IRQ handler in mali_c55_runtime_resume()
and frees it in mali_c55_runtime_suspend(). If the device is active during
removal, open-coding the power-off with __mali_c55_power_off() bypasses the
free_irq() call.

Could a subsequent interrupt on this line (or shared IRQ) trigger the handler
and cause a use-after-free when accessing the freed device context?

>  	pm_runtime_disable(&pdev->dev);
>  	kfree(mali_c55->context.registers);
> -	of_reserved_mem_device_release(&pdev->dev);
>  }