Re: [PATCH] input: sur40: fix use-after-free in disconnect

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Calling vb2_wait_for_all_buffers() in sur40_stop_streaming() can cause a permanent deadlock if the input device is not opened or if the USB device is disconnected while streaming.
- [High] Failing to use vb2_video_unregister_device() causes blocked video streams to hang forever, preventing memory release.
--

commit ef5f3b0676d23060459c7e109fbbbad456afb073
Author: Pei Xiao <xiaopei01@kylinos.cn>
input: sur40: fix use-after-free in disconnect

This commit fixes a use-after-free issue in sur40_disconnect() by providing
a proper release callback for the video device. It relies on the kref
mechanism triggered by device unregistration rather than immediately
freeing the state.

Closes: https://lore.kernel.org/all/6a8e44ca.dbb3a75c.7844.002f.GAE@google.com/

> diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
> index 09d8c5f8d09fc..2d93234cc86a0 100644
> --- a/drivers/input/touchscreen/sur40.c
> +++ b/drivers/input/touchscreen/sur40.c

[ ... ]

> @@ -820,19 +824,23 @@ static int sur40_probe(struct usb_interface *interface,
>  	return error;
>  }
>  
> -/* Unregister device & clean up. */
> +static void sur40_video_release(struct video_device *vdev)
> +{
> +	struct sur40_state *sur40 = container_of(vdev, struct sur40_state, vdev);
> +
> +	v4l2_ctrl_handler_free(&sur40->hdl);
> +	v4l2_device_unregister(&sur40->v4l2);
> +	kfree(sur40->bulk_in_buffer);
> +	kfree(sur40);
> +}
> +
>  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);

[Severity: High]
This is a pre-existing issue, but does failing to use
vb2_video_unregister_device() here at line 843 of
drivers/input/touchscreen/sur40.c cause blocked video streams to hang forever?

During a physical USB disconnect, sur40_disconnect() calls
video_unregister_device() instead of vb2_video_unregister_device(). The
standard video_unregister_device() does not signal the vb2_queue to abort.

Because the input poller is stopped during disconnect, no new video frames
will arrive. Any userspace thread blocking in VIDIOC_DQBUF or poll() will
never wake up, preventing it from closing the file descriptor. This means
vdev->release is never executed and the sur40_state memory leaks.

[Severity: High]
This is also a pre-existing issue, but can calling
vb2_wait_for_all_buffers() in sur40_stop_streaming() cause a permanent
deadlock?

If the USB device is disconnected, input_unregister_device() stops the
poller. A subsequent close() of the video file descriptor will call
sur40_stop_streaming() (at line 940 in drivers/input/touchscreen/sur40.c),
which blocks waiting for buffers:

sur40_stop_streaming()
    ...
    vb2_wait_for_all_buffers(vq);

Since the poller (sur40_poll at line 525) is stopped, the buffers are never
returned, causing a hang in uninterruptible sleep. This can also be
triggered if a user opens only the video device and starts streaming without
opening the input device, as sur40_poll is only active while the input
device is opened.

> -	v4l2_device_unregister(&sur40->v4l2);
> -
> -	kfree(sur40->bulk_in_buffer);
> -	kfree(sur40);
>  
>  	usb_set_intfdata(interface, NULL);
>  	dev_dbg(&interface->dev, "%s is now disconnected\n", DRIVER_DESC);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/51e68e6de6d9e7a864a5729bac1b33bd083802f6.1787715605.git.xiaopei01@kylinos.cn?part=1