| Message ID | 20200908194557.198335-6-linux@roeck-us.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: uvcvideo: Fix race conditions | expand |
Hi Guenter,
url: https://github.com/0day-ci/linux/commits/Guenter-Roeck/media-uvcvideo-Fix-race-conditions/20200909-121927
base: git://linuxtv.org/media_tree.git master
config: x86_64-randconfig-m001-20200909 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
smatch warnings:
drivers/media/usb/uvc/uvc_v4l2.c:553 uvc_v4l2_open() warn: possible memory leak of 'handle'
# https://github.com/0day-ci/linux/commit/50911975ff9b21d08ff5408e496683b8ac567b1c
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Guenter-Roeck/media-uvcvideo-Fix-race-conditions/20200909-121927
git checkout 50911975ff9b21d08ff5408e496683b8ac567b1c
vim +/handle +553 drivers/media/usb/uvc/uvc_v4l2.c
bec43661b1dc00 drivers/media/video/uvc/uvc_v4l2.c Hans Verkuil 2008-12-30 530 static int uvc_v4l2_open(struct file *file)
c0efd232929c2c drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2008-06-30 531 {
35f02a681b72ec drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2009-06-28 532 struct uvc_streaming *stream;
c0efd232929c2c drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2008-06-30 533 struct uvc_fh *handle;
c0efd232929c2c drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2008-06-30 534 int ret = 0;
c0efd232929c2c drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2008-06-30 535
c0efd232929c2c drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2008-06-30 536 uvc_trace(UVC_TRACE_CALLS, "uvc_v4l2_open\n");
35f02a681b72ec drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2009-06-28 537 stream = video_drvdata(file);
c0efd232929c2c drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2008-06-30 538
35f02a681b72ec drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2009-06-28 539 ret = usb_autopm_get_interface(stream->dev->intf);
c0efd232929c2c drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2008-06-30 540 if (ret < 0)
716fdee110ceb8 drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2009-09-29 541 return ret;
c0efd232929c2c drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2008-06-30 542
c0efd232929c2c drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2008-06-30 543 /* Create the device handle. */
f14d4988c28e52 drivers/media/usb/uvc/uvc_v4l2.c Laurent Pinchart 2018-01-16 544 handle = kzalloc(sizeof(*handle), GFP_KERNEL);
^^^^^^^^^^^^^^^^
c0efd232929c2c drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2008-06-30 545 if (handle == NULL) {
35f02a681b72ec drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2009-06-28 546 usb_autopm_put_interface(stream->dev->intf);
716fdee110ceb8 drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2009-09-29 547 return -ENOMEM;
c0efd232929c2c drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2008-06-30 548 }
c0efd232929c2c drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2008-06-30 549
17706f5653a90f drivers/media/usb/uvc/uvc_v4l2.c Laurent Pinchart 2013-04-25 550 mutex_lock(&stream->dev->lock);
50911975ff9b21 drivers/media/usb/uvc/uvc_v4l2.c Guenter Roeck 2020-09-08 551 if (!video_is_registered(&stream->vdev)) {
50911975ff9b21 drivers/media/usb/uvc/uvc_v4l2.c Guenter Roeck 2020-09-08 552 mutex_unlock(&stream->dev->lock);
50911975ff9b21 drivers/media/usb/uvc/uvc_v4l2.c Guenter Roeck 2020-09-08 @553 return -ENODEV;
^^^^^^^^^^^^^^
kfree(handle);
50911975ff9b21 drivers/media/usb/uvc/uvc_v4l2.c Guenter Roeck 2020-09-08 554 }
17706f5653a90f drivers/media/usb/uvc/uvc_v4l2.c Laurent Pinchart 2013-04-25 555 if (stream->dev->users == 0) {
17706f5653a90f drivers/media/usb/uvc/uvc_v4l2.c Laurent Pinchart 2013-04-25 556 ret = uvc_status_start(stream->dev, GFP_KERNEL);
35f02a681b72ec drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2009-06-28 557 if (ret < 0) {
17706f5653a90f drivers/media/usb/uvc/uvc_v4l2.c Laurent Pinchart 2013-04-25 558 mutex_unlock(&stream->dev->lock);
a82a45f65377b0 drivers/media/usb/uvc/uvc_v4l2.c Oliver Neukum 2013-01-10 559 usb_autopm_put_interface(stream->dev->intf);
04a37e0f32f988 drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2009-05-19 560 kfree(handle);
716fdee110ceb8 drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2009-09-29 561 return ret;
04a37e0f32f988 drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2009-05-19 562 }
04a37e0f32f988 drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2009-05-19 563 }
04a37e0f32f988 drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2009-05-19 564
17706f5653a90f drivers/media/usb/uvc/uvc_v4l2.c Laurent Pinchart 2013-04-25 565 stream->dev->users++;
17706f5653a90f drivers/media/usb/uvc/uvc_v4l2.c Laurent Pinchart 2013-04-25 566 mutex_unlock(&stream->dev->lock);
17706f5653a90f drivers/media/usb/uvc/uvc_v4l2.c Laurent Pinchart 2013-04-25 567
d8da7513bcf983 drivers/media/usb/uvc/uvc_v4l2.c Hans Verkuil 2015-03-09 568 v4l2_fh_init(&handle->vfh, &stream->vdev);
b4012002f3a398 drivers/media/video/uvc/uvc_v4l2.c Hans de Goede 2012-04-08 569 v4l2_fh_add(&handle->vfh);
8e113595edf074 drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2009-07-01 570 handle->chain = stream->chain;
35f02a681b72ec drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2009-06-28 571 handle->stream = stream;
c0efd232929c2c drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2008-06-30 572 handle->state = UVC_HANDLE_PASSIVE;
c0efd232929c2c drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2008-06-30 573 file->private_data = handle;
c0efd232929c2c drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2008-06-30 574
716fdee110ceb8 drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2009-09-29 575 return 0;
c0efd232929c2c drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2008-06-30 576 }
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On 9/9/20 5:19 AM, Dan Carpenter wrote: > Hi Guenter, > > url: https://github.com/0day-ci/linux/commits/Guenter-Roeck/media-uvcvideo-Fix-race-conditions/20200909-121927 > base: git://linuxtv.org/media_tree.git master > config: x86_64-randconfig-m001-20200909 (attached as .config) > compiler: gcc-9 (Debian 9.3.0-15) 9.3.0 > > If you fix the issue, kindly add following tag as appropriate > Reported-by: kernel test robot <lkp@intel.com> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > > smatch warnings: > drivers/media/usb/uvc/uvc_v4l2.c:553 uvc_v4l2_open() warn: possible memory leak of 'handle' > Good catch. It is also missing a call to usb_autopm_put_interface(). I'll fix that in v3. Thanks, Guenter > # https://github.com/0day-ci/linux/commit/50911975ff9b21d08ff5408e496683b8ac567b1c > git remote add linux-review https://github.com/0day-ci/linux > git fetch --no-tags linux-review Guenter-Roeck/media-uvcvideo-Fix-race-conditions/20200909-121927 > git checkout 50911975ff9b21d08ff5408e496683b8ac567b1c > vim +/handle +553 drivers/media/usb/uvc/uvc_v4l2.c > > bec43661b1dc00 drivers/media/video/uvc/uvc_v4l2.c Hans Verkuil 2008-12-30 530 static int uvc_v4l2_open(struct file *file) > c0efd232929c2c drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2008-06-30 531 { > 35f02a681b72ec drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2009-06-28 532 struct uvc_streaming *stream; > c0efd232929c2c drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2008-06-30 533 struct uvc_fh *handle; > c0efd232929c2c drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2008-06-30 534 int ret = 0; > c0efd232929c2c drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2008-06-30 535 > c0efd232929c2c drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2008-06-30 536 uvc_trace(UVC_TRACE_CALLS, "uvc_v4l2_open\n"); > 35f02a681b72ec drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2009-06-28 537 stream = video_drvdata(file); > c0efd232929c2c drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2008-06-30 538 > 35f02a681b72ec drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2009-06-28 539 ret = usb_autopm_get_interface(stream->dev->intf); > c0efd232929c2c drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2008-06-30 540 if (ret < 0) > 716fdee110ceb8 drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2009-09-29 541 return ret; > c0efd232929c2c drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2008-06-30 542 > c0efd232929c2c drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2008-06-30 543 /* Create the device handle. */ > f14d4988c28e52 drivers/media/usb/uvc/uvc_v4l2.c Laurent Pinchart 2018-01-16 544 handle = kzalloc(sizeof(*handle), GFP_KERNEL); > ^^^^^^^^^^^^^^^^ > > c0efd232929c2c drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2008-06-30 545 if (handle == NULL) { > 35f02a681b72ec drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2009-06-28 546 usb_autopm_put_interface(stream->dev->intf); > 716fdee110ceb8 drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2009-09-29 547 return -ENOMEM; > c0efd232929c2c drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2008-06-30 548 } > c0efd232929c2c drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2008-06-30 549 > 17706f5653a90f drivers/media/usb/uvc/uvc_v4l2.c Laurent Pinchart 2013-04-25 550 mutex_lock(&stream->dev->lock); > 50911975ff9b21 drivers/media/usb/uvc/uvc_v4l2.c Guenter Roeck 2020-09-08 551 if (!video_is_registered(&stream->vdev)) { > 50911975ff9b21 drivers/media/usb/uvc/uvc_v4l2.c Guenter Roeck 2020-09-08 552 mutex_unlock(&stream->dev->lock); > 50911975ff9b21 drivers/media/usb/uvc/uvc_v4l2.c Guenter Roeck 2020-09-08 @553 return -ENODEV; > ^^^^^^^^^^^^^^ > kfree(handle); > > 50911975ff9b21 drivers/media/usb/uvc/uvc_v4l2.c Guenter Roeck 2020-09-08 554 } > 17706f5653a90f drivers/media/usb/uvc/uvc_v4l2.c Laurent Pinchart 2013-04-25 555 if (stream->dev->users == 0) { > 17706f5653a90f drivers/media/usb/uvc/uvc_v4l2.c Laurent Pinchart 2013-04-25 556 ret = uvc_status_start(stream->dev, GFP_KERNEL); > 35f02a681b72ec drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2009-06-28 557 if (ret < 0) { > 17706f5653a90f drivers/media/usb/uvc/uvc_v4l2.c Laurent Pinchart 2013-04-25 558 mutex_unlock(&stream->dev->lock); > a82a45f65377b0 drivers/media/usb/uvc/uvc_v4l2.c Oliver Neukum 2013-01-10 559 usb_autopm_put_interface(stream->dev->intf); > 04a37e0f32f988 drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2009-05-19 560 kfree(handle); > 716fdee110ceb8 drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2009-09-29 561 return ret; > 04a37e0f32f988 drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2009-05-19 562 } > 04a37e0f32f988 drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2009-05-19 563 } > 04a37e0f32f988 drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2009-05-19 564 > 17706f5653a90f drivers/media/usb/uvc/uvc_v4l2.c Laurent Pinchart 2013-04-25 565 stream->dev->users++; > 17706f5653a90f drivers/media/usb/uvc/uvc_v4l2.c Laurent Pinchart 2013-04-25 566 mutex_unlock(&stream->dev->lock); > 17706f5653a90f drivers/media/usb/uvc/uvc_v4l2.c Laurent Pinchart 2013-04-25 567 > d8da7513bcf983 drivers/media/usb/uvc/uvc_v4l2.c Hans Verkuil 2015-03-09 568 v4l2_fh_init(&handle->vfh, &stream->vdev); > b4012002f3a398 drivers/media/video/uvc/uvc_v4l2.c Hans de Goede 2012-04-08 569 v4l2_fh_add(&handle->vfh); > 8e113595edf074 drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2009-07-01 570 handle->chain = stream->chain; > 35f02a681b72ec drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2009-06-28 571 handle->stream = stream; > c0efd232929c2c drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2008-06-30 572 handle->state = UVC_HANDLE_PASSIVE; > c0efd232929c2c drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2008-06-30 573 file->private_data = handle; > c0efd232929c2c drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2008-06-30 574 > 716fdee110ceb8 drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2009-09-29 575 return 0; > c0efd232929c2c drivers/media/video/uvc/uvc_v4l2.c Laurent Pinchart 2008-06-30 576 } > > --- > 0-DAY CI Kernel Test Service, Intel Corporation > https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org > > > _______________________________________________ > kbuild mailing list -- kbuild@lists.01.org > To unsubscribe send an email to kbuild-leave@lists.01.org >
diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c index 7e5e583dae5e..1f7d454557b9 100644 --- a/drivers/media/usb/uvc/uvc_v4l2.c +++ b/drivers/media/usb/uvc/uvc_v4l2.c @@ -548,6 +548,10 @@ static int uvc_v4l2_open(struct file *file) } mutex_lock(&stream->dev->lock); + if (!video_is_registered(&stream->vdev)) { + mutex_unlock(&stream->dev->lock); + return -ENODEV; + } if (stream->dev->users == 0) { ret = uvc_status_start(stream->dev, GFP_KERNEL); if (ret < 0) { @@ -590,7 +594,7 @@ static int uvc_v4l2_release(struct file *file) file->private_data = NULL; mutex_lock(&stream->dev->lock); - if (--stream->dev->users == 0) + if (--stream->dev->users == 0 && video_is_registered(&stream->vdev)) uvc_status_stop(stream->dev); mutex_unlock(&stream->dev->lock);
uvc_v4l2_open() acquires the uvc device mutex. After doing so, it does not check if the video device is still registered. This may result in race conditions and can result in an attempt to submit an urb to a disconnected USB interface (from uvc_status_start). The problem was observed after adding a call to msleep() just before acquiring the mutex and disconnecting the camera during the sleep. Check if the video device is still registered after acquiring the mutex to avoid the problem. In the release function, only call uvc_status_stop() if the video device is still registered. If the video device has been unregistered, the urb associated with uvc status has already been killed in uvc_status_unregister(). Trying to kill it again won't do any good and might have unexpected side effects. 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> --- v2: Fix typo in description. The USB message is sent from uvc_status_start() in the open function, not from uvc_v4l2_open(). drivers/media/usb/uvc/uvc_v4l2.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)