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

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]

Date:	Thu, 4 Feb 2016 11:27:57 -0500 (EST)
From:	Alan Stern <stern@...land.harvard.edu>
To:	Emilio López <emilio.lopez@...labora.co.uk>
cc:	gregkh@...uxfoundation.org, <kborer@...il.com>, <bjorn@...k.no>,
	<reillyg@...omium.org>, <keescook@...omium.org>,
	<linux-api@...r.kernel.org>, <linux-usb@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <jorgelo@...omium.org>,
	<dan.carpenter@...cle.com>
Subject: 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@...omium.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@...omium.org>
> Signed-off-by: Emilio López <emilio.lopez@...labora.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