linux-kernel - Re: [PATCH v4 1/4] media: uvcvideo: stop stream during unregister

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: <7d06c6e4-c555-4117-a22b-5c614d7f6f8a@xs4all.nl>
Date: Tue, 28 May 2024 09:55:32 +0200
From: Hans Verkuil <hverkuil-cisco@...all.nl>
To: Ricardo Ribalda <ribalda@...omium.org>,
 Mauro Carvalho Chehab <mchehab@...nel.org>
Cc: Guenter Roeck <linux@...ck-us.net>, Max Staudt <mstaudt@...omium.org>,
 Tomasz Figa <tfiga@...omium.org>,
 Laurent Pinchart <laurent.pinchart@...asonboard.com>,
 Alan Stern <stern@...land.harvard.edu>, 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 v4 1/4] media: uvcvideo: stop stream during unregister

On 27/03/2024 09:24, Ricardo Ribalda wrote:
> uvc_unregister_video() can be called asynchronously from
> uvc_disconnect(). If the device is still streaming when that happens, a
> plethora of race conditions can happen.
> 
> Make sure that the device has stopped streaming before exiting this
> function.
> 
> If the user still holds handles to the driver's file descriptors, any
> ioctl will return -ENODEV from the v4l2 core.
> 
> This change make uvc more consistent with the rest of the v4l2 drivers
> using the vb2_fop_* and vb2_ioctl_* helpers.
> 
> Suggested-by: Hans Verkuil <hverkuil-cisco@...all.nl>
> Signed-off-by: Ricardo Ribalda <ribalda@...omium.org>
> ---
>  drivers/media/usb/uvc/uvc_driver.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index bbd90123a4e76..17fc945c8deb6 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1911,8 +1911,19 @@ static void uvc_unregister_video(struct uvc_device *dev)
>  		if (!video_is_registered(&stream->vdev))
>  			continue;
>  
> +		/*
> +		 * Serialize other access to the stream.
> +		 */
> +		mutex_lock(&stream->mutex);
> +		uvc_queue_streamoff(&stream->queue, stream->type);
>  		video_unregister_device(&stream->vdev);
>  		video_unregister_device(&stream->meta.vdev);
> +		mutex_unlock(&stream->mutex);

This sequence isn't fool proof. You have to follow the same sequence as
vb2_video_unregister_device(): take a reference to the video device,
then unregister, then take the stream mutex and call vb2_queue_release
for each queue. Finally unlock the mutex and call put_device.

Doing the video_unregister_device first ensures no new ioctls can be
called on that device node. Taking the mutex ensures that any still
running ioctls will finish (since it will sleep until they release the
mutex), and then you can release the queue.

This does require that you call get_device before calling video_unregister_device,
otherwise everything might be released at that point.

Instead of vb2_queue_release() you might have to call uvc_queue_streamoff,
but note that there are two queues here: video and meta. The code above
just calls streamoff for the video device.

For the meta device I think you can just use vb2_video_unregister_device()
directly, since that sets vdev->queue and vdev->queue.lock to the correct
values. That would just leave the video device where you need to do this
manually.

Regards,

	Hans

> +
> +		/*
> +		 * Now the vdev is not streaming and all the ioctls will
> +		 * return -ENODEV
> +		 */
>  
>  		uvc_debugfs_cleanup_stream(stream);
>  	}
>