linux-kernel - Re: [PATCH v2] 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:	Fri, 22 Jan 2016 11:10:43 -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>,
	<k.opasiak@...sung.com>, <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 v2] usb: devio: Add ioctl to disallow detaching kernel
 USB drivers.

On Thu, 21 Jan 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>


>  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 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.
> +	 */

This comment should be rephrased.  It should say something like:
"Don't allow if the process has dropped its privilege to do such
things and any of the interfaces are claimed."

You also might consider allowing the reset if the interfaces are
claimed only by the current process (or more precisely, by ps).

> +static int proc_drop_privileges(struct usb_dev_state *ps, void __user *arg)
> +{
> +	struct usbdevfs_drop_privs data;
> +
> +	if (copy_from_user(&data, arg, sizeof(data)))
> +		return -EFAULT;
> +
> +	/* This is a one way operation. Once privileges were dropped,
> +	 * you cannot do it again (Otherwise unprivileged processes
> +	 * would be able to change their allowed interfaces mask)
> +	 */

If you're going to keep a mask of claimable interfaces then there's no
reason this has to be a one-time operation.  Processes should always be
allowed to shrink the mask, just not to grow it.

Alan Stern