media: sun4i-csi: fix lifecycle bugs (leak, IRQ, unbind)

drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c static int sun4i_csi_notify_complete(struct v4l2_async_notifier *notifier) · line 118

[Severity: High]
This is a pre-existing issue, but could this error handling path lead to a use-after-free regression if userspace races to open the device?

Because the csi structure is allocated with devm_kzalloc() in sun4i_csi_probe(), it will be automatically freed when probe fails. Meanwhile, sun4i_csi_v4l2_register() sets vdev->release to video_device_release_empty.

If media_device_register() fails, this new error path calls vb2_video_unregister_device(), which unregisters the sysfs node but cannot revoke file descriptors if userspace has already opened /dev/videoX.

When the file descriptor is eventually closed, could this lead to a dereference of the freed csi structure in sun4i_csi_release() and when the V4L2 core attempts to put the device?

drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c static int sun4i_csi_notify_complete(struct v4l2_async_notifier *notifier) · line 123

[Severity: Medium]
This isn't a bug introduced by this patch, but does the error path in sun4i_csi_probe() cause a memory leak regression for async connection structures and fwnode references?

When sun4i_csi_notifier_init() succeeds, it allocates an async connection and grabs a fwnode reference. If the subsequent v4l2_async_nf_register() call fails, the error path exits without calling v4l2_async_nf_cleanup():

drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c:sun4i_csi_probe() {
    ...
err_unregister_media:
	media_device_unregister(&csi->mdev);
    ...
}

Could this result in abandoning those allocations and leaking fwnode references since the cleanup isn't performed?

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

drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.h struct sun4i_csi { · line 118

[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.

[ ... ]

drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c static void sun4i_csi_stop_streaming(struct vb2_queue *vq) · line 367

[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.

drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c static void sun4i_csi_stop_streaming(struct vb2_queue *vq) · line 369

[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.

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

drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c static int sun4i_csi_notify_complete(struct v4l2_async_notifier *notifier) · line 137

[Severity: High]
This is a pre-existing issue, but the new .unbind callback performs an asymmetric teardown.

If the remote sensor module is unloaded, the .unbind callback is triggered but fails to undo the local subdev and media device registrations. When the sensor is reloaded, sun4i_csi_notify_complete() runs again and calls v4l2_device_register_subdev() on the already-registered subdev.

Can this lead to list corruption and a potential kernel panic?

[Severity: High]
When this unbind callback unregisters the embedded video device (csi->vdev), it ends the lifecycle of its kobject.

When the sensor is re-bound, sun4i_csi_notify_complete() invokes sun4i_csi_v4l2_register(), which calls video_register_device() on the same unregistered, embedded video_device. Re-initializing a released kobject is forbidden and can lead to kernel warnings and potential panics.

Can the driver be restructured to avoid re-registering an embedded video device that has already been unregistered?

[Severity: High]
This is a pre-existing issue, but the sun4i_csi structure is allocated using devm_kzalloc() in sun4i_csi_probe(), while its embedded video_device relies on video_device_release_empty.

If an unprivileged user opens /dev/videoX and the driver is unbound, the video device is unregistered. This eventually causes devres to free the sun4i_csi structure. When the user later closes the file descriptor, sun4i_csi_release() accesses the freed csi pointer to lock csi->lock.

Does this create a Use-After-Free condition when the file descriptor is closed after the driver is unbound?

drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c static int sun4i_csi_notify_complete(struct v4l2_async_notifier *notifier) · line 118

[Severity: High]
This is a pre-existing issue, but does this error path leave a use-after-free vulnerability if userspace opens the device node before it gets unregistered?

The video_device is embedded in struct sun4i_csi which is allocated with devm_kzalloc() during sun4i_csi_probe(). In sun4i_csi_v4l2_register(), the release callback is overridden:

vdev->release = video_device_release_empty;

If sun4i_csi_notify_complete() executes synchronously during probe and exposes the device node, userspace can open it immediately. If a subsequent setup step fails, this error path unregisters the video device and returns the error back up to probe().

When probe() fails, the devres core automatically frees the sun4i_csi memory. Because video_device_release_empty is used instead of a refcounted release, any active file descriptors held by userspace will now point to freed memory, causing a use-after-free on subsequent access or close.

drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c static int sun4i_csi_start_streaming(struct vb2_queue *vq, unsigned int count) · line 353

[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?

drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c static void sun4i_csi_stop_streaming(struct vb2_queue *vq) · line 379

[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

drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c static int sun4i_csi_notify_complete(struct v4l2_async_notifier *notifier) · line 128

[Severity: High]
This is a pre-existing issue, but doesn't this callback need to clean up the video and media devices that were registered in sun4i_csi_notify_complete()?

If the remote sensor module is unbound and then re-bound, sun4i_csi_notify_complete() will execute again. Since the previous devices were not unregistered, calling video_register_device() on an already-registered device can perform a double list_add and corrupt video_device_list.

drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c static int sun4i_csi_notify_complete(struct v4l2_async_notifier *notifier) · line 138

[Severity: High]
Could this introduce a time-of-check to time-of-use race condition with streaming operations?

This clears csi->src_subdev without holding csi->lock. Meanwhile, functions like sun4i_csi_start_streaming() and sun4i_csi_stop_streaming() in sun4i_dma.c evaluate csi->src_subdev via v4l2_subdev_call() while holding csi->lock.

If the streamon ioctl enters sun4i_csi_start_streaming(), v4l2_subdev_call() will cache the csi->src_subdev pointer into a local variable. If the remote sensor module is concurrently unloaded, sun4i_csi_notify_unbind() will clear csi->src_subdev without acquiring the lock and return, allowing the async core to free the subdevice. The streaming thread could then resume and dereference the freed subdevice pointer.