linux-kernel - Re: uvcvideo: Race on dev->state between uvc_disconnect() and uvc_v4l2

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3959395.nkzPWg6atK@avalon>
Date:	Mon, 25 May 2015 01:32:39 +0300
From:	Laurent Pinchart <laurent.pinchart@...asonboard.com>
To:	Eugene Shatokhin <eugene.shatokhin@...alab.ru>
Cc:	Mauro Carvalho Chehab <mchehab@....samsung.com>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: uvcvideo: Race on dev->state between uvc_disconnect() and uvc_v4l2_open()

Hi Eugene,

On Wednesday 20 May 2015 17:48:41 Eugene Shatokhin wrote:
> Hi,
> 
> There is a race in uvcvideo module between uvc_disconnect() and
> uvc_v4l2_open() on dev->state. Checked and reproduced that with kernel
> 4.1-rc1.
> 
> drivers/media/usb/uvc/uvc_driver.c, uvc_disconnect():
> 
> 	dev->state |= UVC_DEV_DISCONNECTED;
> 
> drivers/media/usb/uvc/uvc_v4l2.c, uvc_v4l2_open():
> 
> 	if (stream->dev->state & UVC_DEV_DISCONNECTED)
> 		return -ENODEV;
> 
> I checked that the race does happen by introducing a delay in
> uvc_disconnect() right before that assignment and armed a hardware
> breakpoint to detect the access to stream->dev->state from
> uvc_v4l2_open(). When I disconnected the webcam while Google Hangout was
> running, the hardware breakpoint triggered several times for that read
> in uvc_v4l2_open (uvc_v4l2.c:484). uvc_v4l2_open() was called in the
> context of GoogleTalkPlugin processes.
> 
> Not sure if the race is intentional but I guess, better to report it
> anyway. Nothing has crashed during my (brief) testing yet, but still.

The race condition between disconnect and open is unavoidable. What is 
avoidable, though, is the crashes and other ill side-effects that could result 
from it. The following commit handles that.

commit ca9afe6f87b569cdf8e797395381f18ae23a2905
Author: Hans Verkuil <hverkuil@...all.nl>
Date:   Fri Nov 26 06:54:53 2010 -0300

    [media] v4l2-dev: fix race condition
    
    The unregister function had a race condition with the v4l2_open
    function. Ensure that both functions test and clear the REGISTER
    flag from within a critical section.
    
    Thanks to Laurent Pinchart for finding this race.
    
    Signed-off-by: Hans Verkuil <hverkuil@...all.nl>
    Signed-off-by: Mauro Carvalho Chehab <mchehab@...hat.com>


The race was previously handled by the uvcvideo driver, and the code got 
removed in the following commit after the above commit got merged.

commit 716fdee110ceb816cca8c46c0890d08c5a1addb9
Author: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Date:   Tue Sep 29 21:07:19 2009 -0300

    V4L/DVB (13152): uvcvideo: Rely on videodev to reference-count the device
    
    The uvcvideo driver has a driver-wide lock and a reference count to 
protect
    against a disconnect/open race. Now that videodev handles the race itself,
    reference-counting in the driver can be removed.
    
    Signed-off-by: Laurent Pinchart <laurent.pinchart@...asonboard.com>
    Signed-off-by: Mauro Carvalho Chehab <mchehab@...hat.com>


Setting the UVC_DEV_DISCONNECTED flag seems unneeded nowadays. I'll have to 
carefully think about it though, and it's too late right now to do so :-)

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/