Re: [PATCH] Input: sur40: fix MAX_CONTACTS value based on PixelSense specification

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Synchronously freeing the driver state in `sur40_disconnect()` while the V4L2 device may still be open leads to use-after-free vulnerabilities.
- [High] The error path in `sur40_probe` incorrectly calls `input_free_device()` on a successfully registered input device instead of `input_unregister_device()`, leading to potential use-after-free and state corruption.
- [High] The input device is registered and exposed to userspace before the driver's private video queue and V4L2 components are fully initialized.
--

commit 36ed451619f33b71bb9cc6a733570423cba1565e
Author: Oliver <oliverburns.kernel@gmail.com>

Input: sur40: fix MAX_CONTACTS value based on PixelSense specification

This patch updates the maximum number of contacts for the Samsung SUR40
touchscreen to 52 to match the documented hardware specification. It also
removes an older FIXME comment that guessed the maximum contacts was 64.

> diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
> index fe63d53d56db0..77ec2c94b91fd 100644
> --- a/drivers/input/touchscreen/sur40.c
> +++ b/drivers/input/touchscreen/sur40.c
[ ... ]
> @@ -128,8 +128,8 @@ struct sur40_image_header {
>  /* polling interval (ms) */
>  #define POLL_INTERVAL 1
>  
> -/* maximum number of contacts FIXME: this is a guess? */
> -#define MAX_CONTACTS 64
> +/* maximum number of contacts */
> +#define MAX_CONTACTS 52
>  
>  /* control commands */
>  #define SUR40_GET_VERSION 0xb0 /* 12 bytes string    */

[Severity: High]
This isn't a bug introduced by this patch, but I noticed something in
sur40_probe(). The input device is registered and exposed to userspace
before the driver's private video queue and V4L2 components are fully
initialized:

drivers/input/touchscreen/sur40.c:sur40_probe() {
    ...
	/* register the polled input device */
	error = input_register_device(input);
	if (error) {
		dev_err(&interface->dev,
			"Unable to register polled input device.");
		goto err_free_buffer;
	}
    ...
	/* initialize the queue */
	error = vb2_queue_init(&sur40->queue);
    ...
}

Does this leave a window where an open operation could trigger sur40_open()
and start the sur40_poll() worker thread? If the worker thread invokes
sur40_process_video() and checks vb2_start_streaming_called(&sur40->queue)
concurrently with the main thread's initialization, could this cause a data
race regression on partially initialized structures?

[Severity: High]
This is a pre-existing issue, but looking at the sur40_probe() error handling,
if a failure occurs after the input device is successfully registered, the
code jumps to error labels that fall through to err_free_input:

drivers/input/touchscreen/sur40.c:sur40_probe() {
    ...
err_unreg_video:
	video_unregister_device(&sur40->vdev);
err_unreg_v4l2:
	v4l2_device_unregister(&sur40->v4l2);
err_free_buffer:
	kfree(sur40->bulk_in_buffer);
err_free_input:
	input_free_device(input);
err_free_dev:
	kfree(sur40);

	return error;
}

Can calling input_free_device() on a successfully registered device lead
to state corruption? If a userspace process opens this lingering device node,
would the polling thread dereference the freed sur40 pointer and cause a
use-after-free regression on the error path? It seems like
input_unregister_device() should be used instead.

[Severity: Critical]
This is also a pre-existing issue, but I noticed sur40_disconnect()
synchronously frees the driver state while the V4L2 device may still be open:

drivers/input/touchscreen/sur40.c:sur40_disconnect() {
	struct sur40_state *sur40 = usb_get_intfdata(interface);

	v4l2_ctrl_handler_free(&sur40->hdl);
	video_unregister_device(&sur40->vdev);
	v4l2_device_unregister(&sur40->v4l2);

	input_unregister_device(sur40->input);
	kfree(sur40->bulk_in_buffer);
	kfree(sur40);
    ...
}

Could this lead to a use-after-free regression? If userspace holds a
file descriptor open to the video device during a USB disconnect,
video_unregister_device() does not forcefully close the active file
descriptor. When userspace later calls ioctl(), poll(), or close() on the
descriptor, wouldn't the V4L2 core invoke file operations like
vb2_fop_release() which dereference the already freed sur40 memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260614230847.4938-1-oliverburns.kernel@gmail.com?part=1