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

[Alan Stern]

On Thu, 4 Feb 2016, 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>
> Signed-off-by: Emilio LÃpez <emilio.lopez@xxxxxxxxxxxxxxx>
> 
> ---
> 
> 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