linux-kernel - Re: [PATCH v6 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: <f4c49ccf-9dc9-475a-8fc9-4ef4c85a729a@xs4all.nl>
Date: Wed, 25 Sep 2024 10:32:58 +0200
From: Hans Verkuil <hverkuil-cisco@...all.nl>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: Guenter Roeck <linux@...ck-us.net>, Tomasz Figa <tfiga@...omium.org>,
 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>,
 Mauro Carvalho Chehab <mchehab@...nel.org>,
 Ricardo Ribalda <ribalda@...omium.org>
Subject: Re: [PATCH v6 1/4] media: uvcvideo: Stop stream during unregister

Hi Laurent,

We discussed this patch last week, and you thought that there was still
a race condition if the uvc device was unplugged while an application was
in the VIDIOC_DQBUF call waiting for a buffer to arrive (so the vb2 wait_prepare
op is called, which unlocks the serialization mutex).

I'll go through the code below, explaining why that isn't an issue.

On 14/06/2024 14:41, 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 occur.
> 
> 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 makes uvc more consistent with the rest of the v4l2 drivers
> using the vb2_fop_* and vb2_ioctl_* helpers.
> 
> Reviewed-by: Hans Verkuil <hverkuil-cisco@...all.nl>
> Suggested-by: Hans Verkuil <hverkuil-cisco@...all.nl>
> Signed-off-by: Ricardo Ribalda <ribalda@...omium.org>
> ---
>  drivers/media/usb/uvc/uvc_driver.c | 32 +++++++++++++++++++++++++++++++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index bbd90123a4e7..55132688e363 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1908,11 +1908,41 @@ static void uvc_unregister_video(struct uvc_device *dev)
>  	struct uvc_streaming *stream;
>  
>  	list_for_each_entry(stream, &dev->streams, list) {
> +		/* Nothing to do here, continue. */
>  		if (!video_is_registered(&stream->vdev))
>  			continue;
>  
> +		/*
> +		 * For stream->vdev we follow the same logic as:
> +		 * vb2_video_unregister_device().
> +		 */
> +
> +		/* 1. Take a reference to vdev */
> +		get_device(&stream->vdev.dev);

This ensures that the device refcount won't go to 0 if video_unregister_device
is called (which calls put_device).

But note that if an application called VIDIOC_DQBUF and is waiting for a buffer,
then that open filehandle also called get_device(). So while that application is
waiting, the device refcount will never go to 0.

> +
> +		/* 2. Ensure that no new ioctls can be called. */
>  		video_unregister_device(&stream->vdev);
> -		video_unregister_device(&stream->meta.vdev);
> +
> +		/* 3. Wait for old ioctls to finish. */
> +		mutex_lock(&stream->mutex);

If VIDIOC_DQBUF is waiting for a buffer to arrive, then indeed we can take this
lock here. So in that case this won't wait for that specific ioctl to finish.

> +
> +		/* 4. Stop streaming. */
> +		uvc_queue_release(&stream->queue);

This will __vb2_queue_cancel() which will stop streaming and wake up the wait for
buffers in VIDIOC_DQBUF. It will try to lock this mutex again, and sleeps while
waiting for the mutex to become available.

> +
> +		mutex_unlock(&stream->mutex);

At this point it can take the mutex again. But since q->streaming is now false,
(due to the __vb2_queue_cancel call) this will return an error which is returned
to userspace.

> +
> +		put_device(&stream->vdev.dev);

This releases the reference we took earlier. If the application has already closed
the filehandle, then this will release all memory. If the application still has the
fh open, then only when it closes that fh will the memory be released.

Conclusion: there is no race condition here, this is handled correctly by the core.

> +
> +		/*
> +		 * For stream->meta.vdev we can directly call:
> +		 * vb2_video_unregister_device().
> +		 */
> +		vb2_video_unregister_device(&stream->meta.vdev);

Perhaps a patch adding more comments to the vb2_video_unregister_device()
function might help document this sequence better.

Regards,

	Hans

> +
> +		/*
> +		 * Now both vdevs are not streaming and all the ioctls will
> +		 * return -ENODEV.
> +		 */
>  
>  		uvc_debugfs_cleanup_stream(stream);
>  	}
>