media: uvc_driver: fix USB Camera ref leak denial of service

PATCH In review David Fries 2 participants 4 messages May 26, 2018

Subsystems (3)

Stable branch Media (V4L/DVB) USB video class

lore

Share & tools

lore

David Fries

author

2018-05-26 16:01 UTC #0 lore raw

PATCH 1/1 media: uvc_driver: fix USB Camera ref leak denial of service

Commit 9d15cd958c17 ("media: uvcvideo: Convert from using an atomic variable to a reference count")
didn't take into account that while the old counter was initialized to 0 (no stream open), kref_init starts with a reference of 1. The reference count on unplug no longer reaches 0, uvc_delete isn't called, and evdev doesn't release /dev/input/event*. Plug and unplug enough times and it runs out of device minors preventing any new input device and the use of newly plugged in USB video cameras until the system is rebooted.

Signed-off-by David Fries <[email protected]>

Cc Guennadi Liakhovetski <[email protected]>

Cc Laurent Pinchart <[email protected]>

Cc Mauro Carvalho Chehab <[email protected]>

Cc [email protected]

---
 drivers/media/usb/uvc/uvc_driver.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

drivers/media/usb/uvc/uvc_driver.c +4 -7

index 2469b49..3cbdc87 100644
@@ -1871,13 +1871,6 @@ static void uvc_unregister_video(struct uvc_device *dev)
 {
 	struct uvc_streaming *stream;
 
-	/* Unregistering all video devices might result in uvc_delete() being
-	 * called from inside the loop if there's no open file handle. To avoid
-	 * that, increment the refcount before iterating over the streams and
-	 * decrement it when done.
-	 */
-	kref_get(&dev->ref);
-
 	list_for_each_entry(stream, &dev->streams, list) {
 		if (!video_is_registered(&stream->vdev))
 			continue;

@@ -1888,6 +1881,10 @@ static void uvc_unregister_video(struct uvc_device *dev)
 		uvc_debugfs_cleanup_stream(stream);
 	}
 
+	/* Release the reference implicit in kref_init from uvc_probe,
+	 * the UVC device won't be deleted until the last file descriptor
+	 * is also closed.
+	 */
 	kref_put(&dev->ref, uvc_delete);
 }

Signature · 1 line

2.1.4

Quoted 1 line

From 32a612bc06a2a1b9215f3b7166342c98043bd925 Mon Sep 17 00:00:00 2001

From: David Fries <[email protected]>
Date: Thu, 24 May 2018 23:43:15 -0500
Subject: [PATCH] uvc_driver: UVC kref never reaches zero leading to denial of
service

Commit 9d15cd958c17 ("media: uvcvideo: Convert from using an atomic variable to a reference count")
didn't take into account that while the counter was initialized to 0 (no stream open), kref_init starts with a reference of 1, leading to the device
never reaching 0 and uvc_delete never getting called. This leads to evdev never getting released and /dev/input/event* eventually running out of minors preventing any new event devices and new USB cameras from being usable until the system is rebooted.

In my case "disabled by hub (EMI?), re-enabling..." kept removing/inserting the device until days later it ran out of minors and I lost the video security feed.

Now that the device is actually being removed other problems are showing up. Specifically the following if the camera is removed or rmmod ehci_pci while an application is getting video from it. It doesn't happen if the camera is not in use. How do I track that down?

sysfs group 'power' not found for kobject 'event10' sysfs group 'power' not found for kobject 'input32' sysfs group 'id' not found for kobject 'input32' sysfs group 'capabilities' not found for kobject 'input32' sysfs group 'power' not found for kobject 'media0'

Signed-off-by David Fries <[email protected]>

---
 drivers/media/usb/uvc/uvc_driver.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

drivers/media/usb/uvc/uvc_driver.c +4 -7

index 2469b49..3cbdc87 100644
@@ -1871,13 +1871,6 @@ static void uvc_unregister_video(struct uvc_device *dev)
 {
 	struct uvc_streaming *stream;
 
-	/* Unregistering all video devices might result in uvc_delete() being
-	 * called from inside the loop if there's no open file handle. To avoid
-	 * that, increment the refcount before iterating over the streams and
-	 * decrement it when done.
-	 */
-	kref_get(&dev->ref);
-
 	list_for_each_entry(stream, &dev->streams, list) {
 		if (!video_is_registered(&stream->vdev))
 			continue;

@@ -1888,6 +1881,10 @@ static void uvc_unregister_video(struct uvc_device *dev)
 		uvc_debugfs_cleanup_stream(stream);
 	}
 
+	/* Release the reference implicit in kref_init from uvc_probe,
+	 * the UVC device won't be deleted until the last file descriptor
+	 * is also closed.
+	 */
 	kref_put(&dev->ref, uvc_delete);
 }

Signature · 1 line

2.1.4

Laurent Pinchart

maintainer

2018-05-26 16:21 UTC #1 lore raw

Hi David,

Thank you for the patch.

Quoted 10 lines · On Saturday, 26 May 2018 18:50:46 EEST David Fries wrote:

Commit 9d15cd958c17 ("media: uvcvideo: Convert from using an atomic variable to a reference count")
didn't take into account that while the old counter was initialized to 0 (no stream open), kref_init starts with a reference of 1. The reference count on unplug no longer reaches 0, uvc_delete isn't called, and evdev doesn't release /dev/input/event*. Plug and unplug enough times and it runs out of device minors preventing any new input device and the use of newly plugged in USB video cameras until the system is rebooted.

Quoted 5 lines

Signed-off-by David Fries <[email protected]>
Cc Guennadi Liakhovetski <[email protected]>
Cc Laurent Pinchart <[email protected]>
Cc Mauro Carvalho Chehab <[email protected]>
Cc [email protected]

Philipp Zabel has already posted a similar patch, see https://
patchwork.linuxtv.org/patch/49770/

Mauro,

This is a serious issue so I'd like to get the patch merged in v4.18, but it could be argued that it's getting late for that, especially given that the bug has been there since v4.14. Would you be OK merging this in v4.18 ? If so could you pick https://patchwork.linuxtv.org/patch/49770/ up, or would you like me to send a pull request ?

Quoted 32 lines

---
 drivers/media/usb/uvc/uvc_driver.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

(quoted diff) +0 -0

b/drivers/media/usb/uvc/uvc_driver.c index 2469b49..3cbdc87 100644

drivers/media/usb/uvc/uvc_driver.c +4 -7

@@ -1871,13 +1871,6 @@ static void uvc_unregister_video(struct uvc_device
 *dev) {
 	struct uvc_streaming *stream;
 
-	/* Unregistering all video devices might result in uvc_delete() being
-	 * called from inside the loop if there's no open file handle. To avoid
-	 * that, increment the refcount before iterating over the streams and
-	 * decrement it when done.
-	 */
-	kref_get(&dev->ref);
-
 	list_for_each_entry(stream, &dev->streams, list) {
 		if (!video_is_registered(&stream->vdev))
 			continue;

@@ -1888,6 +1881,10 @@ static void uvc_unregister_video(struct uvc_device
 *dev) uvc_debugfs_cleanup_stream(stream);
 	}
 
+	/* Release the reference implicit in kref_init from uvc_probe,
+	 * the UVC device won't be deleted until the last file descriptor
+	 * is also closed.
+	 */
 	kref_put(&dev->ref, uvc_delete);
 }

Signature · 3 lines

Regards,

Laurent Pinchart

David Fries

author

2018-05-26 16:37 UTC #2 lore raw

Laurent and Mauro,

I've reviewed Philipp Zabel's 49770 patch, it looks like they equivalent, but take a different path. My version does the unref in uvc_unregister_video, Zabel's does the unref in each caller of uvc_unregister_video. I think I would prefer it to be in fewer places.

Any ideas on the sysfs group 'power' not found bug on removal while on use?

Thanks, it will be great to get this fixed.

Quoted 71 lines · Laurent Pinchart

Hi David,
Thank you for the patch.
Quoted 10 lines · On Saturday, 26 May 2018 18:50:46 EEST David Fries wrote:
Commit 9d15cd958c17 ("media: uvcvideo: Convert from using an atomic variable to a reference count")
didn't take into account that while the old counter was initialized to 0 (no stream open), kref_init starts with a reference of 1. The reference count on unplug no longer reaches 0, uvc_delete isn't called, and evdev doesn't release /dev/input/event*. Plug and unplug enough times and it runs out of device minors preventing any new input device and the use of newly plugged in USB video cameras until the system is rebooted.
Quoted 5 lines
Signed-off-by David Fries <[email protected]>
Cc Guennadi Liakhovetski <[email protected]>
Cc Laurent Pinchart <[email protected]>
Cc Mauro Carvalho Chehab <[email protected]>
Cc [email protected]
Philipp Zabel has already posted a similar patch, see https://
patchwork.linuxtv.org/patch/49770/
Mauro,
This is a serious issue so I'd like to get the patch merged in v4.18, but it could be argued that it's getting late for that, especially given that the bug has been there since v4.14. Would you be OK merging this in v4.18 ? If so could you pick https://patchwork.linuxtv.org/patch/49770/ up, or would you like me to send a pull request ?
Quoted 32 lines
---
 drivers/media/usb/uvc/uvc_driver.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)
(quoted diff) +0 -0
b/drivers/media/usb/uvc/uvc_driver.c index 2469b49..3cbdc87 100644
drivers/media/usb/uvc/uvc_driver.c +4 -7
@@ -1871,13 +1871,6 @@ static void uvc_unregister_video(struct uvc_device
 *dev) {
 	struct uvc_streaming *stream;
 
-	/* Unregistering all video devices might result in uvc_delete() being
-	 * called from inside the loop if there's no open file handle. To avoid
-	 * that, increment the refcount before iterating over the streams and
-	 * decrement it when done.
-	 */
-	kref_get(&dev->ref);
-
 	list_for_each_entry(stream, &dev->streams, list) {
 		if (!video_is_registered(&stream->vdev))
 			continue;
@@ -1888,6 +1881,10 @@ static void uvc_unregister_video(struct uvc_device
 *dev) uvc_debugfs_cleanup_stream(stream);
 	}
 
+	/* Release the reference implicit in kref_init from uvc_probe,
+	 * the UVC device won't be deleted until the last file descriptor
+	 * is also closed.
+	 */
 	kref_put(&dev->ref, uvc_delete);
 }
Signature · 3 lines
Regards,

Laurent Pinchart

Signature · 1 line

David Fries <[email protected]>

Laurent Pinchart

maintainer

2018-05-26 16:47 UTC #3 lore raw

Hi David,

Quoted 7 lines · On Saturday, 26 May 2018 19:37:18 EEST David Fries wrote:

Laurent and Mauro,
I've reviewed Philipp Zabel's 49770 patch, it looks like they equivalent, but take a different path. My version does the unref in uvc_unregister_video, Zabel's does the unref in each caller of uvc_unregister_video. I think I would prefer it to be in fewer places.

While I agree that avoiding code duplication is a good idea, we also need to ensure that the unref call is the very last operation we perform, otherwise we'll risk a use-after-free. The uvc_unregister_video() function is called last so that's guaranteed with your patch. However, by hiding the unref there from the callers, I wonder whether it couldn't result in issues in the future if someone decides to add code after the uvc_unregister_video() calls without noticing the unref issue.

Quoted 2 lines

Any ideas on the sysfs group 'power' not found bug on removal while on use?

I haven't had time to investigate them yet, please feel free to give it a go without waiting for me :-)

Quoted 48 lines

Thanks, it will be great to get this fixed.
Quoted 45 lines · Laurent Pinchart
Hi David,
Thank you for the patch.
Quoted 10 lines · On Saturday, 26 May 2018 18:50:46 EEST David Fries wrote:
Commit 9d15cd958c17 ("media: uvcvideo: Convert from using an atomic variable to a reference count")
didn't take into account that while the old counter was initialized to 0 (no stream open), kref_init starts with a reference of 1. The reference count on unplug no longer reaches 0, uvc_delete isn't called, and evdev doesn't release /dev/input/event*. Plug and unplug enough times and it runs out of device minors preventing any new input device and the use of newly plugged in USB video cameras until the system is rebooted.
Quoted 5 lines
Signed-off-by David Fries <[email protected]>
Cc Guennadi Liakhovetski <[email protected]>
Cc Laurent Pinchart <[email protected]>
Cc Mauro Carvalho Chehab <[email protected]>
Cc [email protected]
Philipp Zabel has already posted a similar patch, see https://
patchwork.linuxtv.org/patch/49770/
Mauro,
This is a serious issue so I'd like to get the patch merged in v4.18, but it could be argued that it's getting late for that, especially given that the bug has been there since v4.14. Would you be OK merging this in v4.18 ? If so could you pick https://patchwork.linuxtv.org/patch/49770/ up, or would you like me to send a pull request ?
Quoted 13 lines
---
 drivers/media/usb/uvc/uvc_driver.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)
(quoted diff) +0 -0
b/drivers/media/usb/uvc/uvc_driver.c index 2469b49..3cbdc87 100644
drivers/media/usb/uvc/uvc_driver.c +0 -0
@@ -1871,13 +1871,6 @@ static void uvc_unregister_video(struct
uvc_device
*dev) {

Quoted 3 lines

Quoted 3 lines
drivers/media/usb/uvc/uvc_driver.c static void uvc_unregister_video(struct uvc_device *dev)
Quoted 3 lines
drivers/media/usb/uvc/uvc_driver.c +0 -1
 	struct uvc_streaming *stream;
 
-	/* Unregistering all video devices might result in uvc_delete() 

being

Quoted 3 lines

Quoted 3 lines
drivers/media/usb/uvc/uvc_driver.c
Quoted 3 lines
drivers/media/usb/uvc/uvc_driver.c +0 -1
-	 * called from inside the loop if there's no open file handle. To
avoid
drivers/media/usb/uvc/uvc_driver.c +0 -1
-	 * that, increment the refcount before iterating over the streams 

and

Quoted 25 lines

drivers/media/usb/uvc/uvc_driver.c +0 -4

-	 * decrement it when done.
-	 */
-	kref_get(&dev->ref);
-
 
 	list_for_each_entry(stream, &dev->streams, list) {
 
 		if (!video_is_registered(&stream->vdev))
 
 			continue;

@@ -1888,6 +1881,10 @@ static void uvc_unregister_video(struct

uvc_device
*dev) uvc_debugfs_cleanup_stream(stream);

drivers/media/usb/uvc/uvc_driver.c +4 -0

 	}
 
+	/* Release the reference implicit in kref_init from uvc_probe,
+	 * the UVC device won't be deleted until the last file descriptor
+	 * is also closed.
+	 */
 
 	kref_put(&dev->ref, uvc_delete);
 
 }

Signature · 3 lines

Regards,

Laurent Pinchart