[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

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

[Hans Verkuil]

Can Nuvoton test this patch? I feel happier if this is given a quick test.

Regards,

	Hans

On 16/07/2026 12:15, Fan Wu wrote:
> 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)

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

[KFLIN]

Hi Hans, Fan,

I've tested this patch on our platform and did not observe any issues.
Thanks for the patch.

Tested-by: Marvin Lin <kflin@nuvoton.com>

Regards,
Marvin

> -----Original Message-----
> From: Hans Verkuil <hverkuil+cisco@kernel.org>
> Sent: Tuesday, July 28, 2026 8:12 PM
> To: Fan Wu <fanwu01@zju.edu.cn>; CS20 KWLiu <KWLIU@nuvoton.com>;
> CS20 KFLin <KFLIN@nuvoton.com>
> Cc: hverkuil@kernel.org; mchehab@kernel.org; linux-media@vger.kernel.org;
> openbmc@lists.ozlabs.org; linux-kernel@vger.kernel.org;
> stable@vger.kernel.org
> Subject: Re: [PATCH v2] media: nuvoton: npcm-video: quiesce VCD IRQ before
> teardown
>
> CAUTION - External Email: Do not click links or open attachments unless you
> acknowledge the sender and content.
>
>
> Can Nuvoton test this patch? I feel happier if this is given a quick test.
>
> Regards,
>
>         Hans
>
> On 16/07/2026 12:15, Fan Wu wrote:
> > 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
> > video->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)

________________________________
________________________________
 The privileged confidential information contained in this email is intended for use only by the addressees as indicated by the original sender of this email. If you are not the addressee indicated in this email or are not responsible for delivery of the email to such a person, please kindly reply to the sender indicating this fact and delete all copies of it from your computer and network server immediately. Your cooperation is highly appreciated. It is advised that any unauthorized use of confidential information of Nuvoton is strictly prohibited; and any information in this email irrelevant to the official business of Nuvoton shall be deemed as neither given nor endorsed by Nuvoton.