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

[sashiko-bot@kernel.org]

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