diff mbox series

[4/5] media: uvcvideo: Protect uvc queue file operations against disconnect

Message ID 20200830150443.167286-5-linux@roeck-us.net (mailing list archive)
State New, archived
Headers show
Series media: uvcvideo: Fix race conditions | expand

Commit Message

Guenter Roeck Aug. 30, 2020, 3:04 p.m. UTC
uvc queue file operations have no mutex protection against USB
disconnect. This is questionable at least for the poll operation,
which has to wait for the uvc queue mutex. By the time that mutex
has been acquired, is it possible that the video device has been
unregistered.

Protect all file operations by using the queue mutex to avoid
possible race conditions. After acquiring the mutex, check if
the video device is still registered, and bail out if not.

Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/media/usb/uvc/uvc_queue.c | 32 +++++++++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)

Comments

kernel test robot Sept. 1, 2020, 4:51 p.m. UTC | #1
Hi Guenter,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on v5.9-rc3 next-20200828]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Guenter-Roeck/media-uvcvideo-Fix-race-conditions/20200830-230715
base:   git://linuxtv.org/media_tree.git master
config: x86_64-randconfig-s031-20200901 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.2-191-g10164920-dirty
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)

>> drivers/media/usb/uvc/uvc_queue.c:402:21: sparse: sparse: incorrect type in assignment (different base types) @@     expected restricted __poll_t [usertype] ret @@     got int @@
>> drivers/media/usb/uvc/uvc_queue.c:402:21: sparse:     expected restricted __poll_t [usertype] ret
>> drivers/media/usb/uvc/uvc_queue.c:402:21: sparse:     got int

# https://github.com/0day-ci/linux/commit/9128827e77bc077a6fefd3ca886a8466e84f7154
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Guenter-Roeck/media-uvcvideo-Fix-race-conditions/20200830-230715
git checkout 9128827e77bc077a6fefd3ca886a8466e84f7154
vim +402 drivers/media/usb/uvc/uvc_queue.c

   393	
   394	__poll_t uvc_queue_poll(struct uvc_video_queue *queue, struct file *file,
   395				    poll_table *wait)
   396	{
   397		struct uvc_streaming *stream = uvc_queue_to_stream(queue);
   398		__poll_t ret;
   399	
   400		mutex_lock(&queue->mutex);
   401		if (!video_is_registered(&stream->vdev)) {
 > 402			ret = -ENODEV;
   403			goto unlock;
   404		}
   405		ret = vb2_poll(&queue->queue, file, wait);
   406	unlock:
   407		mutex_unlock(&queue->mutex);
   408	
   409		return ret;
   410	}
   411	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Guenter Roeck Sept. 1, 2020, 4:58 p.m. UTC | #2
On 9/1/20 9:51 AM, kernel test robot wrote:
> Hi Guenter,
> 
> I love your patch! Perhaps something to improve:
> 
> [auto build test WARNING on linuxtv-media/master]
> [also build test WARNING on v5.9-rc3 next-20200828]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
> 
> url:    https://github.com/0day-ci/linux/commits/Guenter-Roeck/media-uvcvideo-Fix-race-conditions/20200830-230715
> base:   git://linuxtv.org/media_tree.git master
> config: x86_64-randconfig-s031-20200901 (attached as .config)
> compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
> reproduce:
>         # apt-get install sparse
>         # sparse version: v0.6.2-191-g10164920-dirty
>         # save the attached .config to linux build tree
>         make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=x86_64 
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> 
> sparse warnings: (new ones prefixed by >>)
> 
>>> drivers/media/usb/uvc/uvc_queue.c:402:21: sparse: sparse: incorrect type in assignment (different base types) @@     expected restricted __poll_t [usertype] ret @@     got int @@
>>> drivers/media/usb/uvc/uvc_queue.c:402:21: sparse:     expected restricted __poll_t [usertype] ret
>>> drivers/media/usb/uvc/uvc_queue.c:402:21: sparse:     got int
> 
> # https://github.com/0day-ci/linux/commit/9128827e77bc077a6fefd3ca886a8466e84f7154
> git remote add linux-review https://github.com/0day-ci/linux
> git fetch --no-tags linux-review Guenter-Roeck/media-uvcvideo-Fix-race-conditions/20200830-230715
> git checkout 9128827e77bc077a6fefd3ca886a8466e84f7154
> vim +402 drivers/media/usb/uvc/uvc_queue.c
> 
>    393	
>    394	__poll_t uvc_queue_poll(struct uvc_video_queue *queue, struct file *file,
>    395				    poll_table *wait)
>    396	{
>    397		struct uvc_streaming *stream = uvc_queue_to_stream(queue);
>    398		__poll_t ret;
>    399	
>    400		mutex_lock(&queue->mutex);
>    401		if (!video_is_registered(&stream->vdev)) {
>  > 402			ret = -ENODEV;

Looks like this has to return EPOLLERR. I'll make that change in v2.

Guenter

>    403			goto unlock;
>    404		}
>    405		ret = vb2_poll(&queue->queue, file, wait);
>    406	unlock:
>    407		mutex_unlock(&queue->mutex);
>    408	
>    409		return ret;
>    410	}
>    411	
> 
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
>
diff mbox series

Patch

diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
index cd60c6c1749e..c7964177a99d 100644
--- a/drivers/media/usb/uvc/uvc_queue.c
+++ b/drivers/media/usb/uvc/uvc_queue.c
@@ -358,24 +358,52 @@  int uvc_queue_streamoff(struct uvc_video_queue *queue, enum v4l2_buf_type type)
 
 int uvc_queue_mmap(struct uvc_video_queue *queue, struct vm_area_struct *vma)
 {
-	return vb2_mmap(&queue->queue, vma);
+	struct uvc_streaming *stream = uvc_queue_to_stream(queue);
+	int ret;
+
+	mutex_lock(&queue->mutex);
+	if (!video_is_registered(&stream->vdev)) {
+		ret = -ENODEV;
+		goto unlock;
+	}
+	ret = vb2_mmap(&queue->queue, vma);
+unlock:
+	mutex_unlock(&queue->mutex);
+	return ret;
 }
 
 #ifndef CONFIG_MMU
 unsigned long uvc_queue_get_unmapped_area(struct uvc_video_queue *queue,
 		unsigned long pgoff)
 {
-	return vb2_get_unmapped_area(&queue->queue, 0, 0, pgoff, 0);
+	struct uvc_streaming *stream = uvc_queue_to_stream(queue);
+	unsigned long ret;
+
+	mutex_lock(&queue->mutex);
+	if (!video_is_registered(&stream->vdev)) {
+		ret = -ENODEV;
+		goto unlock;
+	}
+	ret = vb2_get_unmapped_area(&queue->queue, 0, 0, pgoff, 0);
+unlock:
+	mutex_unlock(&queue->mutex);
+	return ret;
 }
 #endif
 
 __poll_t uvc_queue_poll(struct uvc_video_queue *queue, struct file *file,
 			    poll_table *wait)
 {
+	struct uvc_streaming *stream = uvc_queue_to_stream(queue);
 	__poll_t ret;
 
 	mutex_lock(&queue->mutex);
+	if (!video_is_registered(&stream->vdev)) {
+		ret = -ENODEV;
+		goto unlock;
+	}
 	ret = vb2_poll(&queue->queue, file, wait);
+unlock:
 	mutex_unlock(&queue->mutex);
 
 	return ret;