Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1326364 > unrolled thread

[PATCH v3] usb: devio: Add ioctl to disallow detaching kernel USB drivers.

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v3] usb: devio: Add ioctl to disallow detaching kernel USB drivers. Emilio López <emilio.lopez@collabora.co.uk> - 2016-02-04 04:30 +0100
    Re: [PATCH v3] usb: devio: Add ioctl to disallow detaching kernel  USB drivers. Greg KH <gregkh@linuxfoundation.org> - 2016-02-04 04:50 +0100
    Re: [PATCH v3] usb: devio: Add ioctl to disallow detaching kernel  USB drivers. Alan Stern <stern@rowland.harvard.edu> - 2016-02-04 17:30 +0100
      Re: [PATCH v3] usb: devio: Add ioctl to disallow detaching kernel USB  drivers. Emilio López <emilio.lopez@collabora.co.uk> - 2016-02-08 03:10 +0100

#1326364 — [PATCH v3] usb: devio: Add ioctl to disallow detaching kernel USB drivers.

From: Reilly Grant <reillyg@chromium.org>

The new USBDEVFS_DROP_PRIVILEGES ioctl allows a process to voluntarily
relinquish the ability to issue other ioctls that may interfere with
other processes and drivers that have claimed an interface on the
device.

Signed-off-by: Reilly Grant <reillyg@chromium.org>
Signed-off-by: Emilio López <emilio.lopez@collabora.co.uk>

---

Changes in v3:
- Switch ioctl to use a __u32 given the iface qty is capped at 32
- Reword comments as requested by Alan
- Allow callers to shrink the allowed interfaces mask

Changes in v2:
- Added a parameter to the ioctl, a mask of interfaces an unprivileged
  process is allowed to claim.
- Drop Tested-by's

 drivers/usb/core/devio.c          | 59 ++++++++++++++++++++++++++++++++++++---
 include/uapi/linux/usbdevice_fs.h |  1 +
 2 files changed, 56 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index 59e7a33..3750255 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -77,6 +77,8 @@ struct usb_dev_state {
 	unsigned long ifclaimed;
 	u32 secid;
 	u32 disabled_bulk_eps;
+	bool privileges_dropped;
+	unsigned long interface_allowed_mask;
 };
 
 struct async {
@@ -624,6 +626,10 @@ static int claimintf(struct usb_dev_state *ps, unsigned int ifnum)
 	if (test_bit(ifnum, &ps->ifclaimed))
 		return 0;
 
+	if (ps->privileges_dropped
+	 && !test_bit(ifnum, &ps->interface_allowed_mask))
+		return -EACCES;
+
 	intf = usb_ifnum_to_if(dev, ifnum);
 	if (!intf)
 		err = -ENOENT;
@@ -861,7 +867,7 @@ static int usbdev_open(struct inode *inode, struct file *file)
 	int ret;
 
 	ret = -ENOMEM;
-	ps = kmalloc(sizeof(struct usb_dev_state), GFP_KERNEL);
+	ps = kzalloc(sizeof(struct usb_dev_state), GFP_KERNEL);
 	if (!ps)
 		goto out_free_ps;
 
@@ -889,16 +895,14 @@ static int usbdev_open(struct inode *inode, struct file *file)
 
 	ps->dev = dev;
 	ps->file = file;
+	ps->interface_allowed_mask = 0xFFFFFFFF; /* 32 bits */
 	spin_lock_init(&ps->lock);
 	INIT_LIST_HEAD(&ps->list);
 	INIT_LIST_HEAD(&ps->async_pending);
 	INIT_LIST_HEAD(&ps->async_completed);
 	init_waitqueue_head(&ps->wait);
-	ps->discsignr = 0;
 	ps->disc_pid = get_pid(task_pid(current));
 	ps->cred = get_current_cred();
-	ps->disccontext = NULL;
-	ps->ifclaimed = 0;
 	security_task_getsecid(current, &ps->secid);
 	smp_wmb();
 	list_add_tail(&ps->list, &dev->filelist);
@@ -1198,6 +1202,27 @@ static int proc_connectinfo(struct usb_dev_state *ps, void __user *arg)
 
 static int proc_resetdevice(struct usb_dev_state *ps)
 {
+	struct usb_host_config *actconfig = ps->dev->actconfig;
+	struct usb_interface *interface;
+	int i, number;
+
+	/* Don't allow a device reset if the process has dropped the
+	 * privilege to do such things and any of the interfaces are
+	 * currently claimed.
+	 */
+	if (ps->privileges_dropped && actconfig) {
+		for (i = 0; i < actconfig->desc.bNumInterfaces; ++i) {
+			interface = actconfig->interface[i];
+			number = interface->cur_altsetting->desc.bInterfaceNumber;
+			if (usb_interface_claimed(interface)) {
+				dev_warn(&ps->dev->dev,
+					"usbfs: interface %d claimed by %s while '%s' resets device\n",
+					number,	interface->dev.driver->name, current->comm);
+				return -EACCES;
+			}
+		}
+	}
+
 	return usb_reset_device(ps->dev);
 }
 
@@ -1915,6 +1940,9 @@ static int proc_ioctl(struct usb_dev_state *ps, struct usbdevfs_ioctl *ctl)
 	struct usb_interface    *intf = NULL;
 	struct usb_driver       *driver = NULL;
 
+	if (ps->privileges_dropped)
+		return -EACCES;
+
 	/* alloc buffer */
 	size = _IOC_SIZE(ctl->ioctl_code);
 	if (size > 0) {
@@ -2067,6 +2095,9 @@ static int proc_disconnect_claim(struct usb_dev_state *ps, void __user *arg)
 	if (intf->dev.driver) {
 		struct usb_driver *driver = to_usb_driver(intf->dev.driver);
 
+		if (ps->privileges_dropped)
+			return -EACCES;
+
 		if ((dc.flags & USBDEVFS_DISCONNECT_CLAIM_IF_DRIVER) &&
 				strncmp(dc.driver, intf->dev.driver->name,
 					sizeof(dc.driver)) != 0)
@@ -2123,6 +2154,23 @@ static int proc_free_streams(struct usb_dev_state *ps, void __user *arg)
 	return r;
 }
 
+static int proc_drop_privileges(struct usb_dev_state *ps, void __user *arg)
+{
+	u32 data;
+
+	if (copy_from_user(&data, arg, sizeof(data)))
+		return -EFAULT;
+
+	/* This is an one way operation. Once privileges are
+	 * dropped, you cannot regain them. You may however reissue
+	 * this ioctl to shrink the allowed interfaces mask.
+	 */
+	ps->interface_allowed_mask &= data;
+	ps->privileges_dropped = true;
+
+	return 0;
+}
+
 /*
  * NOTE:  All requests here that have interface numbers as parameters
  * are assuming that somehow the configuration has been prevented from
@@ -2311,6 +2359,9 @@ static long usbdev_do_ioctl(struct file *file, unsigned int cmd,
 	case USBDEVFS_FREE_STREAMS:
 		ret = proc_free_streams(ps, p);
 		break;
+	case USBDEVFS_DROP_PRIVILEGES:
+		ret = proc_drop_privileges(ps, p);
+		break;
 	}
 
  done:
diff --git a/include/uapi/linux/usbdevice_fs.h b/include/uapi/linux/usbdevice_fs.h
index 019ba1e..2fad9f85 100644
--- a/include/uapi/linux/usbdevice_fs.h
+++ b/include/uapi/linux/usbdevice_fs.h
@@ -187,5 +187,6 @@ struct usbdevfs_streams {
 #define USBDEVFS_DISCONNECT_CLAIM  _IOR('U', 27, struct usbdevfs_disconnect_claim)
 #define USBDEVFS_ALLOC_STREAMS     _IOR('U', 28, struct usbdevfs_streams)
 #define USBDEVFS_FREE_STREAMS      _IOR('U', 29, struct usbdevfs_streams)
+#define USBDEVFS_DROP_PRIVILEGES   _IOR('U', 30, __u32)
 
 #endif /* _UAPI_LINUX_USBDEVICE_FS_H */
-- 
2.5.0

[toc] | [next] | [standalone]

#1326370 — Re: [PATCH v3] usb: devio: Add ioctl to disallow detaching kernel USB drivers.

On Thu, Feb 04, 2016 at 12:20:57AM -0300, Emilio López wrote:
> From: Reilly Grant <reillyg@chromium.org>
> 
> The new USBDEVFS_DROP_PRIVILEGES ioctl allows a process to voluntarily
> relinquish the ability to issue other ioctls that may interfere with
> other processes and drivers that have claimed an interface on the
> device.
> 
> Signed-off-by: Reilly Grant <reillyg@chromium.org>
> Signed-off-by: Emilio López <emilio.lopez@collabora.co.uk>
> 
> ---
> 
> Changes in v3:
> - Switch ioctl to use a __u32 given the iface qty is capped at 32
> - Reword comments as requested by Alan
> - Allow callers to shrink the allowed interfaces mask

No documentation?  Proof it works?  Test scripts?

This isn't a trivial feature to add, how do we know it is correct?

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1326971 — Re: [PATCH v3] usb: devio: Add ioctl to disallow detaching kernel USB drivers.

On Thu, 4 Feb 2016, Emilio López wrote:

> From: Reilly Grant <reillyg@chromium.org>
> 
> The new USBDEVFS_DROP_PRIVILEGES ioctl allows a process to voluntarily
> relinquish the ability to issue other ioctls that may interfere with
> other processes and drivers that have claimed an interface on the
> device.
> 
> Signed-off-by: Reilly Grant <reillyg@chromium.org>
> Signed-off-by: Emilio López <emilio.lopez@collabora.co.uk>
> 
> ---
> 
> Changes in v3:
> - Switch ioctl to use a __u32 given the iface qty is capped at 32
> - Reword comments as requested by Alan
> - Allow callers to shrink the allowed interfaces mask

> @@ -624,6 +626,10 @@ static int claimintf(struct usb_dev_state *ps, unsigned int ifnum)
>  	if (test_bit(ifnum, &ps->ifclaimed))
>  		return 0;
>  
> +	if (ps->privileges_dropped
> +	 && !test_bit(ifnum, &ps->interface_allowed_mask))

Continuation lines in this file are indented by 2 tab stops, not 1 
space.

> @@ -1198,6 +1202,27 @@ static int proc_connectinfo(struct usb_dev_state *ps, void __user *arg)
>  
>  static int proc_resetdevice(struct usb_dev_state *ps)
>  {
> +	struct usb_host_config *actconfig = ps->dev->actconfig;
> +	struct usb_interface *interface;
> +	int i, number;
> +
> +	/* Don't allow a device reset if the process has dropped the
> +	 * privilege to do such things and any of the interfaces are
> +	 * currently claimed.
> +	 */
> +	if (ps->privileges_dropped && actconfig) {
> +		for (i = 0; i < actconfig->desc.bNumInterfaces; ++i) {
> +			interface = actconfig->interface[i];
> +			number = interface->cur_altsetting->desc.bInterfaceNumber;
> +			if (usb_interface_claimed(interface)) {

The test should be:

			if (usb_interface_claimed(interface) &&
					!test_bit(number, &ps->ifclaimed)) {

We don't want to prevent people from resetting a device merely because 
they have claimed an interface.  Only if someone else has claimed one.

> +				dev_warn(&ps->dev->dev,
> +					"usbfs: interface %d claimed by %s while '%s' resets device\n",
> +					number,	interface->dev.driver->name, current->comm);
> +				return -EACCES;
> +			}
> +		}
> +	}
> +
>  	return usb_reset_device(ps->dev);
>  }

Also, it wouldn't hurt to change proc_get_capabilities() and
include/uapi/linux/usbdevicefs.h to add a USBDEVFS_CAP_DROP_PRIVILEGES
bit.

Alan Stern

[toc] | [prev] | [next] | [standalone]

#1328697 — Re: [PATCH v3] usb: devio: Add ioctl to disallow detaching kernel USB drivers.

Hello Alan,

El 04/02/16 a las 13:27, Alan Stern escribió:
> On Thu, 4 Feb 2016, Emilio López wrote:
>
>> From: Reilly Grant <reillyg@chromium.org>
>>
>> The new USBDEVFS_DROP_PRIVILEGES ioctl allows a process to voluntarily
>> relinquish the ability to issue other ioctls that may interfere with
>> other processes and drivers that have claimed an interface on the
>> device.
>>
>> Signed-off-by: Reilly Grant <reillyg@chromium.org>
>> Signed-off-by: Emilio López <emilio.lopez@collabora.co.uk>
>>
>> ---
>>
>> Changes in v3:
>> - Switch ioctl to use a __u32 given the iface qty is capped at 32
>> - Reword comments as requested by Alan
>> - Allow callers to shrink the allowed interfaces mask
>
>> @@ -624,6 +626,10 @@ static int claimintf(struct usb_dev_state *ps, unsigned int ifnum)
>>   	if (test_bit(ifnum, &ps->ifclaimed))
>>   		return 0;
>>
>> +	if (ps->privileges_dropped
>> +	 && !test_bit(ifnum, &ps->interface_allowed_mask))
>
> Continuation lines in this file are indented by 2 tab stops, not 1
> space.

Ok, I'll change it.

>> @@ -1198,6 +1202,27 @@ static int proc_connectinfo(struct usb_dev_state *ps, void __user *arg)
>>
>>   static int proc_resetdevice(struct usb_dev_state *ps)
>>   {
>> +	struct usb_host_config *actconfig = ps->dev->actconfig;
>> +	struct usb_interface *interface;
>> +	int i, number;
>> +
>> +	/* Don't allow a device reset if the process has dropped the
>> +	 * privilege to do such things and any of the interfaces are
>> +	 * currently claimed.
>> +	 */
>> +	if (ps->privileges_dropped && actconfig) {
>> +		for (i = 0; i < actconfig->desc.bNumInterfaces; ++i) {
>> +			interface = actconfig->interface[i];
>> +			number = interface->cur_altsetting->desc.bInterfaceNumber;
>> +			if (usb_interface_claimed(interface)) {
>
> The test should be:
>
> 			if (usb_interface_claimed(interface) &&
> 					!test_bit(number, &ps->ifclaimed)) {
>
> We don't want to prevent people from resetting a device merely because
> they have claimed an interface.  Only if someone else has claimed one.

Sounds sensible, now changed as well.

>> +				dev_warn(&ps->dev->dev,
>> +					"usbfs: interface %d claimed by %s while '%s' resets device\n",
>> +					number,	interface->dev.driver->name, current->comm);
>> +				return -EACCES;
>> +			}
>> +		}
>> +	}
>> +
>>   	return usb_reset_device(ps->dev);
>>   }
>
> Also, it wouldn't hurt to change proc_get_capabilities() and
> include/uapi/linux/usbdevicefs.h to add a USBDEVFS_CAP_DROP_PRIVILEGES
> bit.

Good idea, I've added a bit now. I'm writing some docs and polishing a 
program to test this as Greg requested and I'll submit v4 then.

Thanks!
Emilio

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web