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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <56A581F9.5070707@collabora.co.uk>
Date:	Sun, 24 Jan 2016 23:01:29 -0300
From:	Emilio López <emilio.lopez@...labora.co.uk>
To:	Alan Stern <stern@...land.harvard.edu>
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.

Hi Alan,

El 22/01/16 a las 13:10, Alan Stern escribió:
> 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."

I have replaced it with the following now

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

> 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.

Good point, I've changed this to look like the following

         /* 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.
          */
         if (ps->privileges_dropped)
                 ps->interface_allowed_mask &= data.interface_allowed_mask;
         else
                 ps->interface_allowed_mask = data.interface_allowed_mask;

         ps->privileges_dropped = true;

Or maybe I could change the default mask to ~0 and simplify this a bit, hm.

Thank you for the review!

Emilio