Re: [PATCH v2] input: sur40: fix error recovery in sur40_probe()

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

Pre-existing issues:
- [High] The driver frees the `sur40_state` structure synchronously in `sur40_disconnect()` while userspace may still hold open file descriptors to the V4L2 video device, leading to a Use-After-Free when the file is later closed or accessed.
- [High] Freeing `sur40->bulk_in_buffer` before unregistering the input device creates a use-after-free window if polling is active.
--

--- Patch [1]: [PATCH v2] input: sur40: fix error recovery in sur40_probe() ---
commit e90ee91ad15aa01f67bf74f5cd08571c1f3ecf61
Author: Dmitry Antipov <[email protected]>

input: sur40: fix error recovery in sur40_probe()
    
Since an input device which has passed 'input_register_device()' should
be released with 'input_unregister_device()', adjust 'sur40_probe()'
to do so and call 'input_free_device()' to handle device register error
only. To avoid V4L2 memory leak, ensure that 'v4l2_ctrl_handler_free()'
is called on all error handling paths, including the one after
'video_register_device()' failure.

Reported-by: [email protected]
Closes: https://syzkaller.appspot.com/bug?extid=2a123f6269da57ffefaa
Fixes: 08b936012964 ("Input: sur40 - switch to using polled mode of input devices")

> diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
> index 77ec2c94b91fd..9208d8404c539 100644
> --- a/drivers/input/touchscreen/sur40.c
> +++ b/drivers/input/touchscreen/sur40.c
[ ... ]
> @@ -806,12 +807,17 @@ static int sur40_probe(struct usb_interface *interface,
>  
>  err_unreg_video:
>  	video_unregister_device(&sur40->vdev);
> +err_unreg_ctrl:
> +	v4l2_ctrl_handler_free(&sur40->hdl);
>  err_unreg_v4l2:
>  	v4l2_device_unregister(&sur40->v4l2);
>  err_free_buffer:
>  	kfree(sur40->bulk_in_buffer);
>  err_free_input:
> -	input_free_device(input);
> +	if (sur40->input)
> +		input_unregister_device(input);
> +	else
> +		input_free_device(input);

[Severity: High]
This isn't a bug introduced by this patch, but does freeing
sur40->bulk_in_buffer before unregistering the input device create a
use-after-free window?

If an error occurs after input_register_device() succeeds, userspace might
have already opened the device, starting the sur40_poll() poller.

Since the poller reads directly from sur40->bulk_in_buffer, freeing it here
before calling input_unregister_device() (which stops the poller) allows the
poller to access freed memory.

Should the error path call input_unregister_device() before freeing
the bulk_in_buffer?

>  err_free_dev:
>  	kfree(sur40);

[Severity: High]
This is a pre-existing issue, but does the driver have a use-after-free
risk when handling the V4L2 video device lifecycle?

In sur40_disconnect(), the driver synchronously frees the sur40_state
structure:

	video_unregister_device(&sur40->vdev);
	...
	kfree(sur40);

However, the video device (sur40->vdev) uses video_device_release_empty:

	.release = video_device_release_empty,

If a userspace process still has the video device open when the USB device
is disconnected, closing the file descriptor later will invoke
vb2_fop_release(). This will dereference the freed vdev and its queue
locks.

Could the driver be adjusted to use a properly refcounted release callback
for the video device instead of video_device_release_empty and a
synchronous kfree()?

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