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

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

New issues:
- [High] The driver attempts to re-register an embedded `video_device` after it has been unregistered, which violates kobject lifecycle rules.

Pre-existing issues:
- [High] The `.unbind` callback performs incomplete resource teardown, leading to list corruption on re-bind.
- [High] The `sun4i_csi` structure is allocated using `devm_kzalloc`, but its embedded `video_device` relies on `video_device_release_empty`, creating a Use-After-Free.
--

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

media: sun4i-csi: add notifier unbind callback to drop the source subdev

This commit adds an .unbind callback to the V4L2 async notifier for the
sun4i-csi driver. It unregisters the video device and drops the source
subdev pointer. This is intended to prevent a use-after-free when the
remote sensor module is unloaded.

> diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
> index a8711336a7540..6610ada1c06d1 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. Tear down the video node so
> +	 * userspace can no longer reach sun4i_csi_start_streaming() and
> +	 * dereference the now dangling source subdev, and drop the pointer.
> +	 */
> +	vb2_video_unregister_device(&csi->vdev);

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

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

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