linux-kernel - Re: [PATCH v3 3/3] media: uvcvideo: Lock video streams and queues while unregistering

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]

Date:   Wed, 22 Nov 2023 17:22:57 +0900
From:   Sergey Senozhatsky <senozhatsky@...omium.org>
To:     Ricardo Ribalda <ribalda@...omium.org>
Cc:     Mauro Carvalho Chehab <mchehab@...nel.org>,
        Guenter Roeck <linux@...ck-us.net>,
        Tomasz Figa <tfiga@...omium.org>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Alan Stern <stern@...land.harvard.edu>,
        Hans Verkuil <hverkuil-cisco@...all.nl>,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
        Sean Paul <seanpaul@...omium.org>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>
Subject: Re: [PATCH v3 3/3] media: uvcvideo: Lock video streams and queues
 while unregistering

On (23/11/21 19:53), Ricardo Ribalda wrote:
> The call to uvc_disconnect() is not protected by any mutex.
> This means it can and will be called while other accesses to the video
> device are in progress. This can cause all kinds of race conditions,
> including crashes such as the following.
> 
[..]
> 
> Call Trace:
>  usb_hcd_alloc_bandwidth+0x1ee/0x30f
>  usb_set_interface+0x1a3/0x2b7
>  uvc_video_start_transfer+0x29b/0x4b8 [uvcvideo]
>  uvc_video_start_streaming+0x91/0xdd [uvcvideo]
>  uvc_start_streaming+0x28/0x5d [uvcvideo]
>  vb2_start_streaming+0x61/0x143 [videobuf2_common]
>  vb2_core_streamon+0xf7/0x10f [videobuf2_common]
>  uvc_queue_streamon+0x2e/0x41 [uvcvideo]
>  uvc_ioctl_streamon+0x42/0x5c [uvcvideo]
>  __video_do_ioctl+0x33d/0x42a
>  video_usercopy+0x34e/0x5ff
>  ? video_ioctl2+0x16/0x16
>  v4l2_ioctl+0x46/0x53
>  do_vfs_ioctl+0x50a/0x76f
>  ksys_ioctl+0x58/0x83
>  __x64_sys_ioctl+0x1a/0x1e
>  do_syscall_64+0x54/0xde
> 
> usb_set_interface() should not be called after the USB device has been
> unregistered. However, in the above case the disconnect happened after
> v4l2_ioctl() was called, but before the call to usb_ifnum_to_if().
> 
> Acquire various mutexes in uvc_unregister_video() to fix the majority
> (maybe all) of the observed race conditions.
> 
> The uvc_device lock prevents races against suspend and resume calls
> and the poll function.
> 
> The uvc_streaming lock prevents races against stream related functions;
> for the most part, those are ioctls. This lock also requires other
> functions using this lock to check if a video device is still registered
> after acquiring it. For example, it was observed that the video device
> was already unregistered by the time the stream lock was acquired in
> uvc_ioctl_streamon().
> 
> The uvc_queue lock prevents races against queue functions, Most of
> those are already protected by the uvc_streaming lock, but some
> are called directly. This is done as added protection; an actual race
> was not (yet) observed.
> 
> Cc: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> Cc: Alan Stern <stern@...land.harvard.edu>
> Cc: Hans Verkuil <hverkuil-cisco@...all.nl>
> Reviewed-by: Tomasz Figa <tfiga@...omium.org>
> Reviewed-by: Sean Paul <seanpaul@...omium.org>
> Signed-off-by: Guenter Roeck <linux@...ck-us.net>
> Signed-off-by: Ricardo Ribalda <ribalda@...omium.org>

Reviewed-by: Sergey Senozhatsky <senozhatsky@...omium.org>