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] [day] [month] [year] [list]

Message-ID: <318aa048-ffdf-4379-929b-54358b018c94@xs4all.nl>
Date: Wed, 25 Sep 2024 11:50:41 +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

On 25/09/2024 10:32, Hans Verkuil wrote:
> 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.

Update: I added an extra check for this scenario to the test-media script to make
sure we catch any potential regressions in how this is handled in the core.

Regards,

	Hans

> 
> 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);
>>  	}
>>
> 
>