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

[Hans Verkuil <hverkuil+cisco@kernel.org>]

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