Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1312383 > unrolled thread

Re: [PATCH v1 0/1] ioctl to disallow detaching kernel USB drivers

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v1 0/1] ioctl to disallow detaching kernel USB drivers Emilio López <emilio.lopez@collabora.co.uk> - 2016-01-19 17:50 +0100
    Re: [PATCH v1 0/1] ioctl to disallow detaching kernel USB drivers Greg KH <gregkh@linuxfoundation.org> - 2016-01-19 19:10 +0100
      [PATCH v2] usb: devio: Add ioctl to disallow detaching kernel USB drivers. Emilio López <emilio.lopez@collabora.co.uk> - 2016-01-22 01:00 +0100
        Re: [PATCH v2] usb: devio: Add ioctl to disallow detaching kernel USB drivers. Bjørn Mork <bjorn@mork.no> - 2016-01-22 10:50 +0100
          Re: [PATCH v2] usb: devio: Add ioctl to disallow detaching kernel USB  drivers. Emilio López <emilio.lopez@collabora.co.uk> - 2016-01-25 03:10 +0100
            Re: [PATCH v2] usb: devio: Add ioctl to disallow detaching kernel USB drivers. Bjørn Mork <bjorn@mork.no> - 2016-01-25 09:50 +0100
              Re: [PATCH v2] usb: devio: Add ioctl to disallow detaching kernel  USB drivers. Alan Stern <stern@rowland.harvard.edu> - 2016-01-25 16:30 +0100
                Re: [PATCH v2] usb: devio: Add ioctl to disallow detaching kernel USB drivers. Bjørn Mork <bjorn@mork.no> - 2016-01-25 16:40 +0100
                  Re: [PATCH v2] usb: devio: Add ioctl to disallow detaching kernel  USB drivers. Alan Stern <stern@rowland.harvard.edu> - 2016-01-25 16:50 +0100
        Re: [PATCH v2] usb: devio: Add ioctl to disallow detaching kernel  USB drivers. Alan Stern <stern@rowland.harvard.edu> - 2016-01-22 17:20 +0100
          Re: [PATCH v2] usb: devio: Add ioctl to disallow detaching kernel USB  drivers. Emilio López <emilio.lopez@collabora.co.uk> - 2016-01-25 03:10 +0100

#1312383 — Re: [PATCH v1 0/1] ioctl to disallow detaching kernel USB drivers

Hi,

I'm reviving this thread as there's no consensus so far, and I'd like to 
see this supported in some way in mainline.

El 30/11/15 a las 15:48, Krzysztof Opasiak escribió:
>
>
> On 11/30/2015 06:20 PM, Greg KH wrote:
>> On Mon, Nov 30, 2015 at 06:12:22PM +0100, Krzysztof Opasiak wrote:
>>>
>>>
>>> On 11/30/2015 05:16 PM, Alan Stern wrote:
>>>> On Fri, 27 Nov 2015, Krzysztof Opasiak wrote:
>>>>
>>>>>>> I run through your code and as far as I understand above is not
>>>>>>> exactly
>>>>>>> true. Your patch allows only to prevent userspace from accessing
>>>>>>> interfaces
>>>>>>> which has kernel drivers, there is no way to stop an application
>>>>>>> from taking
>>>>>>> control over all free interfaces.
>>>>>>>
>>>>>>> Let's say that your device has 3 interfaces. First of them has a
>>>>>>> kernel
>>>>>>> driver but second and third doesn't. You have 2 apps. One should
>>>>>>> communicate
>>>>>>> using second interface and another one third. But first app is
>>>>>>> malicious and
>>>>>>> it claims all free interfaces of received device (your patch
>>>>>>> doesn't prevent
>>>>>>> this). And when second app starts it is unable to do anything
>>>>>>> with the
>>>>>>> device because all interfaces are taken. How would you like to
>>>>>>> handle this?
>>>>>>
>>>>>> You can't, and why would you ever want to, as you can't tell what
>>>>>> an app
>>>>>> "should" or "should not" do.  If you really care about this, then
>>>>>> use a
>>>>>> LSM policy to prevent this.
>>>>>
>>>>> Well, an app can declare what it does and what it needs in it's
>>>>> manifest
>>>>> file (or some equivalent of this) and the platform should ensure that
>>>>> app can do only what it has declared.
>>>>>
>>>>> I would really like to use LSM policy in here but currently it is
>>>>> impossible as one device node represents whole device. Permissions
>>>>> (even
>>>>> those from LSM) are being checked only on open() not on each
>>>>> ioctl() so
>>>>> as far as I know there is nothing which prevents any owner of
>>>>> opened fd
>>>>> to claim all available (not taken by someone else) interfaces and LSM
>>>>> policy is unable to filter those calls (unless we add some LSM hooks
>>>>> over there).
>>>>
>>>> How about this approach?  Once a process has dropped its usbfs
>>>> privileges, it's not allowed to claim any interfaces (either explicitly
>>>> or implicitly).  Instead, it or some manager program must claim the
>>>> appropriate interfaces before dropping privileges.
>>>>
>>>
>>> I agree that restricting interface claiming only to privileged
>>> process is a
>>> good idea. Unfortunately this generates a problem when program needs
>>> more
>>> than one interface (like in cdc - data + control for example). We
>>> need to
>>> declare both of them in first call to "usb-manager" or reopen the dev
>>> node
>>> at second call and claim all interfaces claimed using this fd till
>>> now and
>>> claim one more and then drop privileges and send a new fd.

I talked with Reilly some time back and they proposed using an extra 
parameter in the ioctl. This parameter would be a mask that specifies 
which interfaces the unprivileged process is allowed to claim (but 
doesn't necessarily have to do so). Providing a mask of ~0 would retain 
the current patch behaviour, and allow existing programs using 
out-of-tree implementations to continue working with little changes.

Now, if this were to be used on a system that knew better, via an app 
manifest or similar, the manager process would generate a suitable mask 
based on the interfaces required by the unprivileged process and use it 
as a parameter when dropping privileges. This way, the unprivileged 
process would be unable to claim interfaces it is not supposed to claim, 
while allowing it the liberty to do as it wishes with the interfaces it 
is allowed to use.

Krzysztof et al, would you find such an interface suitable? If you think 
it could be a way forward I'll gladly write the code and send a new patch.

Cheers,
Emilio

[toc] | [next] | [standalone]

#1312414

On Tue, Jan 19, 2016 at 01:39:59PM -0300, Emilio López wrote:
> Hi,
> 
> I'm reviving this thread as there's no consensus so far, and I'd like to see
> this supported in some way in mainline.

I don't see any patches to comment on here :)

[toc] | [prev] | [next] | [standalone]

#1314649 — [PATCH v2] usb: devio: Add ioctl to disallow detaching kernel USB drivers.

From: Reilly Grant <reillyg@chromium.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@chromium.org>
Signed-off-by: Emilio López <emilio.lopez@collabora.co.uk>

---

I guess code talks more than words :) This patch is only build-tested,
and is meant to showcase the approach. The basic idea is to allow
limiting the interface claims via an extra parameter to resolve
Krzysztof's worries.

A longer paragraph explaining the idea can be seen at

http://www.spinics.net/lists/linux-usb/msg135271.html

Cheers!
Emilio

Changes in v2:
- Added a parameter to the ioctl, a mask of interfaces an unprivileged
  process is allowed to claim.
- Drop Tested-by's

 drivers/usb/core/devio.c          | 65 ++++++++++++++++++++++++++++++++++++---
 include/uapi/linux/usbdevice_fs.h |  5 +++
 2 files changed, 66 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index 38ae877c..bf40aa6 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -77,6 +77,8 @@ struct usb_dev_state {
 	unsigned long ifclaimed;
 	u32 secid;
 	u32 disabled_bulk_eps;
+	bool privileges_dropped;
+	unsigned long interface_allowed_mask;
 };
 
 struct async {
@@ -641,6 +643,14 @@ static int claimintf(struct usb_dev_state *ps, unsigned int ifnum)
 	if (test_bit(ifnum, &ps->ifclaimed))
 		return 0;
 
+	if (ps->privileges_dropped) {
+		if (ifnum >= 8*sizeof(ps->interface_allowed_mask))
+			return -EINVAL;
+
+		if (!test_bit(ifnum, &ps->interface_allowed_mask))
+			return -EACCES;
+	}
+
 	intf = usb_ifnum_to_if(dev, ifnum);
 	if (!intf)
 		err = -ENOENT;
@@ -878,7 +888,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 +921,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;
 	security_task_getsecid(current, &ps->secid);
 	smp_wmb();
 	list_add_tail(&ps->list, &dev->filelist);
@@ -1215,6 +1222,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 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 (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)) {
+				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);
 }
 
@@ -1922,6 +1950,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) {
@@ -2074,6 +2105,9 @@ static int proc_disconnect_claim(struct usb_dev_state *ps, void __user *arg)
 	if (intf->dev.driver) {
 		struct usb_driver *driver = to_usb_driver(intf->dev.driver);
 
+		if (ps->privileges_dropped)
+			return -EACCES;
+
 		if ((dc.flags & USBDEVFS_DISCONNECT_CLAIM_IF_DRIVER) &&
 				strncmp(dc.driver, intf->dev.driver->name,
 					sizeof(dc.driver)) != 0)
@@ -2130,6 +2164,26 @@ 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, 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 (ps->privileges_dropped)
+		return -EACCES;
+
+	ps->interface_allowed_mask = data.interface_allowed_mask;
+	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 +2372,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, p);
+		break;
 	}
 
  done:
diff --git a/include/uapi/linux/usbdevice_fs.h b/include/uapi/linux/usbdevice_fs.h
index 019ba1e..9abcb34 100644
--- a/include/uapi/linux/usbdevice_fs.h
+++ b/include/uapi/linux/usbdevice_fs.h
@@ -154,6 +154,10 @@ struct usbdevfs_streams {
 	unsigned char eps[0];
 };
 
+struct usbdevfs_drop_privs {
+	unsigned long interface_allowed_mask;
+};
+
 #define USBDEVFS_CONTROL           _IOWR('U', 0, struct usbdevfs_ctrltransfer)
 #define USBDEVFS_CONTROL32           _IOWR('U', 0, struct usbdevfs_ctrltransfer32)
 #define USBDEVFS_BULK              _IOWR('U', 2, struct usbdevfs_bulktransfer)
@@ -187,5 +191,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   _IOR('U', 30, struct usbdevfs_drop_privs)
 
 #endif /* _UAPI_LINUX_USBDEVICE_FS_H */
-- 
2.5.0

[toc] | [prev] | [next] | [standalone]

#1314857 — Re: [PATCH v2] usb: devio: Add ioctl to disallow detaching kernel USB drivers.

Emilio López <emilio.lopez@collabora.co.uk> writes:

> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> index 38ae877c..bf40aa6 100644
> --- a/drivers/usb/core/devio.c
> +++ b/drivers/usb/core/devio.c
> @@ -77,6 +77,8 @@ struct usb_dev_state {
>  	unsigned long ifclaimed;
>  	u32 secid;
>  	u32 disabled_bulk_eps;
> +	bool privileges_dropped;
> +	unsigned long interface_allowed_mask;
>  };
>  
>  struct async {
> @@ -641,6 +643,14 @@ static int claimintf(struct usb_dev_state *ps, unsigned int ifnum)
>  	if (test_bit(ifnum, &ps->ifclaimed))
>  		return 0;
>  
> +	if (ps->privileges_dropped) {
> +		if (ifnum >= 8*sizeof(ps->interface_allowed_mask))
> +			return -EINVAL;


I don't think you need this runtime test. You can just make sure that
sizeof(ps->interface_allowed_mask) == sizeof(ps->ifclaimed) at build
time.

I do find this variable and arbitrary limit a bit confusing, but that's
not your fault - I guess it is an indication that ifnums > 31 are rare
:)


> diff --git a/include/uapi/linux/usbdevice_fs.h b/include/uapi/linux/usbdevice_fs.h
> index 019ba1e..9abcb34 100644
> --- a/include/uapi/linux/usbdevice_fs.h
> +++ b/include/uapi/linux/usbdevice_fs.h
> @@ -154,6 +154,10 @@ struct usbdevfs_streams {
>  	unsigned char eps[0];
>  };
>  
> +struct usbdevfs_drop_privs {
> +	unsigned long interface_allowed_mask;
> +};
> +

"unsigned long" isn't a very good choice here, is it?



Bjørn

[toc] | [prev] | [next] | [standalone]

#1316165 — Re: [PATCH v2] usb: devio: Add ioctl to disallow detaching kernel USB drivers.

Hi Bjørn,

El 22/01/16 a las 06:41, Bjørn Mork escribió:
> Emilio López <emilio.lopez@collabora.co.uk> writes:
>
>> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
>> index 38ae877c..bf40aa6 100644
>> --- a/drivers/usb/core/devio.c
>> +++ b/drivers/usb/core/devio.c
>> @@ -77,6 +77,8 @@ struct usb_dev_state {
>>   	unsigned long ifclaimed;
>>   	u32 secid;
>>   	u32 disabled_bulk_eps;
>> +	bool privileges_dropped;
>> +	unsigned long interface_allowed_mask;
>>   };
>>
>>   struct async {
>> @@ -641,6 +643,14 @@ static int claimintf(struct usb_dev_state *ps, unsigned int ifnum)
>>   	if (test_bit(ifnum, &ps->ifclaimed))
>>   		return 0;
>>
>> +	if (ps->privileges_dropped) {
>> +		if (ifnum >= 8*sizeof(ps->interface_allowed_mask))
>> +			return -EINVAL;
>
>
> I don't think you need this runtime test. You can just make sure that
> sizeof(ps->interface_allowed_mask) == sizeof(ps->ifclaimed) at build
> time.
>
> I do find this variable and arbitrary limit a bit confusing, but that's
> not your fault - I guess it is an indication that ifnums > 31 are rare
> :)
>
>
>> diff --git a/include/uapi/linux/usbdevice_fs.h b/include/uapi/linux/usbdevice_fs.h
>> index 019ba1e..9abcb34 100644
>> --- a/include/uapi/linux/usbdevice_fs.h
>> +++ b/include/uapi/linux/usbdevice_fs.h
>> @@ -154,6 +154,10 @@ struct usbdevfs_streams {
>>   	unsigned char eps[0];
>>   };
>>
>> +struct usbdevfs_drop_privs {
>> +	unsigned long interface_allowed_mask;
>> +};
>> +
>
> "unsigned long" isn't a very good choice here, is it?

I went with a type matching ifclaimed on struct usb_dev_state to keep 
the limit the same, but I guess it's not the best idea for an ioctl. I 
can switch it to __u32, keeping the runtime check above as is, or use 
__u64. Which one would you prefer?

Thanks for the review!
Emilio

[toc] | [prev] | [next] | [standalone]

#1316292 — Re: [PATCH v2] usb: devio: Add ioctl to disallow detaching kernel USB drivers.

Emilio López <emilio.lopez@collabora.co.uk> writes:

>>> diff --git a/include/uapi/linux/usbdevice_fs.h b/include/uapi/linux/usbdevice_fs.h
>>> index 019ba1e..9abcb34 100644
>>> --- a/include/uapi/linux/usbdevice_fs.h
>>> +++ b/include/uapi/linux/usbdevice_fs.h
>>> @@ -154,6 +154,10 @@ struct usbdevfs_streams {
>>>   	unsigned char eps[0];
>>>   };
>>>
>>> +struct usbdevfs_drop_privs {
>>> +	unsigned long interface_allowed_mask;
>>> +};
>>> +
>>
>> "unsigned long" isn't a very good choice here, is it?
>
> I went with a type matching ifclaimed on struct usb_dev_state to keep
> the limit the same, but I guess it's not the best idea for an ioctl. I
> can switch it to __u32, keeping the runtime check above as is, or use
> __u64. Which one would you prefer?

I don't feel much like an expert here, but I can certainly make up an
opinion anyway :)

Since 64bits kernels allow usb devio with interface numbers up to 63, I
guess you need __u64 to avoid limiting the range?  Limiting will create
all sorts of followup problems, so it's definitely easiest to just go
with __u64.



Bjørn

[toc] | [prev] | [next] | [standalone]

#1316818 — Re: [PATCH v2] usb: devio: Add ioctl to disallow detaching kernel USB drivers.

On Mon, 25 Jan 2016, Bjørn Mork wrote:

> I don't feel much like an expert here, but I can certainly make up an
> opinion anyway :)
> 
> Since 64bits kernels allow usb devio with interface numbers up to 63, I
> guess you need __u64 to avoid limiting the range?  Limiting will create
> all sorts of followup problems, so it's definitely easiest to just go
> with __u64.

But the Linux USB stack only allows up to 32 interfaces (see
include/linux/usb.h):

/* this maximum is arbitrary */
#define USB_MAXINTERFACES	32

So there's no point using a 64-bit value.

On the other hand, this value is supposed to be the same size as 
ps->ifclaimed, which is used as an argument to clear_bit(), set_bit(), 
and test_bit().  Those routines require unsigned long.

Alan Stern

[toc] | [prev] | [next] | [standalone]

#1316832 — Re: [PATCH v2] usb: devio: Add ioctl to disallow detaching kernel USB drivers.

Alan Stern <stern@rowland.harvard.edu> writes:
> On Mon, 25 Jan 2016, Bjørn Mork wrote:
>
>> I don't feel much like an expert here, but I can certainly make up an
>> opinion anyway :)
>> 
>> Since 64bits kernels allow usb devio with interface numbers up to 63, I
>> guess you need __u64 to avoid limiting the range?  Limiting will create
>> all sorts of followup problems, so it's definitely easiest to just go
>> with __u64.
>
> But the Linux USB stack only allows up to 32 interfaces (see
> include/linux/usb.h):
>
> /* this maximum is arbitrary */
> #define USB_MAXINTERFACES	32


Ah, I totally missed that.  Thanks


> So there's no point using a 64-bit value.
>
> On the other hand, this value is supposed to be the same size as 
> ps->ifclaimed, which is used as an argument to clear_bit(), set_bit(), 
> and test_bit().  Those routines require unsigned long.

Maybe the input to these should be clamped to USB_MAXINTERFACES?


Bjørn

[toc] | [prev] | [next] | [standalone]

#1316856 — Re: [PATCH v2] usb: devio: Add ioctl to disallow detaching kernel USB drivers.

On Mon, 25 Jan 2016, Bjørn Mork wrote:

> Alan Stern <stern@rowland.harvard.edu> writes:
> > On Mon, 25 Jan 2016, Bjørn Mork wrote:
> >
> >> I don't feel much like an expert here, but I can certainly make up an
> >> opinion anyway :)
> >> 
> >> Since 64bits kernels allow usb devio with interface numbers up to 63, I
> >> guess you need __u64 to avoid limiting the range?  Limiting will create
> >> all sorts of followup problems, so it's definitely easiest to just go
> >> with __u64.
> >
> > But the Linux USB stack only allows up to 32 interfaces (see
> > include/linux/usb.h):
> >
> > /* this maximum is arbitrary */
> > #define USB_MAXINTERFACES	32
> 
> 
> Ah, I totally missed that.  Thanks
> 
> 
> > So there's no point using a 64-bit value.
> >
> > On the other hand, this value is supposed to be the same size as 
> > ps->ifclaimed, which is used as an argument to clear_bit(), set_bit(), 
> > and test_bit().  Those routines require unsigned long.
> 
> Maybe the input to these should be clamped to USB_MAXINTERFACES?

If you want.  Right now it's clamped to 8 * sizeof(ps->ifclaimed),
which ought to be good enough.

Oh yes, there's one other thing to notice.  The value passed to these
routines is an interface number (as opposed to an index).  According to
the USB spec, interfaces are supposed to be numbered sequentially
starting from 0, but there may be some devices that mess this up.  So
it's possible we'll see an interface number which is larger than the
number of interfaces!  :-)

Alan Stern

[toc] | [prev] | [next] | [standalone]

#1315102 — 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@chromium.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@chromium.org>
> Signed-off-by: Emilio López <emilio.lopez@collabora.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

[toc] | [prev] | [next] | [standalone]

#1316164 — 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@chromium.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@chromium.org>
>> Signed-off-by: Emilio López <emilio.lopez@collabora.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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web