Re: [PATCH v2 3/3] media: sun4i-csi: add notifier unbind callback to drop the source subdev

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

New issues:
- [High] The newly added `.unbind` callback clears `csi->src_subdev` without holding `csi->lock`, creating a time-of-check to time-of-use (TOCTOU) race condition with streaming operations.

Pre-existing issues:
- [High] The `.unbind` callback fails to tear down video and media devices registered in `.complete`, leading to double-registration crashes and resource leaks upon dynamic rebind.
--

--- Patch [3]: [PATCH v2 3/3] media: sun4i-csi: add notifier unbind callback to drop the source subdev ---
commit 25c69500b21e1d763945dbe2dba9e12acab04857
Author: Cong Nguyen <[email protected]>

    media: sun4i-csi: add notifier unbind callback to drop the source subdev
    
    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.

> diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
> index a8711336a7540..f19508f2c7ed4 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)
> +{

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

> +	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;

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

> +}
> +
>  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,
>  };

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