[PATCH] Input: sur40 - fix use-after-free in v4l2_release on disconnect

[PATCH] Input: sur40 - fix use-after-free in v4l2_release on disconnect

[Deepanshu Kartikey]

sur40_disconnect() unconditionally freed the sur40_state struct
(which embeds struct video_device vdev) via kfree(), even while a
userspace process could still hold an open file descriptor on the
video device. video_unregister_device() does not wait for open file
descriptors to close before returning, so if a process closed its fd
after disconnect() had already run, v4l2_release() would dereference
the already-freed vdev, resulting in a slab-use-after-free.

Fix this by giving vdev a real .release callback (sur40_video_release)
instead of video_device_release_empty, and move the v4l2_device
unregister and the kfree() calls into it. This defers the actual free
until the last reference to vdev drops to zero, whether that happens
during disconnect() (no fd open) or later when the last open fd is
closed.

Tested-by: syzbot+34957180b0ed2581edaf@syzkaller.appspotmail.com
Reported-by: syzbot+34957180b0ed2581edaf@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=34957180b0ed2581edaf
Assisted-by: Claude
Signed-off-by: Deepanshu Kartikey <kartikey406@gmail.com>
---
 drivers/input/touchscreen/sur40.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
index 09d8c5f8d09f..bd8d9f6190fb 100644
--- a/drivers/input/touchscreen/sur40.c
+++ b/drivers/input/touchscreen/sur40.c
@@ -179,6 +179,7 @@ static uint gain = SUR40_GAIN_DEF;
 module_param(gain, uint, 0644);
 MODULE_PARM_DESC(gain, "set initial gain"
 	SUR40_PARAM_RANGE(SUR40_GAIN_MIN, SUR40_GAIN_MAX));
+static void sur40_video_release(struct video_device *vdev);
 
 static const struct v4l2_pix_format sur40_pix_format[] = {
 	{
@@ -750,6 +751,7 @@ static int sur40_probe(struct usb_interface *interface,
 	sur40->vdev.v4l2_dev = &sur40->v4l2;
 	sur40->vdev.lock = &sur40->lock;
 	sur40->vdev.queue = &sur40->queue;
+	sur40->vdev.release = sur40_video_release;
 	video_set_drvdata(&sur40->vdev, sur40);
 
 	/* initialize the control handler for 4 controls */
@@ -820,6 +822,16 @@ static int sur40_probe(struct usb_interface *interface,
 	return error;
 }
 
+static void  sur40_video_release(struct video_device *vdev)
+{
+	struct sur40_state *sur40 = container_of(
+						vdev, struct sur40_state, vdev);
+
+	v4l2_device_unregister(&sur40->v4l2);
+	kfree(sur40->bulk_in_buffer);
+	kfree(sur40);
+}
+
 /* Unregister device & clean up. */
 static void sur40_disconnect(struct usb_interface *interface)
 {
@@ -829,10 +841,6 @@ static void sur40_disconnect(struct usb_interface *interface)
 
 	v4l2_ctrl_handler_free(&sur40->hdl);
 	video_unregister_device(&sur40->vdev);
-	v4l2_device_unregister(&sur40->v4l2);
-
-	kfree(sur40->bulk_in_buffer);
-	kfree(sur40);
 
 	usb_set_intfdata(interface, NULL);
 	dev_dbg(&interface->dev, "%s is now disconnected\n", DRIVER_DESC);
-- 
2.43.0

Re: [PATCH] Input: sur40 - fix use-after-free in v4l2_release on disconnect

[sashiko-bot]

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

New issues:
- [High] The `sur40_probe` error path executes a newly added `sur40_video_release` callback synchronously, causing catastrophic Use-After-Frees and double frees of the `sur40` structure.

Pre-existing issues:
- [High] Premature destruction of the V4L2 control handler in `sur40_disconnect` allows active or new ioctls to access freed memory and a destroyed mutex.
--

commit d6c7a72108e0520323af3e9a931c367af91b2e23
Author: Deepanshu Kartikey <kartikey406@gmail.com>

Input: sur40 - fix use-after-free in v4l2_release on disconnect

The commit attempts to fix a use-after-free in v4l2_release on disconnect
by introducing a proper .release callback for the video device. This defers
freeing the sur40 structure until the last reference is dropped.

Tested-by: syzbot+34957180b0ed2581edaf@syzkaller.appspotmail.com
Reported-by: syzbot+34957180b0ed2581edaf@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=34957180b0ed2581edaf

> diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
> index 09d8c5f8d09fc..7936f3d78454d 100644
> --- a/drivers/input/touchscreen/sur40.c
> +++ b/drivers/input/touchscreen/sur40.c

[ ... ]

> @@ -820,6 +822,16 @@ static int sur40_probe(struct usb_interface *interface,
>  	return error;
>  }
>  
> +static void  sur40_video_release(struct video_device *vdev)
> +{
> +	struct sur40_state *sur40 = container_of(
> +						vdev, struct sur40_state, vdev);
> +
> +	v4l2_device_unregister(&sur40->v4l2);
> +	kfree(sur40->bulk_in_buffer);
> +	kfree(sur40);
> +}

[Severity: High]
If input_register_device() fails in sur40_probe(), the error path calls
video_unregister_device(&sur40->vdev). Since there are no open file
descriptors during probe, won't this synchronously invoke
sur40_video_release() and free the sur40 structure?

The error path in sur40_probe() then continues to execute:

err_unreg_video:
	video_unregister_device(&sur40->vdev);
err_free_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);
err_free_dev:
	kfree(sur40);

Does this lead to use-after-free and double-free bugs on the probe error
path?

> +
>  /* Unregister device & clean up. */
>  static void sur40_disconnect(struct usb_interface *interface)
>  {
> @@ -829,10 +841,6 @@ static void sur40_disconnect(struct usb_interface *interface)
>  
>  	v4l2_ctrl_handler_free(&sur40->hdl);
>  	video_unregister_device(&sur40->vdev);

[Severity: High]
This isn't a bug introduced by this patch, but won't freeing the control
handler here allow a use-after-free if a userspace process holds an open file
descriptor and issues concurrent VIDIOC_S_CTRL ioctls?

If v4l2_ctrl_handler_free() destroys the handler and its mutex while the
video device is still open, concurrent ioctls can access the freed nodes and
destroyed mutex. Should v4l2_ctrl_handler_free() also be deferred until
sur40_video_release()?

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

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