diff mbox series

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

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

Commit Message

Mukesh Ojha Sept. 2, 2026, 8:06 p.m. UTC 
Switch to devm_of_reserved_mem_device_init() so the reserved memory
region is released automatically on probe failure or device removal.
Remove the three explicit of_reserved_mem_device_release() calls and
rename the error label from err_release_reserved_mem to
err_unprepare_vclk to reflect that it now only unwinds clock state.

Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
---
 drivers/media/platform/aspeed/aspeed-video.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

Comments

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

Pre-existing issues:
- [High] Preexisting use-after-free of `struct aspeed_video` on driver unbind due to devm_kzalloc and embedded video_device.
- [High] Preexisting use-after-free/corruption of CMA reserved memory on driver unbind.
- [High] IRQ registered and unmasked before device clocks are enabled.
- [High] Uncanceled delayed work accesses freed memory on device removal.
--

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

media: aspeed: 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 three explicit of_reserved_mem_device_release() calls and
rename the error label from err_release_reserved_mem to
err_unprepare_vclk to reflect that it now only unwinds clock state.

> diff --git a/drivers/media/platform/aspeed/aspeed-video.c b/drivers/media/platform/aspeed/aspeed-video.c
> index a292275f6b7b9..7ba9314fdbd2a 100644
> --- a/drivers/media/platform/aspeed/aspeed-video.c
> +++ b/drivers/media/platform/aspeed/aspeed-video.c
> @@ -2267,19 +2267,19 @@ static int aspeed_video_init(struct aspeed_video *video)
>  	if (rc)
>  		goto err_unprepare_eclk;
>  
> -	of_reserved_mem_device_init(dev);
> +	devm_of_reserved_mem_device_init(dev);

[Severity: High]
This isn't a bug introduced by this patch, but does tying the CMA reserved
memory release to devres cleanup here risk a use-after-free or corruption
of CMA reserved memory on driver unbind?

If a user holds an open file descriptor when the device is unbound, active
VB2 DMA buffers may still exist. When the user later closes the file
descriptor, VB2 calls dma_free_coherent() on the buffers:

drivers/media/platform/aspeed/aspeed-video.c:aspeed_video_remove() {
    ...
    vb2_video_unregister_device(&video->vdev);
    ...
}

Since devres releases the CMA region immediately after remove() returns,
could the DMA framework incorrectly handle the free operation on the detached
region and corrupt allocator pools?

As another pre-existing issue in this initialization path, the interrupt
handler is registered earlier in this function:

drivers/media/platform/aspeed/aspeed-video.c:aspeed_video_init() {
    ...
    rc = devm_request_threaded_irq(dev, irq, NULL, aspeed_video_irq,
                                   IRQF_ONESHOT, DEVICE_NAME, video);
    ...
}

This registers and unmasks the IRQ before the device clocks are actually
enabled. If the IRQ line is shared or the bootloader left the device in an
active state, could the interrupt handler fire immediately and access device
registers via aspeed_video_read(), causing a bus fault because the clocks
are disabled?

[ ... ]
> @@ -2343,7 +2342,6 @@ static int aspeed_video_probe(struct platform_device *pdev)
>  	rc = aspeed_video_setup_video(video);
>  	if (rc) {
>  		aspeed_video_free_buf(video, &video->jpeg);
> -		of_reserved_mem_device_release(&pdev->dev);
>  		clk_unprepare(video->vclk);

[Severity: High]
This is a pre-existing issue, but could the use of devres for the driver
context allocation in probe lead to a use-after-free on unbind?

drivers/media/platform/aspeed/aspeed-video.c:aspeed_video_probe() {
    ...
    video = devm_kzalloc(&pdev->dev, sizeof(*video), GFP_KERNEL);
    ...
}

Because vdev->release is set to video_device_release_empty in
aspeed_video_setup_video(), the V4L2 core does not defer freeing the memory:

drivers/media/platform/aspeed/aspeed-video.c:aspeed_video_setup_video() {
    ...
    vdev->release = video_device_release_empty;
    ...
}

When a user closes the file descriptor after an unbind, devres will have
already freed the struct aspeed_video, but aspeed_video_release() will
still try to access it:

drivers/media/platform/aspeed/aspeed-video.c:aspeed_video_release() {
    ...
    aspeed_video_stop(video);
    ...
}

Could this result in memory corruption when the last user closes the file
descriptor?

[ ... ]
> @@ -2374,8 +2372,6 @@ static void aspeed_video_remove(struct platform_device *pdev)
>  	v4l2_device_unregister(v4l2_dev);
>  
>  	aspeed_video_free_buf(video, &video->jpeg);
> -
> -	of_reserved_mem_device_release(dev);
>  }

[Severity: High]
This is a pre-existing issue, but does this remove function lack a
cancellation of the delayed resolution work, potentially causing a
use-after-free?

A resolution change interrupt can schedule video->res_work:

drivers/media/platform/aspeed/aspeed-video.c:aspeed_video_irq_res_change() {
    ...
    schedule_delayed_work(&video->res_work, delay);
    ...
}

If a driver unbind occurs before the delay expires, aspeed_video_remove()
turns off the hardware but does not call
cancel_delayed_work_sync(&video->res_work).

Could the workqueue execute aspeed_video_resolution_work() after devres
has freed the struct aspeed_video, resulting in a use-after-free?
diff mbox series

Patch

diff --git a/drivers/media/platform/aspeed/aspeed-video.c b/drivers/media/platform/aspeed/aspeed-video.c
index a292275f6b7b..7ba9314fdbd2 100644
--- a/drivers/media/platform/aspeed/aspeed-video.c
+++ b/drivers/media/platform/aspeed/aspeed-video.c
@@ -2267,19 +2267,19 @@  static int aspeed_video_init(struct aspeed_video *video)
 	if (rc)
 		goto err_unprepare_eclk;
 
-	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");
-		goto err_release_reserved_mem;
+		goto err_unprepare_vclk;
 	}
 
 	if (!aspeed_video_alloc_buf(video, &video->jpeg,
 				    VE_JPEG_HEADER_SIZE)) {
 		dev_err(dev, "Failed to allocate DMA for JPEG header\n");
 		rc = -ENOMEM;
-		goto err_release_reserved_mem;
+		goto err_unprepare_vclk;
 	}
 	dev_info(video->dev, "alloc mem size(%d) at %pad for jpeg header\n",
 		 VE_JPEG_HEADER_SIZE, &video->jpeg.dma);
@@ -2288,8 +2288,7 @@  static int aspeed_video_init(struct aspeed_video *video)
 
 	return 0;
 
-err_release_reserved_mem:
-	of_reserved_mem_device_release(dev);
+err_unprepare_vclk:
 	clk_unprepare(video->vclk);
 err_unprepare_eclk:
 	clk_unprepare(video->eclk);
@@ -2343,7 +2342,6 @@  static int aspeed_video_probe(struct platform_device *pdev)
 	rc = aspeed_video_setup_video(video);
 	if (rc) {
 		aspeed_video_free_buf(video, &video->jpeg);
-		of_reserved_mem_device_release(&pdev->dev);
 		clk_unprepare(video->vclk);
 		clk_unprepare(video->eclk);
 		return rc;
@@ -2374,8 +2372,6 @@  static void aspeed_video_remove(struct platform_device *pdev)
 	v4l2_device_unregister(v4l2_dev);
 
 	aspeed_video_free_buf(video, &video->jpeg);
-
-	of_reserved_mem_device_release(dev);
 }
 
 static struct platform_driver aspeed_video_driver = {