Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To:     David Fries <David@fries.net>
Cc:     linux-kernel@vger.kernel.org,
        Guennadi Liakhovetski <g.liakhovetski@gmx.de>,
        Mauro Carvalho Chehab <mchehab@s-opensource.com>,
        stable@vger.kernel.org
Subject: Re: [PATCH 1/1] media: uvc_driver: fix USB Camera ref leak denial of service
Date:   Sat, 26 May 2018 19:21:11 +0300
Message-ID: <3294600.sHRg43ppdH@avalon>
Organization: Ideas on Board Oy
In-Reply-To: <20180526155046.GA3327@spacedout.fries.net>
References: <20180526155046.GA3327@spacedout.fries.net>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi David,

Thank you for the patch.

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.
> 
> Signed-off-by: David Fries <David@Fries.net>
> Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> Cc: stable@vger.kernel.org

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 ?

> ---
>  drivers/media/usb/uvc/uvc_driver.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c
> b/drivers/media/usb/uvc/uvc_driver.c index 2469b49..3cbdc87 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -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);
>  }

-- 
Regards,

Laurent Pinchart