All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 2/2 RESEND] media: usbvision: Fix races among open, close, and disconnect
@ 2019-10-07 15:09 Alan Stern
  2019-10-09 12:58 ` Hans Verkuil
  0 siblings, 1 reply; 2+ messages in thread
From: Alan Stern @ 2019-10-07 15:09 UTC (permalink / raw)
  To: Mauro Carvalho Chehab, Hans Verkuil; +Cc: linux-media, USB list, syzkaller-bugs

Visual inspection of the usbvision driver shows that it suffers from
three races between its open, close, and disconnect handlers.  In
particular, the driver is careful to update its usbvision->user and
usbvision->remove_pending flags while holding the private mutex, but:

	usbvision_v4l2_close() and usbvision_radio_close() don't hold
	the mutex while they check the value of
	usbvision->remove_pending;

	usbvision_disconnect() doesn't hold the mutex while checking
	the value of usbvision->user; and

	also, usbvision_v4l2_open() and usbvision_radio_open() don't
	check whether the device has been unplugged before allowing
	the user to open the device files.

Each of these can potentially lead to usbvision_release() being called
twice and use-after-free errors.

This patch fixes the races by reading the flags while the mutex is
still held and checking for pending removes before allowing an open to
succeed.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
CC: <stable@vger.kernel.org>

---

[as1920]


 drivers/media/usb/usbvision/usbvision-video.c |   21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

Index: usb-devel/drivers/media/usb/usbvision/usbvision-video.c
===================================================================
--- usb-devel.orig/drivers/media/usb/usbvision/usbvision-video.c
+++ usb-devel/drivers/media/usb/usbvision/usbvision-video.c
@@ -314,6 +314,10 @@ static int usbvision_v4l2_open(struct fi
 	if (mutex_lock_interruptible(&usbvision->v4l2_lock))
 		return -ERESTARTSYS;
 
+	if (usbvision->remove_pending) {
+		err_code = -ENODEV;
+		goto unlock;
+	}
 	if (usbvision->user) {
 		err_code = -EBUSY;
 	} else {
@@ -377,6 +381,7 @@ unlock:
 static int usbvision_v4l2_close(struct file *file)
 {
 	struct usb_usbvision *usbvision = video_drvdata(file);
+	int r;
 
 	PDEBUG(DBG_IO, "close");
 
@@ -391,9 +396,10 @@ static int usbvision_v4l2_close(struct f
 	usbvision_scratch_free(usbvision);
 
 	usbvision->user--;
+	r = usbvision->remove_pending;
 	mutex_unlock(&usbvision->v4l2_lock);
 
-	if (usbvision->remove_pending) {
+	if (r) {
 		printk(KERN_INFO "%s: Final disconnect\n", __func__);
 		usbvision_release(usbvision);
 		return 0;
@@ -1076,6 +1082,11 @@ static int usbvision_radio_open(struct f
 
 	if (mutex_lock_interruptible(&usbvision->v4l2_lock))
 		return -ERESTARTSYS;
+
+	if (usbvision->remove_pending) {
+		err_code = -ENODEV;
+		goto out;
+	}
 	err_code = v4l2_fh_open(file);
 	if (err_code)
 		goto out;
@@ -1108,6 +1119,7 @@ out:
 static int usbvision_radio_close(struct file *file)
 {
 	struct usb_usbvision *usbvision = video_drvdata(file);
+	int r;
 
 	PDEBUG(DBG_IO, "");
 
@@ -1121,9 +1133,10 @@ static int usbvision_radio_close(struct
 	usbvision_audio_off(usbvision);
 	usbvision->radio = 0;
 	usbvision->user--;
+	r = usbvision->remove_pending;
 	mutex_unlock(&usbvision->v4l2_lock);
 
-	if (usbvision->remove_pending) {
+	if (r) {
 		printk(KERN_INFO "%s: Final disconnect\n", __func__);
 		v4l2_fh_release(file);
 		usbvision_release(usbvision);
@@ -1555,6 +1568,7 @@ err_usb:
 static void usbvision_disconnect(struct usb_interface *intf)
 {
 	struct usb_usbvision *usbvision = to_usbvision(usb_get_intfdata(intf));
+	int u;
 
 	PDEBUG(DBG_PROBE, "");
 
@@ -1571,13 +1585,14 @@ static void usbvision_disconnect(struct
 	v4l2_device_disconnect(&usbvision->v4l2_dev);
 	usbvision_i2c_unregister(usbvision);
 	usbvision->remove_pending = 1;	/* Now all ISO data will be ignored */
+	u = usbvision->user;
 
 	usb_put_dev(usbvision->dev);
 	usbvision->dev = NULL;	/* USB device is no more */
 
 	mutex_unlock(&usbvision->v4l2_lock);
 
-	if (usbvision->user) {
+	if (u) {
 		printk(KERN_INFO "%s: In use, disconnect pending\n",
 		       __func__);
 		wake_up_interruptible(&usbvision->wait_frame);




^ permalink raw reply	[flat|nested] 2+ messages in thread
* Re: [PATCH 2/2 RESEND] media: usbvision: Fix races among open, close, and disconnect
  2019-10-07 15:09 [PATCH 2/2 RESEND] media: usbvision: Fix races among open, close, and disconnect Alan Stern
@ 2019-10-09 12:58 ` Hans Verkuil
  0 siblings, 0 replies; 2+ messages in thread
From: Hans Verkuil @ 2019-10-09 12:58 UTC (permalink / raw)
  To: Alan Stern, Mauro Carvalho Chehab; +Cc: linux-media, USB list, syzkaller-bugs

On 10/7/19 5:09 PM, Alan Stern wrote:
> Visual inspection of the usbvision driver shows that it suffers from
> three races between its open, close, and disconnect handlers.  In
> particular, the driver is careful to update its usbvision->user and
> usbvision->remove_pending flags while holding the private mutex, but:
> 
> 	usbvision_v4l2_close() and usbvision_radio_close() don't hold
> 	the mutex while they check the value of
> 	usbvision->remove_pending;
> 
> 	usbvision_disconnect() doesn't hold the mutex while checking
> 	the value of usbvision->user; and
> 
> 	also, usbvision_v4l2_open() and usbvision_radio_open() don't
> 	check whether the device has been unplugged before allowing
> 	the user to open the device files.
> 
> Each of these can potentially lead to usbvision_release() being called
> twice and use-after-free errors.
> 
> This patch fixes the races by reading the flags while the mutex is
> still held and checking for pending removes before allowing an open to
> succeed.

I suspect usbvision is full of races like that. It isn't using the proper
frameworks to take care of this. The problem is that it's old hardware and
nobody really cares about reworking the driver.

I'll take the patch, but if there is anyone interested in doing work on
this driver, then let me know. Otherwise I might just deprecate it.

Regards,

	Hans

> 
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> CC: <stable@vger.kernel.org>
> 
> ---
> 
> [as1920]
> 
> 
>  drivers/media/usb/usbvision/usbvision-video.c |   21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> Index: usb-devel/drivers/media/usb/usbvision/usbvision-video.c
> ===================================================================
> --- usb-devel.orig/drivers/media/usb/usbvision/usbvision-video.c
> +++ usb-devel/drivers/media/usb/usbvision/usbvision-video.c
> @@ -314,6 +314,10 @@ static int usbvision_v4l2_open(struct fi
>  	if (mutex_lock_interruptible(&usbvision->v4l2_lock))
>  		return -ERESTARTSYS;
>  
> +	if (usbvision->remove_pending) {
> +		err_code = -ENODEV;
> +		goto unlock;
> +	}
>  	if (usbvision->user) {
>  		err_code = -EBUSY;
>  	} else {
> @@ -377,6 +381,7 @@ unlock:
>  static int usbvision_v4l2_close(struct file *file)
>  {
>  	struct usb_usbvision *usbvision = video_drvdata(file);
> +	int r;
>  
>  	PDEBUG(DBG_IO, "close");
>  
> @@ -391,9 +396,10 @@ static int usbvision_v4l2_close(struct f
>  	usbvision_scratch_free(usbvision);
>  
>  	usbvision->user--;
> +	r = usbvision->remove_pending;
>  	mutex_unlock(&usbvision->v4l2_lock);
>  
> -	if (usbvision->remove_pending) {
> +	if (r) {
>  		printk(KERN_INFO "%s: Final disconnect\n", __func__);
>  		usbvision_release(usbvision);
>  		return 0;
> @@ -1076,6 +1082,11 @@ static int usbvision_radio_open(struct f
>  
>  	if (mutex_lock_interruptible(&usbvision->v4l2_lock))
>  		return -ERESTARTSYS;
> +
> +	if (usbvision->remove_pending) {
> +		err_code = -ENODEV;
> +		goto out;
> +	}
>  	err_code = v4l2_fh_open(file);
>  	if (err_code)
>  		goto out;
> @@ -1108,6 +1119,7 @@ out:
>  static int usbvision_radio_close(struct file *file)
>  {
>  	struct usb_usbvision *usbvision = video_drvdata(file);
> +	int r;
>  
>  	PDEBUG(DBG_IO, "");
>  
> @@ -1121,9 +1133,10 @@ static int usbvision_radio_close(struct
>  	usbvision_audio_off(usbvision);
>  	usbvision->radio = 0;
>  	usbvision->user--;
> +	r = usbvision->remove_pending;
>  	mutex_unlock(&usbvision->v4l2_lock);
>  
> -	if (usbvision->remove_pending) {
> +	if (r) {
>  		printk(KERN_INFO "%s: Final disconnect\n", __func__);
>  		v4l2_fh_release(file);
>  		usbvision_release(usbvision);
> @@ -1555,6 +1568,7 @@ err_usb:
>  static void usbvision_disconnect(struct usb_interface *intf)
>  {
>  	struct usb_usbvision *usbvision = to_usbvision(usb_get_intfdata(intf));
> +	int u;
>  
>  	PDEBUG(DBG_PROBE, "");
>  
> @@ -1571,13 +1585,14 @@ static void usbvision_disconnect(struct
>  	v4l2_device_disconnect(&usbvision->v4l2_dev);
>  	usbvision_i2c_unregister(usbvision);
>  	usbvision->remove_pending = 1;	/* Now all ISO data will be ignored */
> +	u = usbvision->user;
>  
>  	usb_put_dev(usbvision->dev);
>  	usbvision->dev = NULL;	/* USB device is no more */
>  
>  	mutex_unlock(&usbvision->v4l2_lock);
>  
> -	if (usbvision->user) {
> +	if (u) {
>  		printk(KERN_INFO "%s: In use, disconnect pending\n",
>  		       __func__);
>  		wake_up_interruptible(&usbvision->wait_frame);
> 
> 
> 


^ permalink raw reply	[flat|nested] 2+ messages in thread
end of thread, other threads:[~2019-10-09 12:58 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-07 15:09 [PATCH 2/2 RESEND] media: usbvision: Fix races among open, close, and disconnect Alan Stern
2019-10-09 12:58 ` Hans Verkuil

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.