linux-kernel - Re: [PATCH] media: usbtv: Remove useless locks in usbtv_video

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b8400b7b-0716-4e8c-9ae5-f594778066c5@xs4all.nl>
Date: Wed, 28 Feb 2024 09:45:17 +0100
From: Hans Verkuil <hverkuil-cisco@...all.nl>
To: Benjamin Gaignard <benjamin.gaignard@...labora.com>, mchehab@...nel.org,
 tfiga@...omium.org
Cc: linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
 kernel@...labora.com
Subject: Re: [PATCH] media: usbtv: Remove useless locks in usbtv_video_free()

On 20/02/2024 09:06, Benjamin Gaignard wrote:
> Remove locks calls in usbtv_video_free() because
> are useless and may led to a deadlock as reported here:
> https://syzkaller.appspot.com/x/bisect.txt?x=166dc872180000
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@...labora.com>
> Fixes: c838530d230b ("media: media videobuf2: Be more flexible on the number of queue stored buffers")

Hmm, the code was really always wrong, it is just that before this commit
it would only fail if you did a disconnect while streaming, and now it also
fails if you disconnect when you are not streaming, so it is much more
noticeable now.

That should probably be explained better in the commit log. You could add a
second Fixes line:

Fixes: f3d27f34fdd7 ("[media] usbtv: Add driver for Fushicai USBTV007 video frame grabber")

but I am not sure how useful that is.

> ---
>  drivers/media/usb/usbtv/usbtv-video.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/media/usb/usbtv/usbtv-video.c b/drivers/media/usb/usbtv/usbtv-video.c
> index 62a583040cd4..96276358d116 100644
> --- a/drivers/media/usb/usbtv/usbtv-video.c
> +++ b/drivers/media/usb/usbtv/usbtv-video.c
> @@ -963,15 +963,9 @@ int usbtv_video_init(struct usbtv *usbtv)
>  
>  void usbtv_video_free(struct usbtv *usbtv)
>  {
> -	mutex_lock(&usbtv->vb2q_lock);
> -	mutex_lock(&usbtv->v4l2_lock);
> -
>  	usbtv_stop(usbtv);

This call should also be dropped...

>  	vb2_video_unregister_device(&usbtv->vdev);

..since this function will call stop_streaming if streaming is in progress,
which will call usbtv_stop with all the correct locks held.

Regards,

	Hans

>  	v4l2_device_disconnect(&usbtv->v4l2_dev);
>  
> -	mutex_unlock(&usbtv->v4l2_lock);
> -	mutex_unlock(&usbtv->vb2q_lock);
> -
>  	v4l2_device_put(&usbtv->v4l2_dev);
>  }