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

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Calling `v4l2_device_disconnect()` after `video_unregister_device()` causes a deterministic use-after-free on device unplug.
- [Critical] Double-free and use-after-free of the `sur40` structure and V4L2 components if `video_register_device()` fails during probe.

Pre-existing issues:
- [High] `sur40_stop_streaming()` hangs forever waiting for buffers to be returned by a stopped polling thread.
- [High] Lingering input device file descriptor allows Use-After-Free access to sur40->phys via EVIOCGPHYS.
--

commit c1c922112f8294f71a1841f179b4f4b61138851b
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Input: sur40 - fix V4L2 video device lifetime

This commit introduces a V4L2 release callback to clean up components and
free the sur40 structure only when the last video file descriptor is closed.
It updates the error path and disconnect sequence to handle this new
lifecycle.

> diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
> index 1ad68131e3a64..2f0efee23d1ec 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);

[Severity: High]
This is a pre-existing issue, but could there be a use-after-free when
userspace queries EVIOCGPHYS?

Inside sur40_init_input(), the input device's phys path is pointed directly
to the sur40->phys array:

sur40_init_input()
    input->phys = sur40->phys;
    
If the video device is closed, the new release callback frees the sur40
structure. If a userspace process still holds the input device open, a
subsequent EVIOCGPHYS ioctl will read from the freed sur40 memory.

> -	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:

[Severity: Critical]
Does this error path cause a double-free if video_register_device() fails
earlier in sur40_probe()?

If video_register_device() fails internally during device_register(), the
V4L2 core handles cleanup by dropping the reference count, which
synchronously invokes the new sur40_video_release() callback. That
callback frees the control handler, buffer, and the sur40 structure.

Jumping to err_free_ctrl on failure will then call v4l2_ctrl_handler_free()
and kfree() again on the already-freed pointers.

> @@ -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);

[Severity: High]
This is a pre-existing issue, but does this sequence cause a deadlock when
sur40_stop_streaming() is eventually called?

Calling input_unregister_device() synchronously stops the input polling
thread (sur40_poll). Later, when the video device is closed and
sur40_stop_streaming() is triggered, it invokes vb2_wait_for_all_buffers().
This waits indefinitely for buffers that can only be returned by the
now-dead polling thread.

> -
> -	v4l2_ctrl_handler_free(&sur40->hdl);
>  	video_unregister_device(&sur40->vdev);
> -	v4l2_device_unregister(&sur40->v4l2);
> -
> -	kfree(sur40->bulk_in_buffer);
> -	kfree(sur40);
> +	v4l2_device_disconnect(&sur40->v4l2);

[Severity: Critical]
Will this dereference the newly freed sur40 memory?

If there are no open file descriptors, video_unregister_device() drops the
final reference to the video device. This synchronously triggers the
new sur40_video_release() callback, which executes kfree(sur40).

Immediately after, v4l2_device_disconnect(&sur40->v4l2) is called, which
would access the freed memory. Should the order of these two calls be
swapped?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260616051235.1549517-1-dmitry.torokhov@gmail.com?part=4