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

[Peter Chen]

On Wed, Nov 25, 2015 at 12:45:34PM -0300, Emilio López wrote:
> From: Reilly Grant <reillyg@xxxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxx>
> Reviewed-by: Jorge Lucangeli Obes <jorgelo@xxxxxxxxxxxx>
> Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx>
> [emilio.lopez: fix build for v4.4-rc2 and adjust patch title prefix]
> Signed-off-by: Emilio López <emilio.lopez@xxxxxxxxxxxxxxx>
> 
> ---
> 
>  drivers/usb/core/devio.c          | 50 +++++++++++++++++++++++++++++++++++----
>  include/uapi/linux/usbdevice_fs.h |  1 +
>  2 files changed, 47 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> index 38ae877c..a5ccc50 100644
> --- a/drivers/usb/core/devio.c
> +++ b/drivers/usb/core/devio.c
> @@ -77,6 +77,7 @@ struct usb_dev_state {
>  	unsigned long ifclaimed;
>  	u32 secid;
>  	u32 disabled_bulk_eps;
> +	bool privileges_dropped;
>  };
>  
>  struct async {
> @@ -878,7 +879,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;
>  
> @@ -911,11 +912,8 @@ static int usbdev_open(struct inode *inode, struct file *file)
>  	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;

The above changes seem to be not related with this change.

>  	security_task_getsecid(current, &ps->secid);
>  	smp_wmb();
>  	list_add_tail(&ps->list, &dev->filelist);
> @@ -1215,6 +1213,35 @@ static int proc_connectinfo(struct usb_dev_state *ps, void __user *arg)
>  
>  static int proc_resetdevice(struct usb_dev_state *ps)
>  {
> +	if (ps->privileges_dropped) {
> +		struct usb_host_config *actconfig = ps->dev->actconfig;
> +
> +		/* Don't touch the device if any interfaces are claimed. It
> +		 * could interfere with other drivers' operations and this
> +		 * process has dropped its privileges to do such things.
> +		 */
> +		if (actconfig) {
> +			int i;
> +
> +			for (i = 0; i < actconfig->desc.bNumInterfaces; ++i) {
> +				if (usb_interface_claimed(
> +					actconfig->interface[i])) {
> +					dev_warn(&ps->dev->dev,
> +						"usbfs: interface %d claimed by"
> +						" %s while '%s'"
> +						" resets device\n",
> +						actconfig->interface[i]
> +							->cur_altsetting
> +							->desc.bInterfaceNumber,

The length of line is 80 characters

> +						actconfig->interface[i]
> +							->dev.driver->name,
> +						current->comm);
> +					return -EACCES;
> +				}
> +			}
> +		}
> +	}
> +
>  	return usb_reset_device(ps->dev);
>  }
>  
> @@ -1922,6 +1949,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) {
> @@ -2084,6 +2114,9 @@ static int proc_disconnect_claim(struct usb_dev_state *ps, void __user *arg)
>  					sizeof(dc.driver)) == 0)
>  			return -EBUSY;
>  
> +		if (ps->privileges_dropped)
> +			return -EACCES;
> +
>  		dev_dbg(&intf->dev, "disconnect by usbfs\n");
>  		usb_driver_release_interface(driver, intf);
>  	}
> @@ -2130,6 +2163,12 @@ 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)
> +{
> +	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
> @@ -2318,6 +2357,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);
> +		break;
>  	}
>  
>   done:
> diff --git a/include/uapi/linux/usbdevice_fs.h b/include/uapi/linux/usbdevice_fs.h
> index 019ba1e..99c296b 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   _IO('U', 30)
>  
>  #endif /* _UAPI_LINUX_USBDEVICE_FS_H */
> -- 
> 2.5.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 

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