Re: [PATCH 4/4] Input: sur40 - fix V4L2 video device lifetime

[Hans Verkuil <hverkuil+cisco@kernel.org>]

On 16/06/2026 07:12, Dmitry Torokhov wrote:
> sur40_disconnect() synchronously frees the sur40_state structure (kfree(sur40))
> while userspace might still hold an open file descriptor to the V4L2 video
> device node. When userspace later accesses or closes the lingering file
> descriptor, the V4L2 core invokes file operations (such as vb2_fop_release)
> that dereference the already freed sur40 memory, resulting in a use-after-free
> vulnerability.
> 
> Fix this by implementing a V4L2 release callback (sur40_video_release) in
> sur40_video_device to clean up V4L2 components and free the sur40 structure
> only when the last video file descriptor is closed.
> 
> Additionally, update the sur40_probe() error path to call video_unregister_device()
> and return inline if input initialization fails after video device registration
> succeeded, allowing the V4L2 release callback to manage cleanup.
> 
> Also, call v4l2_device_disconnect() in sur40_disconnect() to safely dissociate
> the V4L2 device from the parent USB device during unplug.
> 
> Reported-by: sashiko-bot@kernel.org
> Cc: stable@vger.kernel.org
> Assisted-by: Antigravity:gemini-3.5-flash
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/input/touchscreen/sur40.c | 27 ++++++++++++++++-----------
>  1 file changed, 16 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
> index 1ad68131e3a6..2f0efee23d1e 100644
> --- a/drivers/input/touchscreen/sur40.c
> +++ b/drivers/input/touchscreen/sur40.c
> @@ -806,8 +806,10 @@ static int sur40_probe(struct usb_interface *interface,
>  	}
>  
>  	error = sur40_init_input(sur40);
> -	if (error)
> -		goto err_unreg_video;
> +	if (error) {
> +		video_unregister_device(&sur40->vdev);
> +		return error;
> +	}
>  
>  	/* we can register the device now, as it is ready */
>  	usb_set_intfdata(interface, sur40);
> @@ -815,8 +817,6 @@ static int sur40_probe(struct usb_interface *interface,
>  
>  	return 0;
>  
> -err_unreg_video:
> -	video_unregister_device(&sur40->vdev);
>  err_free_ctrl:
>  	v4l2_ctrl_handler_free(&sur40->hdl);
>  err_unreg_v4l2:
> @@ -835,13 +835,8 @@ static void sur40_disconnect(struct usb_interface *interface)
>  	struct sur40_state *sur40 = usb_get_intfdata(interface);
>  
>  	input_unregister_device(sur40->input);
> -
> -	v4l2_ctrl_handler_free(&sur40->hdl);
>  	video_unregister_device(&sur40->vdev);

This call can free sur40,

> -	v4l2_device_unregister(&sur40->v4l2);
> -
> -	kfree(sur40->bulk_in_buffer);
> -	kfree(sur40);
> +	v4l2_device_disconnect(&sur40->v4l2);

but this call still uses it.

The easiest fix is just to swap the two lines.

Regards,

	Hans

>  
>  	usb_set_intfdata(interface, NULL);
>  	dev_dbg(&interface->dev, "%s is now disconnected\n", DRIVER_DESC);
> @@ -1176,11 +1171,21 @@ static const struct v4l2_ioctl_ops sur40_video_ioctl_ops = {
>  	.vidioc_streamoff	= vb2_ioctl_streamoff,
>  };
>  
> +static void sur40_video_release(struct video_device *vdev)
> +{
> +	struct sur40_state *sur40 = video_get_drvdata(vdev);
> +
> +	v4l2_device_unregister(&sur40->v4l2);
> +	v4l2_ctrl_handler_free(&sur40->hdl);
> +	kfree(sur40->bulk_in_buffer);
> +	kfree(sur40);
> +}
> +
>  static const struct video_device sur40_video_device = {
>  	.name = DRIVER_LONG,
>  	.fops = &sur40_video_fops,
>  	.ioctl_ops = &sur40_video_ioctl_ops,
> -	.release = video_device_release_empty,
> +	.release = sur40_video_release,
>  	.device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_TOUCH |
>  		       V4L2_CAP_READWRITE | V4L2_CAP_STREAMING,
>  };