Re: [PATCH 1/4] Input: sur40 - fix input device registration ordering

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] A malicious USB device can cause the input polling thread to hang indefinitely in an infinite loop.
- [High] Device unplug while streaming causes a deadlock in `sur40_stop_streaming()`.
--

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

Input: sur40 - fix input device registration ordering

This patch changes the device registration ordering in sur40_probe() to
ensure V4L2 components are initialized before the input device is registered,
preventing a race condition. It also updates sur40_disconnect() to
unregister the input device first, stopping the polling worker thread
before cleaning up V4L2 structures.

> diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
> index fe63d53d56db0..8639ec3ad7038 100644
> --- a/drivers/input/touchscreen/sur40.c
> +++ b/drivers/input/touchscreen/sur40.c
[ ... ]
> @@ -823,11 +823,12 @@ 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 isn't a bug introduced by this patch, but is a pre-existing deadlock
when the device is unplugged while streaming.

If the device is physically disconnected, sur40_disconnect() calls
input_unregister_device() which synchronously stops the polling worker thread.
When the application then closes the video file descriptor,
sur40_stop_streaming() is invoked:

drivers/input/touchscreen/sur40.c:sur40_stop_streaming() {
    ...
    vb2_wait_for_all_buffers(vq);
    sur40->sequence = -1;

    /* Release all active buffers */
    return_all_buffers(sur40, VB2_BUF_STATE_ERROR);
    ...
}

Because vb2_wait_for_all_buffers() blocks waiting for the now-dead polling
worker to return queued buffers, and the manual buffer release
return_all_buffers() is executed after the wait, will this code deadlock the
system call?

>  	v4l2_device_unregister(&sur40->v4l2);
>  
> -	input_unregister_device(sur40->input);
>  	kfree(sur40->bulk_in_buffer);

[Severity: High]
This isn't a bug introduced by this patch, but is a pre-existing Denial of
Service vulnerability where a malicious USB device can cause the input
polling thread to hang indefinitely.

In sur40_poll(), the driver reads bulk messages and calculates packet_blobs:

drivers/input/touchscreen/sur40.c:sur40_poll() {
    ...
    packet_blobs = result / sizeof(struct sur40_blob);
    // ...
    for (i = 0; i < packet_blobs; i++) {
        need_blobs--;
    ...
}

If a malicious USB device sends exactly 16 bytes (the size of struct
sur40_header) with count > 0, result will be 0, leading to packet_blobs = 0.
The inner loop is skipped, leaving need_blobs unchanged.

Because need_blobs never decrements, could the outer do { ... } while
(need_blobs > 0) loop spin indefinitely, hanging the worker thread and
blocking cancel_delayed_work_sync() during module removal and device
cleanup?

>  	kfree(sur40);
>

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