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

Groups > linux.kernel > #1277497 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v1 0/1] ioctl to disallow detaching kernel USB drivers Emilio López <emilio.lopez@collabora.co.uk> - 2015-11-25 16:50 +0100
    [PATCH v1] usb: devio: Add ioctl to disallow detaching kernel USB drivers. Emilio López <emilio.lopez@collabora.co.uk> - 2015-11-25 16:50 +0100
      Re: [PATCH v1] usb: devio: Add ioctl to disallow detaching kernel  USB drivers. Dan Carpenter <dan.carpenter@oracle.com> - 2015-11-26 10:30 +0100
    Re: [PATCH v1 0/1] ioctl to disallow detaching kernel USB drivers Krzysztof Opasiak <k.opasiak@samsung.com> - 2015-11-26 10:30 +0100
      Re: [PATCH v1 0/1] ioctl to disallow detaching kernel USB drivers Greg KH <gregkh@linuxfoundation.org> - 2015-11-26 18:30 +0100
        Re: [PATCH v1 0/1] ioctl to disallow detaching kernel USB drivers Krzysztof Opasiak <k.opasiak@samsung.com> - 2015-11-27 09:50 +0100
          Re: [PATCH v1 0/1] ioctl to disallow detaching kernel USB drivers Greg KH <gregkh@linuxfoundation.org> - 2015-11-28 03:40 +0100
            Re: [PATCH v1 0/1] ioctl to disallow detaching kernel USB drivers Oliver Neukum <oneukum@suse.com> - 2015-11-30 10:20 +0100
          Re: [PATCH v1 0/1] ioctl to disallow detaching kernel USB drivers Alan Stern <stern@rowland.harvard.edu> - 2015-11-30 17:20 +0100
            Re: [PATCH v1 0/1] ioctl to disallow detaching kernel USB drivers Krzysztof Opasiak <k.opasiak@samsung.com> - 2015-11-30 18:20 +0100
              Re: [PATCH v1 0/1] ioctl to disallow detaching kernel USB drivers Greg KH <gregkh@linuxfoundation.org> - 2015-11-30 18:30 +0100
                Re: [PATCH v1 0/1] ioctl to disallow detaching kernel USB drivers Krzysztof Opasiak <k.opasiak@samsung.com> - 2015-11-30 19:50 +0100

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

Hi everyone,

This patch introduces a new ioctl, USBDEVFS_DROP_PRIVILEGES,
to voluntarily forgo the ability to issue ioctls which may
interfere with other users of the USB device.

This feature allows a privileged process (in the case of Chrome OS,
permission_broker) to open a USB device node and then drop a number
of capabilities that are considered "privileged". These privileges
include the ability to reset the device if there are other users
(most notably a kernel driver) or to disconnect a kernel driver
from the device. The file descriptor can then be passed to an
unprivileged process.

This is useful for granting a process access to a device with
multiple functions. It won't be able to use its access to one
function to disrupt or take over control of another function.

This patch is currently being used in Chrome OS; I have updated it
to be in line with changes in v4.4-rc.

Cheers!
Emilio


Reilly Grant (1):
  usb: devio: Add ioctl to disallow detaching kernel USB drivers.

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

-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1277506 — [PATCH v1] 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>
Reviewed-by: Jorge Lucangeli Obes <jorgelo@chromium.org>
Reviewed-by: Kees Cook <keescook@chromium.org>
[emilio.lopez: fix build for v4.4-rc2 and adjust patch title prefix]
Signed-off-by: Emilio López <emilio.lopez@collabora.co.uk>

---

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

diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index 38ae877c..a5ccc50 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -77,6 +77,7 @@ struct usb_dev_state {
 	unsigned long ifclaimed;
 	u32 secid;
 	u32 disabled_bulk_eps;
+	bool privileges_dropped;
 };
 
 struct async {
@@ -878,7 +879,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 +912,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 +1213,35 @@ static int proc_connectinfo(struct usb_dev_state *ps, void __user *arg)
 
 static int proc_resetdevice(struct usb_dev_state *ps)
 {
+	if (ps->privileges_dropped) {
+		struct usb_host_config *actconfig = ps->dev->actconfig;
+
+		/* 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 (actconfig) {
+			int i;
+
+			for (i = 0; i < actconfig->desc.bNumInterfaces; ++i) {
+				if (usb_interface_claimed(
+					actconfig->interface[i])) {
+					dev_warn(&ps->dev->dev,
+						"usbfs: interface %d claimed by"
+						" %s while '%s'"
+						" resets device\n",
+						actconfig->interface[i]
+							->cur_altsetting
+							->desc.bInterfaceNumber,
+						actconfig->interface[i]
+							->dev.driver->name,
+						current->comm);
+					return -EACCES;
+				}
+			}
+		}
+	}
+
 	return usb_reset_device(ps->dev);
 }
 
@@ -1922,6 +1949,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) {
@@ -2084,6 +2114,9 @@ static int proc_disconnect_claim(struct usb_dev_state *ps, void __user *arg)
 					sizeof(dc.driver)) == 0)
 			return -EBUSY;
 
+		if (ps->privileges_dropped)
+			return -EACCES;
+
 		dev_dbg(&intf->dev, "disconnect by usbfs\n");
 		usb_driver_release_interface(driver, intf);
 	}
@@ -2130,6 +2163,12 @@ 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)
+{
+	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 +2357,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);
+		break;
 	}
 
  done:
diff --git a/include/uapi/linux/usbdevice_fs.h b/include/uapi/linux/usbdevice_fs.h
index 019ba1e..99c296b 100644
--- a/include/uapi/linux/usbdevice_fs.h
+++ b/include/uapi/linux/usbdevice_fs.h
@@ -187,5 +187,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   _IO('U', 30)
 
 #endif /* _UAPI_LINUX_USBDEVICE_FS_H */
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

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

On Thu, Nov 26, 2015 at 04:59:25PM +0800, Peter Chen wrote:
> On Wed, Nov 25, 2015 at 12:45:34PM -0300, 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>
> > Reviewed-by: Jorge Lucangeli Obes <jorgelo@chromium.org>
> > Reviewed-by: Kees Cook <keescook@chromium.org>
> > [emilio.lopez: fix build for v4.4-rc2 and adjust patch title prefix]
> > Signed-off-by: Emilio López <emilio.lopez@collabora.co.uk>
> > 
> > ---
> > 
> >  drivers/usb/core/devio.c          | 50 +++++++++++++++++++++++++++++++++++----
> >  include/uapi/linux/usbdevice_fs.h |  1 +
> >  2 files changed, 47 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> > index 38ae877c..a5ccc50 100644
> > --- a/drivers/usb/core/devio.c
> > +++ b/drivers/usb/core/devio.c
> > @@ -77,6 +77,7 @@ struct usb_dev_state {
> >  	unsigned long ifclaimed;
> >  	u32 secid;
> >  	u32 disabled_bulk_eps;
> > +	bool privileges_dropped;
> >  };
> >  
> >  struct async {
> > @@ -878,7 +879,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 +912,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;
> 
> The above changes seem to be not related with this change.
> 

It is though.  They added a new struct member and thought that now it
was cleaner to use kzalloc() instead of initializing the new member to
zero.

> >  	security_task_getsecid(current, &ps->secid);
> >  	smp_wmb();
> >  	list_add_tail(&ps->list, &dev->filelist);
> > @@ -1215,6 +1213,35 @@ static int proc_connectinfo(struct usb_dev_state *ps, void __user *arg)
> >  
> >  static int proc_resetdevice(struct usb_dev_state *ps)
> >  {
> > +	if (ps->privileges_dropped) {
> > +		struct usb_host_config *actconfig = ps->dev->actconfig;
> > +
> > +		/* 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 (actconfig) {
> > +			int i;
> > +
> > +			for (i = 0; i < actconfig->desc.bNumInterfaces; ++i) {
> > +				if (usb_interface_claimed(
> > +					actconfig->interface[i])) {
> > +					dev_warn(&ps->dev->dev,
> > +						"usbfs: interface %d claimed by"
> > +						" %s while '%s'"
> > +						" resets device\n",
> > +						actconfig->interface[i]
> > +							->cur_altsetting
> > +							->desc.bInterfaceNumber,
> 
> The length of line is 80 characters
> 
> > +						actconfig->interface[i]
> > +							->dev.driver->name,
> > +						current->comm);
> > +					return -EACCES;
> > +				}
> > +			}
> > +		}
> > +	}

Yeah.  Better to flip this around:

static int proc_resetdevice(struct usb_dev_state *ps)
{
        struct usb_host_config *actconfig = ps->dev->actconfig;
        struct usb_interface *interface;
        int i;

        if (ps->privileges_dropped && actconfig) {
                for (i = 0; i < actconfig->desc.bNumInterfaces; ++i) {
                        interface = actconfig->interface[i];
                        if (usb_interface_claimed(interface)) {
                                dev_warn(&ps->dev->dev,
                                        "usbfs: interface %d claimed by %s while '%s' resets device\n",
                                        interface->cur_altsetting->desc.bInterfaceNumber,
                                        interface->dev.driver->name,
                                        current->comm);
                                return -EACCES;
                        }
                }
        }

        return usb_reset_device(ps->dev);
}

It still goes over the 80 character limit, but that's cleaner than
breaking up the variable.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1278108

On 11/25/2015 04:45 PM, Emilio López wrote:
> Hi everyone,
>
> This patch introduces a new ioctl, USBDEVFS_DROP_PRIVILEGES,
> to voluntarily forgo the ability to issue ioctls which may
> interfere with other users of the USB device.
>
> This feature allows a privileged process (in the case of Chrome OS,
> permission_broker) to open a USB device node and then drop a number
> of capabilities that are considered "privileged".

We had the same idea in Tizen but for now we didn't have time to 
implement it.

  These privileges
> include the ability to reset the device if there are other users
> (most notably a kernel driver) or to disconnect a kernel driver
> from the device. The file descriptor can then be passed to an
> unprivileged process.

And how about switching configuration? This can be also harmful even if 
the are no other users for any interface in this configuration.
(Just imagine the situation in which only second config contains an HID 
function and when app switch configuration it is activated without user 
knowing about this;))

>
> This is useful for granting a process access to a device with
> multiple functions. It won't be able to use its access to one
> function to disrupt or take over control of another function.

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?

Moreover I'm not convinced to this patch as it hardcodes the *policy* in 
kernel code. Generally our approach (with passing fd from broker to 
unprivileged process) was similar but we found out that if we would like 
to do this correctly there is much more things to filter than in this 
patch. We had two main ideas:

- implement some LSM hooks in ioctls() but this leads to a lot of 
additional callbacks in lsm ops struct which even now is very big. But 
as a benefit we would get a very flexible policy consistent with other 
system policies

- split single usb device node into multiple files which could represent 
single endpoins only for io and separate control file for privileged but 
it's quite a lot of work and I don't know if any one is going to accept 
such a change

Best regards,
-- 
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1278360

On Thu, Nov 26, 2015 at 10:19:29AM +0100, Krzysztof Opasiak wrote:
> 
> 
> On 11/25/2015 04:45 PM, Emilio López wrote:
> >Hi everyone,
> >
> >This patch introduces a new ioctl, USBDEVFS_DROP_PRIVILEGES,
> >to voluntarily forgo the ability to issue ioctls which may
> >interfere with other users of the USB device.
> >
> >This feature allows a privileged process (in the case of Chrome OS,
> >permission_broker) to open a USB device node and then drop a number
> >of capabilities that are considered "privileged".
> 
> We had the same idea in Tizen but for now we didn't have time to implement
> it.
> 
>  These privileges
> >include the ability to reset the device if there are other users
> >(most notably a kernel driver) or to disconnect a kernel driver
> >from the device. The file descriptor can then be passed to an
> >unprivileged process.
> 
> And how about switching configuration? This can be also harmful even if the
> are no other users for any interface in this configuration.
> (Just imagine the situation in which only second config contains an HID
> function and when app switch configuration it is activated without user
> knowing about this;))

Adding this option might be nice.

> >This is useful for granting a process access to a device with
> >multiple functions. It won't be able to use its access to one
> >function to disrupt or take over control of another function.
> 
> 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.

> Moreover I'm not convinced to this patch as it hardcodes the *policy* in
> kernel code.

What policy is that?

> Generally our approach (with passing fd from broker to
> unprivileged process) was similar but we found out that if we would like to
> do this correctly there is much more things to filter than in this patch. We
> had two main ideas:
> 
> - implement some LSM hooks in ioctls() but this leads to a lot of additional
> callbacks in lsm ops struct which even now is very big. But as a benefit we
> would get a very flexible policy consistent with other system policies
> 
> - split single usb device node into multiple files which could represent
> single endpoins only for io and separate control file for privileged but
> it's quite a lot of work and I don't know if any one is going to accept such
> a change

I've been asking for that for well over a decade, but no one ever did
the work.  I think if you work through the options, it ends up not being
a viable solution...

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1278635

On 11/26/2015 06:29 PM, Greg KH wrote:
> On Thu, Nov 26, 2015 at 10:19:29AM +0100, Krzysztof Opasiak wrote:
>>
>>
>> On 11/25/2015 04:45 PM, Emilio López wrote:
>>> Hi everyone,
>>>
>>> This patch introduces a new ioctl, USBDEVFS_DROP_PRIVILEGES,
>>> to voluntarily forgo the ability to issue ioctls which may
>>> interfere with other users of the USB device.
>>>
>>> This feature allows a privileged process (in the case of Chrome OS,
>>> permission_broker) to open a USB device node and then drop a number
>>> of capabilities that are considered "privileged".
>>
>> We had the same idea in Tizen but for now we didn't have time to implement
>> it.
>>
>>   These privileges
>>> include the ability to reset the device if there are other users
>>> (most notably a kernel driver) or to disconnect a kernel driver
>> >from the device. The file descriptor can then be passed to an
>>> unprivileged process.
>>
>> And how about switching configuration? This can be also harmful even if the
>> are no other users for any interface in this configuration.
>> (Just imagine the situation in which only second config contains an HID
>> function and when app switch configuration it is activated without user
>> knowing about this;))
>
> Adding this option might be nice.
>
>>> This is useful for granting a process access to a device with
>>> multiple functions. It won't be able to use its access to one
>>> function to disrupt or take over control of another function.
>>
>> 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).

>
>> Moreover I'm not convinced to this patch as it hardcodes the *policy* in
>> kernel code.
>
> What policy is that?

It's a policy which defines set of ioctls which cannot be issued in 
"restricted mode".

>
>> Generally our approach (with passing fd from broker to
>> unprivileged process) was similar but we found out that if we would like to
>> do this correctly there is much more things to filter than in this patch. We
>> had two main ideas:
>>
>> - implement some LSM hooks in ioctls() but this leads to a lot of additional
>> callbacks in lsm ops struct which even now is very big. But as a benefit we
>> would get a very flexible policy consistent with other system policies
>>
>> - split single usb device node into multiple files which could represent
>> single endpoins only for io and separate control file for privileged but
>> it's quite a lot of work and I don't know if any one is going to accept such
>> a change
>
> I've been asking for that for well over a decade, but no one ever did
> the work.  I think if you work through the options, it ends up not being
> a viable solution...
>

I'm not surprised that no one ever did this as it looks like quite a lot 
of work and current interface is still working;) Do you have some link 
to a discussion or sth which shows why it's not a good solution?

-- 
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1279087

On Fri, Nov 27, 2015 at 09:44:45AM +0100, Krzysztof Opasiak wrote:
> 
> 
> On 11/26/2015 06:29 PM, Greg KH wrote:
> >On Thu, Nov 26, 2015 at 10:19:29AM +0100, Krzysztof Opasiak wrote:
> >>
> >>
> >>On 11/25/2015 04:45 PM, Emilio López wrote:
> >>>Hi everyone,
> >>>
> >>>This patch introduces a new ioctl, USBDEVFS_DROP_PRIVILEGES,
> >>>to voluntarily forgo the ability to issue ioctls which may
> >>>interfere with other users of the USB device.
> >>>
> >>>This feature allows a privileged process (in the case of Chrome OS,
> >>>permission_broker) to open a USB device node and then drop a number
> >>>of capabilities that are considered "privileged".
> >>
> >>We had the same idea in Tizen but for now we didn't have time to implement
> >>it.
> >>
> >>  These privileges
> >>>include the ability to reset the device if there are other users
> >>>(most notably a kernel driver) or to disconnect a kernel driver
> >>>from the device. The file descriptor can then be passed to an
> >>>unprivileged process.
> >>
> >>And how about switching configuration? This can be also harmful even if the
> >>are no other users for any interface in this configuration.
> >>(Just imagine the situation in which only second config contains an HID
> >>function and when app switch configuration it is activated without user
> >>knowing about this;))
> >
> >Adding this option might be nice.
> >
> >>>This is useful for granting a process access to a device with
> >>>multiple functions. It won't be able to use its access to one
> >>>function to disrupt or take over control of another function.
> >>
> >>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.

"should"?  Depending on what?  :)

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

Yes, it's tough, I know, good luck.

Also deal with multiple devices, busses that are ordered differently
depending on the phase of the moon, and other fun things with dynamic
devices and ioctls.  It's a loosing battle :)

> >>Moreover I'm not convinced to this patch as it hardcodes the *policy* in
> >>kernel code.
> >
> >What policy is that?
> 
> It's a policy which defines set of ioctls which cannot be issued in
> "restricted mode".
> 
> >
> >>Generally our approach (with passing fd from broker to
> >>unprivileged process) was similar but we found out that if we would like to
> >>do this correctly there is much more things to filter than in this patch. We
> >>had two main ideas:
> >>
> >>- implement some LSM hooks in ioctls() but this leads to a lot of additional
> >>callbacks in lsm ops struct which even now is very big. But as a benefit we
> >>would get a very flexible policy consistent with other system policies
> >>
> >>- split single usb device node into multiple files which could represent
> >>single endpoins only for io and separate control file for privileged but
> >>it's quite a lot of work and I don't know if any one is going to accept such
> >>a change
> >
> >I've been asking for that for well over a decade, but no one ever did
> >the work.  I think if you work through the options, it ends up not being
> >a viable solution...
> >
> 
> I'm not surprised that no one ever did this as it looks like quite a lot of
> work and current interface is still working;) Do you have some link to a
> discussion or sth which shows why it's not a good solution?

Dig through the archives, I think the last time this was brought up was
way before USB 3 came out.  As for "not a good solution", you have to
map endpoints together somehow in some way in userspace, which gets
messy fast, and then you would have to somehow modify all userspace
programs to use the new model.

Good luck!

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1279717

On Fri, 2015-11-27 at 18:39 -0800, Greg KH wrote:
> Yes, it's tough, I know, good luck.
> 
> Also deal with multiple devices, busses that are ordered differently
> depending on the phase of the moon, and other fun things with dynamic
> devices and ioctls.  It's a loosing battle :)

IMHO the fundamental problem is using one device node per device.
And I think it should be tackled. Yet I have no idea how to do
this in a compatible manner.

	Regards
		Oliver


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1280053

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.

Alan Stern


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1280121

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.

Maybe better option would be to add optional argument to claim interface 
ioctl() and allow to claim interface for other fd than the current one? 
So "usb-manager" could have fd with full control and claim interfaces 
for apps which have fds with restricted privileges.

Best regards,
-- 
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1280135

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.

Have you seen such a device that is controlled this way in userspace?
Don't over-engineer something that is probably pretty rare...

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1280192

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.
>
> Have you seen such a device that is controlled this way in userspace?
> Don't over-engineer something that is probably pretty rare...
>

Yes I have seen such devices (not cdc of course) and they were driven 
using libusb (vendor specific service + "driver" to bypass publishing 
protocol code due to kernel's GPL). I have even seen an android app 
written in java which claims and uses multiple interfaces using 
android's USB API, so it's real;)

-- 
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web