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

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

[Dmitry Antipov]

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: syzbot+2a123f6269da57ffefaa@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=2a123f6269da57ffefaa
Fixes: 08b936012964 ("Input: sur40 - switch to using polled mode of input devices")
Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
 drivers/input/touchscreen/sur40.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
index fe63d53d56db..fe675b937b35 100644
--- a/drivers/input/touchscreen/sur40.c
+++ b/drivers/input/touchscreen/sur40.c
@@ -730,6 +730,8 @@ static int sur40_probe(struct usb_interface *interface,
 	if (error) {
 		dev_err(&interface->dev,
 			"Unable to register polled input device.");
+		input_free_device(input);
+		input = NULL;
 		goto err_free_buffer;
 	}
 
@@ -786,9 +788,8 @@ static int sur40_probe(struct usb_interface *interface,
 	if (sur40->hdl.error) {
 		dev_err(&interface->dev,
 			"Unable to register video controls.");
-		v4l2_ctrl_handler_free(&sur40->hdl);
 		error = sur40->hdl.error;
-		goto err_unreg_v4l2;
+		goto err_unreg_ctrl;
 	}
 
 	error = video_register_device(&sur40->vdev, VFL_TYPE_TOUCH, -1);
@@ -806,12 +807,15 @@ 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 (input)
+		input_unregister_device(input);
 err_free_dev:
 	kfree(sur40);
 
-- 
2.55.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Calling `input_unregister_device()` on an unregistered input device during early probe failures leads to a kernel crash.
--

--- Patch [1]: [PATCH] input: sur40: fix error recovery in sur40_probe() ---
commit ae7595899669d474b491f275a114a4cdae739843
Author: Dmitry Antipov <dmantipov@yandex.ru>

input: sur40: fix error recovery in sur40_probe()

This commit modifies the error recovery path in sur40_probe() by
unregistering the input device instead of freeing it. It also ensures
that the v4l2 control handler is freed on all error paths.

Link: https://syzkaller.appspot.com/bug?extid=2a123f6269da57ffefaa

> diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
> index 77ec2c94b91fd..9f32208b94fd7 100644
> --- a/drivers/input/touchscreen/sur40.c
> +++ b/drivers/input/touchscreen/sur40.c
[ ... ]
> @@ -806,12 +807,15 @@ 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 (input)
> +		input_unregister_device(input);

[Severity: High]
Can this cause a kernel panic if an error occurs before
input_register_device() is called?

During sur40_probe(), if an early initialization step like
sur40_input_setup_events() or kmalloc() for sur40->bulk_in_buffer fails,
the code jumps to err_free_input. At that point, input is non-NULL but
input_register_device() has not been called yet.

Calling input_unregister_device() on an allocated but unregistered device
attempts to remove it from the sysfs hierarchy, which can lead to a NULL
pointer dereference since device_add() was never called.

>  err_free_dev:
>  	kfree(sur40);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260810141319.118259-1-dmantipov@yandex.ru?part=1

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

[Vasileios Almpanis]

> 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: syzbot+2a123f6269da57ffefaa@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=2a123f6269da57ffefaa
> Fixes: 08b936012964 ("Input: sur40 - switch to using polled mode of input devices")
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
>
> diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
> index fe63d53d56db..fe675b937b35 100644
> --- a/drivers/input/touchscreen/sur40.c
> +++ b/drivers/input/touchscreen/sur40.c
> @@ -730,6 +730,8 @@ static int sur40_probe(struct usb_interface *interface,
>  	if (error) {
>  		dev_err(&interface->dev,
>  			"Unable to register polled input device.");
> +		input_free_device(input);
> +		input = NULL;
>  		goto err_free_buffer;
>  	}
>  
> @@ -786,9 +788,8 @@ static int sur40_probe(struct usb_interface *interface,
>  	if (sur40->hdl.error) {
>  		dev_err(&interface->dev,
>  			"Unable to register video controls.");
> -		v4l2_ctrl_handler_free(&sur40->hdl);
>  		error = sur40->hdl.error;
> -		goto err_unreg_v4l2;
> +		goto err_unreg_ctrl;
>  	}
>  
>  	error = video_register_device(&sur40->vdev, VFL_TYPE_TOUCH, -1);
> @@ -806,12 +807,15 @@ 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);
This label is shared by three other gotos which run before input_register_device.
So for example if kmalloc(sur40->bulk_in_size) fails we will go to err_free_input
which will call input_unregister_device with a device that was never registered,
and could potentially lead to some NULL-ptr deref.

-- 
Vasileios Almpanis <vasilisalmpanis@gmail.com>

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

[Dmitry Antipov]

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: syzbot+2a123f6269da57ffefaa@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=2a123f6269da57ffefaa
Fixes: 08b936012964 ("Input: sur40 - switch to using polled mode of input devices")
Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
v2: setup sur40 input device pointer after 'input_register_device()' and
    adjust error handling path to ensure that 'input_unregister_device()'
    is always called for a registered device only (Vasileios)
---
 drivers/input/touchscreen/sur40.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
index fe63d53d56db..35a946979cb0 100644
--- a/drivers/input/touchscreen/sur40.c
+++ b/drivers/input/touchscreen/sur40.c
@@ -713,7 +713,6 @@ static int sur40_probe(struct usb_interface *interface,
 
 	sur40->usbdev = usbdev;
 	sur40->dev = &interface->dev;
-	sur40->input = input;
 
 	/* use the bulk-in endpoint tested above */
 	sur40->bulk_in_size = usb_endpoint_maxp(endpoint);
@@ -733,6 +732,9 @@ static int sur40_probe(struct usb_interface *interface,
 		goto err_free_buffer;
 	}
 
+	/* now we have a completely initialized input device */
+	sur40->input = input;
+
 	/* register the video master device */
 	snprintf(sur40->v4l2.name, sizeof(sur40->v4l2.name), "%s", DRIVER_LONG);
 	error = v4l2_device_register(sur40->dev, &sur40->v4l2);
@@ -786,9 +788,8 @@ static int sur40_probe(struct usb_interface *interface,
 	if (sur40->hdl.error) {
 		dev_err(&interface->dev,
 			"Unable to register video controls.");
-		v4l2_ctrl_handler_free(&sur40->hdl);
 		error = sur40->hdl.error;
-		goto err_unreg_v4l2;
+		goto err_unreg_ctrl;
 	}
 
 	error = video_register_device(&sur40->vdev, VFL_TYPE_TOUCH, -1);
@@ -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);
 err_free_dev:
 	kfree(sur40);
 
-- 
2.55.0

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

[sashiko-bot]

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 <dmantipov@yandex.ru>

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: syzbot+2a123f6269da57ffefaa@syzkaller.appspotmail.com
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/20260810153509.124147-1-dmantipov@yandex.ru?part=1

[PATCH v3] Input: sur40: fix error recovery in sur40_probe()

[Dmitry Antipov]

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. Finally,
since 'sur40_poll()' which reads 'bulk_in_buffer' may become active
immediately after 'input_register_device()' has succeeded, ensure
that this buffer is freed after 'input_unregister_device()', just
like it's done in 'sur40_disconnect()'.

Reported-by: syzbot+2a123f6269da57ffefaa@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=2a123f6269da57ffefaa
Fixes: 08b936012964 ("Input: sur40 - switch to using polled mode of input devices")
Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
v3: free 'bulk_in_buffer' after an input device has passed
    'input_unregister_device()' (Sashiko)
v2: setup sur40 input device pointer after 'input_register_device()' and
    adjust error handling path to ensure that 'input_unregister_device()'
    is always called for a registered device only (Vasileios)
---
 drivers/input/touchscreen/sur40.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
index fe63d53d56db..f7d6d89a028b 100644
--- a/drivers/input/touchscreen/sur40.c
+++ b/drivers/input/touchscreen/sur40.c
@@ -713,7 +713,6 @@ static int sur40_probe(struct usb_interface *interface,
 
 	sur40->usbdev = usbdev;
 	sur40->dev = &interface->dev;
-	sur40->input = input;
 
 	/* use the bulk-in endpoint tested above */
 	sur40->bulk_in_size = usb_endpoint_maxp(endpoint);
@@ -730,9 +729,12 @@ static int sur40_probe(struct usb_interface *interface,
 	if (error) {
 		dev_err(&interface->dev,
 			"Unable to register polled input device.");
-		goto err_free_buffer;
+		goto err_free_input;
 	}
 
+	/* announce an input device as completely initialized */
+	sur40->input = input;
+
 	/* register the video master device */
 	snprintf(sur40->v4l2.name, sizeof(sur40->v4l2.name), "%s", DRIVER_LONG);
 	error = v4l2_device_register(sur40->dev, &sur40->v4l2);
@@ -786,9 +788,8 @@ static int sur40_probe(struct usb_interface *interface,
 	if (sur40->hdl.error) {
 		dev_err(&interface->dev,
 			"Unable to register video controls.");
-		v4l2_ctrl_handler_free(&sur40->hdl);
 		error = sur40->hdl.error;
-		goto err_unreg_v4l2;
+		goto err_unreg_ctrl;
 	}
 
 	error = video_register_device(&sur40->vdev, VFL_TYPE_TOUCH, -1);
@@ -806,12 +807,19 @@ 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)
+		/* device was successfully registered */
+		input_unregister_device(input);
+	else
+		/* device just needs to be freed */
+		input_free_device(input);
+	/* this should be done after unregistering an input device */
+	kfree(sur40->bulk_in_buffer);
 err_free_dev:
 	kfree(sur40);
 
-- 
2.55.0