diff mbox series

[v1,2/3] media: sun4i-csi: disable interrupts when stopping streaming

Message ID f0f044148ec5c160d76e6e38d1f53393ec56578a.1786184456.git.congnt264@gmail.com (mailing list archive)
State New
Headers
Series media: sun4i-csi: fix lifecycle bugs (leak, IRQ, unbind) |

Commit Message

Cong Nguyen Aug. 8, 2026, 11:17 a.m. UTC 
sun4i_csi_start_streaming() enables the frame-done interrupt in
CSI_INT_EN_REG, but sun4i_csi_stop_streaming() only stops the capture
engine (CSI_CPT_CTRL_REG) via sun4i_csi_capture_stop(). It never disables
the interrupt source nor synchronizes with the handler.

Capture stops at the end of the current frame, so a frame-done interrupt
can still fire shortly after stop_streaming() returns. If userspace then
closes the device, sun4i_csi_release() calls pm_runtime_put() and the CSI
block is powered down (clocks gated, reset asserted). A delayed interrupt
handler would then read/write CSI registers on the gated block, which can
hang or crash the system.

Clear CSI_INT_EN_REG and call synchronize_irq() in stop_streaming(), before
returning the active buffers and freeing the scratch buffer, so no handler
can run past this point. Store the IRQ number in struct sun4i_csi so it is
available here.

Fixes: 577bbf23b758 ("media: sunxi: Add A10 CSI driver")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-4
Signed-off-by: Cong Nguyen <congnt264@gmail.com>
---
 drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.h |  1 +
 drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c | 11 +++++++++++
 2 files changed, 12 insertions(+)

Comments

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

New issues:
- [High] Missing read-back to flush the posted write before `synchronize_irq()`.

Pre-existing issues:
- [High] The error path in `sun4i_csi_start_streaming()` fails to disable the frame-done interrupt and synchronize the IRQ, causing DMA use-after-free and system crashes.
- [High] The `sun4i_csi` structure is subjected to a Use-After-Free because its embedded `video_device` uses an empty release callback.
- [High] The driver frees DMA buffers while the hardware may still be actively writing to them.
--

--- Patch [2]: [PATCH v1 2/3] media: sun4i-csi: disable interrupts when stopping streaming ---
commit dbface35706640e625b0b1ec0a6ec2b6918d0fc4
Author: Cong Nguyen <congnt264@gmail.com>

media: sun4i-csi: disable interrupts when stopping streaming

sun4i_csi_start_streaming() enables the frame-done interrupt in
CSI_INT_EN_REG, but sun4i_csi_stop_streaming() only stops the capture
engine (CSI_CPT_CTRL_REG) via sun4i_csi_capture_stop().

Clear CSI_INT_EN_REG and call synchronize_irq() in stop_streaming(), before
returning the active buffers and freeing the scratch buffer, so no handler
can run past this point.

Fixes: 577bbf23b758 ("media: sunxi: Add A10 CSI driver")
Cc: stable@vger.kernel.org

> diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.h b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.h
> index 4e0c2df45d4d2..51173faea8719 100644
> --- a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.h
> +++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.h
> @@ -112,6 +112,7 @@ struct sun4i_csi {
>  	const struct sun4i_csi_traits	*traits;
>  
>  	void __iomem			*regs;
> +	int				irq;
>  	struct clk			*bus_clk;
>  	struct clk			*isp_clk;
>  	struct clk			*ram_clk;

[Severity: High]
This isn't a regression introduced by this patch, but since the struct is
modified here, is it possible this structure suffers from a use-after-free?

The sun4i_csi structure is allocated using devm_kzalloc() in sun4i_csi_probe():

    csi = devm_kzalloc(&pdev->dev, sizeof(*csi), GFP_KERNEL);

However, the embedded video_device struct sets its release callback to
video_device_release_empty in sun4i_csi_v4l2_register():

    vdev->release = video_device_release_empty;

This breaks the V4L2 reference counting model. If the device is unbound while
a userspace application holds the video device file descriptor open,
devm_kzalloc frees the csi structure. A subsequent close of the file
descriptor or ioctl will access the freed csi structure, leading to a
use-after-free.

[ ... ]

> diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c b/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c
> index 4781db21c2055..8804189a6e38c 100644
> --- a/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c
> +++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c
> @@ -356,6 +356,16 @@ static void sun4i_csi_stop_streaming(struct vb2_queue *vq)
>  	v4l2_subdev_call(csi->src_subdev, video, s_stream, 0);
>  	sun4i_csi_capture_stop(csi);
>  
> +	/*
> +	 * Disable the frame done interrupt and wait for the handler to
> +	 * finish. A frame may complete right as capture is stopped, so an
> +	 * interrupt can still be pending here; without this the handler could
> +	 * run after the device is powered down (pm_runtime_put() on release)
> +	 * and access registers on a gated block.
> +	 */
> +	writel(0, csi->regs + CSI_INT_EN_REG);
> +	synchronize_irq(csi->irq);

[Severity: High]
Does this regression need a read back to flush the posted write before calling
synchronize_irq()?

writel() performs a posted write on the memory bus, meaning it is buffered
and may not reach the interrupt controller immediately. Without a dummy
readl() to the same register to flush the write buffer, synchronize_irq()
may execute and return before the hardware actually disables the interrupt.

This could leave a race window where the device could raise a new interrupt
just after synchronize_irq() finishes, leading to a handler running after
the device is powered down or buffers are freed.

> +
>  	/* Release all active buffers */
>  	spin_lock_irqsave(&csi->qlock, flags);
>  	return_all_buffers(csi, VB2_BUF_STATE_ERROR);

[Severity: High]
This isn't a regression introduced by this patch, but could this code free DMA
buffers while the hardware may still be actively writing to them?

As noted in the commit message, stopping the capture engine via
sun4i_csi_capture_stop() only stops it at the end of the current frame.
However, sun4i_csi_stop_streaming() immediately frees the DMA scratch buffer
and returns all active buffers to the core without waiting for the hardware
to become idle.

Although the interrupt is disabled, the hardware DMA engine remains active
and will continue writing the remainder of the frame into the freed scratch
memory or into buffers that userspace might have already reclaimed.

Additionally, would the error path in sun4i_csi_start_streaming() also suffer
from the same problem?

drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c:sun4i_csi_start_streaming() {
    ...
    ret = v4l2_subdev_call(csi->src_subdev, video, s_stream, 1);
    if (ret < 0 && ret != -ENOIOCTLCMD)
        goto err_disable_device;

    return 0;

err_disable_device:
    sun4i_csi_capture_stop(csi);
    ...
err_free_scratch_buffer:
    dma_free_coherent(csi->dev, csi->scratch.size, csi->scratch.vaddr,
                      csi->scratch.paddr);

err_clear_dma_queue:
    spin_lock_irqsave(&csi->qlock, flags);
    return_all_buffers(csi, VB2_BUF_STATE_QUEUED);
    spin_unlock_irqrestore(&csi->qlock, flags);
    ...
}

The err_disable_device path stops capture but does not disable the interrupt
or wait for the hardware to finish before freeing the scratch buffer and
returning all queued buffers.
diff mbox series

Patch

diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.h b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.h
index 4e0c2df45d4d..51173faea871 100644
--- a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.h
+++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.h
@@ -112,6 +112,7 @@  struct sun4i_csi {
 	const struct sun4i_csi_traits	*traits;
 
 	void __iomem			*regs;
+	int				irq;
 	struct clk			*bus_clk;
 	struct clk			*isp_clk;
 	struct clk			*ram_clk;
diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c b/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c
index e911c7f7acc5..da697f39f2bc 100644
--- a/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c
+++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c
@@ -354,6 +354,16 @@  static void sun4i_csi_stop_streaming(struct vb2_queue *vq)
 	v4l2_subdev_call(csi->src_subdev, video, s_stream, 0);
 	sun4i_csi_capture_stop(csi);
 
+	/*
+	 * Disable the frame done interrupt and wait for the handler to
+	 * finish. A frame may complete right as capture is stopped, so an
+	 * interrupt can still be pending here; without this the handler could
+	 * run after the device is powered down (pm_runtime_put() on release)
+	 * and access registers on a gated block.
+	 */
+	writel(0, csi->regs + CSI_INT_EN_REG);
+	synchronize_irq(csi->irq);
+
 	/* Release all active buffers */
 	spin_lock_irqsave(&csi->qlock, flags);
 	return_all_buffers(csi, VB2_BUF_STATE_ERROR);
@@ -438,6 +448,7 @@  int sun4i_csi_dma_register(struct sun4i_csi *csi, int irq)
 		dev_err(csi->dev, "Couldn't register our interrupt\n");
 		goto err_unregister_device;
 	}
+	csi->irq = irq;
 
 	return 0;