Re: [PATCH v2 2/3] media: sun4i-csi: disable interrupts when stopping streaming

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] DMA use-after-free and memory corruption due to freeing buffers while capture engine is still running
--

--- Patch [2]: [PATCH v2 2/3] media: sun4i-csi: disable interrupts when stopping streaming ---
commit f908e51d92a92db663d06b38deb5bee451c659e7
Author: Cong Nguyen <[email protected]>

media: sun4i-csi: disable interrupts when stopping streaming

This commit addresses a crash where a delayed frame-done interrupt could
access the device after it is powered down. It fixes this by explicitly
disabling the interrupt and waiting for the handler to complete before
proceeding with device shutdown.

Cc: [email protected]

> diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c b/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c
> index 4781db21c2055..3401a91cb310f 100644
> --- a/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c
> +++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c
[ ... ]
> @@ -330,6 +347,7 @@ static int sun4i_csi_start_streaming(struct vb2_queue *vq, unsigned int count)
>  
>  err_disable_device:
>  	sun4i_csi_capture_stop(csi);
> +	sun4i_csi_disable_irq(csi);
>  
>  err_disable_pipeline:
>  	video_device_pipeline_stop(&csi->vdev);

[Severity: Critical]
This isn't a bug introduced by this patch, but does the hardware DMA engine
continue writing to memory after sun4i_csi_capture_stop() returns?

As noted in the commit message, capture stops at the end of the current
frame. If the DMA engine is still running, it looks like we might free the
scratch buffer immediately afterwards in the error path:

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

Could this cause memory corruption if the hardware writes to the freed
scratch buffer before it finishes the current frame?

> @@ -355,6 +373,7 @@ 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);
> +	sun4i_csi_disable_irq(csi);
>  
>  	/* Release all active buffers */
>  	spin_lock_irqsave(&csi->qlock, flags);

[Severity: Critical]
This is a pre-existing issue, but similar to the error path above, are we
returning active buffers to userspace while the DMA engine might still be
actively writing to them?

	sun4i_csi_capture_stop(csi);
	sun4i_csi_disable_irq(csi);

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

	video_device_pipeline_stop(&csi->vdev);

	dma_free_coherent(csi->dev, csi->scratch.size, csi->scratch.vaddr,
			  csi->scratch.paddr);

If capture stops asynchronously at the end of the frame, returning these
buffers and freeing the scratch buffer immediately afterwards could allow
the DMA engine to overwrite memory that has already been reclaimed or
reused. Is there a need to poll or wait for the DMA engine to completely
halt before cleaning up?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2