[PATCH] media: nuvoton: npcm-video: quiesce VCD IRQ before teardown in remove

[PATCH] media: nuvoton: npcm-video: quiesce VCD IRQ before teardown in remove

[Fan Wu]

The VCD interrupt is requested with devm_request_threaded_irq() in
npcm_video_init(), so it stays registered until the devm release that
follows npcm_video_remove().  remove() itself calls kfree(video), so a
VCD interrupt delivered between kfree(video) and that devm release
dereferences freed memory: npcm_video_irq() reads VCD_STAT off
video->vcd_regmap before the VIDEO_STREAMING flag early-return, so the
use-after-free is unconditional on delivery, not gated by streaming
state.

With streaming active, stop_streaming() (run from vb2_queue_release())
masks VCD_INTE and resets the VCD, but an in-flight handler can
re-enable VCD_INTE afterward.  On a DONE or FIFO-overrun/overflow
interrupt the handler finishes its buffer under buffer_lock and then
calls npcm_video_start_frame().  start_frame() drops buffer_lock before
it re-enables VCD_INTE (VCD_INTE_DONE_IE) and starts the next capture
(npcm_video_command() with VCD_CMD_GO); it can therefore perform those
operations after stop_streaming() masks and resets the VCD.

For the re-arm to happen, start_frame() must take buffer_lock, find a
next queued buffer, and release the lock before stop_streaming() empties
the list.  Once start_frame() has released the lock, the VCD re-enable
and capture start that follow are outside buffer_lock, so emptying the
list afterwards cannot stop them.  buffer_lock protects the buffer list
only; it is not held for the re-arm and capture start, nor for
stop_streaming()'s mask and reset, so those VCD writes are not
serialized with each other.  start_frame() returns without re-arming
when no next buffer is queued, there is no video signal, or the VCD
stays busy past its poll timeout.

That capture can complete and raise VCD_STAT_DONE; with VCD_INTE
re-armed, a new interrupt can then fire after kfree(video), and the
handler dereferences the freed per-device structure.

Unregister the video device, then mask the VCD interrupt source and
unregister and drain the threaded handler with devm_free_irq() before
releasing the vb2 buffers, the ECE state and the per-device structure.
devm_free_irq() also clears the devm action, so the later devm release
is a no-op and does not double-free.  Gating the re-arm alone would not
close the window: the handler dereferences the per-device structure
before any streaming-flag check, so any interrupt delivered after
kfree(video) is fatal regardless of re-arm.

This issue was found by an in-house static analysis tool.

Fixes: 46c15a4ff1f4 ("media: nuvoton: Add driver for NPCM video capture and encoding engine")
Cc: stable@vger.kernel.org
Assisted-by: Codex:gpt-5.5
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
---
 drivers/media/platform/nuvoton/npcm-video.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/media/platform/nuvoton/npcm-video.c b/drivers/media/platform/nuvoton/npcm-video.c
index 52505af35c08..637b1fe54824 100644
--- a/drivers/media/platform/nuvoton/npcm-video.c
+++ b/drivers/media/platform/nuvoton/npcm-video.c
@@ -120,6 +120,7 @@ struct npcm_video {
 
 	struct list_head buffers;
 	struct mutex buffer_lock; /* buffer list lock */
+	int irq;
 	unsigned long flags;
 	unsigned int sequence;
 
@@ -1707,6 +1708,7 @@ static int npcm_video_init(struct npcm_video *video)
 		dev_err(dev, "Failed to find VCD IRQ\n");
 		return -ENODEV;
 	}
+	video->irq = irq;
 
 	rc = devm_request_threaded_irq(dev, irq, NULL, npcm_video_irq,
 				       IRQF_ONESHOT, DEVICE_NAME, video);
@@ -1808,6 +1810,8 @@ static void npcm_video_remove(struct platform_device *pdev)
 	struct npcm_video *video = to_npcm_video(v4l2_dev);
 
 	video_unregister_device(&video->vdev);
+	regmap_write(video->vcd_regmap, VCD_INTE, 0);
+	devm_free_irq(dev, video->irq, video);
 	vb2_queue_release(&video->queue);
 	v4l2_ctrl_handler_free(&video->ctrl_handler);
 	v4l2_device_unregister(v4l2_dev);
-- 
2.34.1

Re: [PATCH] media: nuvoton: npcm-video: quiesce VCD IRQ before teardown in remove

[Hans Verkuil]

On 14/07/2026 04:04, Fan Wu wrote:
> The VCD interrupt is requested with devm_request_threaded_irq() in
> npcm_video_init(), so it stays registered until the devm release that
> follows npcm_video_remove().  remove() itself calls kfree(video), so a
> VCD interrupt delivered between kfree(video) and that devm release
> dereferences freed memory: npcm_video_irq() reads VCD_STAT off
> video->vcd_regmap before the VIDEO_STREAMING flag early-return, so the
> use-after-free is unconditional on delivery, not gated by streaming
> state.
> 
> With streaming active, stop_streaming() (run from vb2_queue_release())
> masks VCD_INTE and resets the VCD, but an in-flight handler can
> re-enable VCD_INTE afterward.  On a DONE or FIFO-overrun/overflow
> interrupt the handler finishes its buffer under buffer_lock and then
> calls npcm_video_start_frame().  start_frame() drops buffer_lock before
> it re-enables VCD_INTE (VCD_INTE_DONE_IE) and starts the next capture
> (npcm_video_command() with VCD_CMD_GO); it can therefore perform those
> operations after stop_streaming() masks and resets the VCD.
> 
> For the re-arm to happen, start_frame() must take buffer_lock, find a
> next queued buffer, and release the lock before stop_streaming() empties
> the list.  Once start_frame() has released the lock, the VCD re-enable
> and capture start that follow are outside buffer_lock, so emptying the
> list afterwards cannot stop them.  buffer_lock protects the buffer list
> only; it is not held for the re-arm and capture start, nor for
> stop_streaming()'s mask and reset, so those VCD writes are not
> serialized with each other.  start_frame() returns without re-arming
> when no next buffer is queued, there is no video signal, or the VCD
> stays busy past its poll timeout.
> 
> That capture can complete and raise VCD_STAT_DONE; with VCD_INTE
> re-armed, a new interrupt can then fire after kfree(video), and the
> handler dereferences the freed per-device structure.
> 
> Unregister the video device, then mask the VCD interrupt source and
> unregister and drain the threaded handler with devm_free_irq() before
> releasing the vb2 buffers, the ECE state and the per-device structure.
> devm_free_irq() also clears the devm action, so the later devm release
> is a no-op and does not double-free.  Gating the re-arm alone would not
> close the window: the handler dereferences the per-device structure
> before any streaming-flag check, so any interrupt delivered after
> kfree(video) is fatal regardless of re-arm.
> 
> This issue was found by an in-house static analysis tool.
> 
> Fixes: 46c15a4ff1f4 ("media: nuvoton: Add driver for NPCM video capture and encoding engine")
> Cc: stable@vger.kernel.org
> Assisted-by: Codex:gpt-5.5
> Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
> ---
>  drivers/media/platform/nuvoton/npcm-video.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/media/platform/nuvoton/npcm-video.c b/drivers/media/platform/nuvoton/npcm-video.c
> index 52505af35c08..637b1fe54824 100644
> --- a/drivers/media/platform/nuvoton/npcm-video.c
> +++ b/drivers/media/platform/nuvoton/npcm-video.c
> @@ -120,6 +120,7 @@ struct npcm_video {
>  
>  	struct list_head buffers;
>  	struct mutex buffer_lock; /* buffer list lock */
> +	int irq;
>  	unsigned long flags;
>  	unsigned int sequence;
>  
> @@ -1707,6 +1708,7 @@ static int npcm_video_init(struct npcm_video *video)
>  		dev_err(dev, "Failed to find VCD IRQ\n");
>  		return -ENODEV;
>  	}
> +	video->irq = irq;
>  
>  	rc = devm_request_threaded_irq(dev, irq, NULL, npcm_video_irq,
>  				       IRQF_ONESHOT, DEVICE_NAME, video);
> @@ -1808,6 +1810,8 @@ static void npcm_video_remove(struct platform_device *pdev)
>  	struct npcm_video *video = to_npcm_video(v4l2_dev);
>  
>  	video_unregister_device(&video->vdev);
> +	regmap_write(video->vcd_regmap, VCD_INTE, 0);
> +	devm_free_irq(dev, video->irq, video);
>  	vb2_queue_release(&video->queue);
>  	v4l2_ctrl_handler_free(&video->ctrl_handler);
>  	v4l2_device_unregister(v4l2_dev);

I think this can be done easier: in devm_request_threaded_irq add the IRQF_NO_AUTOEN
flag, then just call enable_irq in start_streaming and disable_irq in stop_streaming.

In npcm_video_remove() you should replace video_unregister_device by vb2_video_unregister_device
(as that ensures that stop_streaming is called if streaming is in progress) and drop
vb2_queue_release (since vb2_video_unregister_device calls that).

I think that will be a clean approach.

But nuvoton devs need to test that as well to make sure it doesn't break anything.

Regards,

	Hans

[PATCH v2] media: nuvoton: npcm-video: quiesce VCD IRQ before teardown

[Fan Wu]

The VCD IRQ is devm-requested, but npcm_video_remove() frees the video
object before devres releases that IRQ.  The threaded handler dereferences
video->vcd_regmap before checking VIDEO_STREAMING, so an interrupt in that
interval can access freed memory.

Request the IRQ with IRQF_NO_AUTOEN.  Enable it after starting capture and
setting VIDEO_STREAMING, and disable it first in stop_streaming().
disable_irq() waits for an in-flight threaded handler to finish, after
which stop_streaming() can mask and reset the VCD without a handler
re-enabling it.

Use vb2_video_unregister_device() during remove.  It releases the vb2
queue and calls stop_streaming() for an active stream, ensuring that the
IRQ is disabled before the video object is freed.  Do not release the queue
separately.

If streaming is never started, IRQF_NO_AUTOEN keeps the IRQ disabled
until devres releases it.

This issue was found by an in-house static analysis tool.

Fixes: 46c15a4ff1f4 ("media: nuvoton: Add driver for NPCM video capture and encoding engine")
Cc: stable@vger.kernel.org
Assisted-by: Codex:gpt-5.6
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
---
Changes since v1:
- Follow Hans Verkuil's suggestion to tie IRQ enablement to the streaming
  lifecycle (IRQF_NO_AUTOEN + enable_irq/disable_irq) and to use
  vb2_video_unregister_device() for teardown.

Compile-tested only; I do not have NPCM hardware, so runtime testing by
the Nuvoton maintainers would be appreciated.
---
 drivers/media/platform/nuvoton/npcm-video.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/nuvoton/npcm-video.c b/drivers/media/platform/nuvoton/npcm-video.c
index 52505af35c08..c28d9d7edd83 100644
--- a/drivers/media/platform/nuvoton/npcm-video.c
+++ b/drivers/media/platform/nuvoton/npcm-video.c
@@ -120,6 +120,7 @@ struct npcm_video {
 
 	struct list_head buffers;
 	struct mutex buffer_lock; /* buffer list lock */
+	int irq;
 	unsigned long flags;
 	unsigned int sequence;
 
@@ -1486,6 +1487,7 @@ static int npcm_video_start_streaming(struct vb2_queue *q, unsigned int count)
 	}
 
 	set_bit(VIDEO_STREAMING, &video->flags);
+	enable_irq(video->irq);
 	return 0;
 }
 
@@ -1494,6 +1496,7 @@ static void npcm_video_stop_streaming(struct vb2_queue *q)
 	struct npcm_video *video = vb2_get_drv_priv(q);
 	struct regmap *vcd = video->vcd_regmap;
 
+	disable_irq(video->irq);
 	clear_bit(VIDEO_STREAMING, &video->flags);
 	regmap_write(vcd, VCD_INTE, 0);
 	regmap_write(vcd, VCD_STAT, VCD_STAT_CLEAR);
@@ -1707,9 +1710,10 @@ static int npcm_video_init(struct npcm_video *video)
 		dev_err(dev, "Failed to find VCD IRQ\n");
 		return -ENODEV;
 	}
+	video->irq = irq;
 
 	rc = devm_request_threaded_irq(dev, irq, NULL, npcm_video_irq,
-				       IRQF_ONESHOT, DEVICE_NAME, video);
+				       IRQF_ONESHOT | IRQF_NO_AUTOEN, DEVICE_NAME, video);
 	if (rc < 0) {
 		dev_err(dev, "Failed to request IRQ %d\n", irq);
 		return rc;
@@ -1807,8 +1811,7 @@ static void npcm_video_remove(struct platform_device *pdev)
 	struct v4l2_device *v4l2_dev = dev_get_drvdata(dev);
 	struct npcm_video *video = to_npcm_video(v4l2_dev);
 
-	video_unregister_device(&video->vdev);
-	vb2_queue_release(&video->queue);
+	vb2_video_unregister_device(&video->vdev);
 	v4l2_ctrl_handler_free(&video->ctrl_handler);
 	v4l2_device_unregister(v4l2_dev);
 	if (video->ece.enable)
-- 
2.34.1