[PATCH v1 0/3] media: sun4i-csi: fix lifecycle bugs (leak, IRQ, unbind)

Cong Nguyen posted 3 patches 1 month, 3 weeks ago
.../platform/sunxi/sun4i-csi/sun4i_csi.c      | 25 +++++++++++++++++--
.../platform/sunxi/sun4i-csi/sun4i_csi.h      |  1 +
.../platform/sunxi/sun4i-csi/sun4i_dma.c      | 11 ++++++++
3 files changed, 35 insertions(+), 2 deletions(-)
[PATCH v1 0/3] media: sun4i-csi: fix lifecycle bugs (leak, IRQ, unbind)
Posted by Cong Nguyen 1 month, 3 weeks ago
While addressing an async-notifier cleanup leak in this driver, I audited
the surrounding device lifecycle and found three separate pre-existing
issues, all present since the driver was introduced. They are independent
of each other; this series groups them because they touch the same driver.

1/3: sun4i_csi_notify_complete() registers the video device and the bridge
     subdev before creating the media links. On a failure in the later
     steps it only unregistered the media device, leaving /dev/videoX
     registered. Since probe then aborts and the devm-managed context
     (embedding the video_device) is freed, an open() would touch freed
     memory. Unwind the registrations in reverse order.

2/3: sun4i_csi_stop_streaming() stops the capture engine but never disables
     the frame-done interrupt or synchronizes with the handler. A late IRQ
     after the block is runtime-suspended (on release) can access gated
     registers and crash. Clear CSI_INT_EN_REG and synchronize_irq().

3/3: The async notifier ops lack an .unbind callback, so csi->src_subdev is
     left dangling when the remote sensor is unbound; a later STREAMON
     dereferences the freed subdev. Add .unbind to unregister the video
     device and clear the pointer, matching stm32-dcmi/atmel-isi/mcam.

All three are marked for stable. Compile-tested (COMPILE_TEST) and
checkpatch --strict clean. I do not have the hardware, so testing on a
real A10/A20 CSI setup would be appreciated.

Cong Nguyen (3):
  media: sun4i-csi: fix video device and subdev leak in
    notify_complete()
  media: sun4i-csi: disable interrupts when stopping streaming
  media: sun4i-csi: add notifier unbind callback to drop the source
    subdev

 .../platform/sunxi/sun4i-csi/sun4i_csi.c      | 25 +++++++++++++++++--
 .../platform/sunxi/sun4i-csi/sun4i_csi.h      |  1 +
 .../platform/sunxi/sun4i-csi/sun4i_dma.c      | 11 ++++++++
 3 files changed, 35 insertions(+), 2 deletions(-)

-- 
2.25.1
[PATCH v2 0/3] media: sun4i-csi: fix probe/streaming lifecycle bugs
Posted by Cong Nguyen 1 month, 2 weeks ago
This series fixes three independent, pre-existing lifecycle bugs in the
sun4i-csi driver, all present since the driver was added (577bbf23b758):

  1/3 - notify_complete() left the video device and bridge subdev
        registered if a later step failed, so /dev/videoX stayed live over
        the freed devm context.
  2/3 - the frame-done interrupt was never disabled on the stop path, so a
        late IRQ could touch a gated block after runtime suspend.
  3/3 - the async notifier had no .unbind, so the cached source subdev
        pointer dangled after the sensor unbound.

Changes in v2:
  - 2/3: read CSI_INT_EN_REG back after disabling it to flush the posted
    write before synchronize_irq() (Sashiko AI review).
  - 2/3: apply the same interrupt teardown to the start_streaming() error
    path (err_disable_device), which had the identical defect - the IRQ is
    enabled and capture started before the s_stream call that can fail.
    Both paths now share a new sun4i_csi_disable_irq() helper.
  - 3/3: only clear csi->src_subdev in .unbind; drop the
    vb2_video_unregister_device() call. Since notify_complete() registers
    the video device, unregistering it on unbind would make it re-register
    an already-unregistered embedded video_device on a subsequent re-bind.
    Clearing the cached pointer alone prevents the use-after-free.
  - No functional changes to 1/3.

Two further pre-existing issues raised in review are not addressed here, as
they are out of scope for these fixes: the embedded video_device using
video_device_release_empty in a devm-allocated struct (a broader lifetime
rework), and notify_complete() not being re-bind safe (it re-registers the
bridge subdev, which needs a larger restructuring of the notifier flow).

Cong Nguyen (3):
  media: sun4i-csi: fix video device and subdev leak in
    notify_complete()
  media: sun4i-csi: disable interrupts when stopping streaming
  media: sun4i-csi: add notifier unbind callback to drop the source
    subdev

 .../platform/sunxi/sun4i-csi/sun4i_csi.c      | 25 +++++++++++++++++--
 .../platform/sunxi/sun4i-csi/sun4i_csi.h      |  1 +
 .../platform/sunxi/sun4i-csi/sun4i_dma.c      | 20 +++++++++++++++
 3 files changed, 44 insertions(+), 2 deletions(-)

-- 
2.25.1
[PATCH v2 1/3] media: sun4i-csi: fix video device and subdev leak in notify_complete()
Posted by Cong Nguyen 1 month, 2 weeks ago
sun4i_csi_notify_complete() registers the bridge subdev with
v4l2_device_register_subdev() and the video device with
sun4i_csi_v4l2_register() (which calls video_register_device()) before it
creates the media pad links and registers the subdev nodes. If any of the
later steps fail, the error path only unregistered the media device:

	err_clean_media:
		media_device_unregister(&csi->mdev);
		return ret;

The already registered video device and bridge subdev were left behind.
Because this failure propagates back through v4l2_async_nf_register() and
aborts probe, the driver's devm-managed struct sun4i_csi (which embeds the
video_device) is freed while /dev/videoX is still registered, so a
subsequent open() from userspace dereferences freed memory.

Unwind the registrations in reverse order on error, mirroring the teardown
in sun4i_csi_remove(): unregister the video device with
vb2_video_unregister_device() and the bridge subdev with
v4l2_device_unregister_subdev(). Also unwind the intermediate v4l2/media
registration steps so every early return leaves no half-registered state.

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.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
index e53a07b770b7..a8711336a754 100644
--- a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
+++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
@@ -85,11 +85,11 @@ static int sun4i_csi_notify_complete(struct v4l2_async_notifier *notifier)
 
 	ret = sun4i_csi_v4l2_register(csi);
 	if (ret < 0)
-		return ret;
+		goto err_unregister_subdev;
 
 	ret = media_device_register(&csi->mdev);
 	if (ret)
-		return ret;
+		goto err_unregister_video;
 
 	/* Create link from subdev to main device */
 	ret = media_create_pad_link(&subdev->entity, CSI_SUBDEV_SOURCE,
@@ -114,6 +114,10 @@ static int sun4i_csi_notify_complete(struct v4l2_async_notifier *notifier)
 
 err_clean_media:
 	media_device_unregister(&csi->mdev);
+err_unregister_video:
+	vb2_video_unregister_device(&csi->vdev);
+err_unregister_subdev:
+	v4l2_device_unregister_subdev(subdev);
 
 	return ret;
 }
-- 
2.25.1
[PATCH v2 2/3] media: sun4i-csi: disable interrupts when stopping streaming
Posted by Cong Nguyen 1 month, 2 weeks ago
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.

Add a helper that clears CSI_INT_EN_REG, reads it back to flush the posted
write, and calls synchronize_irq(), so no handler can run past that point.
Call it from stop_streaming() before returning the active buffers and
freeing the scratch buffer.

The start_streaming() error path (err_disable_device) has the same
problem: the frame-done interrupt is enabled and capture is started
before the s_stream call that can fail, so a handler can run while the
error path frees the scratch buffer and returns the queued buffers.
Apply the same teardown there. Store the IRQ number in struct sun4i_csi
so it is available in both paths.

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>
---
 .../platform/sunxi/sun4i-csi/sun4i_csi.h      |  1 +
 .../platform/sunxi/sun4i-csi/sun4i_dma.c      | 20 +++++++++++++++++++
 2 files changed, 21 insertions(+)

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..fd83dbd1a3ff 100644
--- a/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c
+++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c
@@ -43,6 +43,23 @@ static void sun4i_csi_capture_stop(struct sun4i_csi *csi)
 	writel(0, csi->regs + CSI_CPT_CTRL_REG);
 }
 
+static void sun4i_csi_disable_irq(struct sun4i_csi *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.
+	 *
+	 * Read the register back to flush the posted write so the disable has
+	 * reached the device before synchronize_irq() waits for the handler.
+	 */
+	writel(0, csi->regs + CSI_INT_EN_REG);
+	readl(csi->regs + CSI_INT_EN_REG);
+	synchronize_irq(csi->irq);
+}
+
 static int sun4i_csi_queue_setup(struct vb2_queue *vq,
 				 unsigned int *nbuffers,
 				 unsigned int *nplanes,
@@ -328,6 +345,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);
@@ -353,6 +371,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);
@@ -438,6 +457,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;
 
-- 
2.25.1
[PATCH v2 3/3] media: sun4i-csi: add notifier unbind callback to drop the source subdev
Posted by Cong Nguyen 1 month, 2 weeks ago
sun4i_csi_notify_ops only implements .bound and .complete. The .bound
callback caches the remote sensor's subdevice in csi->src_subdev. When
that subdevice goes away (e.g. its module is unloaded), the V4L2 async
core frees it, but without an .unbind callback the driver keeps the
stale pointer.

A subsequent VIDIOC_STREAMON reaches sun4i_csi_start_streaming(), which
calls v4l2_subdev_call(csi->src_subdev, video, s_stream, 1) on the freed
subdev, resulting in a use-after-free.

Add an .unbind callback that clears csi->src_subdev. The pointer is only
dereferenced by v4l2_subdev_call() (NULL-guarded, returns -ENODEV, so
streaming fails cleanly) and by sun4i_csi_notify_complete() (which only
runs while a subdev is bound), so clearing it is sufficient to prevent
the use-after-free.

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>
---
 .../media/platform/sunxi/sun4i-csi/sun4i_csi.c  | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
index a8711336a754..f19508f2c7ed 100644
--- a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
+++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
@@ -122,8 +122,25 @@ static int sun4i_csi_notify_complete(struct v4l2_async_notifier *notifier)
 	return ret;
 }
 
+static void sun4i_csi_notify_unbind(struct v4l2_async_notifier *notifier,
+				    struct v4l2_subdev *subdev,
+				    struct v4l2_async_connection *asd)
+{
+	struct sun4i_csi *csi = container_of(notifier, struct sun4i_csi,
+					     notifier);
+
+	/*
+	 * The remote subdev is being freed. Drop our cached pointer so that a
+	 * subsequent sun4i_csi_start_streaming() does not issue a
+	 * v4l2_subdev_call() on the now dangling source subdev. The call is
+	 * NULL-guarded and returns -ENODEV, so streaming fails cleanly.
+	 */
+	csi->src_subdev = NULL;
+}
+
 static const struct v4l2_async_notifier_operations sun4i_csi_notify_ops = {
 	.bound		= sun4i_csi_notify_bound,
+	.unbind		= sun4i_csi_notify_unbind,
 	.complete	= sun4i_csi_notify_complete,
 };
 
-- 
2.25.1